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

[Kiryl Shutsemau]

On Wed, Sep 23, 2026 at 03:08:55PM +0200, David Hildenbrand (Arm) wrote:
> On 9/16/26 11:31, 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, when the scan found work, 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.
> > 
> 
> I'm not sure I see the benefit. The code in the previous collapse_single_pmd()
> callers certainly gets more messy?
> 
> Is there some other patches in this series that depend on it or what's the
> motivation?

The locking. Scan and run have different locking expectations.

I tried to explain it multiple times. Probably not well enough.
Let me reiterate.

The scan reads a PTE table under mmap_lock, fails often, and does not
drop the lock to move on to the next table.

The collapse allocates, may sleep in writeback and takes mmap_lock for
write itself, so the lock inherited from the scan is no good to it.

collapse_single_pmd() hid that boundary inside one call. It dropped the
lock somewhere in the middle, on some paths and not others, and
lock_dropped was the only way for the caller to find out.

With the two calls each has one rule: the scan runs in the caller's
locking context and never touches the lock, the run is called unlocked
and takes what it needs.

There is nothing left to report, so lock_dropped and mmap_unlocked go.
It is the same move as Nico's da98790891a4 ("require collapse_huge_page
to enter/exit with the lock dropped"), one level up.

It also makes moving the scan to per-VMA locking trivial: the caller
owns the lock and the engine never sees it. I said as much in reply to
your note on v2:

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

Patch 12 depends on it, since madvise.c gets the two calls and their
lock rules rather than a bool.

On messier: what the callers gained is an mmap_read_unlock() where they
decide to run, and what they lost is a bool telling them whether
somebody else had dropped their lock. Each caller now takes and drops
its own lock and knows it, and the engine never touches a lock it did
not take.

That is more lines at the call site and a simpler locking rules.

-- 
  Kiryl Shutsemau / Kirill A. Shutemov