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

[Hugh Dickins <hughd@google.com>]

On Thu, 3 Sep 2026, Kiryl Shutsemau wrote:
> 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);
> }
> 
> -- 

Thanks a lot for taking this further, and we may indeed want it later;
but you guess correctly that I don't want to go there for the initial
series, and I've not yet convinced myself that it's what we want anyway.

Assuming that the existing pre-series code is correct just to use
folio_set_lru() throughout, what I've added are these additional lru_next
transitions, unprotected by lruvec lock, and it's just these which need
the additional protection.

So, I haven't finished thinking on it, but what I'm currently inclined
to, is putting an smp_mb__before_atomic() after the LIST_POISON1 in
lru_add_del_folio(). That's a long way from the folio_set_lru() it's
intended for, which is unusual, but it seems more to the point than
always using folio_set_lru_release(). We can switch to a proper
folio_set_lru_release() later, if that's usually seen to work better.

Hugh