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

On Fri, Aug 21, 2026 at 11:09:12AM -0400, Gregory Price 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

Hmm, but doesn't converting to a folio_trylock() introduce entirely new spurious
failures due to folio lock contention?

>
> 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

I am _so_ glad to see an actual reproducer used in a sashiko bug fix. THANKS. :)

>
> 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.
>
> Reported-by: sashiko-bot <[email protected]>
> Closes: https://sashiko.dev/#/patchset/20260817220810.1175596-1-gourry%40gourry.net
> Assisted-by: Claude:claude-opus-5
> Signed-off-by: Gregory Price (Meta) <[email protected]>
> ---
>  mm/madvise.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/mm/madvise.c b/mm/madvise.c
> index 07a21ca31bad..bd9119880ef2 100644
> --- a/mm/madvise.c
> +++ b/mm/madvise.c
> @@ -405,9 +405,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;

Doesn't this violate lock ordering?

From rmap.c:

       folio_lock
	  ...
                 mm->page_table_lock or pte_lock

So now you hold the ptl lock _before_ you obtain the folio lock?

I'm not sure if it being a trylock gets us out of that particular situation? And
I'd be reticent for us to violate it... unless I'm missing something :)

>  			folio_get(folio);
>  			spin_unlock(ptl);
> -			folio_lock(folio);
>  			err = split_folio(folio);
>  			folio_unlock(folio);
>  			folio_put(folio);
> --
> 2.55.0
>

--
Cheers, Lorenzo