[PATCH] hugetlbfs: add MTE support

[PATCH] hugetlbfs: add MTE support

[Yang Shi]

MTE can be supported on ram based filesystem. It is supported on tmpfs.
There is use case to use MTE on hugetlbfs as well, adding MTE support.

Signed-off-by: Yang Shi <yang@os.amperecomputing.com>
---
 fs/hugetlbfs/inode.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
index ecad73a4f713..c34faef62daf 100644
--- a/fs/hugetlbfs/inode.c
+++ b/fs/hugetlbfs/inode.c
@@ -110,7 +110,7 @@ static int hugetlbfs_file_mmap(struct file *file, struct vm_area_struct *vma)
 	 * way when do_mmap unwinds (may be important on powerpc
 	 * and ia64).
 	 */
-	vm_flags_set(vma, VM_HUGETLB | VM_DONTEXPAND);
+	vm_flags_set(vma, VM_HUGETLB | VM_DONTEXPAND | VM_MTE_ALLOWED);
 	vma->vm_ops = &hugetlb_vm_ops;
 
 	ret = seal_check_write(info->seals, vma);
-- 
2.41.0

Re: [PATCH] hugetlbfs: add MTE support

[Andrew Morton]

On Tue, 25 Jun 2024 16:37:17 -0700 Yang Shi <yang@os.amperecomputing.com> wrote:

> MTE can be supported on ram based filesystem. It is supported on tmpfs.
> There is use case to use MTE on hugetlbfs as well, adding MTE support.
> 
> --- a/fs/hugetlbfs/inode.c
> +++ b/fs/hugetlbfs/inode.c
> @@ -110,7 +110,7 @@ static int hugetlbfs_file_mmap(struct file *file, struct vm_area_struct *vma)
>  	 * way when do_mmap unwinds (may be important on powerpc
>  	 * and ia64).
>  	 */
> -	vm_flags_set(vma, VM_HUGETLB | VM_DONTEXPAND);
> +	vm_flags_set(vma, VM_HUGETLB | VM_DONTEXPAND | VM_MTE_ALLOWED);
>  	vma->vm_ops = &hugetlb_vm_ops;
>  
>  	ret = seal_check_write(info->seals, vma);

How thoroughly has this been tested?

Can we expect normal linux-next testing to exercise this, or must
testers make special arangements to get the coverage?

Re: [PATCH] hugetlbfs: add MTE support

[Yang Shi]

On Wed, Jun 26, 2024 at 1:40 PM Andrew Morton <akpm@linux-foundation.org> wrote:
>
> On Tue, 25 Jun 2024 16:37:17 -0700 Yang Shi <yang@os.amperecomputing.com> wrote:
>
> > MTE can be supported on ram based filesystem. It is supported on tmpfs.
> > There is use case to use MTE on hugetlbfs as well, adding MTE support.
> >
> > --- a/fs/hugetlbfs/inode.c
> > +++ b/fs/hugetlbfs/inode.c
> > @@ -110,7 +110,7 @@ static int hugetlbfs_file_mmap(struct file *file, struct vm_area_struct *vma)
> >        * way when do_mmap unwinds (may be important on powerpc
> >        * and ia64).
> >        */
> > -     vm_flags_set(vma, VM_HUGETLB | VM_DONTEXPAND);
> > +     vm_flags_set(vma, VM_HUGETLB | VM_DONTEXPAND | VM_MTE_ALLOWED);
> >       vma->vm_ops = &hugetlb_vm_ops;
> >
> >       ret = seal_check_write(info->seals, vma);
>
> How thoroughly has this been tested?
>
> Can we expect normal linux-next testing to exercise this, or must
> testers make special arangements to get the coverage?

It requires special arrangements. This needs hardware support and
custom-patched QEMU. We did in-house test on AmpereOne platform with
patched QEMU 8.1.

>

Re: [PATCH] hugetlbfs: add MTE support

[Yang Shi]

On Wed, Jun 26, 2024 at 1:45 PM Yang Shi <shy828301@gmail.com> wrote:
>
> On Wed, Jun 26, 2024 at 1:40 PM Andrew Morton <akpm@linux-foundation.org> wrote:
> >
> > On Tue, 25 Jun 2024 16:37:17 -0700 Yang Shi <yang@os.amperecomputing.com> wrote:
> >
> > > MTE can be supported on ram based filesystem. It is supported on tmpfs.
> > > There is use case to use MTE on hugetlbfs as well, adding MTE support.
> > >
> > > --- a/fs/hugetlbfs/inode.c
> > > +++ b/fs/hugetlbfs/inode.c
> > > @@ -110,7 +110,7 @@ static int hugetlbfs_file_mmap(struct file *file, struct vm_area_struct *vma)
> > >        * way when do_mmap unwinds (may be important on powerpc
> > >        * and ia64).
> > >        */
> > > -     vm_flags_set(vma, VM_HUGETLB | VM_DONTEXPAND);
> > > +     vm_flags_set(vma, VM_HUGETLB | VM_DONTEXPAND | VM_MTE_ALLOWED);
> > >       vma->vm_ops = &hugetlb_vm_ops;
> > >
> > >       ret = seal_check_write(info->seals, vma);
> >
> > How thoroughly has this been tested?
> >
> > Can we expect normal linux-next testing to exercise this, or must
> > testers make special arangements to get the coverage?
>
> It requires special arrangements. This needs hardware support and
> custom-patched QEMU. We did in-house test on AmpereOne platform with
> patched QEMU 8.1.

To correct, custom-patched QEMU is not required for a minimum test.
But a special test program is definitely needed. We used
custom-patched QEMU to test VM backed by hugetlbfs with MTE enabled.

>
> >

Re: [PATCH] hugetlbfs: add MTE support

[Catalin Marinas]

On Tue, Jun 25, 2024 at 04:37:17PM -0700, Yang Shi wrote:
> MTE can be supported on ram based filesystem. It is supported on tmpfs.
> There is use case to use MTE on hugetlbfs as well, adding MTE support.
> 
> Signed-off-by: Yang Shi <yang@os.amperecomputing.com>
> ---
>  fs/hugetlbfs/inode.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
> index ecad73a4f713..c34faef62daf 100644
> --- a/fs/hugetlbfs/inode.c
> +++ b/fs/hugetlbfs/inode.c
> @@ -110,7 +110,7 @@ static int hugetlbfs_file_mmap(struct file *file, struct vm_area_struct *vma)
>  	 * way when do_mmap unwinds (may be important on powerpc
>  	 * and ia64).
>  	 */
> -	vm_flags_set(vma, VM_HUGETLB | VM_DONTEXPAND);
> +	vm_flags_set(vma, VM_HUGETLB | VM_DONTEXPAND | VM_MTE_ALLOWED);
>  	vma->vm_ops = &hugetlb_vm_ops;

Last time I checked, about a year ago, this was not sufficient. One
issue is that there's no arch_clear_hugetlb_flags() implemented by your
patch, leaving PG_arch_{2,3} set on a page. The other issue was that I
initially tried to do this only on the head page but this did not go
well with the folio_copy() -> copy_highpage() which expects the
PG_arch_* flags on each individual page. The alternative was for
arch_clear_hugetlb_flags() to iterate over all the pages in a folio.

I'd also like to see some tests added to
tools/testing/selftest/arm64/mte to exercise MAP_HUGETLB with PROT_MTE:
write/read tags, a series of mman+munmap (mostly to check if old page
flags are still around), force some copy on write. I don't think we
should merge the patch without proper tests.

An untested hunk on top of your changes:

