Re: [RFC PATCH 00/57] mm/collapse: rebuild collapse on migration primitives

["David Hildenbrand (Arm)" <david@kernel.org>]

On 8/17/26 00:45, Kiryl Shutsemau wrote:
> From: "Kiryl Shutsemau (Meta)" <kas@kernel.org>
> 
> Yes, I know, this is a lot of changes. But I'm happy with the overall state
> of the patchset and the only reason I tag it as RFC is that it is tricky
> to get 57 patches upstream.
> 
> I wanted to give a view of the end state first. I will suggest a possible
> way to split it below.
> 
> I would appreciate any feedback.

Replying here on the overall design first before reading into the other
discussions (and process related topics).

> 
> TL;DR
> =====
> 
> This replaces khugepaged's anonymous collapse with an engine that
> can collapse sub-PMD ranges. It is built around migration entries and
> frozen folios instead of heavy locking and isolation, aiming for better
> scalability and less disruption to the workload being collapsed.

I recall us discussing something around using some PTE/PMD markers (e.g.,
migration entries) in the past.

One thing that needed care is handling concurrent MADV_DONTNEED + faultin after
dropping relevant locks.

> 
> Why
> ===
> 
> mTHP collapse landed in khugepaged in 7.2 and I was glad to see it.  We
> at Meta run arm64 with 64K base pages, where a PMD is 512M: PMD-order THP
> is of limited use at that size, and mTHP is exactly what we want.

Right, as the first step, we decided to go for the simpler and minimally
intrusive approach of using the existing mechanism that always operates on PMD
ranges, keeping using the existing pmdp_collapse_flush()-based mechanism and
locking in place.

That's why anything more elaborate will require significantly more LOC :)

> 
> It turned out not to help us.
> 
> khugepaged only ever looks at PMD-aligned windows, and it is not an easy
> limitation to lift.


I recall we discussed some simpler way to make this work with VMAs that don't
fully span PMDs: I think write-locking VMAs (+ rmap) that cover the PMD was
discussed as a low-hanging fruit, such that other page table walkers would not
suddenly stumble over the temporarily removed page table.

So the mmap_write_lock() + vma write-locks prevents concurrent mmap+page faults
and the rmap locks prevent concurrent rmap walks.

There were discussions on the impact when a PMD spans many VMAs, and I think one
conclusion was that such scenarios are likely not worth considering (e.g., 512
VMAs in a single PMD, all with different rmap locks; your workload sucks already).

> 
> Fixing the alignment is a one-line change, but what it feeds assumes the
> PMD everywhere that matters: collapse_huge_page() clears and flushes the
> whole PMD whatever order it is collapsing, installs a PMD leaf because
> that is the only thing it can produce, and keeps everyone out with
> mmap_write_lock, anon_vma_lock_write() and an IPI broadcast while it
> does.
> 
> Which is why hugepage_vma_revalidate() demands that the VMA span the
> whole PMD even for an mTHP order -- "we'd need to lock all VMAs in the
> PMD range to support this", as the comment there puts it.  A PMD-granular
> operation is only safe when one VMA owns the PMD, and that is exactly the
> restriction in the way.  The alignment is the symptom; the PMD is the
> design.

I disagree with "A PMD-granular operation is only safe when one VMA owns the
PMD". It's safe when all page table walkers can be stopped (see above).

> 
> So both roots have to go.
> 
> Design
> ======
> 
> The old mechanism holds the address space still because it has nothing
> else stopping the sources from moving under the copy.  The new engine
> makes the sources themselves inert instead, with the two barriers
> migration already uses, raised in that order:
> 
>   1. migration entries replace the source PTEs.  Faults and GUP-slow
>      now wait on the source folio's lock, which is taken before the
>      first entry becomes visible.
>   2. the source folio's refcount is frozen to its expected value.
>      GUP-fast, pfn walkers, reclaim, compaction and memory-failure all
>      fail folio_try_get() and back off.
> 
> Between the two, nothing can reach a source, so the copy runs with no
> lock held at all -- and the address space is left alone while it does.

Right. Concurrent MADV_DONTNEED can zap migration PTEs and other faults even
re-fault fresh anon folios. So that must be detected before replacing migration
entries again I guess.

> 
> What that removes from every collapse path:
> 
>   mmap_write_lock              -> mmap_read
>   anon_vma_lock_write()        -> nothing: an rmap walk needs the folio
>                                   locked, and the engine holds that lock
>                                   from freeze to putback
>   tlb_remove_table_sync_one()  -> nothing: one ranged flush per round
>   LRU isolation                -> nothing: sources are inert in place
> 

Not sure how you handle PMD collapse. I recall problems with migration entries
on the PMD level for non-folio things (we discussed something along these lines
also in the past).

> Working in windows rather than whole PMDs takes care of the other root.
> A sub-PMD window is collapsed under the page table lock, so a collapse
> disturbs only the window it collapses, and each candidate is validated

I recall us discussing that holding the PT lock for a longer collapse operation
(especially on 64k) is problematic. But I don't get all the details from your
description here.

