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

[David Hildenbrand (Arm)]

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.

E.g.,:

commit a155d945b73c5b0668e898df5495afe45bb261cd
Author: Nico Pache <nico.pache@xxxxxxxxx>
Date:   Wed Mar 25 05:40:22 2026 -0600

    mm/khugepaged: unify khugepaged and madv_collapse with collapse_single_pmd()

    The khugepaged daemon and madvise_collapse have two different
    implementations that do almost the same thing.  Create collapse_single_pmd
    to increase code reuse and create an entry point to these two users.

    Refactor madvise_collapse and collapse_scan_mm_slot to use the new
    collapse_single_pmd function.  To help reduce confusion around the
    mmap_locked variable, we rename mmap_locked to lock_dropped in the
    collapse_scan_mm_slot() function, and remove the redundant mmap_locked in
    madvise_collapse(); this further unifies the code readiblity.  the
    SCAN_PTE_MAPPED_HUGEPAGE enum is no longer reachable in the
    madvise_collapse() function, so we drop it from the list of "continuing"
    enums.

    This introduces a minor behavioral change that is most likely an
    undiscovered bug.  The current implementation of khugepaged tests
    collapse_test_exit_or_disable() before calling collapse_pte_mapped_thp,
    but we weren't doing it in the madvise_collapse case.  By unifying these
    two callers madvise_collapse now also performs this check.  We also modify
    the return value to be SCAN_ANY_PROCESS which properly indicates that this
    process is no longer valid to operate on.

-- 
Cheers,

David