Re: [PATCH v4 09/13] mm/collapse: separate scanning a PTE table from collapsing it

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

On 9/28/26 12:06, Kiryl Shutsemau wrote:
> From: "Kiryl Shutsemau (Meta)" <kas@kernel.org>
> 
> A collapse is two jobs.  One reads a PTE table under mmap_lock and decides
> whether the range is worth collapsing.  The other allocates, isolates,
> copies and flushes, and wants the lock given up first.
> 
> collapse_single_pmd() did both, so the boundary between them was somewhere
> in the middle of a function.
> 
> Give each half its own function:
> 
>   - collapse_scan_pmd() scans one table.  The anonymous scan that used to
>     carry that name keeps its body as collapse_scan_anon_pmd(), and
>     collapse_scan_pmd() is now the entry that picks the anonymous or the
>     file side.
> 
>   - collapse_run_pmd() does the collapse the scan asked for, and is
>     handed what the scan returned.  SCAN_SUCCEED means there is
>     something to collapse.  SCAN_PTE_MAPPED_HUGEPAGE means the page
>     cache already holds the PMD folio and only the PTE table is left to
>     retract.  Both are work for the run; anything else is why there is
>     nothing to do.

I raised that I don't like the "run" in collapse_run_pmd(). That hasn't changed,
as we are adding more inconsistency.

We do have after your patch set

	collapse_scan_pmd calling
	-> collapse_scan_file
	-> collapse_scan_anon_pmd

Inconsistency: why not simply collapse_scan_anon? Alternatively
collapse_scan_file_pmd, although for this internal helper including the "pmd" is
not particularly helpful.

The we have

	collapse_run_pmd calling
	-> mthp_collapse
	-> collapse_file

Inconsistency: a lot, including the "run" thing.


So I suggest the following in the context of this patch here:

	collapse_scan_pmd
	-> collapse_scan_file
	-> collapse_scan_anon

	collapse_pmd
	-> collapse_anon
	-> collapse_file

(as I previously said, "scanned" could be thrown in there, to make the
expectation clearer, but I don't particularly care about that)

> 
> collapse_single_pmd() is now the two of them with the mmap_lock drop in
> between, so its callers see what they saw before.  collapse_control_init()
> sets a control up before its first scan.

You should mention that collapse_single_pmd() is only temporary.

> 
> What the scan found and the run needs travels in collapse_control.  For
> an anonymous table that is the orders and the referenced and swapped-out
> counts, which mthp_collapse() and collapse_huge_page() now read from
> there instead of taking as arguments.  For a file it is the file itself
> and the offset in it: a file collapse works on the page cache and never
> sees a VMA, so the scan takes the reference while it still has one and
> the run gives it back.
> 
> The file scan moves under mmap_lock with the anonymous one, where before
> the lock was given up first.  The lock is now held over the page cache
> walk, an RCU walk over one table's worth of slots with no PTL, and taken
> fewer times.  collapse_scan_mm_slot() ends its walk whenever the lock was
> dropped, so a refused file table used to cost khugepaged an unlock, a
> trip back through khugepaged_do_scan(), a relock and a VMA lookup.  Now
> only a table that goes on to be collapsed does.

[...]

>  
> +/*
> + * Try to collapse a single PMD starting at a PMD aligned addr, and return
> + * the results.
> + */

I'd just not add that doc if you intend to remove the whole thing in the same
comment.


-- 
Cheers,

David