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

[Zi Yan <ziy@nvidia.com>]

On 11 Sep 2026, at 11:24, Kiryl Shutsemau wrote:

> On Fri, Sep 11, 2026 at 10:57:46AM -0400, Zi Yan wrote:
>> 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(-)
>>>
>>
>> LGTM. Thanks.
>>
>> Reviewed-by: Zi Yan <ziy@nvidia.com>
>>
>> One question:
>>
>> What prevents us from doing:
>> while () {
>>     1. mmap_lock
>>     2. scan_pmd
>>     3. mmap_unlock, bail out if needed
>>     4. run_pmd
>> }
>>
>> for both cases? It improves readability. What is the downside of
>> dropping the lock during multiple scans?
>
> The scan/run ratio.
>
> Once memory is mostly huge nearly every scan refuses, and a refused
> table is cheap: one pmd read for SCAN_PMD_MAPPED, one PTE walk under the
> PTL otherwise.
>
> In your version of the collapse loop, mmap lock/unlock plus VMA
> revalidation would dominate the cost. It is not productive.
>
> Note that scan in khugepaged is bounded by pages_to_scan so we would not
> hog the lock.
>
>> for madvise_collapse(), I see mmap_read_lock is held when it is called,
>> so it can be dropped at the entry and the code makes sure it is held at
>> the exit.
>
> That makes every MADV_COLLAPSE report lock_dropped, including one on a
> range that is already huge, which today never lets the lock go. The same
> performance consideration as above.

Got it. Thank you for the explanation.

Best Regards,
Yan, Zi