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

[David Hildenbrand (Arm)]

On 2/23/26 15:24, David Hildenbrand (Arm) wrote:
> On 2/12/26 21:26, Nico Pache wrote:
> > On Thu, Feb 12, 2026 at 1:04 PM David Hildenbrand (Arm)
> > <david@xxxxxxxxxx> wrote:
> > > Having triggered_wb set where writeback is not actually triggered is
> > > suboptimal.
> >
> > It took me a second to figure out what you were referring to, but I
> > see it now. if we return SCAN_PAGE_D_OR_WB but the can_writeback fails
> > it still retries.
> >
> > Would be an appropriate solution if can_writeback fails to modify the
> > return value.
> > ie)
> >
> > if (!cc->is_khugepaged && result == SCAN_PAGE_DIRTY_OR_WRITEBACK) {
> >      if (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);
> >      } else {
> >         result = SCAN_(SOMETHING?)
> >      }
> > }
> > fput(file);
> >
> > we dont have a enum that fits this description but we want one that
> > will continue.
>
> I stared at the patch and possible ways to change it, but I wondered 
> whether this refactoring is really the right approach.
>
> The whole mmap locking just makes this all very weird.
>
> Let me think about it some more.

As we don't really need to re-grab the MM lock, I think we can just do:

diff --git a/mm/khugepaged.c b/mm/khugepaged.c
index 844a8a27845e..d546ff173833 100644
--- a/mm/khugepaged.c
+++ b/mm/khugepaged.c
@@ -2395,6 +2395,7 @@ static enum scan_result collapse_single_pmd(unsigned long addr,
                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;
@@ -2409,14 +2410,22 @@ static enum scan_result collapse_single_pmd(unsigned long addr,
 
        mmap_read_unlock(mm);
        *mmap_locked = false;
+
+retry:
        result = collapse_scan_file(mm, addr, file, pgoff, cc);
 
+       /*
+        * For MADV_COLLAPSE, try to writeback once to retry the collapse
+        * immediately.
+        */
        if (!cc->is_khugepaged && result == SCAN_PAGE_DIRTY_OR_WRITEBACK &&
-           mapping_can_writeback(file->f_mapping)) {
+           !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;
        }
        fput(file);
 
@@ -2814,9 +2823,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);
@@ -2838,11 +2845,6 @@ int madvise_collapse(struct vm_area_struct *vma, unsigned long start,
                if (!mmap_locked)
                        *lock_dropped = true;
 
-               if (result == SCAN_PAGE_DIRTY_OR_WRITEBACK && !triggered_wb) {
-                       triggered_wb = true;
-                       goto retry;
-               }
-
                switch (result) {
                case SCAN_SUCCEED:
                case SCAN_PMD_MAPPED:


--
Cheers,

David