Re: [PATCH v2 00/12] mm/collapse: separate a collapse from its callers

[Kiryl Shutsemau]

On Fri, Sep 11, 2026 at 04:56:06PM +0100, Kiryl Shutsemau wrote:
> On Fri, Sep 11, 2026 at 05:06:58PM +0200, David Hildenbrand (Arm) wrote:
> > On 9/10/26 14:02, Kiryl Shutsemau wrote:
> > > From: "Kiryl Shutsemau (Meta)" <kas@xxxxxxxxxx>
> > > 
> > > [ This is the first of the cleanups I said I would front-load ]
> > > 
> > > There is no line between the collapse engine and the callers that ask for
> > > a collapse.  khugepaged.c holds both, and they reach into each other.
> > > 
> > >  - Sixteen tests through the collapse path read cc->is_khugepaged to work
> > >    out what they are allowed to do, when every one of those decisions was
> > >    made by the caller before it asked.
> > > 
> > >  - collapse_single_pmd() does both halves of a collapse behind one call and
> > >    drops mmap_lock somewhere in the middle.  Which of its paths dropped it
> > >    is not something a caller can see, so it hands back a bool and the
> > >    caller keeps track.
> > > 
> > >  - MADV_COLLAPSE's implementation -- the walk over the user's range, the
> > >    per-PMD loop, the errno translation -- sits in khugepaged.c, which is
> > >    the daemon's file.
> > > 
> > > So: draw the line.  State what a caller allows in a policy, split the call
> > > in two with the lock as the boundary, and move the syscall to madvise.c.
> > > What the engine offers is then four calls, with the lock state written
> > > down against each, and a policy the caller fills for itself:
> > > 
> > >     collapse_control_init(cc)         once, before the first table
> > >     collapse_policy_*(&cc->policy)    what this caller allows
> > >     collapse_scan_pmd(vma, addr, ...) per table, under mmap_lock
> > >     collapse_run_pmd(mm, addr, cc)    when a scan found work, no mmap_lock
> > >     collapse_control_release(cc)      once, when done
> > > 
> > > The engine stays in khugepaged.c for now; what changes is that it has an
> > > interface, and that neither half has to ask about the other.  madvise.c
> > > gains the operation it should have had all along.
> > > 
> > > Changes since v1
> > > ================
> > > 
> > >   https://lore.kernel.org/all/cover.1788533997.git.kas@xxxxxxxxxx/
> > > 
> > >  - Rebased onto mm-new with Vernon Yang's tracepoint fixes in it.  Patch 8
> > >    no longer merges the two calls to each scan tracepoint, since the base
> > >    already has one; its changelog now says what the status field reports.
> > > 
> > >  - Patch 3: nr_occupied_ptes is nr_eligible_ptes, and the mthp_collapse()
> > >    comment counts eligible PTEs too (Zi, Baolin).
> > > 
> > >  - Patch 4: no comments on the two constants (Baolin).
> > > 
> > >  - Patch 5: one line per policy field (Baolin).
> > > 
> > >  - Patch 8: the file side is split like the anonymous one (Zi).
> > >    collapse_scan_file() runs under mmap_lock in the scan and only judges;
> > >    collapse_file() runs in the run.  See Behaviour below.
> > > 
> > >  - Reviewed-by from Zi Yan and Baolin Wang on 1-4, 6 and 7.
> > > 
> > 
> > I'll hopefully get too look at this soon (after digging through older stuff in
> > my queue).
> > 
> > Skimming over some patches,  a note that we should not be undoing recent
> > cleanups without a very good reason.
> 
> I don't think we undo it.
> 
> Both madvise and khugepaged use the same interface to the collapse
> engine. Anon and file paths are handled internally in the engine. What
> changed is that we have two calls into the engine instead of one.
> 
> Collapse consists of two phases: finding what to collapse and collapsing
> the found range. These two phases have vastly different locking
> expectations.
> 
> The scan reads a PTE table under mmap_lock, fails often and doesn't drop
> the lock to move to next range.
> 
> The collapse allocates, may sleep in writeback and takes mmap_lock for
> write itself. So the lock inherited from scan is no good.
> 
> collapse_single_pmd() hid that boundary inside one call. It had to drop
> the lock somewhere in the middle, on some paths and not others, and the
> only way for the caller to find out was the lock_dropped bool.
> 
> With scan and run as separate calls each has one lock rule: scan is
> called locked and returns locked, run is called unlocked. There is
> nothing left to report, so the ugly lock_dropped goes away.
> 
> It is the same move as Nico's da98790891a4 ("require collapse_huge_page
> to enter/exit with the lock dropped"), one level up.

Forgot to mention, this kind of split by lock boundary makes it trivial
to switch scan to per-VMA locking.

-- 
  Kiryl Shutsemau / Kirill A. Shutemov