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

["Alejandro Vallejo" <alejandro.garciavallejo@xxxxxxx>]

On Wed Sep 30, 2026 at 10:32 AM CEST, Roger Pau Monné wrote:
> 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]

(void)arch_fetch_and_add() would shut up the warning, but...

>
> 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.

... I agree. My point was rather on ACCESS_ONCE() avoidance. Let that be
for now, it's clearly meat for another barbequeue.

Cheers,
Alejandro

>
> Thanks, Roger.