Re: [PATCH v2 09/12] mm/collapse: open-code collapse_single_pmd() in its two callers

["Zi Yan" <ziy@nvidia.com>]

On Thu Sep 10, 2026 at 8:02 AM EDT, Kiryl Shutsemau wrote:
> From: "Kiryl Shutsemau (Meta)" <kas@kernel.org>
>
> collapse_scan_pmd() and collapse_run_pmd() each have a clear locking
> contract.  The scan is called with mmap_lock held for reading and returns
> with it still held.  The collapse is called without it.
>
> collapse_single_pmd() kept that boundary inside itself.  It dropped the
> lock on some paths and not others, and reported which by way of a bool its
> callers had to carry along and then act on.
>
> Open-code it in the two callers.  Each scans under the lock it already
> holds and, on SCAN_SUCCEED, gives the lock up before running the collapse.
> khugepaged's lock_dropped and madvise_collapse()'s mmap_unlocked both go:
> the code dropping the lock is now the code that wanted to know.
>
> khugepaged's walk carries on to the next table while the scan keeps
> refusing, and ends once a collapse has taken the lock from under it.
> madvise_collapse() re-finds its VMA after a collapse, which it did before,
> and now uses a NULL vma to say that it has to.  It still reports the drop
> to its own caller, from the line that does it.
>
> The lock is given up and taken again at the same points as before.  No
> functional change.
>
> Assisted-by: LLM
> Signed-off-by: Kiryl Shutsemau (Meta) <kas@kernel.org>
> ---
>  mm/khugepaged.c | 102 +++++++++++++++++++++++-------------------------
>  1 file changed, 49 insertions(+), 53 deletions(-)
>
> diff --git a/mm/khugepaged.c b/mm/khugepaged.c
> index c26907300c23..9bdf12128357 100644
> --- a/mm/khugepaged.c
> +++ b/mm/khugepaged.c
> @@ -2857,28 +2857,6 @@ static enum scan_result collapse_run_pmd(struct mm_struct *mm,
>  	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;
> -
> -	/* The collapse takes its own locks, so give this up */
> -	mmap_read_unlock(mm);
> -	*lock_dropped = true;
> -
> -	return collapse_run_pmd(mm, addr, cc);
> -}
> -

Sorry for walking back on this. I think collapse_single_pmd() can be
kept and still get patch 10 to 12 applied. The reason is that by looking at the
code after patch 11 is applied, the collapse_scan_pmd() +
collapse_run_pmd() patterns in madvise_collapse() and
collapse_scan_mm_slot() look very similar. And it can make
collapse_scan_pmd() and collapse_run_pmd() internal with only
collapse_single_pmd() exported.


-- 
Best Regards,
Yan, Zi