Re: [PATCH v2 1/3] xen/rcu: introduce the concept of RCU epoch

["Roger Pau Monné" <roger@xenproject.org>]

On Wed, Sep 30, 2026 at 12:14:15AM +0200, Alejandro Vallejo wrote:
> On Tue Sep 29, 2026 at 4:52 PM CEST, Jan Beulich wrote:
> > On 29.09.2026 16:20, Roger Pau Monné wrote:
> > > On Mon, Sep 28, 2026 at 10:24:04AM +0200, Alejandro Vallejo wrote:
> > >> On Fri Sep 25, 2026 at 6:30 PM CEST, Roger Pau Monne wrote:
> > >>>  static inline void rcu_quiesce_disable(void)
> > >>>  {
> > >>> +    unsigned int cpu = smp_processor_id();
> > >>> +
> > >>>      preempt_disable();
> > >>> -    this_cpu(rcu_lock_cnt)++;
> > >>> -    barrier();
> > >>> +    if ( !ACCESS_ONCE(per_cpu(rcu_lock_cnt, cpu))++ )
> > >>> +        ACCESS_ONCE(per_cpu(rcu_lock_epoch, cpu)) = ACCESS_ONCE(rcu_epoch);
> > >>
> > >> I'm not terribly convinced about using ACCESS_ONCE() for atomic
> > >> accesses. In particular...
> > >>
> > >>> +    smp_mb();
> > >>>  }
> > >>>  
> > >>>  static inline void rcu_quiesce_enable(void)
> > >>>  {
> > >>> -    barrier();
> > >>> -    this_cpu(rcu_lock_cnt)--;
> > >>> +
> > >>> +    smp_mb();
> > >>> +    ACCESS_ONCE(this_cpu(rcu_lock_cnt))--;
> > >>
> > >> ... this variable is to be read by remote CPUs and autodecrement seems
> > >> like the wrong tool for an atomic decrement.
> > > 
> > > I can use {read,write}_atomic() instead, but there's been a tendency
> > > to phase out the usage of those functions.  On x86 this should be a
> > > plain `addl` or `subl` instruction (the non-locked version of
> > > atomic_{inc,dec}()).
> 
> I don't think the compiler can merge volatile reads and writes. It can
> shatter them (thus my fear), but they are meant to be distinct because
> each might have different side effects. Jan already mentioned this in
> other words.
> 
> You don't need an atomic decrement though, just an atomic write. Note
> IRQs restore it on exit and other CPUs don't write to it.
> 
>   unsigned int *lock_cnt = &this_cpu(rcu_lock_cnt);
>   write_atomic(lock_cnt, *lock_cnt - 1);
> 
> and this yields a plain mov, through a static inline stub (plus the
> mov from the read access + dec)
> 
> The problem with ACCESS_ONCE() is that the write isn't guaranteed to be
> atomic.

It has always been blurry to me using ACCESS_ONCE() to signal
unshattered accesses, but we seem to rely on this property in other
places already (which doesn't mean is correct).  See
ffa_negotiate_version() for example.

This is all kind of moot if we guarantee that all supported compilers
will never tear accesses up to machine word sizes.

> >
> > If remote CPUs are to access the variable, is a non-locked access okay
> > in the first place?
> 
> You use locked access when you need sequential consistency or an atomic
> RMW, and that's not it here. There's a single writer (currrent CPU) and
> IRQs don't play a part because the restore the previous value before
> returning. So it's race-free. All we need to ensure is that remote CPUs
> never see a partial write.
> 
> Another take would be to subsume the prior smp_mb() into the decrement.
> Something like:
> 
>   arch_fetch_add(&this_cpu(rcu_lock_cnt), RCU_LOCK_CNT_MAX);

Kind of a nit (as I've already attempted this approach), but
arch_fetch_and_add() is not usable as-is here.  We we need to consume
the returned value, otherwise the compiler complains.

./arch/x86/include/asm/system.h:189:6: error: value computed is not used [-Werror=unused-value]

Could do:

unsigned int prev = arch_fetch_and_add(&this_cpu(rcu_lock_cnt), -1);
ASSERT(prev > 0);

> This would make the decrement itself atomic (through a locked insn), but
> ensure sequential consistency as well, thus removing the need for the
> prior barrier. It's trickery through twos complement to behave like a unit
> decrement.
> 
> My point isn't so much the "how" (should be obvious by now there's many
> ways), but to not use ACCESS_ONCE() where an atomic read or write was
> meant. Not necessarily a locked instruction. Just atomic.

I don't have a strong opinion of using smp_mb() vs
arch_fetch_and_add().  I think smp_mb() expresses the intention
better, and avoids explicitly using a locked instruction, which is not
required here.

Thanks, Roger.