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

On Wed, Aug 19, 2026 at 07:08:07PM +0100, Kiryl Shutsemau wrote:
> On Tue, Aug 18, 2026 at 04:12:17PM +0200, David Hildenbrand (Arm) wrote:
> > I think we all agree that there is a lot of room for improvement, but the big
> > question is:
> >
> > (a) When does it stop being a cleanup and is a new feature in disguise that
> >     makes the code more complicated and even harder to maintain.
> >
> > (b) Can it just naturally be made looking like a cleanup.
> >
> > Ideally, we'd get b), in small, nice-to-review chunks that incrementally improve
> > the code without inflating it heavily or moving everything around.
>
> It is not a cleanup and I would rather not sell it as one.  It replaces a
> mechanism, so judged as (b) it fails by construction.
>
> I believe the end result is much cleaner. But I might be biased. :)
>
> > The current locking is nasty, so anything that moves us one step closer into
> > something that is not only simpler but also more scalable is nice. I am a bit
> > concerned with the churn in the series as is.
> >
> > After this series, mm/collapse.c itself is way larger than just mm/khugepaged.c
> > originally, which raises some eyebrows.
>
> Line count is a poor proxy for simplicity or scalability.  What the
> engine changes is the serialization model, and that is the part collapse
> needs changed: the PMD granularity and the exclusion both come out of
> the locking.
>
> Incremental does not reach it, though.  The old mechanism is correct
> because it holds mmap_write_lock, the anon_vma write lock and a reference
> from the LRU; the engine is correct because the sources are frozen behind
> migration entries.  There is no halfway state that is correct under both,
> so the switch lands as one patch.
>
> What can be incremental is everything around it: the engine goes in beside
> the old mechanism, patch 25 points the anon path at it, and 28 removes what
> it replaces.  Until 28 both are in the tree with only one of them
> reachable, so the switch can be reverted on its own.
>
> > We should also be aware that people are proposing file/shmem mTHP collapse, so
> > ideally what we refactor would naturally unify some of these code paths.
> >
> > I am wondering whether shmem mTHP collapse should come first. (I'm hoping that
> > shmem mTHP collapse can unify some of the anon+file collapse code in a nice way,
> > to similarly just look like a cleanup while enabling a new scenario.
>
> mTHP collapse as it stands has limited usability: PMD-aligned windows only,
> and one VMA has to own the PMD.  Bolting file collapse onto the same
> structure adds to the debt instead of paying it down.
>
> It would fit the new design.  The frame -- scan, candidate selection, the
> round and its passes -- has nothing anon-specific in it; what is
> anon-specific sits in the freeze (folio_test_anon(), PageAnonExclusive())
> and the unshare in the fault-in pass.  A file source would bring its own
> check, freeze, copy and install.
>
> I am not sure it should, though.
>
> Do we want to find file collapse candidates by walking the virtual
> address space at all?
>
> collapse_file() already works on the mapping -- it builds the folio in
> the page cache and then repairs every mapping through
> retract_page_tables() -- so the VMA walk only picks which inode range to
> try, and it reaches only what a registered mm maps right now.  Large
> folios buy more than TLB reach: fewer page cache entries, cheaper
> writeback, natural locking batch, etc.  Those apply whether the file is
> mapped or not, and going at the inode directly would reach them.
>
> > Agreed, I think we really should unify+cleanup the existing code first before
> > doing more drastic changes.
> >
> > Having a series that throws all of khugepaged.c into a mixer and pours something
> > new into collapse.c is ... concerning :)
>
> The moving around is patches 29-35 and the tracing after them.  None of it
> is needed for the engine: 1-28 add it, switch the anon path over and delete
> the old mechanism, without moving anything else out of khugepaged.c.  If
> the churn is the problem, v2 can stop there and the moves can come later as
> their own series.

This whole reply seems AI-generated...

You replying only to David twice in this sub-thread which isn't exactly giving
me warm fuzzy feelings about the working-around-me concerns I raised here.

So simple feedback - send a relatively small, no-functional-change series that
improves THP code and lays foundations for future changes. After the merge
window.

Can you explicitly ack this please?

>
> --
>   Kiryl Shutsemau / Kirill A. Shutemov

--
Cheers, Lorenzo