Re: [PATCH v4 08/13] mm/collapse: call collapse_file() from collapse_single_pmd()

[David Hildenbrand (Arm)]

On 9/28/26 12:06, Kiryl Shutsemau wrote:
> From: "Kiryl Shutsemau (Meta)" <kas@xxxxxxxxxx>
> 
> collapse_scan_file() reads the page cache to decide whether a table is
> worth collapsing and, when it is, calls collapse_file() itself.  The
> caller cannot get between the decision and the collapse.
> 
> Move the collapse_file() call up into collapse_single_pmd(), so the scan
> stops at the decision.
> 
> Two things change with it.  The writeback retry re-runs collapse_file()> alone instead of rescanning first; collapse_file() repeats the scan's
> checks under the page cache lock anyway.  

That change wasn't actually required for "call collapse_file() from
collapse_single_pmd()", right? You merely decided to move the retry label in the
same patch.

> And mm_khugepaged_scan_file
> fires before the collapse, so for an accepted table its status reads
> SCAN_SUCCEED; what the collapse made of the table is for
> mm_khugepaged_collapse_file to report.

Ack.

> 
> Preparation for splitting a collapse into a scan under mmap_lock and a
> run without it.  The file scan has to stop where the anonymous one will.
> 
> Assisted-by: LLM
> Signed-off-by: Kiryl Shutsemau (Meta) <kas@xxxxxxxxxx>
> ---
>  mm/khugepaged.c | 21 +++++++++++++--------
>  1 file changed, 13 insertions(+), 8 deletions(-)
> 
> diff --git a/mm/khugepaged.c b/mm/khugepaged.c
> index 60ec7e80d554..89a4c3f5a91c 100644
> --- a/mm/khugepaged.c
> +++ b/mm/khugepaged.c
> @@ -2737,13 +2737,9 @@ static enum scan_result collapse_scan_file(struct mm_struct *mm,
>  	else
>  		cc->progress += HPAGE_PMD_NR;
>  
> -	if (result == SCAN_SUCCEED) {
> -		if (present < HPAGE_PMD_NR - max_ptes_none) {
> -			result = SCAN_EXCEED_NONE_PTE;
> -			count_vm_event(THP_SCAN_EXCEED_NONE_PTE);
> -		} else {
> -			result = collapse_file(mm, addr, file, start, cc);
> -		}
> +	if (result == SCAN_SUCCEED && present < HPAGE_PMD_NR - max_ptes_none) {
> +		result = SCAN_EXCEED_NONE_PTE;
> +		count_vm_event(THP_SCAN_EXCEED_NONE_PTE);
>  	}
>  
>  	trace_mm_khugepaged_scan_file(mm, failed_pfn, file, present, swap, result);
> @@ -2774,8 +2770,16 @@ static enum scan_result collapse_single_pmd(unsigned long addr,
>  
>  	mmap_read_unlock(mm);
>  	*lock_dropped = true;
> -retry:
> +
> +	/*
> +	 * SCAN_PTE_MAPPED_HUGEPAGE is work too: the page cache already holds
> +	 * the PMD folio, and only the PTE table is left to retract.
> +	 */
>  	result = collapse_scan_file(mm, addr, file, pgoff, cc);
> +	if (result != SCAN_SUCCEED)
> +		goto put;
> +retry:
> +	result = collapse_file(mm, addr, file, pgoff, cc);
>  
>  	/* Dirty pages are worth a writeback and one more try, if asked for */
>  	if (cc->policy.file_writeback_dirty && result == SCAN_PAGE_DIRTY_OR_WRITEBACK &&
> @@ -2787,6 +2791,7 @@ static enum scan_result collapse_single_pmd(unsigned long addr,
>  		triggered_wb = true;
>  		goto retry;
>  	}
> +put:
>  	fput(file);
>  
>  	if (result == SCAN_PTE_MAPPED_HUGEPAGE) {

Acked-by: David Hildenbrand (Arm) <david@xxxxxxxxxx>

-- 
Cheers,

David