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

[Jan Beulich <jbeulich@xxxxxxxx>]

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)--;
>>>>>>> +
>>>>>>> +    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. There's still the possibility of compiler bugs, 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.)

Jan