Re: [PATCH v9 09/14] khugepaged: avoid unnecessary mTHP collapse attempts

[Nico Pache <npache@redhat.com>]

On Thu, Jul 17, 2025 at 8:15 PM Baolin Wang
<baolin.wang@linux.alibaba.com> wrote:
>
>
>
> On 2025/7/14 08:32, Nico Pache wrote:
> > There are cases where, if an attempted collapse fails, all subsequent
> > orders are guaranteed to also fail. Avoid these collapse attempts by
> > bailing out early.
> >
> > Signed-off-by: Nico Pache <npache@redhat.com>
> > ---
> >   mm/khugepaged.c | 17 +++++++++++++++++
> >   1 file changed, 17 insertions(+)
> >
> > diff --git a/mm/khugepaged.c b/mm/khugepaged.c
> > index a701d9f0f158..7a9c4edf0e23 100644
> > --- a/mm/khugepaged.c
> > +++ b/mm/khugepaged.c
> > @@ -1367,6 +1367,23 @@ static int collapse_scan_bitmap(struct mm_struct *mm, unsigned long address,
> >                               collapsed += (1 << order);
> >                               continue;
> >                       }
>
> After doing more testing, I think you need to add the following changes
> after patch 8.
>
> Because when collapsing mTHP, if we encounter a PTE-mapped large folio
> within the PMD range, we should continue scanning to complete that PMD,
> in case there is another mTHP that can be collapsed within that PMD range.
>
> +                       if (ret == SCAN_PTE_MAPPED_HUGEPAGE)
> +                               continue;
Ah good call, this patch is meant to be an optimization to the formal
approach-- meaning there are cases where trying ('goto next') to
collapse to lower orders is pointless, but I didn't fully consider the
cases where trying the other items (sections of the PMD) in the stack
are viable (ie continue). I'm going to spend some time confirming all
the potential return values that can come from collapse_huge_page, and
which ones belong in each group (goto next, continue, break). This
will probably be turned into a switch statement.
>
> > +                     /*
> > +                      * Some ret values indicate all lower order will also
> > +                      * fail, dont trying to collapse smaller orders
> > +                      */
After reading this comment again, i realized it's rather confusing...
it makes it seem like these ret values are the ones that indicate that
we should not keep trying to collapse to smaller orders. I'll clean up
that comment too.
> > +                     if (ret == SCAN_EXCEED_NONE_PTE ||
> > +                             ret == SCAN_EXCEED_SWAP_PTE ||
> > +                             ret == SCAN_EXCEED_SHARED_PTE ||
> > +                             ret == SCAN_PTE_NON_PRESENT ||
> > +                             ret == SCAN_PTE_UFFD_WP ||
> > +                             ret == SCAN_ALLOC_HUGE_PAGE_FAIL ||
> > +                             ret == SCAN_CGROUP_CHARGE_FAIL ||
> > +                             ret == SCAN_COPY_MC ||
> > +                             ret == SCAN_PAGE_LOCK ||
> > +                             ret == SCAN_PAGE_COUNT)
> > +                             goto next;
> > +                     else
>
> Nit: the 'else' statement can be dropped.
>
> > +                             break;
> >               }
> >
> >   next:
While i'm at it i'll change this to next_order to be more clear.

Thank you !!
-- Nico
>