Re: [PATCH v2 3/3] mm/khugepaged: release contended mmap read lock after scanning enough VMAs

["Jose A. Perez de Azpillaga" <azpijr@gmail.com>]

On 11/6/26 11:03 AM, Lorenzo Stoakes (ARM) wrote:
> On Thu, Nov 05, 2026 at 10:53:13PM +0100, Jose A. Perez de Azpillaga wrote:
>> On 11/5/26 9:57 PM, Lorenzo Stoakes (ARM) wrote:
>>> khugepaged walks an mm's VMAs under the mmap read lock in
>>> collapse_scan_mm_slot(), checking for ineligible VMAs first, and skipping
>>> them if encountered.
>>>
>>> It charges cc->progress 1 page against the /sys/.../pages_to_scan budget
>>> when these are encountered, however only checking this budget once an
>>> eligible PMD-spanning VMA is encountered.
>>>
>>> It can therefore hold the mmap read lock for a long time if there are a
>>> large number of ineligible VMAs present in an mm, over which time no mmap
>>> write locks (and thus no VMA write locks) can be acquired.
>>>
>>> Address this by checking whether the mmap lock is contended after every
>>> KHUGEPAGED_MAX_VMA_SCAN_BATCH VMAs walked, that is a quarter of the
>>> pages_to_scan budget, and if so dropping and re-acquiring the mmap read
>>> lock before resuming the walk from the per-mm cursor.
>>>
>>> As the walk is resumed from the cursor rather than the VMA that was current
>>> when the lock was dropped, the cursor must never move backwards. A VMA
>>> ending at the top of the address space causes ALIGN() to wrap the cursor to
>>> zero, so treat such a VMA as the last in the mm rather than skipping past
>>> it.
>>>
>>> This was tested locally with ~1M ineligible VMAs mapped ahead of a
>>> collapsible region of memory, and resulted in a reduced mmap_lock hold time
>>> of 170us down from 95ms.
>>>
>>> This was also observed to reduce mmap_lock stalls in concurrent threads of
>>> a similar duration. Note that a reader re-acquiring the lock can currently
>>> steal it from a writer that is queued but not yet running, so the full
>>> benefit to writers depends on the separate rwsem change at [0].
>>>
>>> Local measurements indicated no change for ordinary use cases, so this is
>>> all upside.
>>>
>>> This work came out of investigations into mmap stalls when getting latency
>>> numbers for an unrelated patch series.
>>>
>>> [0]: https://lore.kernel.org/all/20261105-rwsem-dont-steal-from-queued-writer-v1-1-275e070f426f@kernel.org/
>>>
>>> Signed-off-by: Lorenzo Stoakes (ARM) <ljs@kernel.org>
>>> ---
>>>  mm/khugepaged.c | 31 +++++++++++++++++++++++++++++--
>>>  1 file changed, 29 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/mm/khugepaged.c b/mm/khugepaged.c
>>> index 216c4b8d28c4..eaa01dae5a03 100644
>>> --- a/mm/khugepaged.c
>>> +++ b/mm/khugepaged.c
>>> @@ -66,6 +66,12 @@ static DEFINE_READ_MOSTLY_HASHTABLE(mm_slots_hash, MM_SLOTS_HASH_BITS);
>>>
>>>  static struct kmem_cache *mm_slot_cache __ro_after_init;
>>>
>>> +/*
>>> + * The number of VMAs to walk before checking for, and relieving, mmap lock
>>> + * contention, as a function of the pages_to_scan budget.
>>> + */
>>> +#define KHUGEPAGED_MAX_VMA_SCAN_BATCH(pages_to_scan) ((pages_to_scan) / 4)
>>> +
>>>  /**
>>>   * struct khugepaged_mm_slot - khugepaged information per mm that is being scanned
>>>   * @slot: hash lookup from mm to mm_slot
>>> @@ -2910,6 +2916,7 @@ static void collapse_scan_mm_slot(unsigned int progress_max,
>>>  	struct mm_struct *mm;
>>>  	struct vm_area_struct *vma;
>>>  	unsigned int progress_prev = cc->progress;
>>> +	unsigned int nr_vmas_batch = 0;
>>>
>>>  	lockdep_assert_held(&khugepaged_mm_lock);
>>>  	*result = SCAN_FAIL;
>>> @@ -2942,15 +2949,35 @@ static void collapse_scan_mm_slot(unsigned int progress_max,
>>>  	for_each_vma(vmi, vma) {
>>>  		unsigned long hstart, hend;
>>>
>>> +		/* Relieve mmap lock contention after walking a batch of VMAs. */
>>> +		if (nr_vmas_batch++ >= KHUGEPAGED_MAX_VMA_SCAN_BATCH(progress_max)) {
>>> +			nr_vmas_batch = 0;
>>> +			if (mmap_lock_is_contended(mm)) {
>>> +				mmap_read_unlock(mm);
>>> +				cond_resched();
>>> +				mmap_read_lock(mm);
>>> +				vma_iter_set(&vmi, mm_slot->address);
>>> +				continue;
>>> +			}
>>> +		}
>>> +
>>
>> with pages_to_scan below 4 the batch is 0, so under constant contention
>> we keep dropping the lock at the cursor and never scan anything, don't we?
> 
> This feels like repackaged AI review...
> 
> The mmap lock won't be eternally contended and if it is, with pages_to_scan
> under 4, I think that's fine.
> 
> Alternatively could make that >= an == and the post-++ a pre-++ and the 0 will
> never be hit in this case.
> 
> Or could have a minimum cap.

with >= a minimum cap needs to be 2, since the cursor can stop at hend
inside the VMA we just scanned and the walk lands on it again after the
drop. with == only batch - 1 VMAs get scanned per cycle, so a batch of 1
scans nothing, today's batch-0 loop at pages_to_scan 4..7, and a batch
of 2 hits the hend case. it's safe from 3 up.

-- 
cheers,
jose a. p-a