Re: [PATCH] hugetlbfs: add MTE support

[Yang Shi <yang@os.amperecomputing.com>]

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;
>>
>