> at its own order -- a window need only fit its own VMA.  A PMD-order
> candidate still has to own the whole PMD, which is the old rule kept
> where it is still needed.
> 
> Candidates are carried through the passes a batch at a time rather than
> one window at a time, so a round pays for its flush and its lock
> acquisitions once.

Now I am starting to feel that there are too many changes packed in a single
series :)

> 
> With the barriers holding the sources still, which read lock the engine
> takes stops being part of the design.  A round works inside a single
> VMA, so patches 43-49 switch it from mmap_read to per-VMA locking: an
> mmap_write elsewhere in the mm then stops waiting for a collapse that
> has nothing to do with it.  That block is the only part of the series
> that needs per-VMA locking to be unconditional, and it is a separate
> dependency (see below); everything before it runs under mmap_read and
> does not care.
> 
> Patch 7 sketches the engine as a comment naming every pass, what lock it
> takes and what it may sleep on; the details are there rather than here.
> 
> What falls out beyond the lock diet:
> 
>  - mTHP collapse in VMAs smaller than a PMD, which is the arm64 case
>    above: a 2M VMA on an arm64/64K machine collapses nothing today at
>    any order, and collapses to mTHP here.
>  - Hole and zeropage population at every order, so partially populated
>    windows collapse to mTHP under the same max_ptes_none policy as PMD.
>  - Sources come in spans -- any stretch of consecutive PTEs mapping
>    consecutive pages of one folio -- so partially mapped and scrambled
>    compound sources (the PTE-mapped-THP re-collapse class) work at
>    every order.
>  - A table that cannot become one huge page still yields the largest
>    windows inside it, where before a single disqualified PTE gave up
>    the whole table.
> 
> Reading the series
> ==================
> 
> 57 patches is a lot to land on a list.  They go in blocks:
> 
>   1-6    helpers and shared state: pte_folio(), pte_none_or_zero(),
>          mm/collapse.h, and the policy that replaces asking whether
>          khugepaged started a collapse
>   7-8    the engine's shape: entry points, a call-tree comment naming
>          every pass, and the scan filled in
>   9-23   the collapse half, top down: the round frame, then each pass
>          in turn, then selection and the retry store

I fail to parse this sentence.

>   24     per-candidate tracing, before the switch takes the old
>          tracepoints away
>   25-28  the switch: point the anon path at the engine, widen coverage
>          to sub-PMD VMAs, delete the mechanism it replaces
>   29-35  move what is left of collapse out of khugepaged.c, and
>          MADV_COLLAPSE into madvise.c
>   36-42  tracing: the engine's own events and trace header
>   43-49  per-VMA locking, and the mm reference that makes it safe
>   50-56  selftests for what the engine can now do
>   57     MAINTAINERS
> 
> The two patches worth reading first if you read nothing else are 7 (the
> design, as a comment naming the whole call tree) and 16 (the freeze,
> which is where the safety argument lives).
> 
> A possible split, if that helps:
> 
>   1-2    two mm helpers, pte_folio() and pte_none_or_zero().  Both
>          convert callers outside collapse and are useful on their own
>   3-27   the engine and the switch-over.  This is the smallest unit
>          that does anything: stop earlier and the tree carries an
>          engine nothing calls
>   28     remove the mechanism the engine replaces
>   29-42  moving what is left of collapse out of khugepaged.c, and the
>          engine's own tracepoints
>   43-49  per-VMA locking
>   50-57  selftests and MAINTAINERS
> 
> Keeping the removal separate leaves both engines in the tree with only
> the new one reachable, so the switch can be reverted on its own if
> something turns up.  The old mechanism is already carried that way for
> three patches inside the series, so this costs nothing but 975 lines of
> unreferenced code until 28 lands.  That safety net only lasts until the
> blocks after it land, though: once collapse has moved out of
> khugepaged.c and the locking has changed, reverting the switch no longer
> gives back a working old engine.

[...]

> Performance
> ===========
> 
> Measuring khugepaged is awkward.  It is a background daemon, so what
> matters is what a workload feels while it runs, not what the daemon
> reports about itself -- and the usual coverage instrument is no help
> below the PMD: smaps AnonHugePages only counts PMD-order folios, so it
> reads zero however much mTHP has been collapsed.
> 
> So I wrote "perf bench mem usemem" for this.  It touches a region while
> khugepaged works on it and reports the workload's own latency
> percentiles and throughput, against per-size counters that can see
> sub-PMD folios.  The branch is above; it is unposted and not a
> dependency.

Something more realistic might be running some workload in a VM whereby the VM
is getting collapsed by khugepaged.

[...]

> There may be a way out -- a PMD migration entry over the table during
> the window, so the CPU never caches a walk to shoot down -- but that
> means teaching every pmd-level walker a new kind of entry, and I have
> not tried it.

I think we discussed that in the past and it's absolutely nasty.

[...]

> Size
> ====
> 
> mm/ grows by 1915 lines net: 4475 added against 2560 deleted.
> 
That's quite a lot for something that reads like a cleanup at first.


Okay, let me read the other discussions.

-- 
Cheers,

David