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

[Nico Pache <npache@redhat.com>]

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.

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

-- Nico

>
> E.g., folio_ref_add() adds "HPAGE_PMD_NR - 1" because we already
> obtained a reference, and we need one per page -- HPAGE_PMD_NR.
>
> So it's something different than when messing with the magical
> khugepaged_max_ptes_none values.
>
> --
> Cheers,
>
> David
>