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

["David Hildenbrand (Arm)" <david@kernel.org>]

On 11/9/26 15:12, Lorenzo Stoakes (ARM) wrote:
> On Mon, Nov 09, 2026 at 03:01:29PM +0100, David Hildenbrand (Arm) wrote:
>> On 11/6/26 13:07, Lorenzo Stoakes (ARM) wrote:
>>> On reflection, I think this would be better implemented as something like:
>>>
>>> static void collapse_scan_mm_slot(unsigned int progress_max, ...)
>>> {
>>> 	unsigned int long lock_start = jiffies;
>>> 	...
>>>   	for_each_vma(vmi, vma) {
>>> 		...
>>>                 if (time_after(jiffies, lock_start) && mmap_lock_is_contended(mm)) {
>>>                         mmap_read_unlock(mm);
>>>                         cond_resched();
>>>                         mmap_read_lock(mm);
>>>                         lock_start = jiffies;
>>>                         vma_iter_set(&vmi, mm_slot->address);
>>>                         continue;
>>>                 }
>>> 	}
>>> }
>>>
>>> Which avoids magic numbers, and means if we hold the lock for 1-10ms (depending
>>> on HZ), which is a long time in the kernel to hold a lock, AND the lock is
>>> contended, we drop/reacquire.
>>
>> That approach indeed sounds better to me regarding lock contention.
> 
> Unfortunately it makes things measurably worse (it's not fine-grained
> enough). Trying to get a fine-grained timer will involve preemption
> disable/enable pairs, so a no go.

Ah. :(

> 
> This is the alternative I have locally which explains the number, puts VMA in
> the name and it applies to _all_ VMAs as requested.
> 
> Let me know what you think!
> 
> The jiffies version would obviously just be this but with the above, but that
> one results in max ~2 ms-level stalls rather than ~200us.
> 
> diff --git a/mm/khugepaged.c b/mm/khugepaged.c
> index 7d1414a9b2a5..59bbef61f104 100644
> --- a/mm/khugepaged.c
> +++ b/mm/khugepaged.c
> @@ -66,6 +66,13 @@ 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. This bounds the maximum mmap read lock to an estimated ~100 -
> + * 300us depending on hardware.
> + */
> +#define KHUGEPAGED_MAX_VMA_SCAN_BATCH 1024
> +
>  /**
>   * struct khugepaged_mm_slot - khugepaged information per mm that is being scanned
>   * @slot: hash lookup from mm to mm_slot
> @@ -2910,6 +2917,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,6 +2950,18 @@ 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) {
> +			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;
> +			}
> +		}
> +
>  		cond_resched();
>  		if (unlikely(collapse_test_exit_or_disable(mm))) {
>  			cc->progress++;

Looks better to me than the original.

@Kiryl, we talked about dropping the mmap lock recently, would this create a
regression for your scanning use-cases?

-- 
Cheers,

David