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

[Kiryl Shutsemau]

On Fri, Sep 11, 2026 at 06:09:32PM -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(-)
> >
> > diff --git a/mm/khugepaged.c b/mm/khugepaged.c
> > index c26907300c23..9bdf12128357 100644
> > --- a/mm/khugepaged.c
> > +++ b/mm/khugepaged.c
> > @@ -2857,28 +2857,6 @@ static enum scan_result collapse_run_pmd(struct mm_struct *mm,
> >  	return result;
> >  }
> >  
> > -/*
> > - * Try to collapse a single PMD starting at a PMD aligned addr, and return
> > - * the results.
> > - */
> > -static enum scan_result collapse_single_pmd(unsigned long addr,
> > -		struct vm_area_struct *vma, bool *lock_dropped,
> > -		struct collapse_control *cc)
> > -{
> > -	struct mm_struct *mm = vma->vm_mm;
> > -	enum scan_result result;
> > -
> > -	result = collapse_scan_pmd(vma, addr, cc);
> > -	if (result != SCAN_SUCCEED)
> > -		return result;
> > -
> > -	/* The collapse takes its own locks, so give this up */
> > -	mmap_read_unlock(mm);
> > -	*lock_dropped = true;
> > -
> > -	return collapse_run_pmd(mm, addr, cc);
> > -}
> > -
> 
> Sorry for walking back on this. I think collapse_single_pmd() can be
> kept and still get patch 10 to 12 applied. The reason is that by looking at the
> code after patch 11 is applied, the collapse_scan_pmd() +
> collapse_run_pmd() patterns in madvise_collapse() and
> collapse_scan_mm_slot() look very similar. And it can make
> collapse_scan_pmd() and collapse_run_pmd() internal with only
> collapse_single_pmd() exported.

I gave more motivation in my reply to David:

https://lore.kernel.org/all/aqQf9hSy0iNjnL6t@thinkstation/

Short version: scan and run have different locking expectations, and
the split puts the boundary where the lock is. It is also what makes
moving the scan to per-VMA locking trivial, since the caller owns the
lock and the engine never touches it.

-- 
  Kiryl Shutsemau / Kirill A. Shutemov