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

[Dev Jain <dev.jain@arm.com>]

On 22/08/25 12:29 pm, Baolin Wang wrote:
>
>
> 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.
>  */

Looks good, had done something similar in my version.


>
>>>       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.