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

[Roger Pau Monné <roger@xxxxxxxxxxxxxx>]

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:
> > An RCU epoch signals the lifetime of RCU references.  Each CPU records the
> > epoch in force when it enters an RCU critical section.  When a callback is
> > queued the current epoch is stored in the callback and the global epoch is
> > then bumped, as a way to know when all CPUs have moved past a specific
> > epoch.  At that point there can be no remaining references to objects
> > fetched during the callback's epoch.
> >
> > The compiler barrier is switched to a full memory barrier, as future uses
> > of rcu_lock_cnt must ensure the count is increased before taking a
> > reference to any RCU protected object.
> >
> > Access rcu_lock_cnt, rcu_lock_epoch and rcu_epoch through ACCESS_ONCE()
> > to stop the compiler shattering the loads and stores.  The reordering
> > prevention aspect of ACCESS_ONCE() is not what we rely on here (the memory
> > barriers cover that); what matters is that these variables now have remote
> > consumers, so each access must be a single, non-torn memory operation.
> >
> > Signed-off-by: Roger Pau Monné <roger@xxxxxxxxxxxxxx>
> > ---
> > Can possibly be folded into the next patch, as it's lacking context on its
> > own to understand the need to introduce the logic.
> > ---
> > Changes since v1:
> >  - Do the setting of head->added in the interrupt disabled section.
> >  - Unconditionally do a memory barrier when enter/exit RCU critical
> >    regions.  Attempting to do selectively is too complex.
> > ---
> >  xen/common/rcupdate.c      |  6 ++++++
> >  xen/include/xen/rcupdate.h | 17 +++++++++++++----
> >  2 files changed, 19 insertions(+), 4 deletions(-)
> >
> > diff --git a/xen/common/rcupdate.c b/xen/common/rcupdate.c
> > index c1b6b2ae768b..babaabbbeefe 100644
> > --- a/xen/common/rcupdate.c
> > +++ b/xen/common/rcupdate.c
> > @@ -49,6 +49,11 @@
> >  #include <asm/atomic.h>
> >  
> >  DEFINE_PER_CPU(unsigned int, rcu_lock_cnt);
> > +/* Store epoch when CPU entered the RCU critical section. */
> > +DEFINE_PER_CPU(unsigned int, rcu_lock_epoch);
> > +
> > +/* Current RCU epoch, bumped every time a new callback is queued. */
> > +unsigned int rcu_epoch;
> 
> nit: I'd suggest calling this rcu_cur_epoch, and then renaming "added"
> below to plain "epoch". "added" doesn't convey what the field is at all.

added_epoch? (or epoch_added?)

I would be fine with epoch also.  I think it's obvious that the
global variable holds the current epoch without it needing the "cur"
addition.

> >  
> >  /* Global control variables for rcupdate callback mechanism. */
> >  static struct rcu_ctrlblk {
> > @@ -283,6 +288,7 @@ void call_rcu(struct rcu_head *head,
> >      head->func = func;
> >      head->next = NULL;
> >      local_irq_save(flags);
> > +    head->added = arch_fetch_and_add(&rcu_epoch, 1);
> >      rdp = &this_cpu(rcu_data);
> >      *rdp->nxttail = head;
> >      rdp->nxttail = &head->next;
> > diff --git a/xen/include/xen/rcupdate.h b/xen/include/xen/rcupdate.h
> > index c57f628107cf..5846e8c169d7 100644
> > --- a/xen/include/xen/rcupdate.h
> > +++ b/xen/include/xen/rcupdate.h
> > @@ -34,24 +34,32 @@
> >  #include <xen/compiler.h>
> >  #include <xen/spinlock.h>
> >  #include <xen/cpumask.h>
> > +#include <xen/lib.h>
> >  #include <xen/percpu.h>
> >  #include <xen/preempt.h>
> >  
> >  #define __rcu
> >  
> >  DECLARE_PER_CPU(unsigned int, rcu_lock_cnt);
> > +DECLARE_PER_CPU(unsigned int, rcu_lock_epoch);
> > +
> > +extern unsigned int rcu_epoch;
> >  
> >  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}()).

Thanks, Roger.