Re: [PATCH v10 10/13] khugepaged: kick khugepaged for enabling none-PMD-sized mTHPs

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

On 2025/8/21 22:18, Lorenzo Stoakes wrote:
> On Tue, Aug 19, 2025 at 07:42:02AM -0600, Nico Pache wrote:
>> From: Baolin Wang <baolin.wang@linux.alibaba.com>
>>
>> When only non-PMD-sized mTHP is enabled (such as only 64K mTHP enabled),
> 
> I don't think this example is very useful, probably just remove it.
> 
> Also 'non-PMD-sized mTHP' implies there is such a thing as PMD-sized mTP :)
> 
>> we should also allow kicking khugepaged to attempt scanning and collapsing
> 
> What is kicking? I think this should be rephrased to something like 'we should
> also allow khugepaged to attempt scanning...'
> 
>> 64K mTHP. Modify hugepage_pmd_enabled() to support mTHP collapse, and
> 
> 64K mTHP -> "of mTHP ranges". Put the 'Modify...' bit in a new paragraph to
> be clear.
> 
>> while we are at it, rename it to make the function name more clear.
> 
> To make this clearer let me suggest:
> 
> 	In order for khugepaged to operate when only mTHP sizes are
> 	specified in sysfs, we must modify the predicate function that
> 	determines whether it ought to run to do so.
> 
> 	This function is currently called hugepage_pmd_enabled(), this
> 	patch renames it to hugepage_enabled() and updates the logic to
> 	check to determine whether any valid orders may exist which would
> 	justify khugepaged running.

Thanks. This looks good to me.

>> Signed-off-by: Baolin Wang <baolin.wang@linux.alibaba.com>
>> Signed-off-by: Nico Pache <npache@redhat.com>
> 
>> ---
>>   mm/khugepaged.c | 20 ++++++++++----------
>>   1 file changed, 10 insertions(+), 10 deletions(-)
>>
>> diff --git a/mm/khugepaged.c b/mm/khugepaged.c
>> index 2cadd07341de..81d2ffd56ab9 100644
>> --- a/mm/khugepaged.c
>> +++ b/mm/khugepaged.c
>> @@ -430,7 +430,7 @@ static inline int collapse_test_exit_or_disable(struct mm_struct *mm)
>>   		mm_flags_test(MMF_DISABLE_THP_COMPLETELY, mm);
>>   }
>>
>> -static bool hugepage_pmd_enabled(void)
>> +static bool hugepage_enabled(void)
>>   {
>>   	/*
>>   	 * We cover the anon, shmem and the file-backed case here; file-backed
>> @@ -442,11 +442,11 @@ static bool hugepage_pmd_enabled(void)
> 
> The comment above this still references PMD-sized, please make sure to update
> comments when you change the described behaviour, as it is now incorrect:
> 
> 	/*
> 	 * We cover the anon, shmem and the file-backed case here; file-backed
> 	 * hugepages, when configured in, are determined by the global control.
> 	 * Anon pmd-sized hugepages are determined by the pmd-size control.
> 	 * Shmem pmd-sized hugepages are also determined by its pmd-size control,
> 	 * except when the global shmem_huge is set to SHMEM_HUGE_DENY.
> 	 */
> 
> Please correct this.

Sure. How about:

/*
  * We cover the anon, shmem and the file-backed case here; file-backed
  * hugepages, when configured in, are determined by the global control.
  * Anon hugepages are determined by its per-size mTHP control.
  * Shmem pmd-sized hugepages are also determined by its pmd-size control,
  * except when the global shmem_huge is set to SHMEM_HUGE_DENY.
  */

>>   	if (IS_ENABLED(CONFIG_READ_ONLY_THP_FOR_FS) &&
>>   	    hugepage_global_enabled())
>>   		return true;
>> -	if (test_bit(PMD_ORDER, &huge_anon_orders_always))
>> +	if (READ_ONCE(huge_anon_orders_always))
>>   		return true;
>> -	if (test_bit(PMD_ORDER, &huge_anon_orders_madvise))
>> +	if (READ_ONCE(huge_anon_orders_madvise))
>>   		return true;
>> -	if (test_bit(PMD_ORDER, &huge_anon_orders_inherit) &&
>> +	if (READ_ONCE(huge_anon_orders_inherit) &&
>>   	    hugepage_global_enabled())
> 
> I guess READ_ONCE() is probably sufficient here as memory ordering isn't
> important here, right?

Yes, I think so.