Re: [PATCH 07/25] mm/fbatch: LRU_NEXT_ACTIVATE bit to optimize folio_activate()

[Kiryl Shutsemau <kas@kernel.org>]

On Wed, Sep 02, 2026 at 10:41:56PM -0700, Hugh Dickins wrote:
> On Mon, 31 Aug 2026, Kiryl Shutsemau wrote:
> > On Fri, Aug 28, 2026 at 01:40:51AM -0700, Hugh Dickins wrote:
> > > Do you have a head for smp_mb__ barriers? I'm more anxious that
> > > I might be missing one or two of those.
> > 
> > I think the release side of PG_lru is missing.
> > 
> > You effectively turn PG_lru into a lock over folio->lru.next.
> > 
> > The acquire side works: test_and_clear_bit() has a return value, so it
> > is fully ordered.
> > 
> > But there's a problem with release. set_bit() is unordered. You
> > correctly placed a fence in __folio_add_lru(), but every other
> > folio_set_lru() is problematic.
> > 
> > For instance:
> > 
> > 	CPU0 				CPU1
> > folio_batch_move_lru()		folio_batch_move_lru()
> >   lru_add_del_folio()
> >     lru.next = LIST_POISON1
> >   lruvec lock
> >     list_add()
> >   /* no barrier */
> >   set_bit(PG_lru)
> > 				  folio_try_get() == true
> > 				  folio_test_clear_lru() == true
> > 				  lru_next == stale BATCHED ???
> >   lruvec unlock
> > 
> > If CPU1 sees a stale BATCHED, lru_add_del_folio() returns true without
> > doing the list_del() or the NR_LRU_BASE accounting, and CPU1 then goes
> > on to lruvec_add_folio() a folio that is already on a list.
> > 
> > I think we need to have a helper that would set PG_lru and enforce
> > release semantics.
> 
> Thank you very much for this, Kiryl: it helps me considerably.
> But I have to cool myself down close to absolute zero to think
> about these things, and can only manage that occasionally.
> 
> I've nothing useful to say yet. I believe I understand you, and in
> particular your last sentence, which I take as an observation that
> clear_bit_unlock() is well-established, but what we want is
> set_bit_unlock(), perhaps better named set_bit_release().
> 
> Of course I'm not competent to add that to N architectures, most of
> them unfamiliar to me.  So I'm looking for a reasonable compromise,
> to minimize the additional overhead needed for correctness here,
> just using what we have already have (test_and_set, smp_mb__).

It would not be N architectures. This should be good enough:

/* include/asm-generic/bitops/lock.h */
#ifndef arch_set_bit_release
static __always_inline void
arch_set_bit_release(unsigned int nr, volatile unsigned long *p)
{
	p += BIT_WORD(nr);
	raw_atomic_long_fetch_or_release(BIT_MASK(nr), (atomic_long_t *)p);
}
#endif

/* include/asm-generic/bitops/instrumented-lock.h */
static inline void set_bit_release(long nr, volatile unsigned long *addr)
{
	kcsan_release();
	instrument_atomic_write(addr + BIT_WORD(nr), sizeof(long));
	arch_set_bit_release(nr, addr);
}

/* arch/x86/include/asm/bitops.h -- mirrors arch_clear_bit_unlock() */
static __always_inline void
arch_set_bit_release(long nr, volatile unsigned long *addr)
{
	barrier();	/* LOCK prefix is already a full barrier */
	arch_set_bit(nr, addr);
}
#define arch_set_bit_release arch_set_bit_release

I don't know if we want to make it _unlock() to match
clear_bit_unlock(). Naming is hard.

x86 does need an override, since it has no locked OR that returns the old
value -- arch_atomic64_fetch_or() is a cmpxchg loop.

arm64 is happy with the generic version.

ppc and riscv need a definition of their own only because they do not
include asm-generic/bitops/lock.h, so the #ifndef above never reaches
them. ppc can take the generic body. and riscv is a one-liner right next
to its existing arch_clear_bit_unlock().

This set_bit_release() gets better results than alternatives:

                set_bit_release()  smp_mb__ + set_bit()   test_and_set_bit()
x86             lock orb           lock orb               lock btsq
arm64 LSE       ldsetl             dmb ish + stset        ldsetal
arm64 LL/SC     ldxr/stlxr         dmb ish + ldxr/stxr    ldxr/stlxr + dmb ish
ppc             lwsync + loop      sync + loop            sync + loop + sync
riscv           amoor.d.rl         fence rw,rw + amoor.d  amoor.d.aqrl

I can prepare a proper patches with what I listed above, if you want, so
you can prepend to your series.

If you don't want to go there for the initial series,
smp_mb__before_atomic() plus folio_set_lru() should be good enough:

static __always_inline bool folio_test_clear_lru_acquire(struct folio *folio)
{
	return folio_test_clear_lru(folio);
}

static __always_inline void folio_set_lru_release(struct folio *folio)
{
	smp_mb__before_atomic();
	folio_set_lru(folio);
}

-- 
  Kiryl Shutsemau / Kirill A. Shutemov