diff --git a/arch/arm64/include/asm/hugetlb.h b/arch/arm64/include/asm/hugetlb.h
index 3954cbd2ff56..5357b00b9087 100644
--- a/arch/arm64/include/asm/hugetlb.h
+++ b/arch/arm64/include/asm/hugetlb.h
@@ -20,7 +20,19 @@ extern bool arch_hugetlb_migration_supported(struct hstate *h);
 
 static inline void arch_clear_hugetlb_flags(struct folio *folio)
 {
-	clear_bit(PG_dcache_clean, &folio->flags);
+	unsigned long i, nr_pages = folio_nr_pages(folio);
+	const unsigned long clear_flags = BIT(PG_dcache_clean) |
+		BIT(PG_arch_2) | BIT(PG_arch_3);
+
+	if (!system_supports_mte()) {
+		clear_bit(PG_dcache_clean, &folio->flags);
+		return;
+	}
+
+	for (i = 0; i < nr_pages; i++) {
+		struct page *page = folio_page(folio, i);
+		page->flags &= ~clear_flags;
+	}
 }
 #define arch_clear_hugetlb_flags arch_clear_hugetlb_flags
 
diff --git a/arch/arm64/include/asm/mman.h b/arch/arm64/include/asm/mman.h
index 5966ee4a6154..304dfc499e68 100644
--- a/arch/arm64/include/asm/mman.h
+++ b/arch/arm64/include/asm/mman.h
@@ -28,7 +28,8 @@ static inline unsigned long arch_calc_vm_flag_bits(unsigned long flags)
 	 * backed by tags-capable memory. The vm_flags may be overridden by a
 	 * filesystem supporting MTE (RAM-based).
 	 */
-	if (system_supports_mte() && (flags & MAP_ANONYMOUS))
+	if (system_supports_mte() &&
+	    (flags & (MAP_ANONYMOUS | MAP_HUGETLB)))
 		return VM_MTE_ALLOWED;
 
 	return 0;

-- 
Catalin

Re: [PATCH] hugetlbfs: add MTE support

[David Hildenbrand]

On 02.07.24 14:34, Catalin Marinas wrote:
> On Tue, Jun 25, 2024 at 04:37:17PM -0700, Yang Shi wrote:
>> MTE can be supported on ram based filesystem. It is supported on tmpfs.
>> There is use case to use MTE on hugetlbfs as well, adding MTE support.
>>
>> Signed-off-by: Yang Shi <yang@os.amperecomputing.com>
>> ---
>>   fs/hugetlbfs/inode.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
>> index ecad73a4f713..c34faef62daf 100644
>> --- a/fs/hugetlbfs/inode.c
>> +++ b/fs/hugetlbfs/inode.c
>> @@ -110,7 +110,7 @@ static int hugetlbfs_file_mmap(struct file *file, struct vm_area_struct *vma)
>>   	 * way when do_mmap unwinds (may be important on powerpc
>>   	 * and ia64).
>>   	 */
>> -	vm_flags_set(vma, VM_HUGETLB | VM_DONTEXPAND);
>> +	vm_flags_set(vma, VM_HUGETLB | VM_DONTEXPAND | VM_MTE_ALLOWED);
>>   	vma->vm_ops = &hugetlb_vm_ops;
> 
> Last time I checked, about a year ago, this was not sufficient. One
> issue is that there's no arch_clear_hugetlb_flags() implemented by your
> patch, leaving PG_arch_{2,3} set on a page. The other issue was that I
> initially tried to do this only on the head page but this did not go
> well with the folio_copy() -> copy_highpage() which expects the
> PG_arch_* flags on each individual page. The alternative was for
> arch_clear_hugetlb_flags() to iterate over all the pages in a folio.

This would likely also add a blocker for 
ARCH_WANT_OPTIMIZE_HUGETLB_VMEMMAP on arm64 (no idea if there are now 
ways to move forward with that now, or if we are still not sure if we 
can actually add support), correct?


-- 
Cheers,

David / dhildenb

Re: [PATCH] hugetlbfs: add MTE support

[Yang Shi]

On 7/2/24 6:09 AM, David Hildenbrand wrote:
> On 02.07.24 14:34, Catalin Marinas wrote:
>> On Tue, Jun 25, 2024 at 04:37:17PM -0700, Yang Shi wrote:
>>> MTE can be supported on ram based filesystem. It is supported on tmpfs.
>>> There is use case to use MTE on hugetlbfs as well, adding MTE support.
>>>
>>> Signed-off-by: Yang Shi <yang@os.amperecomputing.com>
>>> ---
>>>   fs/hugetlbfs/inode.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
>>> index ecad73a4f713..c34faef62daf 100644
>>> --- a/fs/hugetlbfs/inode.c
>>> +++ b/fs/hugetlbfs/inode.c
>>> @@ -110,7 +110,7 @@ static int hugetlbfs_file_mmap(struct file 
>>> *file, struct vm_area_struct *vma)
>>>        * way when do_mmap unwinds (may be important on powerpc
>>>        * and ia64).
>>>        */
>>> -    vm_flags_set(vma, VM_HUGETLB | VM_DONTEXPAND);
>>> +    vm_flags_set(vma, VM_HUGETLB | VM_DONTEXPAND | VM_MTE_ALLOWED);
>>>       vma->vm_ops = &hugetlb_vm_ops;
>>
>> Last time I checked, about a year ago, this was not sufficient. One
>> issue is that there's no arch_clear_hugetlb_flags() implemented by your
>> patch, leaving PG_arch_{2,3} set on a page. The other issue was that I
>> initially tried to do this only on the head page but this did not go
>> well with the folio_copy() -> copy_highpage() which expects the
>> PG_arch_* flags on each individual page. The alternative was for
>> arch_clear_hugetlb_flags() to iterate over all the pages in a folio.
>
> This would likely also add a blocker for 
> ARCH_WANT_OPTIMIZE_HUGETLB_VMEMMAP on arm64 (no idea if there are now 
> ways to move forward with that now, or if we are still not sure if we 
> can actually add support), correct?

IIUC, it is not. We just need to guarantee each subpage has 
PG_mte_tagged flag and allocated tags. The HVO just maps the 7 vmemmap 
pages for sub pages to the first page, they still see the flag and the 
space for tag is not impacted, right? Did I miss something?

>
>

Re: [PATCH] hugetlbfs: add MTE support

[David Hildenbrand]

On 03.07.24 02:20, Yang Shi wrote:
> 
> 
> On 7/2/24 6:09 AM, David Hildenbrand wrote:
>> On 02.07.24 14:34, Catalin Marinas wrote:
>>> On Tue, Jun 25, 2024 at 04:37:17PM -0700, Yang Shi wrote:
>>>> MTE can be supported on ram based filesystem. It is supported on tmpfs.
>>>> There is use case to use MTE on hugetlbfs as well, adding MTE support.
>>>>
>>>> Signed-off-by: Yang Shi <yang@os.amperecomputing.com>
>>>> ---
>>>>    fs/hugetlbfs/inode.c | 2 +-
>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
>>>> index ecad73a4f713..c34faef62daf 100644
>>>> --- a/fs/hugetlbfs/inode.c
>>>> +++ b/fs/hugetlbfs/inode.c
>>>> @@ -110,7 +110,7 @@ static int hugetlbfs_file_mmap(struct file
>>>> *file, struct vm_area_struct *vma)
>>>>         * way when do_mmap unwinds (may be important on powerpc
>>>>         * and ia64).
>>>>         */
>>>> -    vm_flags_set(vma, VM_HUGETLB | VM_DONTEXPAND);
>>>> +    vm_flags_set(vma, VM_HUGETLB | VM_DONTEXPAND | VM_MTE_ALLOWED);
>>>>        vma->vm_ops = &hugetlb_vm_ops;
>>>
>>> Last time I checked, about a year ago, this was not sufficient. One
>>> issue is that there's no arch_clear_hugetlb_flags() implemented by your
>>> patch, leaving PG_arch_{2,3} set on a page. The other issue was that I
>>> initially tried to do this only on the head page but this did not go
>>> well with the folio_copy() -> copy_highpage() which expects the
>>> PG_arch_* flags on each individual page. The alternative was for
>>> arch_clear_hugetlb_flags() to iterate over all the pages in a folio.
>>
>> This would likely also add a blocker for
>> ARCH_WANT_OPTIMIZE_HUGETLB_VMEMMAP on arm64 (no idea if there are now
>> ways to move forward with that now, or if we are still not sure if we
>> can actually add support), correct?
> 
> IIUC, it is not. We just need to guarantee each subpage has
> PG_mte_tagged flag and allocated tags. The HVO just maps the 7 vmemmap
> pages for sub pages to the first page, they still see the flag and the
> space for tag is not impacted, right? Did I miss something?

