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

David Hildenbrand (Arm)
2026-02-25T13:44:16Z

On 2/25/26 01:35, Nico Pache wrote:
> On Thu, Feb 12, 2026 at 12:51 PM David Hildenbrand (Arm)
> <david@kernel.org> 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@redhat.com>
>>> ---
>>> 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