Re: [PATCH] support "THPeligible" semantics for mTHP with anonymous shmem

[Ryan Roberts <ryan.roberts@arm.com>]

On 01/07/2024 09:33, Baolin Wang wrote:
> 
> 
> On 2024/7/1 15:55, Ryan Roberts wrote:
>> On 28/06/2024 11:49, Bang Li wrote:
>>> After the commit 7fb1b252afb5 ("mm: shmem: add mTHP support for
>>> anonymous shmem"), we can configure different policies through
>>> the multi-size THP sysfs interface for anonymous shmem. But
>>> currently "THPeligible" indicates only whether the mapping is
>>> eligible for allocating THP-pages as well as the THP is PMD
>>> mappable or not for anonymous shmem, we need to support semantics
>>> for mTHP with anonymous shmem similar to those for mTHP with
>>> anonymous memory.
>>>
>>> Signed-off-by: Bang Li <libang.li@antgroup.com>
>>> ---
>>>   fs/proc/task_mmu.c      | 10 +++++++---
>>>   include/linux/huge_mm.h | 11 +++++++++++
>>>   mm/shmem.c              |  9 +--------
>>>   3 files changed, 19 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
>>> index 93fb2c61b154..09b5db356886 100644
>>> --- a/fs/proc/task_mmu.c
>>> +++ b/fs/proc/task_mmu.c
>>> @@ -870,6 +870,7 @@ static int show_smap(struct seq_file *m, void *v)
>>>   {
>>>       struct vm_area_struct *vma = v;
>>>       struct mem_size_stats mss = {};
>>> +    bool thp_eligible;
>>>         smap_gather_stats(vma, &mss, 0);
>>>   @@ -882,9 +883,12 @@ static int show_smap(struct seq_file *m, void *v)
>>>         __show_smap(m, &mss, false);
>>>   -    seq_printf(m, "THPeligible:    %8u\n",
>>> -           !!thp_vma_allowable_orders(vma, vma->vm_flags,
>>> -               TVA_SMAPS | TVA_ENFORCE_SYSFS, THP_ORDERS_ALL));
>>> +    thp_eligible = !!thp_vma_allowable_orders(vma, vma->vm_flags,
>>> +                        TVA_SMAPS | TVA_ENFORCE_SYSFS, THP_ORDERS_ALL);
>>> +    if (vma_is_anon_shmem(vma))
>>> +        thp_eligible = !!shmem_allowable_huge_orders(file_inode(vma->vm_file),
>>> +                            vma, vma->vm_pgoff, thp_eligible);
>>
>> Afraid I haven't been following the shmem mTHP support work as much as I would
>> have liked, but is there a reason why we need a separate function for shmem?
> 
> Since shmem_allowable_huge_orders() only uses shmem specific logic to determine
> if huge orders are allowable, there is no need to complicate the
> thp_vma_allowable_orders() function by adding more shmem related logic, making
> it more bloated. In my view, providing a dedicated helper
> shmem_allowable_huge_orders(), specifically for shmem, simplifies the logic.

My point was really that a single interface (thp_vma_allowable_orders) should be
used to get this information. I have no strong opinon on how the implementation
of that interface looks. What you suggest below seems perfectly reasonable to me.

> 
> IIUC, I agree with David's suggestion that the shmem_allowable_huge_orders()
> helper function could be used in thp_vma_allowable_orders() to support shmem
> mTHP. Something like:

I hadn't seen David's suggestion until after I sent my mail. But I think we are
both suggesting the same thing.

> 
> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> index c7ce28f6b7f3..9677fe6cf478 100644
> --- a/mm/huge_memory.c
> +++ b/mm/huge_memory.c
> @@ -151,10 +151,13 @@ unsigned long __thp_vma_allowable_orders(struct
> vm_area_struct *vma,
>          * Must be done before hugepage flags check since shmem has its
>          * own flags.
>          */
> -       if (!in_pf && shmem_file(vma->vm_file))
> -               return shmem_is_huge(file_inode(vma->vm_file), vma->vm_pgoff,
> -                                    !enforce_sysfs, vma->vm_mm, vm_flags)
> -                       ? orders : 0;
> +       if (!in_pf && shmem_file(vma->vm_file)) {
> +               bool global_huge = shmem_is_huge(file_inode(vma->vm_file),
> vma->vm_pgoff,
> +                                    !enforce_sysfs, vma->vm_mm, vm_flags);
> +
> +               return shmem_allowable_huge_orders(file_inode(vma->vm_file),
> +                                       vma, vma->vm_pgoff, global_huge);
> +       }
> 
>         if (!vma_is_anonymous(vma)) {
>                 /*
> 
>> Couldn't (shouldn't) thp_vma_allowable_orders() be taught to handle shmem too?
>>
>>> +    seq_printf(m, "THPeligible:    %8u\n", thp_eligible);
>>>         if (arch_pkeys_enabled())
>>>           seq_printf(m, "ProtectionKey:  %8u\n", vma_pkey(vma));
>>> diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h
>>> index 212cca384d7e..f87136f38aa1 100644
>>> --- a/include/linux/huge_mm.h
>>> +++ b/include/linux/huge_mm.h
>>> @@ -267,6 +267,10 @@ unsigned long thp_vma_allowable_orders(struct
>>> vm_area_struct *vma,
>>>       return __thp_vma_allowable_orders(vma, vm_flags, tva_flags, orders);
>>>   }
>>>   +unsigned long shmem_allowable_huge_orders(struct inode *inode,
>>> +                struct vm_area_struct *vma, pgoff_t index,
>>> +                bool global_huge);
>>> +
>>>   struct thpsize {
>>>       struct kobject kobj;
>>>       struct list_head node;
>>> @@ -460,6 +464,13 @@ static inline unsigned long
>>> thp_vma_allowable_orders(struct vm_area_struct *vma,
>>>       return 0;
>>>   }
>>>   +static inline unsigned long shmem_allowable_huge_orders(struct inode *inode,
>>> +                struct vm_area_struct *vma, pgoff_t index,
>>> +                bool global_huge)
>>> +{
>>> +    return 0;
>>> +}
>>> +
>>>   #define transparent_hugepage_flags 0UL
>>>     #define thp_get_unmapped_area    NULL
>>> diff --git a/mm/shmem.c b/mm/shmem.c
>>> index d495c0701a83..aa85df9c662a 100644
>>> --- a/mm/shmem.c
>>> +++ b/mm/shmem.c
>>> @@ -1622,7 +1622,7 @@ static gfp_t limit_gfp_mask(gfp_t huge_gfp, gfp_t
>>> limit_gfp)
>>>   }
>>>     #ifdef CONFIG_TRANSPARENT_HUGEPAGE
>>> -static unsigned long shmem_allowable_huge_orders(struct inode *inode,
>>> +unsigned long shmem_allowable_huge_orders(struct inode *inode,
>>>                   struct vm_area_struct *vma, pgoff_t index,
>>>                   bool global_huge)
>>>   {
>>> @@ -1707,13 +1707,6 @@ static unsigned long shmem_suitable_orders(struct
>>> inode *inode, struct vm_fault
>>>       return orders;
>>>   }
>>>   #else
>>> -static unsigned long shmem_allowable_huge_orders(struct inode *inode,
>>> -                struct vm_area_struct *vma, pgoff_t index,
>>> -                bool global_huge)
>>> -{
>>> -    return 0;
>>> -}
>>> -
>>>   static unsigned long shmem_suitable_orders(struct inode *inode, struct
>>> vm_fault *vmf,
>>>                          struct address_space *mapping, pgoff_t index,
>>>                          unsigned long orders)