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

["Bang Li" <libang.li@antgroup.com>]

Hi Ryan,

On 2024/7/4 17:46, Ryan Roberts wrote:
> On 03/07/2024 17:02, Bang Li wrote:
>> On 2024/7/3 18:25, Ryan Roberts wrote:
>>> On 03/07/2024 08:33, Bang Li wrote:
>>>> Hi Ryan,
>>>>
>>>> Thanks for the review!
>>>>
>>>> On 2024/7/2 16:18, Ryan Roberts wrote:
>>>>> On 02/07/2024 03:34, 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>
>>>>>> ---
>>>>>> Changes since v1 [1]:
>>>>>>     - Put anonymous shmem mthp related logic into
>>>>>>       thp_vma_allowable_orders() (per David)
>>>>>>
>>>>>> [1]
>>>>>> https://lore.kernel.org/linux-mm/20240628104926.34209-1-libang.li@antgroup.com/
>>>>>> ---
>>>>>>     include/linux/huge_mm.h | 11 +++++++++++
>>>>>>     mm/huge_memory.c        | 13 +++++++++----
>>>>>>     mm/shmem.c              |  9 +--------
>>>>>>     3 files changed, 21 insertions(+), 12 deletions(-)
>>>>>>
>>>>>> 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/huge_memory.c b/mm/huge_memory.c
>>>>>> index c7ce28f6b7f3..ea377bb4af91 100644
>>>>>> --- a/mm/huge_memory.c
>>>>>> +++ b/mm/huge_memory.c
>>>>>> @@ -151,10 +151,15 @@ 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);
>>>>>> +
>>>>>> +        if (!vma_is_anon_shmem(vma))
>>>>>> +            return global_huge? orders : 0;
>>>>>
>>>>> nit: missing space before '?'
>>>>
>>>> Yes, thanks.
>>>>
>>>>>
>>>>>> +        return shmem_allowable_huge_orders(file_inode(vma->vm_file),
>>>>>> +                            vma, vma->vm_pgoff, global_huge);
>>>>>
>>>>> What's the rationale for splitting these functions into shmem_is_huge() and
>>>>> shmem_allowable_huge_orders()? Why not just have a single
>>>>> shmem_allowable_huge_orders() that tells you the answer?
>>>>>
>>>>
>>>> Currently, shmem_is_huge() is used for all shmem implementations to determine
>>>> whether the conditions for using THP are met. And shmem_allowable_huge_orders()
>>>> is currently mainly used for anonymous shmem's mTHP to obtain all orders that
>>>> meet the conditions. In my opinion, there is no problem in separating these two
>>>> functions. In the future, when mTHP of other shmem types is also implemented,
>>>> will shmem_is_huge() be unnecessary?
>>>
>>> Personally, I consider shmem_is_huge() to be superfluous; If you have one
>>> function, shmem_allowable_huge_orders(), that gives you all the information you
>>> need. If the inode only allows PMD-size, then only return that bit in the field.
>>> IMHO removing shmem_is_huge() would make things more readable.
>>
>> Thank you very much for your opinion. I think there is no big problem in keeping
>> the current interface status quo. In my opinion, when we implement mTHP for all
>> shmems, it may be better to remove shmem_is_huge() and use
>> shmem_allowable_huge_orders() uniformly. I believe that it won't be long before
>> someone proposes a non-anonymous shared mTHP patchset. Anyway, thank you for
>> your suggestion :).
> 
> See [1] for an expanded list of concerns I have about the current state of the
> interface and implementation.

OK, could you please send me the corresponding link ;)？

Thanks，
Bang

> 
> Thanks,
> Ryan
> 
>>
>> Thanks,
>> Bang
>>
>>>
>>> Thanks,
>>> Ryan
>>>
>>>>
>>>> Thanks,
>>>> Bang
>>>>
>>>>>> +    }
>>>>>>           if (!vma_is_anonymous(vma)) {
>>>>>>             /*
>>>>>> 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)