Re: [PATCH v2 09/26] mm/fbatch: restore mlock+munlock batching, without extra ref

[Hugh Dickins]

On Thu, 10 Sep 2026, Vlastimil Babka (SUSE) wrote:
> On 9/9/26 11:59, Hugh Dickins wrote:
> > Update mlock_folio(), munlock_folio() and their fbatch callouts and
> > helpers, to do folio_try_get()s at batch processing time, instead of
> > holding a folio reference all the while in mlock_fbatch: as in folio.c.
> > 
> > But more interesting is the use of mod_mlock_count(), using try_cmpxchg()
> > to update folio->mlock_count safely when possible (now when on lru_add
> > fbatch as well as when unevictable). While __mlock_folio() is as hard to
> > think about as before, __munlock_folio() simpler because munlock_folio()
> > can adjust mlock_count itself without clear_lru() or lruvec lock, and so
> > do the folio_test_clear_mlocked() immediately for itself (without which
> > unevictable_pgs_cleared was likely to appear high, when it should be 0
> > or low to indicate good mlock health).
> > 
> > __munlock_folio() is safe for use even when the unreferenced folio has
> > been freed and reused. It appears that __mlock_folio() could affect a
> > folio which has been freed and reused, but only if it is reused as an
> > mlocked folio, in which case its mlock_count is spuriously incremented
> > (but usually a spurious munlock decrement will follow).  How grave is
> > this? If unevictable_pgs_cleared remains low, not so bad.
> > 
> > I've gone back and forth on whether to move mlock_fbatch and these
> > functions into mm/folio.c: for now they stay here in mm/mlock.c.
> > 
> > Signed-off-by: Hugh Dickins <hughd@xxxxxxxxxx>
> > ---
> >  mm/mlock.c | 147 +++++++++++++++++++++++++++++++----------------------
> >  1 file changed, 86 insertions(+), 61 deletions(-)
> > 
> > diff --git a/mm/mlock.c b/mm/mlock.c
> > index 2c690f18031e..1050010bbe0b 100644
> > --- a/mm/mlock.c
> > +++ b/mm/mlock.c
> > @@ -58,6 +58,20 @@ EXPORT_SYMBOL(can_do_mlock);
> >   * indicate the unevictable state.
> >   */
> >  
> > +static long mod_mlock_count(struct folio *folio, long incdec)
> > +{
> > +	long mlock_count = READ_ONCE(folio->mlock_count);
> > +
> > +	while (mlock_count & MLOCK_COUNT_0) {
> > +		if (mlock_count + incdec < MLOCK_COUNT_0)
> > +			return MLOCK_COUNT_0;
> > +		if (try_cmpxchg(&folio->mlock_count, &mlock_count,
> > +				mlock_count + incdec))
> > +			return mlock_count + incdec;
> > +	}
> 
> This never rereads folio->mlock_count to mlock_count inside the loop, so it
> can spin forever?

You give me a nasty moment, have I misunderstood? Isn't it part of the
try_cmpxchg() contract, that it reads folio->mlock_count into mlock_count
when it fails? Hence the "&mlock_count" rather than just "mlock_count"?

> 
> (too late here for me to understand the rest today)

But that I understand very weil: thanks for all that you have managed.

Hugh