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

["David Hildenbrand (Arm)" <david@kernel.org>]

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