In the R/O vmemmap optimization we won't be able to modify the flags of 
the double-mapped vmemmap pages via the double mappings.

Of course, we could find HVO-specific ways to only modify the flags of 
the first vmemmap page, but it does sound wrong ...

Really, the question is if we can have a per-folio flag for hugetlb 
instead and avoid all that?

-- 
Cheers,

David / dhildenb

Re: [PATCH] hugetlbfs: add MTE support

[Catalin Marinas]

On Wed, Jul 03, 2024 at 12:24:40PM +0200, David Hildenbrand wrote:
> On 03.07.24 02:20, Yang Shi wrote:
> > On 7/2/24 6:09 AM, David Hildenbrand wrote:
> > > On 02.07.24 14:34, Catalin Marinas wrote:
> > > > On Tue, Jun 25, 2024 at 04:37:17PM -0700, Yang Shi wrote:
> > > > > MTE can be supported on ram based filesystem. It is supported on tmpfs.
> > > > > There is use case to use MTE on hugetlbfs as well, adding MTE support.
> > > > > 
> > > > > Signed-off-by: Yang Shi <yang@os.amperecomputing.com>
> > > > > ---
> > > > >    fs/hugetlbfs/inode.c | 2 +-
> > > > >    1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
> > > > > index ecad73a4f713..c34faef62daf 100644
> > > > > --- a/fs/hugetlbfs/inode.c
> > > > > +++ b/fs/hugetlbfs/inode.c
> > > > > @@ -110,7 +110,7 @@ static int hugetlbfs_file_mmap(struct file
> > > > > *file, struct vm_area_struct *vma)
> > > > >         * way when do_mmap unwinds (may be important on powerpc
> > > > >         * and ia64).
> > > > >         */
> > > > > -    vm_flags_set(vma, VM_HUGETLB | VM_DONTEXPAND);
> > > > > +    vm_flags_set(vma, VM_HUGETLB | VM_DONTEXPAND | VM_MTE_ALLOWED);
> > > > >        vma->vm_ops = &hugetlb_vm_ops;
> > > > 
> > > > Last time I checked, about a year ago, this was not sufficient. One
> > > > issue is that there's no arch_clear_hugetlb_flags() implemented by your
> > > > patch, leaving PG_arch_{2,3} set on a page. The other issue was that I
> > > > initially tried to do this only on the head page but this did not go
> > > > well with the folio_copy() -> copy_highpage() which expects the
> > > > PG_arch_* flags on each individual page. The alternative was for
> > > > arch_clear_hugetlb_flags() to iterate over all the pages in a folio.
> > > 
> > > This would likely also add a blocker for
> > > ARCH_WANT_OPTIMIZE_HUGETLB_VMEMMAP on arm64 (no idea if there are now
> > > ways to move forward with that now, or if we are still not sure if we
> > > can actually add support), correct?
> > 
> > IIUC, it is not. We just need to guarantee each subpage has
> > PG_mte_tagged flag and allocated tags. The HVO just maps the 7 vmemmap
> > pages for sub pages to the first page, they still see the flag and the
> > space for tag is not impacted, right? Did I miss something?
> 
> In the R/O vmemmap optimization we won't be able to modify the flags of the
> double-mapped vmemmap pages via the double mappings.
> 
> Of course, we could find HVO-specific ways to only modify the flags of the
> first vmemmap page, but it does sound wrong ...
> 
> Really, the question is if we can have a per-folio flag for hugetlb instead
> and avoid all that?

I think it is possible and I have some half-baked changes but got
distracted and never completed. The only issue I came across was
folio_copy() calling copy_highpage() on individual pages that did not
have the original PG_mte_tagged (PG_arch_2) flag. To avoid some races,
we also use PG_mte_lock (PG_arch_3) as some form of locking but for
optimisation we don't clear this flag after copying the tags and setting
PG_mte_tagged. So doing the checks on the head page only confuses the
tail page copying.

Even if we use PG_arch_3 as a proper lock bit and clear it after
tag copying, I'll need to check whether this can race with any
mprotect(PROT_MTE) that could cause losing tags or leaking tags (not
initialising the pages). set_pte_at() relies on the PG_mte_tagged flag
to decide whether to initialise the tags. The arm64 hugetlbfs supports
contiguous ptes, so we'd get multiple set_pte_at() calls.

Anyway, I think with some care it is doable, I just did not have the
time, nor did I see anyone asking for such feature until now.

-- 
Catalin

Re: [PATCH] hugetlbfs: add MTE support

[Yang Shi]

On 7/2/24 5:34 AM, Catalin Marinas wrote:
> On Tue, Jun 25, 2024 at 04:37:17PM -0700, Yang Shi wrote:
>> MTE can be supported on ram based filesystem. It is supported on tmpfs.
>> There is use case to use MTE on hugetlbfs as well, adding MTE support.
>>
>> Signed-off-by: Yang Shi <yang@os.amperecomputing.com>
>> ---
>>   fs/hugetlbfs/inode.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
>> index ecad73a4f713..c34faef62daf 100644
>> --- a/fs/hugetlbfs/inode.c
>> +++ b/fs/hugetlbfs/inode.c
>> @@ -110,7 +110,7 @@ static int hugetlbfs_file_mmap(struct file *file, struct vm_area_struct *vma)
>>   	 * way when do_mmap unwinds (may be important on powerpc
>>   	 * and ia64).
>>   	 */
>> -	vm_flags_set(vma, VM_HUGETLB | VM_DONTEXPAND);
>> +	vm_flags_set(vma, VM_HUGETLB | VM_DONTEXPAND | VM_MTE_ALLOWED);
>>   	vma->vm_ops = &hugetlb_vm_ops;
> Last time I checked, about a year ago, this was not sufficient. One
> issue is that there's no arch_clear_hugetlb_flags() implemented by your
> patch, leaving PG_arch_{2,3} set on a page. The other issue was that I
> initially tried to do this only on the head page but this did not go
> well with the folio_copy() -> copy_highpage() which expects the
> PG_arch_* flags on each individual page. The alternative was for
> arch_clear_hugetlb_flags() to iterate over all the pages in a folio.

Thanks for pointing this out. I did miss this point. I took a quick look 
at when the PG_ flags are set. IIUC, it is set by post_alloc_hook() for 
order-0 anonymous folio (clearing page and tags) and set_ptes() for 
others (just clear tags), for example, THP and hugetlb.

I can see THP does set the PG_mte_tagged flag for each sub pages. But it 
seems it does not do it for hugetlb if I read the code correctly. The 
call path is:

hugetlb_fault() ->
   hugetlb_no_page->
     set_huge_pte_at ->
       __set_ptes() ->
         __sync_cache_and_tags() ->


The __set_ptes() is called in a loop:

if (!pte_present(pte)) {
         for (i = 0; i < ncontig; i++, ptep++, addr += pgsize)
             __set_ptes(mm, addr, ptep, pte, 1);
         return;
     }

