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

Nico Pache
2026-03-18T17:22:20Z

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@redhat.com>
>
> 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@kernel.org>

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!

>
>> + 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.

>
>> + }
>> + 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?

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
>>
>