Re: (sashiko review) Re: [PATCH v4 6/9] mm: shmem: drop has_transparent_hugepage() usage

[Lance Yang]

On Wed, May 06, 2026 at 02:12:41PM -0400, Luiz Capitulino wrote:
>On 2026-05-01 15:18, Luiz Capitulino wrote:
>> Shmem uses has_transparent_hugepage() in the following ways:
>> 
>> - 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 mTHP size support at folio allocation
>> time. Therefore, drop has_transparent_hugepage() usage while keeping the
>> CONFIG_TRANSPARENT_HUGEPAGE checks.
>> 
>> Reviewed-by: Baolin Wang <baolin.wang@xxxxxxxxxxxxxxxxx>
>> Reviewed-by: Lance Yang <lance.yang@xxxxxxxxx>
>> Acked-by: Zi Yan <ziy@xxxxxxxxxx>
>> Signed-off-by: Luiz Capitulino <luizcap@xxxxxxxxxx>
>> ---
>>   mm/shmem.c | 7 +++----
>>   1 file changed, 3 insertions(+), 4 deletions(-)
>> 
>> diff --git a/mm/shmem.c b/mm/shmem.c
>> index 3b5dc21b323c..1948d73fb1e3 100644
>> --- a/mm/shmem.c
>> +++ b/mm/shmem.c
>> @@ -689,7 +689,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;
>>   
>> @@ -4656,8 +4656,7 @@ static int shmem_parse_one(struct fs_context *fc, struct fs_parameter *param)
>>   	case Opt_huge:
>>   		ctx->huge = result.uint_32;
>>   		if (ctx->huge != SHMEM_HUGE_NEVER &&
>> -		    !(IS_ENABLED(CONFIG_TRANSPARENT_HUGEPAGE) &&
>> -		      has_transparent_hugepage()))
>> +		    !IS_ENABLED(CONFIG_TRANSPARENT_HUGEPAGE))
>>   			goto unsupported_parameter;
>>   		ctx->seen |= SHMEM_SEEN_HUGE;
>>   		break;
>
>"""
>By dropping the has_transparent_hugepage() check, will mount -t tmpfs
>-o huge=always now succeed on hardware lacking PMD support?
>
>If so, since hugepage_init() still sets the TRANSPARENT_HUGEPAGE_UNSUPPORTED
>flag, thp_disabled_by_hw() will unconditionally block all large folio
>allocations in shmem_allowable_huge_orders().
>
>Does this create an intermediate state where the mount silently succeeds
>but no huge pages of any size can actually be allocated?
>
>I see this is resolved later in the series by commit cd27430097e8
>("mm: replace thp_disabled_by_hw() with pgtable_has_pmd_leaves()") and
>commit 641a20ae032f ("mm: thp: always enable mTHP support").
>"""
>
>The mount -t tmpfs -o huge=always succeeding on hardware without PMD
>support can happen in this patch, yes. But this seems very minor, the
>impact seems to be someone doing bisection, landing on this patch and
>their reproducer is depedent on mounting tmpfs with -o huge=always on
>hardware without PMD size support? I can fix it if others feel strong
>about this.
>
>> @@ -5449,7 +5448,7 @@ void __init shmem_init(void)
>>   #endif
>>   
>>   #ifdef CONFIG_TRANSPARENT_HUGEPAGE
>> -	if (has_transparent_hugepage() && shmem_huge > SHMEM_HUGE_DENY)
>> +	if (shmem_huge > SHMEM_HUGE_DENY)
>>   		SHMEM_SB(shm_mnt->mnt_sb)->huge = shmem_huge;
>>   	else
>>   		shmem_huge = SHMEM_HUGE_NEVER; /* just in case it was patched */
>
>"""
>Also, by allowing shmem_huge to be set to SHMEM_HUGE_ALWAYS on systems
>without PMD support, does this incorrectly affect shmem_getattr()?
>
>shmem_getattr() relies on shmem_huge_global_enabled(), which only checks
>the software configuration and not hardware PMD support. Consequently,
>shmem_getattr() will erroneously report stat->blksize = HPAGE_PMD_SIZE
>to userspace.
>
>Since subsequent patches in the series do not appear to update
>shmem_getattr(), could this misleading block size cause userspace tools
>to over-allocate IO buffers on hardware where PMD-sized pages are
>structurally impossible?
>"""
>
>This a real issue (albeit small one), the problem is this check in
>shmem_getattr():
>
>	if (shmem_huge_global_enabled(inode, 0, 0, false, NULL, 0))
>		stat->blksize = HPAGE_PMD_SIZE;
>
>So, we may report HPAGE_PMD_SIZE even when PMD size is not supported.
>Looks like we may over-report today as well for the
>SHMEM_HUGE_WITHIN_SIZE case? In any case, I'll fix this.

Well spotted.

@Baolin looks like shmem_getattr() might already be buggy?

shmem_huge_global_enabled() returns an order mask. For huge=always and
huge=within_size it can return THP_ORDERS_ALL_FILE_DEFAULT, which is not
PMD-only and can include smaller file mTHP orders as well ...

So shmem_getattr() treating any non-zero mask as HPAGE_PMD_SIZE looks
like an over-report?

Cheers, Lance