Re: [RESEND PATCH] mm/madvise: use folio_trylock() in the cold/pageout PMD split

[Andrew Morton]

On Sat, 12 Sep 2026 07:05:40 -0400 Gregory Price <gourry@xxxxxxxxxx> wrote:

> MADV_COLD or MADV_PAGEOUT over part of a PMD splits the THP in
> madvise_cold_or_pageout_pte_range().  Two threads doing that to
> the same THP create spurious failures.
> 
>   CPU0                          CPU1
>   ----                          ----
>   folio_get()
>   spin_unlock(ptl)
>   folio_lock()
>                                 folio_get()
>                                 spin_unlock(ptl)
>                                 folio_lock()  <- blocks, keeps its ref
>   split_folio()
>     folio_expected_ref_count(folio) != folio_ref_count(folio) - 1
>     -EAGAIN
> 
> CPU1 cannot drop its reference until it gets the lock CPU0 holds, so CPU0's
> split always fails.  folio_trylock() makes CPU1 leave without ever taking a
> reference.  The PTE branch of this same function already does this, as do
> madvise_free_pte_range() and madvise_free_huge_pmd().
> 
> Reproducer: 400 rounds of eight threads calling MADV_COLD on half of each
> of eight THPs, re-formed with MADV_COLLAPSE between rounds.  From
> /proc/vmstat:
> 
>                      thp_split_page   thp_split_page_failed
>     before                     3186                     860
>     after                      3200                       0
> 
> The short before count is rounds where every thread failed and the
> advice was dropped for that THP entirely.
> 
> On failure the walker returns 0 and nothing retries.  The PMD path becomes
> best effort when the folio lock is held elsewhere - same as the PTE path.

Can this result in more EAGAINs being returned to userspace?

> --- a/mm/madvise.c
> +++ b/mm/madvise.c
> @@ -419,9 +419,10 @@ static int madvise_cold_or_pageout_pte_range(pmd_t *pmd,
>  		if (next - addr != HPAGE_PMD_SIZE) {
>  			int err;
>  
> +			if (!folio_trylock(folio))
> +				goto huge_unlock;
>  			folio_get(folio);
>  			spin_unlock(ptl);
> -			folio_lock(folio);
>  			err = split_folio(folio);
>  			folio_unlock(folio);
>  			folio_put(folio);

Sashiko shares my reaction, "dear god, why does that function exist".

	https://sashiko.dev/#/patchset/20260912110540.3203010-1-gourry@xxxxxxxxxx

Like the entire function, that random mix of "goto foo" with "return
whatever" needs to die.

Anyway, please check it out while you're on a roll.