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

David Hildenbrand (Arm)
2026-02-23T14:41:32Z

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