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

[Roger Pau Monné <roger@xxxxxxxxxxxxxx>]

On Wed, Sep 30, 2026 at 10:34:02AM +0200, Jan Beulich wrote:
> On 30.09.2026 10:09, Roger Pau Monné wrote:
> > On Wed, Sep 30, 2026 at 07:58:55AM +0200, Jan Beulich wrote:
> >> On 30.09.2026 00:14, 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)--;
> >>>>>>> +

Just noticed there's an unwanted newline being added here.

> >>>>>>> +    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 may not be properly written down anywhere, but at least the compilers
> >> we use are known to not tear aligned accesses up to a machine word in
> >> width.
> > 
> > Then I possibly don't need the ACCESS_ONCE() at all, as I was using it
> > to guarantee unshattered accesses.  However this should be written
> > somewhere, maybe in docs/process/coding-best-practices.pandoc?
> 
> To write it down in any of our docs, we'd first need a proper reference
> to somewhere. "Known to" isn't quite sufficient imo to actually nail
> things down.

Fair enough, I also assume compilers would never like to write
this down anywhere in the documentation.

> There's still the possibility of compiler bugs,

Compiler bugs could apply to anything that a compiler generates, and
hence is not likely relevant to mention for each compiler feature or
behavior we rely upon.

> and there's
> also the possibility that volatile accesses have better guarantees than
> non-volatile ones. (Recall that in prior discussions it was actually said
> that we may need to make far more use of ACCESS_ONCE(), which wouldn't be
> warranted if collectively we were certain aligned accesses cannot be
> torn.)

OK, so are we in agreement that ACCESS_ONCE(this_cpu(rcu_lock_cnt))--
ensures accesses to the variable are always non-torn?  Splitting
the access as a RMW is fine, as long as the accesses are not torn.

Thanks, Roger.