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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] mm/khugepaged: vm_end ALIGN() overflow rewinds vmi in collapse_scan_mm_slot()
- [Medium] mm/khugepaged: sub-PMD VMAs bypass skip batching in collapse_scan_mm_slot()
--

commit 0a737288374fc8ce4ac1b20ac082b7190ad6db37
Author: Lorenzo Stoakes (ARM) <ljs@kernel.org>

mm/khugepaged: drop/reacquire mmap read lock if skipped enough VMAs

This commit drops and reacquires the mmap read lock using
mmap_read_lock_nosteal() in collapse_scan_mm_slot() after skipping
KHUGEPAGED_MAX_SKIP_BATCH ineligible VMAs to reduce mmap_lock hold times and
relieve writer contention.

> diff --git a/mm/khugepaged.c b/mm/khugepaged.c
> index 216c4b8d28c40..b9c72ee8b4211 100644
> --- a/mm/khugepaged.c
> +++ b/mm/khugepaged.c

[ ... ]

> @@ -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 skipped = 0;
>  
>  	lockdep_assert_held(&khugepaged_mm_lock);
>  	*result = SCAN_FAIL;
> @@ -2951,6 +2958,14 @@ static void collapse_scan_mm_slot(unsigned int progress_max,
>  			/* Unable to collapse, skip to the next candidate. */
>  			mm_slot->address = ALIGN(vma->vm_end, HPAGE_PMD_SIZE);
>  			cc->progress++;
> +			if (++skipped >= KHUGEPAGED_MAX_SKIP_BATCH) {
> +				/* Skipped a lot of VMAs, so wait for writers. */
> +				mmap_read_unlock(mm);
> +				cond_resched();
> +				mmap_read_lock_nosteal(mm);
> +				vma_iter_set(&vmi, mm_slot->address);
> +				skipped = 0;
> +			}

[Severity: High]
Can mm_slot->address overflow to 0 here and cause vma_iter_set() to rewind
vmi back to the start of the address space?

On architectures where TASK_SIZE extends into the final PMD range of the
unsigned long address space, such as 64-bit s390 in
arch/s390/include/asm/processor.h:

#define TASK_SIZE		(TASK_SIZE_MAX)
#define TASK_UNMAPPED_BASE	(_REGION2_SIZE >> 1)
#define TASK_SIZE_MAX		(-PAGE_SIZE)

a user process can map VMAs with vma->vm_end > (ULONG_MAX & HPAGE_PMD_MASK).
When collapse_scan_mm_slot() skips such a VMA,
ALIGN(vma->vm_end, HPAGE_PMD_SIZE) wraps around to 0.

If ++skipped >= KHUGEPAGED_MAX_SKIP_BATCH triggers on that VMA,
vma_iter_set(&vmi, mm_slot->address) resets the VMA iterator to address 0.
Because the !collapse_possible_orders() path does not check
cc->progress >= progress_max, if the mm contains no eligible PMD-spanning
VMAs (for example, 1024 ineligible VMAs ending in the top PMD range),
wouldn't for_each_vma(vmi, vma) restart from address 0 and loop
indefinitely inside collapse_scan_mm_slot() until the process exits or THP
is disabled?

>  			continue;
>  		}
>  		hstart = ALIGN(vma->vm_start, HPAGE_PMD_SIZE);

[Severity: Medium]
Should the skip batching and progress accounting also cover VMAs that pass
collapse_possible_orders() under sub-PMD mTHP but do not span a full PMD?

In mm/khugepaged.c, collapse_possible_orders() checks all enabled anonymous
mTHP orders when called with TVA_KHUGEPAGED:

static unsigned long collapse_possible_orders(struct vm_area_struct *vma,
		vm_flags_t vm_flags, enum tva_type tva_flags)
{
	unsigned long orders;

	/* If khugepaged is scanning an anonymous vma, allow mTHP collapse */
	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);
}

When a sub-PMD anonymous mTHP order is enabled, a faulted anonymous VMA
smaller than HPAGE_PMD_SIZE passes collapse_possible_orders(), bypassing
the !collapse_possible_orders() block in collapse_scan_mm_slot() and
falling through to:

		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;
		VM_WARN_ON_ONCE(mm_slot->address & ~HPAGE_PMD_MASK);

		while (mm_slot->address < hend) {

For these sub-PMD VMAs, hstart >= hend:
- When mm_slot->address > hend, it increments cc->progress and continues
  without incrementing skipped or dropping mmap_lock.
- When mm_slot->address <= hend (for instance, when a sub-PMD VMA starts on
  a PMD boundary so hstart == hend == mm_slot->address, or on the first
  unaligned sub-PMD VMA in a PMD range where mm_slot->address == hend <
  hstart), both mm_slot->address > hend and mm_slot->address < hend are
  false, so neither skipped nor cc->progress is incremented.

As a result, if an mm has a large number of small faulted anonymous VMAs
while sub-PMD mTHP is enabled, wouldn't collapse_scan_mm_slot() still walk
all of them without ever reaching KHUGEPAGED_MAX_SKIP_BATCH and dropping
mmap_lock?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261103-khugepaged-latency-fix-v1-0-a07ab16ac860@kernel.org?part=5