Re: [PATCH v8 07/14] mm: shmem: allow THP support determination at folio allocation time

[Baolin Wang <baolin.wang@linux.alibaba.com>]

On 10/3/26 11:09 PM, Luiz Capitulino wrote:
> 
> 
> On 10/2/26 3:23 PM, David Hildenbrand (Arm) wrote:
>> On 9/18/26 03:45, Luiz Capitulino wrote:
>>> In order to enable THP support in shmem today, besides the user
>>> configuration required, the CPU must support PMD-sized pages. This
>>> is the case because of the following has_transparent_hugepage()
>>> usage:
>>>
>>> - shmem_parse_one() and shmem_parse_huge(): Check if THP is built-in and
>>>    if the CPU supports PMD-sized pages
>>>
>>> - shmem_init(): Since the CONFIG_TRANSPARENT_HUGEPAGE guard is outside
>>>    the code block calling has_transparent_hugepage(), the
>>>    has_transparent_hugepage() call is exclusively checking if the CPU
>>>    supports PMD-sized pages
>>>
>>> While it's necessary to check if CONFIG_TRANSPARENT_HUGEPAGE is enabled
>>> in all cases, shmem can determine THP size support at folio allocation
>>> time. Therefore, drop the has_transparent_hugepage() usage listed above
>>> while keeping the CONFIG_TRANSPARENT_HUGEPAGE checks.
>>>
>>> Additionally, we need to check if PMD size order is supported in
>>> shmem_getattr(). Use pgtable_has_pmd_leaves() for that.
>>>
>>> Reviewed-by: Baolin Wang <baolin.wang@linux.alibaba.com>
>>> Signed-off-by: Luiz Capitulino <luizcap@redhat.com>
>>> ---
>>>   mm/shmem.c | 9 +++++----
>>>   1 file changed, 5 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/mm/shmem.c b/mm/shmem.c
>>> index 776dff8a848e..930657d05375 100644
>>> --- a/mm/shmem.c
>>> +++ b/mm/shmem.c
>>> @@ -690,7 +690,7 @@ static int shmem_parse_huge(const char *str)
>>>       else
>>>           return -EINVAL;
>>> -    if (!has_transparent_hugepage() &&
>>> +    if (!IS_ENABLED(CONFIG_TRANSPARENT_HUGEPAGE) &&
>>>           huge != SHMEM_HUGE_NEVER && huge != SHMEM_HUGE_DENY)
>>>           return -EINVAL;
>>> @@ -1524,6 +1524,8 @@ static int shmem_getattr(struct mnt_idmap *idmap,
>>>       generic_fillattr(idmap, request_mask, inode, stat);
>>>       orders = shmem_huge_global_enabled(inode, 0, 0, false, NULL, 0);
>>
>> I'm curious: why is that not handled inside 
>> shmem_huge_global_enabled() ? If PMD
>> order is impossible (well, okay, it is possible, but we simply cannot 
>> map these
>> things through PMDs), I would expect that we never list them as 
>> "enabled".
> 
> Yes, you're right. What about renaming current 
> shmem_huge_global_enabled() to
> __shmem_huge_global_enabled() and then having:
> 
> static unsigned int shmem_huge_global_enabled(struct inode *inode, 
> pgoff_t index,
>                            loff_t write_end, bool shmem_huge_force,
>                            struct vm_area_struct *vma,
>                            vm_flags_t vm_flags)
> {
>      unsigned int orders;
> 
>      orders = __shmem_huge_global_enabled(inode, index, write_end,
>                           shmem_huge_force, vma, vm_flags);
>      if (!pgtable_has_pmd_leaves())
>          orders &= ~BIT(PMD_ORDER);
> 
>      return orders;
> }
> 
> Would this be acceptable?

Not a fan of re-adding the wrapper. I think you could refer to the 
changes to shmem_allowable_huge_orders() in patch 14 and filter out the 
'disabled_orders' in shmem_huge_global_enabled() directly.