Re: [PATCH] mm/madvise: reclaim isolated folios if PTE restart fails

[Lorenzo Stoakes (ARM)]

On Sat, Sep 12, 2026 at 07:08:32AM -0400, Gregory Price wrote:
> MADV_PAGEOUT collects isolated folios on a local list before reclaiming
> them after the PTE walk.  The reschedule path drops the PTE lock and then
> restarts the mapping with pte_offset_map_lock().

Is this reschedule path even necessary? It's pretty bloody sketchy.

I thought the modern approach (TM) was to not do cond_resched() and friends so
can we actually look at removing this?

>
> A concurrent operation can remove or replace the PTE table while the lock
> is dropped, causing pte_offset_map_lock() to return NULL.  Returning directly
> in that case bypasses reclaim_pages(), leaving the collected folios off the
> LRU with elevated references.
>
> Route the failure through the existing cleanup path so any isolated folios
> are reclaimed or put back.
>
> Simplest userland pseudo-code reproducer:
>
> p = mmap(PMD_SIZE, ANONYMOUS);
> touch_every_page(p, PMD_SIZE);
> parallel {
>    while (1) madvise(p, PMD_SIZE, MADV_PAGEOUT);
>    while (1) {
>        madvise(p, PMD_SIZE, MADV_DONTNEED);
>        touch_every_page(p, PMD_SIZE);
>    }
> }
>
> Reproduced in qemu trivially with some explicit widening of the race window.
>
> Fixes: b2f557a21bc8 ("mm/madvise: add cond_resched() in madvise_cold_or_pageout_pte_range()")
> Reported-by: sashiko-bot <sashiko-bot@xxxxxxxxxx>
> Closes: https://sashiko.dev/#/patchset/20260821150912.183976-1-gourry@xxxxxxxxxx
> Cc: <stable@xxxxxxxxxxxxxxx>
> Assisted-by: LLM
> Signed-off-by: Gregory Price (Meta) <gourry@xxxxxxxxxx>
> ---
>  mm/madvise.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/mm/madvise.c b/mm/madvise.c
> index 574aa2bb7c7e..ba5a3d77241a 100644
> --- a/mm/madvise.c
> +++ b/mm/madvise.c
> @@ -464,7 +464,7 @@ static int madvise_cold_or_pageout_pte_range(pmd_t *pmd,

This function is truly some of the absolute worst code I've read in
mm. Shocking.

>  restart:
>  	start_pte = pte = pte_offset_map_lock(vma->vm_mm, pmd, addr, &ptl);
>  	if (!start_pte)
> -		return 0;
> +		goto out;
>  	flush_tlb_batched_pending(mm);
>  	lazy_mmu_mode_enable();
>  	for (; addr < end; pte += nr, addr += nr * PAGE_SIZE) {
> @@ -568,6 +568,7 @@ static int madvise_cold_or_pageout_pte_range(pmd_t *pmd,
>  			folio_deactivate(folio);
>  	}
>
> +out:

Seems odd to put this here for a condition that explicitly guarantees
!start_pte? It should be before the if (pageout) reclaim_pages(...);
surely?

I honestly wonder whether, rather than adding yet another label/goto into this
absolute bloody mess, whether we should just live with a bit of duplication and do:

	if (!start_pte) {
		if (pageout)
			reclaim_pages(&folio_list);
		return 0;
	}

I actually think that'd be clearer at this point than throwing in some more
indirection.

That's for the backport but somebody needs to rework this entire bloody
function going forwards...

>  	if (start_pte) {
>  		lazy_mmu_mode_disable();
>  		pte_unmap_unlock(start_pte, ptl);
> --
> 2.55.0
>

--
Cheers, Lorenzo