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

[Matthew Wilcox <willy@infradead.org>]

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