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

[Vlastimil Babka (SUSE)]

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?

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

> +	return 0;
> +}
> +