Re: [PATCH mm-unstable v3 5/5] mm/khugepaged: unify khugepaged and madv_collapse with collapse_single_pmd()

[Lorenzo Stoakes (Oracle)]

On Wed, Mar 18, 2026 at 11:22:07AM -0600, Nico Pache wrote:
>
>
> On 3/16/26 12:54 PM, Lorenzo Stoakes (Oracle) wrote:
> > On Wed, Mar 11, 2026 at 03:13:15PM -0600, Nico Pache wrote:
> >> The khugepaged daemon and madvise_collapse have two different
> >> implementations that do almost the same thing. Create collapse_single_pmd
> >> to increase code reuse and create an entry point to these two users.
> >
> > Ah this is nice :) Thanks!
>
> Thanks :) hopefully more khugepaged cleanups to come after these series' land.
>
> >
> >>
> >> Refactor madvise_collapse and collapse_scan_mm_slot to use the new
> >> collapse_single_pmd function. This introduces a minor behavioral change
> >> that is most likely an undiscovered bug. The current implementation of
> >> khugepaged tests collapse_test_exit_or_disable before calling
> >> collapse_pte_mapped_thp, but we weren't doing it in the madvise_collapse
> >> case. By unifying these two callers madvise_collapse now also performs
> >> this check. We also modify the return value to be SCAN_ANY_PROCESS which
> >> properly indicates that this process is no longer valid to operate on.
> >>
> >> By moving the madvise_collapse writeback-retry logic into the helper
> >> function we can also avoid having to revalidate the VMA.
> >>
> >> We also guard the khugepaged_pages_collapsed variable to ensure its only
> >> incremented for khugepaged.
> >>
> >> Signed-off-by: Nico Pache <npache@xxxxxxxxxx>
> >
> > The logic all seems correct to me, just a bunch of nits below really. This is
> > a really nice refactoring! :)
> >
> > With them addressed:
> >
> > Reviewed-by: Lorenzo Stoakes (Oracle) <ljs@xxxxxxxxxx>
>
> Thanks I will address those!
>
> >
> > Cheers, Lorenzo
> >
> >> ---
> >>  mm/khugepaged.c | 120 +++++++++++++++++++++++++-----------------------
> >>  1 file changed, 63 insertions(+), 57 deletions(-)
> >>
> >> diff --git a/mm/khugepaged.c b/mm/khugepaged.c
> >> index 33ae56e313ed..733c4a42c2ce 100644
> >> --- a/mm/khugepaged.c
> >> +++ b/mm/khugepaged.c
> >> @@ -2409,6 +2409,65 @@ static enum scan_result collapse_scan_file(struct mm_struct *mm,
> >>  	return result;
> >>  }
> >>
> >> +/*
> >> + * Try to collapse a single PMD starting at a PMD aligned addr, and return
> >> + * the results.
> >> + */
> >> +static enum scan_result collapse_single_pmd(unsigned long addr,
> >> +		struct vm_area_struct *vma, bool *mmap_locked,
> >
> > mmap_locked seems mildly pointless here, and it's a semi-code smell to pass 'is
> > locked' flags I think.
> >
> > You never read this, but the parameter implies somebody might pass in mmaplocked
> > == false, but you know it's always true here.
> >
> > Anyway I think it makes more sense to pass in lock_dropped and get rid of
> > mmap_locked in madvise_collapse() and just pass in lock_dropped directly
> > (setting it false if anon).
> >
> > Also obviously update collapse_scan_mm_slot() to use lock_dropped instead just
> > inverted.
> >
> > That's clearer I think since it makes it a verb rather than a noun and the
> > function is dictating whether or not the lock is dropped, it also implies the
> > lock is held on entry.
>
> Ok I will give this a shot!
>
> >
> >> +		struct collapse_control *cc)
> >> +{
> >> +	struct mm_struct *mm = vma->vm_mm;
> >> +	bool triggered_wb = false;
> >> +	enum scan_result result;
> >> +	struct file *file;
> >> +	pgoff_t pgoff;
> >> +
> >
> > Maybe move the mmap_assert_locked() from madvise_collapse() to here? Then we
> > assert it in both cases.
>
> ack, Sounds like a good idea!

Thanks + for above! :)

