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

Baolin Wang
2025-08-22T06:59:41Z

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.