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

[Zi Yan]

On 14 Sep 2026, at 7:47, Kiryl Shutsemau wrote:

> On Fri, Sep 11, 2026 at 06:09:32PM -0400, Zi Yan wrote:
>> On Thu Sep 10, 2026 at 8:02 AM EDT, Kiryl Shutsemau wrote:
>>> From: "Kiryl Shutsemau (Meta)" <kas@xxxxxxxxxx>
>>>
>>> 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@xxxxxxxxxx>
>>> ---
>>>  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.
>
> I gave more motivation in my reply to David:
>
> https://lore.kernel.org/all/aqQf9hSy0iNjnL6t@thinkstation/
>
> Short version: scan and run have different locking expectations, and
> the split puts the boundary where the lock is. It is also what makes
> moving the scan to per-VMA locking trivial, since the caller owns the
> lock and the engine never touches it.

Thanks. You also explained this to me in my early email to patch 9.
It makes sense.

My motivation is to minimize code divergence of madvise_collapse and khugepaged.
But it seems that their workflows prevent the unification.

Best Regards,
Yan, Zi