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

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

On Thu, Nov 05, 2026 at 11:28:47AM +0100, David Hildenbrand (Arm) wrote:
> On 11/3/26 09:27, Lorenzo Stoakes (ARM) wrote:
> >  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.

Yeah, it's trying to find the least impactful way of saying 'we spent a lot of
time in this loop' more or less.

It was empirically figured out to be a good choice (and is very conservative,
obviously).

An alternative here would be to literally track how much time has been spent in
the loop and if exceeded drop/reacquire lock? That felt icky though.

Another option could be to check to see if cc->progress is, say, 1/4 of
khugepage_pages_to_scan, and then drop/reacquire them?

Since actual collapse drops the lock + exits the loop anyway that should work
OK.

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

Ack.

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

That could get tricky, the ineligible VMA rejections are all relatively quick
and in any case we don't even check the budget until we reach an eligible one.

The eligible ones being rejected potentially take longer but it could get
complicated.

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

Hmm yeah true, you might actually find an eligible VMA (spans a PMD) but then
for one of a bunch of different reasons skip it and only cc->progress++, even
though you maybe spent quite some time doing it.

Could keep it simple, something like:

	#define MAX_VMA_SCAN_BATCH	<whatever :)>

	unsigned long nr_vmas_batch = 0;

	...

	for_each_vma(vmi, vma) {
		unsigned long hstart, hend;

		if (nr_vmas_batch++ >= khugepaged_pages_to_scan / 4) {
			... drop + reacquire lock ...
			nr_vmas_batch = 0;
		}

		...
	}

Maybe? Could also potentially only drop the lock if
mmap_lock_is_contended()?

--
Cheers, Lorenzo