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

["Lorenzo Stoakes (ARM)" <ljs@kernel.org>]

On Fri, Nov 06, 2026 at 08:58:05PM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential
> issues to consider:
> - [High] mm/khugepaged: lack of forward progress in
>   collapse_scan_mm_slot()
> - [Medium] mm/khugepaged: sub-PMD mTHP VMAs bypass cursor advance and
>   overflow check
> --
>
> commit 9bc41c7859cdd4759479e883a9c557ea64a1a2c7
> Author: Lorenzo Stoakes (ARM) <ljs@kernel.org>
>
> mm/khugepaged: release contended mmap read lock after scanning enough VMAs
>
> This commit periodically checks whether mmap_lock is contended in
> collapse_scan_mm_slot() after scanning a batch of VMAs, dropping and
> re-acquiring the read lock and resetting the VMA iterator to
> mm_slot->address. It also checks for unsigned long overflow when aligning
> vma->vm_end for ineligible VMAs.
>
> > diff --git a/mm/khugepaged.c b/mm/khugepaged.c
> > index 216c4b8d28c40..eaa01dae5a030 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;
> > +			}
> > +		}
>
> [Severity: High]
> Can collapse_scan_mm_slot() fail to make forward progress here when
> mmap_lock is contended?
>
> In pages_to_scan_store(), khugepaged_pages_to_scan can be set to any positive
> value:
>
> mm/khugepaged.c:pages_to_scan_store() {
> 	err = kstrtouint(buf, 10, &pages);
> 	if (err || !pages)
> 		return -EINVAL;
>
> 	khugepaged_pages_to_scan = pages;
> }
>
> When khugepaged_pages_to_scan (passed as progress_max) is 1, 2, or 3,
> KHUGEPAGED_MAX_VMA_SCAN_BATCH(progress_max) evaluates to 0. Because the batch
> check runs at the top of the for_each_vma() loop before
> collapse_test_exit_or_disable(mm) or any VMA handling, nr_vmas_batch++ >= 0
> triggers on the first VMA before any VMA is scanned. If mmap_lock_is_contended()
> is true, it resets nr_vmas_batch to 0, drops and re-acquires mmap_read_lock(mm),
> rewinds vmi to the unchanged mm_slot->address, and executes continue without
> advancing mm_slot->address, incrementing cc->progress, or checking
> collapse_test_exit_or_disable(mm).
>
> Even when progress_max >= 4, for an eligible VMA whose vm_end is not aligned
> to HPAGE_PMD_SIZE:
>
> mm/khugepaged.c:collapse_scan_mm_slot() {
> 		hstart = ALIGN(vma->vm_start, HPAGE_PMD_SIZE);
> 		hend = ALIGN_DOWN(vma->vm_end, HPAGE_PMD_SIZE);
> 		if (mm_slot->address > hend) {
> 			cc->progress++;
> 			continue;
> 		}
> 		if (mm_slot->address < hstart)
> 			mm_slot->address = hstart;
> 		...
> 		while (mm_slot->address < hend) {
> 			...
> 			mm_slot->address += HPAGE_PMD_SIZE;
> 			...
> 		}
> }
>
> Once the inner loop finishes (or if hstart == hend), mm_slot->address is left
> at hend, which is less than vma->vm_end and still lies inside that VMA. If the
> batch contention check triggers at the start of the next VMA (for example when
> progress_max is 4..7 so the batch threshold is 1), vma_iter_set(&vmi,
> mm_slot->address) rewinds the iterator back to the previous VMA where
> mm_slot->address == hend, which then does no work and neither advances
> mm_slot->address nor increments cc->progress.
>
> > +
> >  		cond_resched();
> >  		if (unlikely(collapse_test_exit_or_disable(mm))) {
> >  			cc->progress++;
> >  			break;
> >  		}
> >  		if (!collapse_possible_orders(vma, vma->vm_flags, TVA_KHUGEPAGED)) {
> > -			/* Unable to collapse, skip to the next candidate. */
> > -			mm_slot->address = ALIGN(vma->vm_end, HPAGE_PMD_SIZE);
> > +			const unsigned long next =
> > +				ALIGN(vma->vm_end, HPAGE_PMD_SIZE);
> > +
> >  			cc->progress++;
> > +			/* Check for overflow. */
> > +			if (unlikely(next < vma->vm_end)) {
> > +				vma = NULL;
> > +				break;
> > +			}
> > +			/* Unable to collapse, skip to the next candidate. */
> > +			mm_slot->address = next;
> >  			continue;
> >  		}
> >  		hstart = ALIGN(vma->vm_start, HPAGE_PMD_SIZE);
>

This is already addressed in newer revision.

> [Severity: Medium]
> As reported by Sashiko on v1 [1], what happens if an anonymous VMA does not
> span a full PMD (so hstart >= hend), but sub-PMD anonymous mTHP orders are
> enabled?
>
> In collapse_possible_orders(), anonymous VMAs are checked against
> THP_ORDERS_ALL_ANON:
>
> mm/khugepaged.c:collapse_possible_orders() {
> 	if ((tva_flags == TVA_KHUGEPAGED) && vma_is_anonymous(vma))
> 		orders = THP_ORDERS_ALL_ANON;
> 	else
> 		orders = BIT(HPAGE_PMD_ORDER);
>
> 	return thp_vma_allowable_orders(vma, vm_flags, tva_flags, orders);
> }
>
> A faulted-in anonymous VMA that is smaller than HPAGE_PMD_SIZE (or does not
> span a full PMD) will return non-zero from collapse_possible_orders() when a
> sub-PMD order is enabled, skipping the !collapse_possible_orders() block:
>
> 1. If the VMA starts at or straddles a PMD boundary (so hstart == hend), it
>    sets mm_slot->address = hstart (< vma->vm_end), skips the inner while
>    (mm_slot->address < hend) loop without incrementing cc->progress, and
>    leaves mm_slot->address inside the VMA, so vma_iter_set(&vmi,
>    mm_slot->address) will rewind back to this VMA on contention.
>
> 2. On architectures where user mappings can reach the top incomplete PMD range
>    above ULONG_MAX - HPAGE_PMD_SIZE + 1 (such as 64-bit s390 where TASK_SIZE
>    is -PAGE_SIZE), a sub-PMD anonymous VMA with vm_start > ULONG_MAX -
>    HPAGE_PMD_SIZE + 1 bypasses the next < vma->vm_end overflow check above.
>    Then hstart = ALIGN(vma->vm_start, HPAGE_PMD_SIZE) wraps to 0 while hend =
>    ALIGN_DOWN(vma->vm_end, HPAGE_PMD_SIZE) is ULONG_MAX - HPAGE_PMD_SIZE + 1,
>    causing the while (mm_slot->address < hend) loop to run from address 0 with
>    the wrong VMA and rewind mm_slot->address.
>
> Should the hstart >= hend case also advance mm_slot->address to
> ALIGN(vma->vm_end, HPAGE_PMD_SIZE) with the overflow check before computing
> hstart?

This seems liek a pre-existing issue. Are they being snuck in by not being
flagged as such now? ;)

>
> [1] https://lore.kernel.org/r/20261103-khugepaged-latency-fix-v1-5-a07ab16ac860@kernel.org
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20261105-khugepaged-latency-fix-v2-0-4384bbfe4606@kernel.org?part=3

--
Cheers, Lorenzo