Re: [PATCH mm-unstable v1 3/5] mm/khugepaged: define COLLAPSE_MAX_PTES_LIMIT as HPAGE_PMD_NR - 1

[David Hildenbrand (Arm)]

On 2/25/26 01:35, Nico Pache wrote:
> On Thu, Feb 12, 2026 at 12:51 PM David Hildenbrand (Arm)
> <david@xxxxxxxxxx> wrote:
>>
>> On 2/12/26 03:18, Nico Pache wrote:
>>> The value (HPAGE_PMD_NR - 1) is used often in the khugepaged code to
>>> signify the limit of the max_ptes_* values. Add a define for this to
>>> increase code readability and reuse.
>>>
>>> Signed-off-by: Nico Pache <npache@xxxxxxxxxx>
>>> ---
>>>   mm/khugepaged.c | 15 ++++++++-------
>>>   1 file changed, 8 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/mm/khugepaged.c b/mm/khugepaged.c
>>> index c362b3b2e08a..3dcce6018f20 100644
>>> --- a/mm/khugepaged.c
>>> +++ b/mm/khugepaged.c
>>> @@ -85,6 +85,7 @@ static DECLARE_WAIT_QUEUE_HEAD(khugepaged_wait);
>>>    *
>>>    * Note that these are only respected if collapse was initiated by khugepaged.
>>>    */
>>> +#define COLLAPSE_MAX_PTES_LIMIT (HPAGE_PMD_NR - 1)
>>>   unsigned int khugepaged_max_ptes_none __read_mostly;
>>>   static unsigned int khugepaged_max_ptes_swap __read_mostly;
>>>   static unsigned int khugepaged_max_ptes_shared __read_mostly;
>>> @@ -252,7 +253,7 @@ static ssize_t max_ptes_none_store(struct kobject *kobj,
>>>       unsigned long max_ptes_none;
>>>
>>>       err = kstrtoul(buf, 10, &max_ptes_none);
>>> -     if (err || max_ptes_none > HPAGE_PMD_NR - 1)
>>> +     if (err || max_ptes_none > COLLAPSE_MAX_PTES_LIMIT)
>>>               return -EINVAL;
>>>
>>>       khugepaged_max_ptes_none = max_ptes_none;
>>> @@ -277,7 +278,7 @@ static ssize_t max_ptes_swap_store(struct kobject *kobj,
>>>       unsigned long max_ptes_swap;
>>>
>>>       err  = kstrtoul(buf, 10, &max_ptes_swap);
>>> -     if (err || max_ptes_swap > HPAGE_PMD_NR - 1)
>>> +     if (err || max_ptes_swap > COLLAPSE_MAX_PTES_LIMIT)
>>>               return -EINVAL;
>>>
>>>       khugepaged_max_ptes_swap = max_ptes_swap;
>>> @@ -303,7 +304,7 @@ static ssize_t max_ptes_shared_store(struct kobject *kobj,
>>>       unsigned long max_ptes_shared;
>>>
>>>       err  = kstrtoul(buf, 10, &max_ptes_shared);
>>> -     if (err || max_ptes_shared > HPAGE_PMD_NR - 1)
>>> +     if (err || max_ptes_shared > COLLAPSE_MAX_PTES_LIMIT)
>>>               return -EINVAL;
>>>
>>>       khugepaged_max_ptes_shared = max_ptes_shared;
>>> @@ -384,7 +385,7 @@ int __init khugepaged_init(void)
>>>               return -ENOMEM;
>>>
>>>       khugepaged_pages_to_scan = HPAGE_PMD_NR * 8;
>>> -     khugepaged_max_ptes_none = HPAGE_PMD_NR - 1;
>>> +     khugepaged_max_ptes_none = COLLAPSE_MAX_PTES_LIMIT;
>>>       khugepaged_max_ptes_swap = HPAGE_PMD_NR / 8;
>>>       khugepaged_max_ptes_shared = HPAGE_PMD_NR / 2;
>>
>>
>> Changing these is ok. I don't like the name either.
> 
> Yeah I prefer COLLAPSE_MAX_PTES as others suggested, its shorter and
> more general.
> 

COLLAPSE_MAX_PTES is misleading. We will happily collapse for 512 PTEs.

It's all about khugepaged_max_ptes_* semantics.

>>
>> It should be clear that they all belong to the "khugepaged_max_ptes" *
>> values. (see below)
>>
>>>
>>> @@ -1869,7 +1870,7 @@ static enum scan_result collapse_file(struct mm_struct *mm, unsigned long addr,
>>>       bool is_shmem = shmem_file(file);
>>>
>>>       VM_BUG_ON(!IS_ENABLED(CONFIG_READ_ONLY_THP_FOR_FS) && !is_shmem);
>>> -     VM_BUG_ON(start & (HPAGE_PMD_NR - 1));
>>> +     VM_BUG_ON(start & (COLLAPSE_MAX_PTES_LIMIT));
>>>
>>>       result = alloc_charge_folio(&new_folio, mm, cc);
>>>       if (result != SCAN_SUCCEED)
>>> @@ -2209,7 +2210,7 @@ static enum scan_result collapse_file(struct mm_struct *mm, unsigned long addr,
>>>        * unwritten page.
>>>        */
>>>       folio_mark_uptodate(new_folio);
>>> -     folio_ref_add(new_folio, HPAGE_PMD_NR - 1);
>>> +     folio_ref_add(new_folio, COLLAPSE_MAX_PTES_LIMIT);
>>>
>>>       if (is_shmem)
>>>               folio_mark_dirty(new_folio);
>>> @@ -2303,7 +2304,7 @@ static enum scan_result hpage_collapse_scan_file(struct mm_struct *mm, unsigned
>>>       memset(cc->node_load, 0, sizeof(cc->node_load));
>>>       nodes_clear(cc->alloc_nmask);
>>>       rcu_read_lock();
>>> -     xas_for_each(&xas, folio, start + HPAGE_PMD_NR - 1) {
>>> +     xas_for_each(&xas, folio, start + COLLAPSE_MAX_PTES_LIMIT) {
>>>               if (xas_retry(&xas, folio))
>>>                       continue;
>>>
>>
>> Changing these is not. They are semantically something different.
> 
> With the new name these are ok right? given we dropped the LIMIT
> aspect. The only thing that comes to mind is that COLLAPSE_MAX_PTES
> would seemingly refer to 512 not 511, but I'm ok with that if no one
> objects.

Just leave it at HPAGE_PMD_NR - 1, any magical constant will make this
code only more confusing.

-- 
Cheers,

David