Re: [PATCH v2 09/12] mm/collapse: open-code collapse_single_pmd() in its two callers

[Kiryl Shutsemau]

On Fri, Sep 11, 2026 at 10:57:46AM -0400, Zi Yan wrote:
> On Thu Sep 10, 2026 at 8:02 AM EDT, Kiryl Shutsemau wrote:
> > From: "Kiryl Shutsemau (Meta)" <kas@xxxxxxxxxx>
> >
> > collapse_scan_pmd() and collapse_run_pmd() each have a clear locking
> > contract.  The scan is called with mmap_lock held for reading and returns
> > with it still held.  The collapse is called without it.
> >
> > collapse_single_pmd() kept that boundary inside itself.  It dropped the
> > lock on some paths and not others, and reported which by way of a bool its
> > callers had to carry along and then act on.
> >
> > Open-code it in the two callers.  Each scans under the lock it already
> > holds and, on SCAN_SUCCEED, gives the lock up before running the collapse.
> > khugepaged's lock_dropped and madvise_collapse()'s mmap_unlocked both go:
> > the code dropping the lock is now the code that wanted to know.
> >
> > khugepaged's walk carries on to the next table while the scan keeps
> > refusing, and ends once a collapse has taken the lock from under it.
> > madvise_collapse() re-finds its VMA after a collapse, which it did before,
> > and now uses a NULL vma to say that it has to.  It still reports the drop
> > to its own caller, from the line that does it.
> >
> > The lock is given up and taken again at the same points as before.  No
> > functional change.
> >
> > Assisted-by: LLM
> > Signed-off-by: Kiryl Shutsemau (Meta) <kas@xxxxxxxxxx>
> > ---
> >  mm/khugepaged.c | 102 +++++++++++++++++++++++-------------------------
> >  1 file changed, 49 insertions(+), 53 deletions(-)
> >
> 
> LGTM. Thanks.
> 
> Reviewed-by: Zi Yan <ziy@xxxxxxxxxx>
> 
> One question:
> 
> What prevents us from doing:
> while () {
>     1. mmap_lock
>     2. scan_pmd
>     3. mmap_unlock, bail out if needed
>     4. run_pmd
> }
> 
> for both cases? It improves readability. What is the downside of
> dropping the lock during multiple scans?

The scan/run ratio.

Once memory is mostly huge nearly every scan refuses, and a refused
table is cheap: one pmd read for SCAN_PMD_MAPPED, one PTE walk under the
PTL otherwise.

In your version of the collapse loop, mmap lock/unlock plus VMA
revalidation would dominate the cost. It is not productive.

Note that scan in khugepaged is bounded by pages_to_scan so we would not
hog the lock.

> for madvise_collapse(), I see mmap_read_lock is held when it is called,
> so it can be dropped at the entry and the code makes sure it is held at
> the exit.

That makes every MADV_COLLAPSE report lock_dropped, including one on a
range that is already huge, which today never lets the lock go. The same
performance consideration as above.

-- 
  Kiryl Shutsemau / Kirill A. Shutemov