The ncontig and pgsize are returned by num_contig_ptes(). For example, 
2M hugetlb, ncontig is 1 and pgsize is 2M IIUC. So it means actually 
just the head page has PG_mte_tagged set. If so the copy_highpage() will 
just copy the old head page's flag to the new head page, and the tag. 
All the sub pages don't have PG_mte_tagged set.


Is it expected behavior? I'm supposed we need tags for every sub pages 
too, right?

>
> I'd also like to see some tests added to
> tools/testing/selftest/arm64/mte to exercise MAP_HUGETLB with PROT_MTE:
> write/read tags, a series of mman+munmap (mostly to check if old page
> flags are still around), force some copy on write. I don't think we
> should merge the patch without proper tests.
>
> An untested hunk on top of your changes:
>
> diff --git a/arch/arm64/include/asm/hugetlb.h b/arch/arm64/include/asm/hugetlb.h
> index 3954cbd2ff56..5357b00b9087 100644
> --- a/arch/arm64/include/asm/hugetlb.h
> +++ b/arch/arm64/include/asm/hugetlb.h
> @@ -20,7 +20,19 @@ extern bool arch_hugetlb_migration_supported(struct hstate *h);
>   
>   static inline void arch_clear_hugetlb_flags(struct folio *folio)
>   {
> -	clear_bit(PG_dcache_clean, &folio->flags);
> +	unsigned long i, nr_pages = folio_nr_pages(folio);
> +	const unsigned long clear_flags = BIT(PG_dcache_clean) |
> +		BIT(PG_arch_2) | BIT(PG_arch_3);
> +
> +	if (!system_supports_mte()) {
> +		clear_bit(PG_dcache_clean, &folio->flags);
> +		return;
> +	}
> +
> +	for (i = 0; i < nr_pages; i++) {
> +		struct page *page = folio_page(folio, i);
> +		page->flags &= ~clear_flags;
> +	}
>   }
>   #define arch_clear_hugetlb_flags arch_clear_hugetlb_flags
>   
> diff --git a/arch/arm64/include/asm/mman.h b/arch/arm64/include/asm/mman.h
> index 5966ee4a6154..304dfc499e68 100644
> --- a/arch/arm64/include/asm/mman.h
> +++ b/arch/arm64/include/asm/mman.h
> @@ -28,7 +28,8 @@ static inline unsigned long arch_calc_vm_flag_bits(unsigned long flags)
>   	 * backed by tags-capable memory. The vm_flags may be overridden by a
>   	 * filesystem supporting MTE (RAM-based).
>   	 */
> -	if (system_supports_mte() && (flags & MAP_ANONYMOUS))
> +	if (system_supports_mte() &&
> +	    (flags & (MAP_ANONYMOUS | MAP_HUGETLB)))
>   		return VM_MTE_ALLOWED;

Do we really need this change? IIRC, the mmap_region() will call 
hugetlbfs's mmap and set VM_MTE_ALLOWED in vma->vm_flags, then update 
vma->vm_page_prot with the new vma->vm_flags.

If this is needed, MTE for tmpfs won't work, right?

>   
>   	return 0;
>

Re: [PATCH] hugetlbfs: add MTE support

[Yang Shi]

On 7/2/24 5:04 PM, Yang Shi wrote:
>
>
> On 7/2/24 5:34 AM, Catalin Marinas wrote:
>> On Tue, Jun 25, 2024 at 04:37:17PM -0700, Yang Shi wrote:
>>> MTE can be supported on ram based filesystem. It is supported on tmpfs.
>>> There is use case to use MTE on hugetlbfs as well, adding MTE support.
>>>
>>> Signed-off-by: Yang Shi <yang@os.amperecomputing.com>
>>> ---
>>>   fs/hugetlbfs/inode.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
>>> index ecad73a4f713..c34faef62daf 100644
>>> --- a/fs/hugetlbfs/inode.c
>>> +++ b/fs/hugetlbfs/inode.c
>>> @@ -110,7 +110,7 @@ static int hugetlbfs_file_mmap(struct file 
>>> *file, struct vm_area_struct *vma)
>>>        * way when do_mmap unwinds (may be important on powerpc
>>>        * and ia64).
>>>        */
>>> -    vm_flags_set(vma, VM_HUGETLB | VM_DONTEXPAND);
>>> +    vm_flags_set(vma, VM_HUGETLB | VM_DONTEXPAND | VM_MTE_ALLOWED);
>>>       vma->vm_ops = &hugetlb_vm_ops;
>> Last time I checked, about a year ago, this was not sufficient. One
>> issue is that there's no arch_clear_hugetlb_flags() implemented by your
>> patch, leaving PG_arch_{2,3} set on a page. The other issue was that I
>> initially tried to do this only on the head page but this did not go
>> well with the folio_copy() -> copy_highpage() which expects the
>> PG_arch_* flags on each individual page. The alternative was for
>> arch_clear_hugetlb_flags() to iterate over all the pages in a folio.
>
> Thanks for pointing this out. I did miss this point. I took a quick 
> look at when the PG_ flags are set. IIUC, it is set by 
> post_alloc_hook() for order-0 anonymous folio (clearing page and tags) 
> and set_ptes() for others (just clear tags), for example, THP and 
> hugetlb.
>
> I can see THP does set the PG_mte_tagged flag for each sub pages. But 
> it seems it does not do it for hugetlb if I read the code correctly. 
> The call path is:
>
> hugetlb_fault() ->
>   hugetlb_no_page->
>     set_huge_pte_at ->
>       __set_ptes() ->
>         __sync_cache_and_tags() ->
>
>
> The __set_ptes() is called in a loop:
>
> if (!pte_present(pte)) {
>         for (i = 0; i < ncontig; i++, ptep++, addr += pgsize)
>             __set_ptes(mm, addr, ptep, pte, 1);
>         return;
>     }
>
> The ncontig and pgsize are returned by num_contig_ptes(). For example, 
> 2M hugetlb, ncontig is 1 and pgsize is 2M IIUC. So it means actually 
> just the head page has PG_mte_tagged set. If so the copy_highpage() 
> will just copy the old head page's flag to the new head page, and the 
> tag. All the sub pages don't have PG_mte_tagged set.
>
>
> Is it expected behavior? I'm supposed we need tags for every sub pages 
> too, right?

We should need something like the below to have tags and page flag set 
up for each sub page:

