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

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

On Wed Sep 30, 2026 at 12:07 PM CEST, Roger Pau Monné wrote:
> 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.

... so long as the variable is no wider than the machine word size. It
(may) shatter should rcu_lock_cnt be 64bits and this be run in arm32.
Which, granted, is not the case today so long as unsigned int is 32bits,
but we're using unsigned int because there's an assumption that width
doesn't matter for it.

I still think ACCESS_ONCE() is the wrong tool when atomic operations are
needed, but I will push no further on it for now. I do think this will
compile fine on current targets.

Cheers,
Alejandro

>
> Thanks, Roger.