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

[Kiryl Shutsemau]

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.

-- 
  Kiryl Shutsemau / Kirill A. Shutemov