Re: [PATCH v2] mm: shmem: don't set large-order range for internal shmem mount

[David Hildenbrand (Arm)]

On 4/15/26 11:04, Baolin Wang wrote:
> 
> 
> On 4/15/26 4:47 PM, David Hildenbrand (Arm) wrote:
>> On 4/15/26 10:22, Baolin Wang wrote:
>>> Anonymous shmem large order allocations are dynamically controlled
>>> via the
>>> global THP sysfs knob (/sys/kernel/mm/transparent_hugepage/
>>> shmem_enabled)
>>> and the per-size mTHP knobs (/sys/kernel/mm/transparent_hugepage/
>>> hugepages-<size>kB/shmem_enabled).
>>>
>>> Therefore, anonymous shmem uses shmem_allowable_huge_orders() to check
>>> which large orders are allowed, rather than relying on
>>> mapping_max_folio_order().
>>> Moreover, mapping_max_folio_order() is intended to control large order
>>> allocations only for tmpfs mounts. Clarify this by not setting a
>>> large-order
>>> range for internal shmem mount (e.g. anonymous shmem), to avoid
>>> confusion,
>>> as discussed in the previous thread[1].
>>>
>>> [1] https://lore.kernel.org/all/
>>> ec927492-4577-4192-8fad-85eb1bb43121@xxxxxxxxxxxxxxxxx/
>>> Signed-off-by: Baolin Wang <baolin.wang@xxxxxxxxxxxxxxxxx>
>>> ---
>>> Changes from v1:
>>>   - Update the comments and commit message, per Lance.
>>> ---
>>>   mm/shmem.c | 12 ++++++++++--
>>>   1 file changed, 10 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/mm/shmem.c b/mm/shmem.c
>>> index 4ecefe02881d..568e1baee90d 100644
>>> --- a/mm/shmem.c
>>> +++ b/mm/shmem.c
>>> @@ -3088,8 +3088,16 @@ static struct inode *__shmem_get_inode(struct
>>> mnt_idmap *idmap,
>>>       if (sbinfo->noswap)
>>>           mapping_set_unevictable(inode->i_mapping);
>>>   -    /* Don't consider 'deny' for emergencies and 'force' for
>>> testing */
>>> -    if (sbinfo->huge)
>>> +    /*
>>> +     * Only set the large order range for tmpfs mounts. The large order
>>> +     * selection for the internal shmem mount is configured dynamically
>>> +     * via the 'shmem_enabled' interfaces, so there is no need to set a
>>> +     * large order range for the internal shmem mount's mapping.
>>> +     *
>>> +     * Note: Don't consider 'deny' for emergencies and 'force' for
>>> +     * testing.
>>> +     */
>>> +    if (sbinfo->huge && !(sb->s_flags & SB_KERNMOUNT))
>>>           mapping_set_large_folios(inode->i_mapping);
>>
>> I don't like that special casing. In an ideal world, any mapping that
>> supports large folios would indicate that.
>>
>> Now, which large folios to allocate is a different question.
>>
>> What's the problem with indicating for all shmem mappings that support
>> large folios that support, but handling *which* folio sizes to allocate
>> elsewhere?
> 
> Thanks for taking a look.

Sorry for the late feedback.

> 
> As I mentioned, the original logic has several issues for anonymous shmem:
> 
> 1. Whether anonymous shmem supports large folios can be dynamically
> configured via sysfs interfaces, so mapping_set_large_folios() set
> during initialization cannot accurately reflect whether anonymous shmem
> actually supports large folios.

Well, the mapping does support large folios, just the folio allocations
are currently disable.

It feels cleaner to say "there might be large folios in this mapping"
than saying "there are no large folios in the mapping as the mapping
does not support it", no?

> 
> 2. Calling mapping_set_large_folios() here by default makes anonymous
> shmem support 'MAX_PAGECACHE_ORDER' by default. However, the range of
> large orders supported by anonymous shmem is also dynamically
> configurable via sysfs interfaces, which could cause more confusion.

Fair enough. The mapping supports it, we just don't want to allocate
some orders (right now).

> 
> 3. Currently, no users will call mapping_large_folio_support() related
> functions to determine whether large folios are supported for anonymous
> shmem.

Right, we special-case shmem all over the place :) For example, in
khugepaged. I wonder if that could help with Zi's changes to get rid of
some shmem checks.

What if we say:

shmem that *will never have*/*does never allow* large folios never sets
mapping_set_large_folios().

shmem that *might* have large folios (in the past, now, or in the
future) sets mapping_set_large_folios().

> 
> Therefore, rather than having anonymous shmem call
> mapping_set_large_folios() and introduce so much confusion, I'd prefer
> to exclude anonymous shmem from calling mapping_set_large_folios().

I think it's more confusing to end up with large folios in a mapping
that claims to not support large folios?

-- 
Cheers,

David