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

["Alejandro Vallejo" <alejandro.garciavallejo@xxxxxxx>]

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.

>  
>  /* 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.

>      preempt_enable();
>  }
>  
> @@ -68,6 +76,7 @@ static inline bool rcu_quiesce_allowed(void)
>  struct rcu_head {
>      struct rcu_head *next;
>      void (*func)(struct rcu_head *head);
> +    unsigned int added;
>  };
>  
>  #define RCU_HEAD_INIT   { .next = NULL, .func = NULL }

Cheers,
Alejandro