>
> >
> >> +	if (vma_is_anonymous(vma)) {
> >> +		result = collapse_scan_pmd(mm, vma, addr, mmap_locked, cc);
> >> +		goto end;
> >> +	}
> >> +
> >> +	file = get_file(vma->vm_file);
> >> +	pgoff = linear_page_index(vma, addr);
> >> +
> >> +	mmap_read_unlock(mm);
> >> +	*mmap_locked = false;
> >> +retry:
> >> +	result = collapse_scan_file(mm, addr, file, pgoff, cc);
> >> +
> >> +	/*
> >> +	 * For MADV_COLLAPSE, when encountering dirty pages, try to writeback,
> >> +	 * then retry the collapse one time.
> >> +	 */
> >> +	if (!cc->is_khugepaged && result == SCAN_PAGE_DIRTY_OR_WRITEBACK &&
> >> +	    !triggered_wb && mapping_can_writeback(file->f_mapping)) {
> >> +		const loff_t lstart = (loff_t)pgoff << PAGE_SHIFT;
> >> +		const loff_t lend = lstart + HPAGE_PMD_SIZE - 1;
> >> +
> >> +		filemap_write_and_wait_range(file->f_mapping, lstart, lend);
> >> +		triggered_wb = true;
> >> +		goto retry;
> >
> > Thinking through this logic I do agree that we don't need to revalidate here,
> > which should be quite a nice win, I just don't know why we previously assumed
> > we'd have to... or maybe it was just because it became too spaghetti to goto
> > around it somehow??
>
> I believe the latter, the retry went at the top of the loop, and the
> revalidation was already being done.

Yeah makes sense!

>
> >
> >> +	}
> >> +	fput(file);
> >> +
> >> +	if (result == SCAN_PTE_MAPPED_HUGEPAGE) {
> >> +		mmap_read_lock(mm);
> >> +		if (collapse_test_exit_or_disable(mm))
> >> +			result = SCAN_ANY_PROCESS;
> >> +		else
> >> +			result = try_collapse_pte_mapped_thp(mm, addr,
> >> +							     !cc->is_khugepaged);
> >> +		if (result == SCAN_PMD_MAPPED)
> >> +			result = SCAN_SUCCEED;
> >> +		mmap_read_unlock(mm);
> >> +	}
> >> +end:
> >> +	if (cc->is_khugepaged && result == SCAN_SUCCEED)
> >> +		++khugepaged_pages_collapsed;
> >> +	return result;
> >> +}
> >> +
> >>  static void collapse_scan_mm_slot(unsigned int progress_max,
> >>  		enum scan_result *result, struct collapse_control *cc)
> >>  	__releases(&khugepaged_mm_lock)
> >> @@ -2479,34 +2538,9 @@ static void collapse_scan_mm_slot(unsigned int progress_max,
> >>  			VM_BUG_ON(khugepaged_scan.address < hstart ||
> >>  				  khugepaged_scan.address + HPAGE_PMD_SIZE >
> >>  				  hend);
> >
> > Nice-to-have, but could we convert these VM_BUG_ON()'s to VM_WARN_ON_ONCE()'s
> > while we're passing?
>
> Yeah sure, I have a question about these, because they do concern me (perhaps
> out of ignorance). does a WARN_ON_ONCE stop the daemon? I would be concerned
> about a rogue khugepaged instance going through and messing with page tables
> when it fails some assertion. Could this not lead to serious memory/file
> corruptions?

It won't, but all of this is in CONFIG_DEBUG_VM anyway, so it's a kernel bug if
it happens in a release kernel, this is just for visibility when debugging and
those systems should be e.g. VMS etc.

The fact this is VM_xxx and hasn't apparently fired before suggests we're good.

>
> Thanks for the reviews!
>
> Cheers,
> -- Nico
>
> >
> >> -			if (!vma_is_anonymous(vma)) {
> >> -				struct file *file = get_file(vma->vm_file);
> >> -				pgoff_t pgoff = linear_page_index(vma,
> >> -						khugepaged_scan.address);
> >> -
> >> -				mmap_read_unlock(mm);
> >> -				mmap_locked = false;
> >> -				*result = collapse_scan_file(mm,
> >> -					khugepaged_scan.address, file, pgoff, cc);
> >> -				fput(file);
> >> -				if (*result == SCAN_PTE_MAPPED_HUGEPAGE) {
> >> -					mmap_read_lock(mm);
> >> -					if (collapse_test_exit_or_disable(mm))
> >> -						goto breakouterloop;
> >> -					*result = try_collapse_pte_mapped_thp(mm,
> >> -						khugepaged_scan.address, false);
> >> -					if (*result == SCAN_PMD_MAPPED)
> >> -						*result = SCAN_SUCCEED;
> >> -					mmap_read_unlock(mm);
> >> -				}
> >> -			} else {
> >> -				*result = collapse_scan_pmd(mm, vma,
> >> -					khugepaged_scan.address, &mmap_locked, cc);
> >> -			}
> >> -
> >> -			if (*result == SCAN_SUCCEED)
> >> -				++khugepaged_pages_collapsed;
> >>
> >> +			*result = collapse_single_pmd(khugepaged_scan.address,
> >> +						      vma, &mmap_locked, cc);
> >>  			/* move to next address */
> >>  			khugepaged_scan.address += HPAGE_PMD_SIZE;
> >>  			if (!mmap_locked)
> >> @@ -2806,9 +2840,7 @@ int madvise_collapse(struct vm_area_struct *vma, unsigned long start,
> >>
> >>  	for (addr = hstart; addr < hend; addr += HPAGE_PMD_SIZE) {
> >>  		enum scan_result result = SCAN_FAIL;
> >> -		bool triggered_wb = false;
> >>
> >> -retry:
> >>  		if (!mmap_locked) {
> >>  			cond_resched();
> >>  			mmap_read_lock(mm);
> >> @@ -2823,46 +2855,20 @@ int madvise_collapse(struct vm_area_struct *vma, unsigned long start,
> >>  			hend = min(hend, vma->vm_end & HPAGE_PMD_MASK);
> >>  		}
> >>  		mmap_assert_locked(mm);
> >> -		if (!vma_is_anonymous(vma)) {
> >> -			struct file *file = get_file(vma->vm_file);
> >> -			pgoff_t pgoff = linear_page_index(vma, addr);
> >>
> >> -			mmap_read_unlock(mm);
> >> -			mmap_locked = false;
> >> -			*lock_dropped = true;
> >> -			result = collapse_scan_file(mm, addr, file, pgoff, cc);
> >> -
> >> -			if (result == SCAN_PAGE_DIRTY_OR_WRITEBACK && !triggered_wb &&
> >> -			    mapping_can_writeback(file->f_mapping)) {
> >> -				loff_t lstart = (loff_t)pgoff << PAGE_SHIFT;
> >> -				loff_t lend = lstart + HPAGE_PMD_SIZE - 1;
> >> +		result = collapse_single_pmd(addr, vma, &mmap_locked, cc);
> >>
> >> -				filemap_write_and_wait_range(file->f_mapping, lstart, lend);
> >> -				triggered_wb = true;
> >> -				fput(file);
> >> -				goto retry;
> >> -			}
> >> -			fput(file);
> >> -		} else {
> >> -			result = collapse_scan_pmd(mm, vma, addr, &mmap_locked, cc);
> >> -		}
> >>  		if (!mmap_locked)
> >>  			*lock_dropped = true;
> >>
> >> -handle_result:
> >>  		switch (result) {
> >>  		case SCAN_SUCCEED:
> >>  		case SCAN_PMD_MAPPED:
> >>  			++thps;
> >>  			break;
> >> -		case SCAN_PTE_MAPPED_HUGEPAGE:
> >> -			BUG_ON(mmap_locked);
> >> -			mmap_read_lock(mm);
> >> -			result = try_collapse_pte_mapped_thp(mm, addr, true);
> >> -			mmap_read_unlock(mm);
> >> -			goto handle_result;
> >>  		/* Whitelisted set of results where continuing OK */
> >>  		case SCAN_NO_PTE_TABLE:
> >> +		case SCAN_PTE_MAPPED_HUGEPAGE:
> >>  		case SCAN_PTE_NON_PRESENT:
> >>  		case SCAN_PTE_UFFD_WP:
> >>  		case SCAN_LACK_REFERENCED_PAGE:
> >> --
> >> 2.53.0
> >>
> >
>

Cheers, Lorenzo