Re: [PATCH 5/5] mm/khugepaged: drop/reacquire mmap read lock if skipped enough VMAs

[David Hildenbrand (Arm)]

On 11/3/26 09:27, 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 8 * HPAGE_PMD_NR 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 dropping + re-acquiring the mmap read lock after
> KHUGEPAGED_MAX_SKIP_BATCH VMAs have been processed. Use the newly
> introduced mmap_read_lock_nosteal() to ensure queued writers take
> precedence when doing so.
> 
> 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.
> 
> KHUGEPAGED_MAX_SKIP_BATCH was derived empirically to a conservative value
> of 1024 VMAs, which should not interfere with mm's that have an ordinary
> number of VMAs.
> 
> 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.
> 
> Signed-off-by: Lorenzo Stoakes (ARM) <ljs@xxxxxxxxxx>
> ---
>  mm/khugepaged.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/mm/khugepaged.c b/mm/khugepaged.c
> index 216c4b8d28c4..b9c72ee8b421 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 with nothing to scan to walk before dropping and
> + * reacquiring the mmap_lock to relieve lock contention.
> + */
> +#define KHUGEPAGED_MAX_SKIP_BATCH 1024

I don't quite enjoy this magic number.

Besides it saying nothing about "VMAS", so the name it self is pretty unclear.

I was wondering whether we can give the "cc->progress += 1" a different weight.
Skipping VMAs seems to be more expensive than we thought.


(Maybe we should also just say "skip after X" VMAs in general, whether we
skipped them or not?)


-- 
Cheers,

David