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

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

[Gregory Price]

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.

Reported-by: sashiko-bot <sashiko-bot@kernel.org>
Closes: https://sashiko.dev/#/patchset/20260817220810.1175596-1-gourry%40gourry.net
Assisted-by: Claude:claude-opus-5
Signed-off-by: Gregory Price (Meta) <gourry@gourry.net>
---
 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;
 			folio_get(folio);
 			spin_unlock(ptl);
-			folio_lock(folio);
 			err = split_folio(folio);
 			folio_unlock(folio);
 			folio_put(folio);
-- 
2.55.0

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

[Lorenzo Stoakes (ARM)]

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 <sashiko-bot@kernel.org>
> Closes: https://sashiko.dev/#/patchset/20260817220810.1175596-1-gourry%40gourry.net
> Assisted-by: Claude:claude-opus-5
> Signed-off-by: Gregory Price (Meta) <gourry@gourry.net>
> ---
>  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

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

[Gregory Price]

On Fri, Aug 21, 2026 at 07:23:34PM +0100, Lorenzo Stoakes (ARM) wrote:
> On Fri, Aug 21, 2026 at 11:09:12AM -0400, Gregory Price wrote:
> 
> I am _so_ glad to see an actual reproducer used in a sashiko bug fix. THANKS. :)
> 

fwiw i try to produce reproducers on all my sashiko submitted fixes,
i've just been dumb about including them in the changelog.  Trying to
fix that from now on.

> > +			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 :)
> 

This was the thing i was least sure about, but the deadlock condition
should only happen if we tried to spin on the folio lock right?

It does look weird, so it would be wrong of me to say i'm 100%
confident this is the best change.

~Gregory

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

[Lorenzo Stoakes (ARM)]

On Fri, Aug 21, 2026 at 02:42:01PM -0400, Gregory Price wrote:
> On Fri, Aug 21, 2026 at 07:23:34PM +0100, Lorenzo Stoakes (ARM) wrote:
> > On Fri, Aug 21, 2026 at 11:09:12AM -0400, Gregory Price wrote:
> >
> > I am _so_ glad to see an actual reproducer used in a sashiko bug fix. THANKS. :)
> >
>
> fwiw i try to produce reproducers on all my sashiko submitted fixes,
> i've just been dumb about including them in the changelog.  Trying to
> fix that from now on.
>
> > > +			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 :)
> >
>
> This was the thing i was least sure about, but the deadlock condition
> should only happen if we tried to spin on the folio lock right?
>
> It does look weird, so it would be wrong of me to say i'm 100%
> confident this is the best change.

Hmm yeah. I worry about subtleties here.

I suppose you are avoiding what you'd obviously worry about with a PTL
lock/folio lock inversion causing a deadlock.

madvise_free_pte_range() seems to 'violate' this also with a trlock, seems
it's not an uncommon pattern.

I suppose the question is - what else is likely to lock this?

It does seem likely to be the race you flag in general.

So probably this is fine then.

And the window is relatively small also...

This kind of stuff gives me the heebie-jeebies :) I know that's not exactly
technical but there we go.

I guess there's just no other way of resolving this either.

That with the reproducer suggests this is fine then!

Since it scares me I think A-b rather than R-b ;)

Acked-by: Lorenzo Stoakes (ARM) <ljs@kernel.org>

>
> ~Gregory

--
Cheers, Lorenzo

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

[Matthew Wilcox]

On Fri, Aug 21, 2026 at 07:23:34PM +0100, Lorenzo Stoakes (ARM) wrote:
> 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?

For the task running on CPU 1, yes.  But the folio does get split rather
than probably both failing.

> > +++ 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 :)

It's a common way of getting out of a lock ordering problem.  Surprised
you've not encountered it as a solution to the Dining Philosophers problem.
We have even weirder solutions to "I want to sleep on the folio lock
but not with a reference held", and such might be appropriate here if
we want to prevent the spurious failure on CPU 1.  See the DROP behavior
in mm/filemap.c.  See folio_put_wait_locked() in mm/filemap.c, not that
it's exported.

We couldn't quite make that work here since the whole point is to _never_
get the refcount on the folio if somebody else has the lock, and once
we've dropped the PTL, the folio might have been split and thus not be
the folio we want any more (indeed it may have been freed, reallocated
and now be a pointer to a tail page instead of a folio).

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

[Gregory Price]

On Fri, Aug 21, 2026 at 08:48:12PM +0100, Matthew Wilcox wrote:
> On Fri, Aug 21, 2026 at 07:23:34PM +0100, Lorenzo Stoakes (ARM) wrote:
> 
> For the task running on CPU 1, yes.  But the folio does get split rather
> than probably both failing.
>

Which, thinking about it, this patch actually solves non-spurious
(but exceedingly rare) failures as well, since prior both CPU0 and CPU1
can fail.  After this patch, one of them is guaranteed to succeed.

The inflated failure count otherwise are the spurious ones.

> We couldn't quite make that work here since the whole point is to _never_
> get the refcount on the folio if somebody else has the lock, and once
> we've dropped the PTL, the folio might have been split and thus not be
> the folio we want any more (indeed it may have been freed, reallocated
> and now be a pointer to a tail page instead of a folio).
>

Makes me wonder how many more of these conditions are peppered around
THPs just eating cpu time.

~Gregory