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

[David Hildenbrand (Arm)]

On 9/11/26 17:56, 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.

Good, I only skimmed it and read "[PATCH v2 09/12] mm/collapse: open-code
collapse_single_pmd() in its two callers".



> 
> 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.
> 

Makes sense. I'll get to this next week!

-- 
Cheers,

David