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

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

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.

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)