diff --git a/arch/arm64/mm/hugetlbpage.c b/arch/arm64/mm/hugetlbpage.c
index 3f09ac73cce3..528164deef27 100644
--- a/arch/arm64/mm/hugetlbpage.c
+++ b/arch/arm64/mm/hugetlbpage.c
@@ -228,9 +228,12 @@ void set_huge_pte_at(struct mm_struct *mm, unsigned 
long addr,
         int ncontig;
         unsigned long pfn, dpfn;
         pgprot_t hugeprot;
+       unsigned long nr = sz >> PAGE_SHIFT;

         ncontig = num_contig_ptes(sz, &pgsize);

+       __sync_cache_and_tags(pte, nr);
+
         if (!pte_present(pte)) {
                 for (i = 0; i < ncontig; i++, ptep++, addr += pgsize)
                         __set_ptes(mm, addr, ptep, pte, 1);

>
>>
>> I'd also like to see some tests added to
>> tools/testing/selftest/arm64/mte to exercise MAP_HUGETLB with PROT_MTE:
>> write/read tags, a series of mman+munmap (mostly to check if old page
>> flags are still around), force some copy on write. I don't think we
>> should merge the patch without proper tests.
>>
>> An untested hunk on top of your changes:
>>
>> diff --git a/arch/arm64/include/asm/hugetlb.h 
>> b/arch/arm64/include/asm/hugetlb.h
>> index 3954cbd2ff56..5357b00b9087 100644
>> --- a/arch/arm64/include/asm/hugetlb.h
>> +++ b/arch/arm64/include/asm/hugetlb.h
>> @@ -20,7 +20,19 @@ extern bool 
>> arch_hugetlb_migration_supported(struct hstate *h);
>>     static inline void arch_clear_hugetlb_flags(struct folio *folio)
>>   {
>> -    clear_bit(PG_dcache_clean, &folio->flags);
>> +    unsigned long i, nr_pages = folio_nr_pages(folio);
>> +    const unsigned long clear_flags = BIT(PG_dcache_clean) |
>> +        BIT(PG_arch_2) | BIT(PG_arch_3);
>> +
>> +    if (!system_supports_mte()) {
>> +        clear_bit(PG_dcache_clean, &folio->flags);
>> +        return;
>> +    }
>> +
>> +    for (i = 0; i < nr_pages; i++) {
>> +        struct page *page = folio_page(folio, i);
>> +        page->flags &= ~clear_flags;
>> +    }
>>   }
>>   #define arch_clear_hugetlb_flags arch_clear_hugetlb_flags
>>   diff --git a/arch/arm64/include/asm/mman.h 
>> b/arch/arm64/include/asm/mman.h
>> index 5966ee4a6154..304dfc499e68 100644
>> --- a/arch/arm64/include/asm/mman.h
>> +++ b/arch/arm64/include/asm/mman.h
>> @@ -28,7 +28,8 @@ static inline unsigned long 
>> arch_calc_vm_flag_bits(unsigned long flags)
>>        * backed by tags-capable memory. The vm_flags may be 
>> overridden by a
>>        * filesystem supporting MTE (RAM-based).
>>        */
>> -    if (system_supports_mte() && (flags & MAP_ANONYMOUS))
>> +    if (system_supports_mte() &&
>> +        (flags & (MAP_ANONYMOUS | MAP_HUGETLB)))
>>           return VM_MTE_ALLOWED;
>
> Do we really need this change? IIRC, the mmap_region() will call 
> hugetlbfs's mmap and set VM_MTE_ALLOWED in vma->vm_flags, then update 
> vma->vm_page_prot with the new vma->vm_flags.
>
> If this is needed, MTE for tmpfs won't work, right?
>
>>         return 0;
>>
>

Re: [PATCH] hugetlbfs: add MTE support

[Catalin Marinas]

On Tue, Jul 02, 2024 at 05:15:12PM -0700, Yang Shi wrote:
> On 7/2/24 5:04 PM, Yang Shi wrote:
> > On 7/2/24 5:34 AM, Catalin Marinas wrote:
> > > Last time I checked, about a year ago, this was not sufficient. One
> > > issue is that there's no arch_clear_hugetlb_flags() implemented by your
> > > patch, leaving PG_arch_{2,3} set on a page. The other issue was that I
> > > initially tried to do this only on the head page but this did not go
> > > well with the folio_copy() -> copy_highpage() which expects the
> > > PG_arch_* flags on each individual page. The alternative was for
> > > arch_clear_hugetlb_flags() to iterate over all the pages in a folio.
> > 
> > Thanks for pointing this out. I did miss this point. I took a quick look
> > at when the PG_ flags are set. IIUC, it is set by post_alloc_hook() for
> > order-0 anonymous folio (clearing page and tags) and set_ptes() for
> > others (just clear tags), for example, THP and hugetlb.
> > 
> > I can see THP does set the PG_mte_tagged flag for each sub pages. But it
> > seems it does not do it for hugetlb if I read the code correctly. The
> > call path is:
> > 
> > hugetlb_fault() ->
> >   hugetlb_no_page->
> >     set_huge_pte_at ->
> >       __set_ptes() ->
> >         __sync_cache_and_tags() ->
> > 
> > 
> > The __set_ptes() is called in a loop:
> > 
> > if (!pte_present(pte)) {
> >         for (i = 0; i < ncontig; i++, ptep++, addr += pgsize)
> >             __set_ptes(mm, addr, ptep, pte, 1);
> >         return;
> >     }
> > 
> > The ncontig and pgsize are returned by num_contig_ptes(). For example,
> > 2M hugetlb, ncontig is 1 and pgsize is 2M IIUC. So it means actually
> > just the head page has PG_mte_tagged set. If so the copy_highpage() will
> > just copy the old head page's flag to the new head page, and the tag.
> > All the sub pages don't have PG_mte_tagged set.
> > 
> > Is it expected behavior? I'm supposed we need tags for every sub pages
> > too, right?
> 
> We should need something like the below to have tags and page flag set up
> for each sub page:
> 
> diff --git a/arch/arm64/mm/hugetlbpage.c b/arch/arm64/mm/hugetlbpage.c
> index 3f09ac73cce3..528164deef27 100644
> --- a/arch/arm64/mm/hugetlbpage.c
> +++ b/arch/arm64/mm/hugetlbpage.c
> @@ -228,9 +228,12 @@ void set_huge_pte_at(struct mm_struct *mm, unsigned
> long addr,
>         int ncontig;
>         unsigned long pfn, dpfn;
>         pgprot_t hugeprot;
> +       unsigned long nr = sz >> PAGE_SHIFT;
> 
>         ncontig = num_contig_ptes(sz, &pgsize);
> 
> +       __sync_cache_and_tags(pte, nr);
> +
>         if (!pte_present(pte)) {
>                 for (i = 0; i < ncontig; i++, ptep++, addr += pgsize)
>                         __set_ptes(mm, addr, ptep, pte, 1);

We only need this for the last loop when we have a present pte. I'd also
avoid calling __set_ptes(...., 1) if we call the __sync_cache_and_tags()
here already. Basically just unroll __set_ptes() in set_huge_pte_at()
and call __set_pte() directly.

It might be better to convert those page flag checks to only happen on
the head page. My stashed changes from over a year ago (before we had
more folio conversions) below. However, as I mentioned, I got stuck on
folio_copy() which also does a cond_resched() between copy_highpage().

---------8<--------------------------------
diff --git a/arch/arm64/kernel/mte.c b/arch/arm64/kernel/mte.c
index f5bcb0dc6267..a84ad0e46f12 100644
--- a/arch/arm64/kernel/mte.c
+++ b/arch/arm64/kernel/mte.c
@@ -49,7 +49,9 @@ static void mte_sync_page_tags(struct page *page, pte_t old_pte,
 		return;
 
 	if (try_page_mte_tagging(page)) {
-		mte_clear_page_tags(page_address(page));
+		unsigned long i, nr_pages = compound_nr(page);
+		for (i = 0; i < nr_pages; i++)
+			mte_clear_page_tags(page_address(page + i));
 		set_page_mte_tagged(page);
 	}
 }
@@ -57,22 +59,23 @@ static void mte_sync_page_tags(struct page *page, pte_t old_pte,
 void mte_sync_tags(pte_t old_pte, pte_t pte)
 {
 	struct page *page = pte_page(pte);
-	long i, nr_pages = compound_nr(page);
-	bool check_swap = nr_pages == 1;
+	bool check_swap = !PageCompound(page);
 	bool pte_is_tagged = pte_tagged(pte);
 
 	/* Early out if there's nothing to do */
 	if (!check_swap && !pte_is_tagged)
 		return;
 
+	/*
+	 * HugeTLB pages are always fully mapped, so only setting head page's
+	 * PG_mte_* flags is enough.
+	 */
+	if (PageHuge(page))
+		page = compound_head(page);
+
 	/* if PG_mte_tagged is set, tags have already been initialised */
-	for (i = 0; i < nr_pages; i++, page++) {
-		if (!page_mte_tagged(page)) {
-			mte_sync_page_tags(page, old_pte, check_swap,
-					   pte_is_tagged);
-			set_page_mte_tagged(page);
-		}
-	}
+	if (!page_mte_tagged(page))
+		mte_sync_page_tags(page, old_pte, check_swap, pte_is_tagged);
 
 	/* ensure the tags are visible before the PTE is set */
 	smp_wmb();
diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c
index 5626ddb540ce..692198d8c0d2 100644
--- a/arch/arm64/kvm/guest.c
+++ b/arch/arm64/kvm/guest.c
@@ -1079,7 +1079,7 @@ long kvm_vm_ioctl_mte_copy_tags(struct kvm *kvm,
 
 			/* uaccess failed, don't leave stale tags */
 			if (num_tags != MTE_GRANULES_PER_PAGE)
-				mte_clear_page_tags(page);
+				mte_clear_page_tags(page_address(page));
 			set_page_mte_tagged(page);
 
 			kvm_release_pfn_dirty(pfn);
diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
index 31d7fa4c7c14..b531b1d75cda 100644
--- a/arch/arm64/kvm/mmu.c
+++ b/arch/arm64/kvm/mmu.c
@@ -1173,11 +1173,10 @@ static void sanitise_mte_tags(struct kvm *kvm, kvm_pfn_t pfn,
 	if (!kvm_has_mte(kvm))
 		return;
 
-	for (i = 0; i < nr_pages; i++, page++) {
-		if (try_page_mte_tagging(page)) {
-			mte_clear_page_tags(page_address(page));
-			set_page_mte_tagged(page);
-		}
+	if (try_page_mte_tagging(page)) {
+		for (i = 0; i < nr_pages; i++)
+			mte_clear_page_tags(page_address(page + i));
+		set_page_mte_tagged(page);
 	}
 }
 

-- 
Catalin

Re: [PATCH] hugetlbfs: add MTE support

[Yang Shi]

On 7/4/24 6:44 AM, Catalin Marinas wrote:
> On Tue, Jul 02, 2024 at 05:15:12PM -0700, Yang Shi wrote:
>> On 7/2/24 5:04 PM, Yang Shi wrote:
>>> On 7/2/24 5:34 AM, Catalin Marinas wrote:
>>>> Last time I checked, about a year ago, this was not sufficient. One
>>>> issue is that there's no arch_clear_hugetlb_flags() implemented by your
>>>> patch, leaving PG_arch_{2,3} set on a page. The other issue was that I
>>>> initially tried to do this only on the head page but this did not go
>>>> well with the folio_copy() -> copy_highpage() which expects the
>>>> PG_arch_* flags on each individual page. The alternative was for
>>>> arch_clear_hugetlb_flags() to iterate over all the pages in a folio.
>>> Thanks for pointing this out. I did miss this point. I took a quick look
>>> at when the PG_ flags are set. IIUC, it is set by post_alloc_hook() for
>>> order-0 anonymous folio (clearing page and tags) and set_ptes() for
>>> others (just clear tags), for example, THP and hugetlb.
>>>
>>> I can see THP does set the PG_mte_tagged flag for each sub pages. But it
>>> seems it does not do it for hugetlb if I read the code correctly. The
>>> call path is:
>>>
>>> hugetlb_fault() ->
>>>    hugetlb_no_page->
>>>      set_huge_pte_at ->
>>>        __set_ptes() ->
>>>          __sync_cache_and_tags() ->
>>>
>>>
>>> The __set_ptes() is called in a loop:
>>>
>>> if (!pte_present(pte)) {
>>>          for (i = 0; i < ncontig; i++, ptep++, addr += pgsize)
>>>              __set_ptes(mm, addr, ptep, pte, 1);
>>>          return;
>>>      }
>>>
>>> The ncontig and pgsize are returned by num_contig_ptes(). For example,
>>> 2M hugetlb, ncontig is 1 and pgsize is 2M IIUC. So it means actually
>>> just the head page has PG_mte_tagged set. If so the copy_highpage() will
>>> just copy the old head page's flag to the new head page, and the tag.
>>> All the sub pages don't have PG_mte_tagged set.
>>>
>>> Is it expected behavior? I'm supposed we need tags for every sub pages
>>> too, right?
>> We should need something like the below to have tags and page flag set up
>> for each sub page:
>>
>> diff --git a/arch/arm64/mm/hugetlbpage.c b/arch/arm64/mm/hugetlbpage.c
>> index 3f09ac73cce3..528164deef27 100644
>> --- a/arch/arm64/mm/hugetlbpage.c
>> +++ b/arch/arm64/mm/hugetlbpage.c
>> @@ -228,9 +228,12 @@ void set_huge_pte_at(struct mm_struct *mm, unsigned
>> long addr,
>>          int ncontig;
>>          unsigned long pfn, dpfn;
>>          pgprot_t hugeprot;
>> +       unsigned long nr = sz >> PAGE_SHIFT;
>>
>>          ncontig = num_contig_ptes(sz, &pgsize);
>>
>> +       __sync_cache_and_tags(pte, nr);
>> +
>>          if (!pte_present(pte)) {
>>                  for (i = 0; i < ncontig; i++, ptep++, addr += pgsize)
>>                          __set_ptes(mm, addr, ptep, pte, 1);
> We only need this for the last loop when we have a present pte. I'd also
> avoid calling __set_ptes(...., 1) if we call the __sync_cache_and_tags()
> here already. Basically just unroll __set_ptes() in set_huge_pte_at()
> and call __set_pte() directly.
>
> It might be better to convert those page flag checks to only happen on
> the head page. My stashed changes from over a year ago (before we had
> more folio conversions) below. However, as I mentioned, I got stuck on
> folio_copy() which also does a cond_resched() between copy_highpage().

We can have the page flags set for head only for hugetlb page. For 
copy_highpage(), we should be able to do something like the below:

if  page_is_head && page_is_hugetlb && page_has_mte_tagged
     set page_mte_tagged flags
     copy tags for all sub pages
else // <-- tail page or non-hugetlb page
     current copy_highpage implementation


The hugetlb folio can't go away under us since migration path should pin 
it so the status of folio is stable. The preemption caused by 
cond_resched() should be fine too due to the pin and the page table 
entry keeps being migration entry until migration is done, so every one 
should just see migration entry and wait for migration is done.

The other concerned user of copy_highpage() is uprobe, but it also pins 
the page then doing copy and it is called with holding write mmap_lock.

IIUC, it should work if I don't miss something. This also should have no 
impact on HVO. The overhead for other users of copy_highpage() should be 
also acceptable.

>
> ---------8<--------------------------------
> diff --git a/arch/arm64/kernel/mte.c b/arch/arm64/kernel/mte.c
> index f5bcb0dc6267..a84ad0e46f12 100644
> --- a/arch/arm64/kernel/mte.c
> +++ b/arch/arm64/kernel/mte.c
> @@ -49,7 +49,9 @@ static void mte_sync_page_tags(struct page *page, pte_t old_pte,
>   		return;
>   
>   	if (try_page_mte_tagging(page)) {
> -		mte_clear_page_tags(page_address(page));
> +		unsigned long i, nr_pages = compound_nr(page);
> +		for (i = 0; i < nr_pages; i++)
> +			mte_clear_page_tags(page_address(page + i));
>   		set_page_mte_tagged(page);
>   	}
>   }
> @@ -57,22 +59,23 @@ static void mte_sync_page_tags(struct page *page, pte_t old_pte,
>   void mte_sync_tags(pte_t old_pte, pte_t pte)
>   {
>   	struct page *page = pte_page(pte);
> -	long i, nr_pages = compound_nr(page);
> -	bool check_swap = nr_pages == 1;
> +	bool check_swap = !PageCompound(page);
>   	bool pte_is_tagged = pte_tagged(pte);
>   
>   	/* Early out if there's nothing to do */
>   	if (!check_swap && !pte_is_tagged)
>   		return;
>   
> +	/*
> +	 * HugeTLB pages are always fully mapped, so only setting head page's
> +	 * PG_mte_* flags is enough.
> +	 */
> +	if (PageHuge(page))
> +		page = compound_head(page);
> +
>   	/* if PG_mte_tagged is set, tags have already been initialised */
> -	for (i = 0; i < nr_pages; i++, page++) {
> -		if (!page_mte_tagged(page)) {
> -			mte_sync_page_tags(page, old_pte, check_swap,
> -					   pte_is_tagged);
> -			set_page_mte_tagged(page);
> -		}
> -	}
> +	if (!page_mte_tagged(page))
> +		mte_sync_page_tags(page, old_pte, check_swap, pte_is_tagged);
>   
>   	/* ensure the tags are visible before the PTE is set */
>   	smp_wmb();
> diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c
> index 5626ddb540ce..692198d8c0d2 100644
> --- a/arch/arm64/kvm/guest.c
> +++ b/arch/arm64/kvm/guest.c
> @@ -1079,7 +1079,7 @@ long kvm_vm_ioctl_mte_copy_tags(struct kvm *kvm,
>   
>   			/* uaccess failed, don't leave stale tags */
>   			if (num_tags != MTE_GRANULES_PER_PAGE)
> -				mte_clear_page_tags(page);
> +				mte_clear_page_tags(page_address(page));
>   			set_page_mte_tagged(page);
>   
>   			kvm_release_pfn_dirty(pfn);
> diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
> index 31d7fa4c7c14..b531b1d75cda 100644
> --- a/arch/arm64/kvm/mmu.c
> +++ b/arch/arm64/kvm/mmu.c
> @@ -1173,11 +1173,10 @@ static void sanitise_mte_tags(struct kvm *kvm, kvm_pfn_t pfn,
>   	if (!kvm_has_mte(kvm))
>   		return;
>   
> -	for (i = 0; i < nr_pages; i++, page++) {
> -		if (try_page_mte_tagging(page)) {
> -			mte_clear_page_tags(page_address(page));
> -			set_page_mte_tagged(page);
> -		}
> +	if (try_page_mte_tagging(page)) {
> +		for (i = 0; i < nr_pages; i++)
> +			mte_clear_page_tags(page_address(page + i));
> +		set_page_mte_tagged(page);
>   	}
>   }
>   
>

Re: [PATCH] hugetlbfs: add MTE support

[Yang Shi]

On 7/9/24 10:42 AM, Yang Shi wrote:
>
>
> On 7/4/24 6:44 AM, Catalin Marinas wrote:
>> On Tue, Jul 02, 2024 at 05:15:12PM -0700, Yang Shi wrote:
>>> On 7/2/24 5:04 PM, Yang Shi wrote:
>>>> On 7/2/24 5:34 AM, Catalin Marinas wrote:
>>>>> Last time I checked, about a year ago, this was not sufficient. One
>>>>> issue is that there's no arch_clear_hugetlb_flags() implemented by 
>>>>> your
>>>>> patch, leaving PG_arch_{2,3} set on a page. The other issue was 
>>>>> that I
>>>>> initially tried to do this only on the head page but this did not go
>>>>> well with the folio_copy() -> copy_highpage() which expects the
>>>>> PG_arch_* flags on each individual page. The alternative was for
>>>>> arch_clear_hugetlb_flags() to iterate over all the pages in a folio.
>>>> Thanks for pointing this out. I did miss this point. I took a quick 
>>>> look
>>>> at when the PG_ flags are set. IIUC, it is set by post_alloc_hook() 
>>>> for
>>>> order-0 anonymous folio (clearing page and tags) and set_ptes() for
>>>> others (just clear tags), for example, THP and hugetlb.
>>>>
>>>> I can see THP does set the PG_mte_tagged flag for each sub pages. 
>>>> But it
>>>> seems it does not do it for hugetlb if I read the code correctly. The
>>>> call path is:
>>>>
>>>> hugetlb_fault() ->
>>>>    hugetlb_no_page->
>>>>      set_huge_pte_at ->
>>>>        __set_ptes() ->
>>>>          __sync_cache_and_tags() ->
>>>>
>>>>
>>>> The __set_ptes() is called in a loop:
>>>>
>>>> if (!pte_present(pte)) {
>>>>          for (i = 0; i < ncontig; i++, ptep++, addr += pgsize)
>>>>              __set_ptes(mm, addr, ptep, pte, 1);
>>>>          return;
>>>>      }
>>>>
>>>> The ncontig and pgsize are returned by num_contig_ptes(). For example,
>>>> 2M hugetlb, ncontig is 1 and pgsize is 2M IIUC. So it means actually
>>>> just the head page has PG_mte_tagged set. If so the copy_highpage() 
>>>> will
>>>> just copy the old head page's flag to the new head page, and the tag.
>>>> All the sub pages don't have PG_mte_tagged set.
>>>>
>>>> Is it expected behavior? I'm supposed we need tags for every sub pages
>>>> too, right?
>>> We should need something like the below to have tags and page flag 
>>> set up
>>> for each sub page:
>>>
>>> diff --git a/arch/arm64/mm/hugetlbpage.c b/arch/arm64/mm/hugetlbpage.c
>>> index 3f09ac73cce3..528164deef27 100644
>>> --- a/arch/arm64/mm/hugetlbpage.c
>>> +++ b/arch/arm64/mm/hugetlbpage.c
>>> @@ -228,9 +228,12 @@ void set_huge_pte_at(struct mm_struct *mm, 
>>> unsigned
>>> long addr,
>>>          int ncontig;
>>>          unsigned long pfn, dpfn;
>>>          pgprot_t hugeprot;
>>> +       unsigned long nr = sz >> PAGE_SHIFT;
>>>
>>>          ncontig = num_contig_ptes(sz, &pgsize);
>>>
>>> +       __sync_cache_and_tags(pte, nr);
>>> +
>>>          if (!pte_present(pte)) {
>>>                  for (i = 0; i < ncontig; i++, ptep++, addr += pgsize)
>>>                          __set_ptes(mm, addr, ptep, pte, 1);
>> We only need this for the last loop when we have a present pte. I'd also
>> avoid calling __set_ptes(...., 1) if we call the __sync_cache_and_tags()
>> here already. Basically just unroll __set_ptes() in set_huge_pte_at()
>> and call __set_pte() directly.
>>
>> It might be better to convert those page flag checks to only happen on
>> the head page. My stashed changes from over a year ago (before we had
>> more folio conversions) below. However, as I mentioned, I got stuck on
>> folio_copy() which also does a cond_resched() between copy_highpage().
>

Ping...

Does this make sense and sound good? If is is good, I will try to 
implement it.

> We can have the page flags set for head only for hugetlb page. For 
> copy_highpage(), we should be able to do something like the below:
>
> if  page_is_head && page_is_hugetlb && page_has_mte_tagged
>     set page_mte_tagged flags
>     copy tags for all sub pages
> else // <-- tail page or non-hugetlb page
>     current copy_highpage implementation
>
>
> The hugetlb folio can't go away under us since migration path should 
> pin it so the status of folio is stable. The preemption caused by 
> cond_resched() should be fine too due to the pin and the page table 
> entry keeps being migration entry until migration is done, so every 
> one should just see migration entry and wait for migration is done.
>
> The other concerned user of copy_highpage() is uprobe, but it also 
> pins the page then doing copy and it is called with holding write 
> mmap_lock.
>
> IIUC, it should work if I don't miss something. This also should have 
> no impact on HVO. The overhead for other users of copy_highpage() 
> should be also acceptable.
>
>>
>> ---------8<--------------------------------
>> diff --git a/arch/arm64/kernel/mte.c b/arch/arm64/kernel/mte.c
>> index f5bcb0dc6267..a84ad0e46f12 100644
>> --- a/arch/arm64/kernel/mte.c
>> +++ b/arch/arm64/kernel/mte.c
>> @@ -49,7 +49,9 @@ static void mte_sync_page_tags(struct page *page, 
>> pte_t old_pte,
>>           return;
>>         if (try_page_mte_tagging(page)) {
>> -        mte_clear_page_tags(page_address(page));
>> +        unsigned long i, nr_pages = compound_nr(page);
>> +        for (i = 0; i < nr_pages; i++)
>> +            mte_clear_page_tags(page_address(page + i));
>>           set_page_mte_tagged(page);
>>       }
>>   }
>> @@ -57,22 +59,23 @@ static void mte_sync_page_tags(struct page *page, 
>> pte_t old_pte,
>>   void mte_sync_tags(pte_t old_pte, pte_t pte)
>>   {
>>       struct page *page = pte_page(pte);
>> -    long i, nr_pages = compound_nr(page);
>> -    bool check_swap = nr_pages == 1;
>> +    bool check_swap = !PageCompound(page);
>>       bool pte_is_tagged = pte_tagged(pte);
>>         /* Early out if there's nothing to do */
>>       if (!check_swap && !pte_is_tagged)
>>           return;
>>   +    /*
>> +     * HugeTLB pages are always fully mapped, so only setting head 
>> page's
>> +     * PG_mte_* flags is enough.
>> +     */
>> +    if (PageHuge(page))
>> +        page = compound_head(page);
>> +
>>       /* if PG_mte_tagged is set, tags have already been initialised */
>> -    for (i = 0; i < nr_pages; i++, page++) {
>> -        if (!page_mte_tagged(page)) {
>> -            mte_sync_page_tags(page, old_pte, check_swap,
>> -                       pte_is_tagged);
>> -            set_page_mte_tagged(page);
>> -        }
>> -    }
>> +    if (!page_mte_tagged(page))
>> +        mte_sync_page_tags(page, old_pte, check_swap, pte_is_tagged);
>>         /* ensure the tags are visible before the PTE is set */
>>       smp_wmb();
>> diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c
>> index 5626ddb540ce..692198d8c0d2 100644
>> --- a/arch/arm64/kvm/guest.c
>> +++ b/arch/arm64/kvm/guest.c
>> @@ -1079,7 +1079,7 @@ long kvm_vm_ioctl_mte_copy_tags(struct kvm *kvm,
>>                 /* uaccess failed, don't leave stale tags */
>>               if (num_tags != MTE_GRANULES_PER_PAGE)
>> -                mte_clear_page_tags(page);
>> +                mte_clear_page_tags(page_address(page));
>>               set_page_mte_tagged(page);
>>                 kvm_release_pfn_dirty(pfn);
>> diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
>> index 31d7fa4c7c14..b531b1d75cda 100644
>> --- a/arch/arm64/kvm/mmu.c
>> +++ b/arch/arm64/kvm/mmu.c
>> @@ -1173,11 +1173,10 @@ static void sanitise_mte_tags(struct kvm 
>> *kvm, kvm_pfn_t pfn,
>>       if (!kvm_has_mte(kvm))
>>           return;
>>   -    for (i = 0; i < nr_pages; i++, page++) {
>> -        if (try_page_mte_tagging(page)) {
>> -            mte_clear_page_tags(page_address(page));
>> -            set_page_mte_tagged(page);
>> -        }
>> +    if (try_page_mte_tagging(page)) {
>> +        for (i = 0; i < nr_pages; i++)
>> +            mte_clear_page_tags(page_address(page + i));
>> +        set_page_mte_tagged(page);
>>       }
>>   }
>>
>

Re: [PATCH] hugetlbfs: add MTE support

[Catalin Marinas]

Sorry for the delay (holidays etc.)

On Tue, Jul 09, 2024 at 10:42:58AM -0700, Yang Shi wrote:
> On 7/4/24 6:44 AM, Catalin Marinas wrote:
> > It might be better to convert those page flag checks to only happen on
> > the head page. My stashed changes from over a year ago (before we had
> > more folio conversions) below. However, as I mentioned, I got stuck on
> > folio_copy() which also does a cond_resched() between copy_highpage().
> 
> We can have the page flags set for head only for hugetlb page. For
> copy_highpage(), we should be able to do something like the below:
> 
> if  page_is_head && page_is_hugetlb && page_has_mte_tagged
>     set page_mte_tagged flags
>     copy tags for all sub pages
> else // <-- tail page or non-hugetlb page
>     current copy_highpage implementation

Ah, so you want in the first copy_highpage() for the head page to
populate the tags for the tail pages. I guess this would work.

> The hugetlb folio can't go away under us since migration path should pin it
> so the status of folio is stable. The preemption caused by cond_resched()
> should be fine too due to the pin and the page table entry keeps being
> migration entry until migration is done, so every one should just see
> migration entry and wait for migration is done.

Yeah, I don't see those pages going away, otherwise folio_copy() would
corrupt data.

> The other concerned user of copy_highpage() is uprobe, but it also pins the
> page then doing copy and it is called with holding write mmap_lock.
> 
> IIUC, it should work if I don't miss something. This also should have no
> impact on HVO. The overhead for other users of copy_highpage() should be
> also acceptable.

I also think so. We also have the copy_user_highpage() on arm64 that
calls copy_highpage() but I think that's also safe.

-- 
Catalin

Re: [PATCH] hugetlbfs: add MTE support

[Yang Shi]

On 8/15/24 3:31 AM, Catalin Marinas wrote:
> Sorry for the delay (holidays etc.)
>
> On Tue, Jul 09, 2024 at 10:42:58AM -0700, Yang Shi wrote:
>> On 7/4/24 6:44 AM, Catalin Marinas wrote:
>>> It might be better to convert those page flag checks to only happen on
>>> the head page. My stashed changes from over a year ago (before we had
>>> more folio conversions) below. However, as I mentioned, I got stuck on
>>> folio_copy() which also does a cond_resched() between copy_highpage().
>> We can have the page flags set for head only for hugetlb page. For
>> copy_highpage(), we should be able to do something like the below:
>>
>> if  page_is_head && page_is_hugetlb && page_has_mte_tagged
>>      set page_mte_tagged flags
>>      copy tags for all sub pages
>> else // <-- tail page or non-hugetlb page
>>      current copy_highpage implementation
> Ah, so you want in the first copy_highpage() for the head page to
> populate the tags for the tail pages. I guess this would work.

Yes, because we know the order of hugetlb page so we know how many tail 
pages we need populate tags for.

A deeper look showed this may be the only way to do it (if we want to 
have mte page flag for head only) because process_huge_page() may 
starting copy from the middle of huge page to have hot sub pages in 
cache. The process_huge_page() is used by hugetlb fork and COW. I think 
it is safe to do so in fork and COW path too since the destination 
hugetlb page won't be seen by the users until fork or COW fault is done.

>
>> The hugetlb folio can't go away under us since migration path should pin it
>> so the status of folio is stable. The preemption caused by cond_resched()
>> should be fine too due to the pin and the page table entry keeps being
>> migration entry until migration is done, so every one should just see
>> migration entry and wait for migration is done.
> Yeah, I don't see those pages going away, otherwise folio_copy() would
> corrupt data.
>
>> The other concerned user of copy_highpage() is uprobe, but it also pins the
>> page then doing copy and it is called with holding write mmap_lock.
>>
>> IIUC, it should work if I don't miss something. This also should have no
>> impact on HVO. The overhead for other users of copy_highpage() should be
>> also acceptable.
> I also think so. We also have the copy_user_highpage() on arm64 that
> calls copy_highpage() but I think that's also safe.

Yes, it is used by fork and COW fault by hugetlb.

>