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

["Alejandro Vallejo" <alejandro.garciavallejo@amd.com>]

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.

>
> 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);

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.

Cheers,
Alejandro