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

[Vlastimil Babka (SUSE)]

On 9/13/26 01:46, Hugh Dickins wrote:
> 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"?

Indeed, sorry! It would have helped if the contract was spelled out
somewhere. But now I found it only for the atomic_try_cmpxchg() variant.
Guess I'm too used to the slab_update_freelist() wrappers which don't follow
this... but maybe should? Hmm.

>> 
>> (too late here for me to understand the rest today)
> 
> But that I understand very weil: thanks for all that you have managed.
> 
> Hugh