Re: [PATCH v2 08/12] mm/collapse: separate scanning a PTE table from collapsing it

[Zi Yan]

On 11 Sep 2026, at 9:37, Kiryl Shutsemau wrote:

> On Thu, Sep 10, 2026 at 10:38:13PM -0400, Zi Yan wrote:
>> On Thu Sep 10, 2026 at 8:02 AM EDT, Kiryl Shutsemau wrote:
>>> From: "Kiryl Shutsemau (Meta)" <kas@xxxxxxxxxx>
>>>
>>> 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 and only reads.  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.
>>>     SCAN_SUCCEED from the scan means there is something to run; anything
>>>     else is why there is not.
>>>
>>> collapse_single_pmd() is now the two of them with the mmap_lock drop in
>>> between, so its callers see what they saw before.
>>>
>>> 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.  For a file it is the file itself, the offset in it, and whether
>>> the PMD folio is already in the page cache.
>>>
>>> The file side moves with the anonymous one.  collapse_scan_file() used to
>>> run with mmap_lock already given up, and called collapse_file() itself
>>> when the page cache looked worth it.  It now runs under the lock like the
>>> anonymous scan and only judges; the run does the collapse.  A file
>>> collapse works on the page cache and never sees a VMA, so the scan takes
>>> the file reference while it still has one and the run gives it back.
>>>
>>> That changes what a refused file table costs khugepaged.  Every file
>>> table it scanned used to end its pass over that mm, because the lock had
>>> been dropped to scan it; now only a table it goes on to collapse does.
>>>
>>> Two things on the file side stop being rescanned.  When the page cache
>>> already holds the PMD folio, the scan says so and the run goes straight
>>> to retracting the PTE table.  A run that refuses dirty pages and may
>>> write them back retries collapse_file() alone.  The checks the scan makes
>>> ahead of it are ones collapse_file() repeats under the page cache lock.
>>>
>>> Tracing changes with it.  mm_khugepaged_scan_pmd and
>>> mm_khugepaged_scan_file used to fire after the collapse, so for an
>>> accepted table their status field carried what the collapse made of it.
>>> They now fire before it and read SCAN_SUCCEED for an accepted table.  What
>>> the collapse then made of it is for mm_collapse_huge_page and
>>> mm_khugepaged_collapse_file to report.
>>>
>>> Assisted-by: LLM
>>> Signed-off-by: Kiryl Shutsemau (Meta) <kas@xxxxxxxxxx>
>>> ---
>>>  mm/collapse.h   |  16 ++++++
>>>  mm/khugepaged.c | 147 ++++++++++++++++++++++++++++++++++++------------
>>>  2 files changed, 128 insertions(+), 35 deletions(-)
>>>
>>> diff --git a/mm/collapse.h b/mm/collapse.h
>>> index 7044dc71c7c2..346859a2184f 100644
>>> --- a/mm/collapse.h
>>> +++ b/mm/collapse.h
>>> @@ -88,6 +88,22 @@ struct collapse_control {
>>>
>>>  	/* Each bit marks a PTE the scan accepted as a collapse source */
>>>  	DECLARE_BITMAP(eligible_ptes, MAX_PTRS_PER_PTE);
>>> +
>>> +	/*
>>> +	 * What a scan found and the run after it needs.  Live only between the
>>> +	 * two, and read by nobody else.
>>> +	 *
>>> +	 * The file side takes a reference while it still has the VMA, since a
>>> +	 * file collapse works on the page cache and never sees one; the run is
>>> +	 * what gives it back.  A scan that found the PMD folio already in the
>>> +	 * cache leaves only the PTE table to retract.
>>> +	 */
>>> +	unsigned long scan_orders;
>>> +	int scan_referenced;
>>> +	int scan_unmapped;
>>> +	struct file *scan_file;
>>> +	pgoff_t scan_pgoff;
>>> +	bool scan_retract_only;
>>
>> scan_retract_pte_only ?
>
> It is the PTE table that gets retracted, not a PTE, and
> scan_retract_pte_table_only is too long for a field read in one place.
>
> But with your suggestion below the field goes away, so the name does
> too.
>
>>> -	mmap_assert_locked(mm);
>>> +	mmap_assert_locked(vma->vm_mm);
>>> +	/* Whatever the last scan found has to have been run by now */
>>> +	if (WARN_ON_ONCE(cc->scan_file)) {
>>> +		fput(cc->scan_file);
>>> +		cc->scan_file = NULL;
>>> +	}
>>
>> scan_file should be set to NULL by collapse_control_init(). Anyway, the
>> code is duplicated here and in collapse_control_release(), maybe add a
>> helper.
>
>
> collapse_control_init() does set it to NULL. This check is for a scan
> that found work and was never run, which no caller does today but the
> engine on top of this will scan many tables before it runs any.
>
> Both copies become one helper in the diff below.
>
>>> +retract:
>>>  	fput(file);
>>>
>>> +	/*
>>> +	 * A PMD folio is in the page cache, whether the collapse just put it
>>> +	 * there or found it: retract the PTE table, and map the PMD if asked.
>>> +	 */
>>>  	if (result == SCAN_PTE_MAPPED_HUGEPAGE) {
>>>  		mmap_read_lock(mm);
>>>  		if (collapse_test_exit_or_disable(mm))
>>
>> result is changed from SCAN_PTE_MAPPED_HUGEPAGE to SCAN_SUCCEED to
>> SCAN_PTE_MAPPED_HUGEPAGE to get here. Is there a way of avoiding this
>> result churn?
>>
>>
>>> @@ -2805,6 +2857,28 @@ static enum scan_result collapse_single_pmd(unsigned long addr,
>>>  	return result;
>>>  }
>>>
>>> +/*
>>> + * Try to collapse a single PMD starting at a PMD aligned addr, and return
>>> + * the results.
>>> + */
>>> +static enum scan_result collapse_single_pmd(unsigned long addr,
>>> +		struct vm_area_struct *vma, bool *lock_dropped,
>>> +		struct collapse_control *cc)
>>> +{
>>> +	struct mm_struct *mm = vma->vm_mm;
>>> +	enum scan_result result;
>>> +
>>> +	result = collapse_scan_pmd(vma, addr, cc);
>>> +	if (result != SCAN_SUCCEED)
>>> +		return result;
>>
>> Can it be changed to?
>>
>> if (result != SCAN_SUCCEED && result != SCAN_PTE_MAPPED_HUGEPAGE)
>> 	return result;
>
> Yes. The scan returns SCAN_PTE_MAPPED_HUGEPAGE as it is, both callers
> treat it as work for the run, and collapse_run_pmd() takes the scan's
> result as an argument and goes straight to the retract when it sees it.
> That removes the flag and the round trip in one go.
>
> The diff below is against the whole series; for v3 it gets folded into
> the patches that introduced each piece.
>
> Looks good?
>

Yep, feel free to add

Reviewed-by: Zi Yan <ziy@xxxxxxxxxx>

in your next version.  Thanks.

I am going to check the remaining patches.

Best Regards,
Yan, Zi