[PATCH 0/3] rcu_seq_done_exact() improvements

[PATCH 0/3] rcu_seq_done_exact() improvements

[Joel Fernandes]

These are small improvements to rcu_seq_done_exact() improving its readability
and adding error checking. Passes rcutorture testing. Will send the rcutorture
wrap around testing separately.

Frederic Weisbecker (1):
  rcu: Comment on the extraneous delta test on rcu_seq_done_exact()

Joel Fernandes (2):
  rcu: Replace magic number with meaningful constant in
    rcu_seq_done_exact()
  rcu: Add warning to ensure rcu_seq_done_exact() is working

 kernel/rcu/rcu.h  | 14 +++++++++++++-
 kernel/rcu/tree.c |  5 +++++
 2 files changed, 18 insertions(+), 1 deletion(-)

-- 
2.43.0

[PATCH 1/3] rcu: Replace magic number with meaningful constant in rcu_seq_done_exact()

[Joel Fernandes]

The rcu_seq_done_exact() function checks if a grace period has completed by
comparing sequence numbers. It includes a guard band to handle sequence number
wraparound, which was previously expressed using the magic number calculation
'3 * RCU_SEQ_STATE_MASK + 1'.

This magic number is not immediately obvious in terms of what it represents.

Instead, the reason we need this tiny guardband is because of the lag between
the setting of rcu_state.gp_seq_polled and root rnp's gp_seq in rcu_gp_init().

This guardband needs to be at least 2 GPs worth of counts, to avoid recognizing
the newly started GP as completed immediately, due to the following sequence
which arises due to the delay between update of rcu_state.gp_seq_polled and
root rnp's gp_seq:

rnp->gp_seq = rcu_state.gp_seq = 0

    CPU 0                                           CPU 1
    -----                                           -----
    // rcu_state.gp_seq = 1
    rcu_seq_start(&rcu_state.gp_seq)
                                                    // snap = 8
                                                    snap = rcu_seq_snap(&rcu_state.gp_seq)
                                                    // Two full GP differences
                                                    rcu_seq_done_exact(&rnp->gp_seq, snap)
    // rnp->gp_seq = 1
    WRITE_ONCE(rnp->gp_seq, rcu_state.gp_seq);

This can happen due to get_state_synchronize_rcu_full() sampling
rcu_state.gp_seq_polled, however the poll_state_synchronize_rcu_full()
sampling the root rnp's gp_seq. The delay between the update of the 2
counters occurs in rcu_gp_init() during which the counters briefly go
out of sync.

Make the guardband explictly 2 GPs. This improves code readability and
maintainability by making the intent clearer as well.

Suggested-by: Frederic Weisbecker <frederic@kernel.org>
Signed-off-by: Joel Fernandes <joelagnelf@nvidia.com>
---
 kernel/rcu/rcu.h | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/kernel/rcu/rcu.h b/kernel/rcu/rcu.h
index eed2951a4962..5e1ee570bb27 100644
--- a/kernel/rcu/rcu.h
+++ b/kernel/rcu/rcu.h
@@ -57,6 +57,9 @@
 /* Low-order bit definition for polled grace-period APIs. */
 #define RCU_GET_STATE_COMPLETED	0x1
 
+/* A complete grace period count */
+#define RCU_SEQ_GP (RCU_SEQ_STATE_MASK + 1)
+
 extern int sysctl_sched_rt_runtime;
 
 /*
@@ -162,7 +165,7 @@ static inline bool rcu_seq_done_exact(unsigned long *sp, unsigned long s)
 {
 	unsigned long cur_s = READ_ONCE(*sp);
 
-	return ULONG_CMP_GE(cur_s, s) || ULONG_CMP_LT(cur_s, s - (3 * RCU_SEQ_STATE_MASK + 1));
+	return ULONG_CMP_GE(cur_s, s) || ULONG_CMP_LT(cur_s, s - (2 * RCU_SEQ_GP));
 }
 
 /*
-- 
2.43.0

Re: [PATCH 1/3] rcu: Replace magic number with meaningful constant in rcu_seq_done_exact()

[Paul E. McKenney]

On Mon, Mar 24, 2025 at 01:01:53PM -0400, Joel Fernandes wrote:
> The rcu_seq_done_exact() function checks if a grace period has completed by
> comparing sequence numbers. It includes a guard band to handle sequence number
> wraparound, which was previously expressed using the magic number calculation
> '3 * RCU_SEQ_STATE_MASK + 1'.
> 
> This magic number is not immediately obvious in terms of what it represents.
> 
> Instead, the reason we need this tiny guardband is because of the lag between
> the setting of rcu_state.gp_seq_polled and root rnp's gp_seq in rcu_gp_init().
> 
> This guardband needs to be at least 2 GPs worth of counts, to avoid recognizing
> the newly started GP as completed immediately, due to the following sequence
> which arises due to the delay between update of rcu_state.gp_seq_polled and
> root rnp's gp_seq:
> 
> rnp->gp_seq = rcu_state.gp_seq = 0
> 
>     CPU 0                                           CPU 1
>     -----                                           -----
>     // rcu_state.gp_seq = 1
>     rcu_seq_start(&rcu_state.gp_seq)
>                                                     // snap = 8
>                                                     snap = rcu_seq_snap(&rcu_state.gp_seq)
>                                                     // Two full GP differences
>                                                     rcu_seq_done_exact(&rnp->gp_seq, snap)
>     // rnp->gp_seq = 1
>     WRITE_ONCE(rnp->gp_seq, rcu_state.gp_seq);
> 
> This can happen due to get_state_synchronize_rcu_full() sampling
> rcu_state.gp_seq_polled, however the poll_state_synchronize_rcu_full()
> sampling the root rnp's gp_seq. The delay between the update of the 2
> counters occurs in rcu_gp_init() during which the counters briefly go
> out of sync.
> 
> Make the guardband explictly 2 GPs. This improves code readability and
> maintainability by making the intent clearer as well.
> 
> Suggested-by: Frederic Weisbecker <frederic@kernel.org>
> Signed-off-by: Joel Fernandes <joelagnelf@nvidia.com>

One concern is that a small error anywhere in the code could cause this
minimal guard band to be too small.  This is not a problem for some
use cases (rcu_barrier() just does an extra operation, and normal grace
periods are protected from forever-idle CPUs by ->gpwrap), but could be
an issue on 32-bit systems for user of polled RCU grace periods.

In contrast, making the guard band a bit longer than it needs to be
has little or no downside.

							Thanx, Paul

> ---
>  kernel/rcu/rcu.h | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/rcu/rcu.h b/kernel/rcu/rcu.h
> index eed2951a4962..5e1ee570bb27 100644
> --- a/kernel/rcu/rcu.h
> +++ b/kernel/rcu/rcu.h
> @@ -57,6 +57,9 @@
>  /* Low-order bit definition for polled grace-period APIs. */
>  #define RCU_GET_STATE_COMPLETED	0x1
>  
> +/* A complete grace period count */
> +#define RCU_SEQ_GP (RCU_SEQ_STATE_MASK + 1)
> +
>  extern int sysctl_sched_rt_runtime;
>  
>  /*
> @@ -162,7 +165,7 @@ static inline bool rcu_seq_done_exact(unsigned long *sp, unsigned long s)
>  {
>  	unsigned long cur_s = READ_ONCE(*sp);
>  
> -	return ULONG_CMP_GE(cur_s, s) || ULONG_CMP_LT(cur_s, s - (3 * RCU_SEQ_STATE_MASK + 1));
> +	return ULONG_CMP_GE(cur_s, s) || ULONG_CMP_LT(cur_s, s - (2 * RCU_SEQ_GP));
>  }
>  
>  /*
> -- 
> 2.43.0
> 

Re: [PATCH 1/3] rcu: Replace magic number with meaningful constant in rcu_seq_done_exact()

[Joel Fernandes]

> On Mar 26, 2025, at 6:33 PM, Paul E. McKenney <paulmck@kernel.org> wrote:
> 
> On Mon, Mar 24, 2025 at 01:01:53PM -0400, Joel Fernandes wrote:
>> The rcu_seq_done_exact() function checks if a grace period has completed by
>> comparing sequence numbers. It includes a guard band to handle sequence number
>> wraparound, which was previously expressed using the magic number calculation
>> '3 * RCU_SEQ_STATE_MASK + 1'.
>> 
>> This magic number is not immediately obvious in terms of what it represents.
>> 
>> Instead, the reason we need this tiny guardband is because of the lag between
>> the setting of rcu_state.gp_seq_polled and root rnp's gp_seq in rcu_gp_init().
>> 
>> This guardband needs to be at least 2 GPs worth of counts, to avoid recognizing
>> the newly started GP as completed immediately, due to the following sequence
>> which arises due to the delay between update of rcu_state.gp_seq_polled and
>> root rnp's gp_seq:
>> 
>> rnp->gp_seq = rcu_state.gp_seq = 0
>> 
>>    CPU 0                                           CPU 1
>>    -----                                           -----
>>    // rcu_state.gp_seq = 1
>>    rcu_seq_start(&rcu_state.gp_seq)
>>                                                    // snap = 8
>>                                                    snap = rcu_seq_snap(&rcu_state.gp_seq)
>>                                                    // Two full GP differences
>>                                                    rcu_seq_done_exact(&rnp->gp_seq, snap)
>>    // rnp->gp_seq = 1
>>    WRITE_ONCE(rnp->gp_seq, rcu_state.gp_seq);
>> 
>> This can happen due to get_state_synchronize_rcu_full() sampling
>> rcu_state.gp_seq_polled, however the poll_state_synchronize_rcu_full()
>> sampling the root rnp's gp_seq. The delay between the update of the 2
>> counters occurs in rcu_gp_init() during which the counters briefly go
>> out of sync.
>> 
>> Make the guardband explictly 2 GPs. This improves code readability and
>> maintainability by making the intent clearer as well.
>> 
>> Suggested-by: Frederic Weisbecker <frederic@kernel.org>
>> Signed-off-by: Joel Fernandes <joelagnelf@nvidia.com>
> 
> One concern is that a small error anywhere in the code could cause this
> minimal guard band to be too small.  This is not a problem for some
> use cases (rcu_barrier() just does an extra operation, and normal grace
> periods are protected from forever-idle CPUs by ->gpwrap), but could be
> an issue on 32-bit systems for user of polled RCU grace periods.

Could you provide more details of the usecase (sequence of steps) causing an issue for 32 bit polled RCU users? I am not able to see how this patch can affect them.

> 
> In contrast, making the guard band a bit longer than it needs to be
> has little or no downside.

Making it 3 GP instead of 2 should be ok with me as long as we document it but at least it will not be a magic number based on an equation. I feel we should not put random magic numbers which is more dangerous since it is hard to explain (and hence debug — just my 2 cents).

Thanks.

> 
>                            Thanx, Paul
> 
>> ---
>> kernel/rcu/rcu.h | 5 ++++-
>> 1 file changed, 4 insertions(+), 1 deletion(-)
>> 
>> diff --git a/kernel/rcu/rcu.h b/kernel/rcu/rcu.h
>> index eed2951a4962..5e1ee570bb27 100644
>> --- a/kernel/rcu/rcu.h
>> +++ b/kernel/rcu/rcu.h
>> @@ -57,6 +57,9 @@
>> /* Low-order bit definition for polled grace-period APIs. */
>> #define RCU_GET_STATE_COMPLETED    0x1
>> 
>> +/* A complete grace period count */
>> +#define RCU_SEQ_GP (RCU_SEQ_STATE_MASK + 1)
>> +
>> extern int sysctl_sched_rt_runtime;
>> 
>> /*
>> @@ -162,7 +165,7 @@ static inline bool rcu_seq_done_exact(unsigned long *sp, unsigned long s)
>> {
>>    unsigned long cur_s = READ_ONCE(*sp);
>> 
>> -    return ULONG_CMP_GE(cur_s, s) || ULONG_CMP_LT(cur_s, s - (3 * RCU_SEQ_STATE_MASK + 1));
>> +    return ULONG_CMP_GE(cur_s, s) || ULONG_CMP_LT(cur_s, s - (2 * RCU_SEQ_GP));
>> }
>> 
>> /*
>> --
>> 2.43.0
>> 

Re: [PATCH 1/3] rcu: Replace magic number with meaningful constant in rcu_seq_done_exact()

[Paul E. McKenney]

On Wed, Mar 26, 2025 at 10:42:52PM +0000, Joel Fernandes wrote:
> 
> 
> > On Mar 26, 2025, at 6:33 PM, Paul E. McKenney <paulmck@kernel.org> wrote:
> > 
> > On Mon, Mar 24, 2025 at 01:01:53PM -0400, Joel Fernandes wrote:
> >> The rcu_seq_done_exact() function checks if a grace period has completed by
> >> comparing sequence numbers. It includes a guard band to handle sequence number
> >> wraparound, which was previously expressed using the magic number calculation
> >> '3 * RCU_SEQ_STATE_MASK + 1'.
> >> 
> >> This magic number is not immediately obvious in terms of what it represents.
> >> 
> >> Instead, the reason we need this tiny guardband is because of the lag between
> >> the setting of rcu_state.gp_seq_polled and root rnp's gp_seq in rcu_gp_init().
> >> 
> >> This guardband needs to be at least 2 GPs worth of counts, to avoid recognizing
> >> the newly started GP as completed immediately, due to the following sequence
> >> which arises due to the delay between update of rcu_state.gp_seq_polled and
> >> root rnp's gp_seq:
> >> 
> >> rnp->gp_seq = rcu_state.gp_seq = 0
> >> 
> >>    CPU 0                                           CPU 1
> >>    -----                                           -----
> >>    // rcu_state.gp_seq = 1
> >>    rcu_seq_start(&rcu_state.gp_seq)
> >>                                                    // snap = 8
> >>                                                    snap = rcu_seq_snap(&rcu_state.gp_seq)
> >>                                                    // Two full GP differences
> >>                                                    rcu_seq_done_exact(&rnp->gp_seq, snap)
> >>    // rnp->gp_seq = 1
> >>    WRITE_ONCE(rnp->gp_seq, rcu_state.gp_seq);
> >> 
> >> This can happen due to get_state_synchronize_rcu_full() sampling
> >> rcu_state.gp_seq_polled, however the poll_state_synchronize_rcu_full()
> >> sampling the root rnp's gp_seq. The delay between the update of the 2
> >> counters occurs in rcu_gp_init() during which the counters briefly go
> >> out of sync.
> >> 
> >> Make the guardband explictly 2 GPs. This improves code readability and
> >> maintainability by making the intent clearer as well.
> >> 
> >> Suggested-by: Frederic Weisbecker <frederic@kernel.org>
> >> Signed-off-by: Joel Fernandes <joelagnelf@nvidia.com>
> > 
> > One concern is that a small error anywhere in the code could cause this
> > minimal guard band to be too small.  This is not a problem for some
> > use cases (rcu_barrier() just does an extra operation, and normal grace
> > periods are protected from forever-idle CPUs by ->gpwrap), but could be
> > an issue on 32-bit systems for user of polled RCU grace periods.
> 
> Could you provide more details of the usecase (sequence of steps) causing an issue for 32 bit polled RCU users? I am not able to see how this patch can affect them.
> 
> > 
> > In contrast, making the guard band a bit longer than it needs to be
> > has little or no downside.
> 
> Making it 3 GP instead of 2 should be ok with me as long as we document it but at least it will not be a magic number based on an equation. I feel we should not put random magic numbers which is more dangerous since it is hard to explain (and hence debug — just my 2 cents).

Apologies, I was getting ahead of us on this one.

That third (and maybe also a fourth) grace period becomes important if
we start getting memory contention on rcu_state.gp_seq, in which case
we would want the polled grace periods to look at the leaf rcu_node
structure's gp_seq, which would introduce extra slop.

For now, for this patch, assuming you guys promise to remember this
should such memory contention prove problematic:

Reviewed-by: Paul E. McKenney <paulmck@kernel.org>

							Thanx, Paul

> Thanks.
> 
> > 
> >                            Thanx, Paul
> > 
> >> ---
> >> kernel/rcu/rcu.h | 5 ++++-
> >> 1 file changed, 4 insertions(+), 1 deletion(-)
> >> 
> >> diff --git a/kernel/rcu/rcu.h b/kernel/rcu/rcu.h
> >> index eed2951a4962..5e1ee570bb27 100644
> >> --- a/kernel/rcu/rcu.h
> >> +++ b/kernel/rcu/rcu.h
> >> @@ -57,6 +57,9 @@
> >> /* Low-order bit definition for polled grace-period APIs. */
> >> #define RCU_GET_STATE_COMPLETED    0x1
> >> 
> >> +/* A complete grace period count */
> >> +#define RCU_SEQ_GP (RCU_SEQ_STATE_MASK + 1)
> >> +
> >> extern int sysctl_sched_rt_runtime;
> >> 
> >> /*
> >> @@ -162,7 +165,7 @@ static inline bool rcu_seq_done_exact(unsigned long *sp, unsigned long s)
> >> {
> >>    unsigned long cur_s = READ_ONCE(*sp);
> >> 
> >> -    return ULONG_CMP_GE(cur_s, s) || ULONG_CMP_LT(cur_s, s - (3 * RCU_SEQ_STATE_MASK + 1));
> >> +    return ULONG_CMP_GE(cur_s, s) || ULONG_CMP_LT(cur_s, s - (2 * RCU_SEQ_GP));
> >> }
> >> 
> >> /*
> >> --
> >> 2.43.0
> >> 

Re: [PATCH 1/3] rcu: Replace magic number with meaningful constant in rcu_seq_done_exact()

[Joel Fernandes]

On 3/27/2025 1:09 PM, Paul E. McKenney wrote:
>>> In contrast, making the guard band a bit longer than it needs to be
>>> has little or no downside.
>> Making it 3 GP instead of 2 should be ok with me as long as we document it but at least it will not be a magic number based on an equation. I feel we should not put random magic numbers which is more dangerous since it is hard to explain (and hence debug — just my 2 cents).
> Apologies, I was getting ahead of us on this one.
> 
> That third (and maybe also a fourth) grace period becomes important if
> we start getting memory contention on rcu_state.gp_seq, in which case
> we would want the polled grace periods to look at the leaf rcu_node
> structure's gp_seq, which would introduce extra slop.

Makes sense.

> For now, for this patch, assuming you guys promise to remember this
> should such memory contention prove problematic:
> 
> Reviewed-by: Paul E. McKenney <paulmck@kernel.org>

Thanks! Will add the tag.

 - Joel

Re: [PATCH 1/3] rcu: Replace magic number with meaningful constant in rcu_seq_done_exact()

[Frederic Weisbecker]

Le Thu, Mar 27, 2025 at 10:09:48AM -0700, Paul E. McKenney a écrit :
> On Wed, Mar 26, 2025 at 10:42:52PM +0000, Joel Fernandes wrote:
> > 
> > 
> > > On Mar 26, 2025, at 6:33 PM, Paul E. McKenney <paulmck@kernel.org> wrote:
> > > 
> > > On Mon, Mar 24, 2025 at 01:01:53PM -0400, Joel Fernandes wrote:
> > >> The rcu_seq_done_exact() function checks if a grace period has completed by
> > >> comparing sequence numbers. It includes a guard band to handle sequence number
> > >> wraparound, which was previously expressed using the magic number calculation
> > >> '3 * RCU_SEQ_STATE_MASK + 1'.
> > >> 
> > >> This magic number is not immediately obvious in terms of what it represents.
> > >> 
> > >> Instead, the reason we need this tiny guardband is because of the lag between
> > >> the setting of rcu_state.gp_seq_polled and root rnp's gp_seq in rcu_gp_init().
> > >> 
> > >> This guardband needs to be at least 2 GPs worth of counts, to avoid recognizing
> > >> the newly started GP as completed immediately, due to the following sequence
> > >> which arises due to the delay between update of rcu_state.gp_seq_polled and
> > >> root rnp's gp_seq:
> > >> 
> > >> rnp->gp_seq = rcu_state.gp_seq = 0
> > >> 
> > >>    CPU 0                                           CPU 1
> > >>    -----                                           -----
> > >>    // rcu_state.gp_seq = 1
> > >>    rcu_seq_start(&rcu_state.gp_seq)
> > >>                                                    // snap = 8
> > >>                                                    snap = rcu_seq_snap(&rcu_state.gp_seq)
> > >>                                                    // Two full GP differences
> > >>                                                    rcu_seq_done_exact(&rnp->gp_seq, snap)
> > >>    // rnp->gp_seq = 1
> > >>    WRITE_ONCE(rnp->gp_seq, rcu_state.gp_seq);
> > >> 
> > >> This can happen due to get_state_synchronize_rcu_full() sampling
> > >> rcu_state.gp_seq_polled, however the poll_state_synchronize_rcu_full()
> > >> sampling the root rnp's gp_seq. The delay between the update of the 2
> > >> counters occurs in rcu_gp_init() during which the counters briefly go
> > >> out of sync.
> > >> 
> > >> Make the guardband explictly 2 GPs. This improves code readability and
> > >> maintainability by making the intent clearer as well.
> > >> 
> > >> Suggested-by: Frederic Weisbecker <frederic@kernel.org>
> > >> Signed-off-by: Joel Fernandes <joelagnelf@nvidia.com>
> > > 
> > > One concern is that a small error anywhere in the code could cause this
> > > minimal guard band to be too small.  This is not a problem for some
> > > use cases (rcu_barrier() just does an extra operation, and normal grace
> > > periods are protected from forever-idle CPUs by ->gpwrap), but could be
> > > an issue on 32-bit systems for user of polled RCU grace periods.
> > 
> > Could you provide more details of the usecase (sequence of steps) causing an issue for 32 bit polled RCU users? I am not able to see how this patch can affect them.
> > 
> > > 
> > > In contrast, making the guard band a bit longer than it needs to be
> > > has little or no downside.
> > 
> > Making it 3 GP instead of 2 should be ok with me as long as we document it but at least it will not be a magic number based on an equation. I feel we should not put random magic numbers which is more dangerous since it is hard to explain (and hence debug — just my 2 cents).
> 
> Apologies, I was getting ahead of us on this one.
> 
> That third (and maybe also a fourth) grace period becomes important if
> we start getting memory contention on rcu_state.gp_seq, in which case
> we would want the polled grace periods to look at the leaf rcu_node
> structure's gp_seq, which would introduce extra slop.
> 
> For now, for this patch, assuming you guys promise to remember this
> should such memory contention prove problematic:
> 
> Reviewed-by: Paul E. McKenney <paulmck@kernel.org>

Taking again a step back on this. This all happens because we have two different
global gp sequence counter, and the two are used on different occasions, even mixed
up together since 85aad7cc4178 ("rcu: Fix get_state_synchronize_rcu_full()
GP-start detection") even though they have different constraints and
reliability.

This can be error prone and confusing.

So I'm unfortunately asking again if it wouldn't be a good idea to have a single
global state counter that lives in the root node so that we don't have it
duplicated in rcu_state.gp_seq.

This involves some special care:

1) As you pointed out, the root can also be the unique leaf. So care must
   be taken that no QS is reported before new online CPUs have been handled
   and QS masks initialized. In order for __note_gp_changes() to handle
   it gracefully we must have two seq states: one to tell the
   grace period has started (before checking new online CPUs): RCU_STATE_STARTED
   and one to tell that grace period is ready to receive quiescent states:
   RCU_STATE_WAIT_QS. Although not strictly required, __note_gp_changes() can
   ignore simple state changes to RCU_STATE_STARTED in order to spare useless
   work and rnp locking. No QS will be reported before ->qsmask are initialized
   in any case.

2) rcu_gp_cleanup() currently ends the grace period top-down through the rnp tree
   and only then on rcu_state. With this change it's simply top-down. I have audited
   rcu_seq_snap() and rcu_start_this_gp() uses and I haven't found an obvious
   issue with that.

The draft below holds 30 minutes of TREE01:

diff --git a/kernel/rcu/rcu.h b/kernel/rcu/rcu.h
index feb3ac1dc5d5..c4bb3eb92a58 100644
--- a/kernel/rcu/rcu.h
+++ b/kernel/rcu/rcu.h
@@ -19,6 +19,12 @@
  * The two least significant bits contain the control flags.
  * The most significant bits contain the grace-period sequence counter.
  *
+ * RCU_STATE_STARTED		:	The gp has started but QS are not
+ *					requested yet.
+ *
+ * RCU_STATE_WAIT_QS		:	The gp has started and waits for
+ *					QS to be reported.
+ *
  * When both control flags are zero, no grace period is in progress.
  * When either bit is non-zero, a grace period has started and is in
  * progress. When the grace period completes, the control flags are reset
@@ -54,6 +60,9 @@
  *					grace-period sequence number.
  */
 
+#define RCU_STATE_STARTED	0x1
+#define RCU_STATE_WAIT_QS	0x2
+
 /* Low-order bit definition for polled grace-period APIs. */
 #define RCU_GET_STATE_COMPLETED	0x1
 
@@ -92,7 +101,13 @@ static inline void rcu_seq_start(unsigned long *sp)
 {
 	WRITE_ONCE(*sp, *sp + 1);
 	smp_mb(); /* Ensure update-side operation after counter increment. */
-	WARN_ON_ONCE(rcu_seq_state(*sp) != 1);
+	WARN_ON_ONCE(rcu_seq_state(*sp) != RCU_STATE_STARTED);
+}
+
+static inline void rcu_seq_wait_qs(unsigned long *sp)
+{
+	WRITE_ONCE(*sp, *sp + 1);
+	WARN_ON_ONCE(rcu_seq_state(*sp) != RCU_STATE_WAIT_QS);
 }
 
 /* Compute the end-of-grace-period value for the specified sequence number. */
@@ -135,6 +150,11 @@ static inline unsigned long rcu_seq_current(unsigned long *sp)
 	return READ_ONCE(*sp);
 }
 
+static inline bool rcu_seq_changed(unsigned long old, unsigned long new)
+{
+	return (old & ~RCU_STATE_STARTED) != (new & ~RCU_STATE_STARTED);
+}
+
 /*
  * Given a snapshot from rcu_seq_snap(), determine whether or not the
  * corresponding update-side operation has started.
diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
index 475f31deed14..54480ee9599f 100644
--- a/kernel/rcu/tree.c
+++ b/kernel/rcu/tree.c
@@ -81,9 +81,9 @@ static DEFINE_PER_CPU_SHARED_ALIGNED(struct rcu_data, rcu_data) = {
 	.gpwrap = true,
 };
 static struct rcu_state rcu_state = {
+	.node[0].gp_seq = (0UL - 300UL) << RCU_SEQ_CTR_SHIFT,
 	.level = { &rcu_state.node[0] },
 	.gp_state = RCU_GP_IDLE,
-	.gp_seq = (0UL - 300UL) << RCU_SEQ_CTR_SHIFT,
 	.barrier_mutex = __MUTEX_INITIALIZER(rcu_state.barrier_mutex),
 	.barrier_lock = __RAW_SPIN_LOCK_UNLOCKED(rcu_state.barrier_lock),
 	.name = RCU_NAME,
@@ -192,6 +192,21 @@ int rcu_get_gp_kthreads_prio(void)
 }
 EXPORT_SYMBOL_GPL(rcu_get_gp_kthreads_prio);
 
+/*
+ * Return the root node of the rcu_state structure.
+ */
+static struct rcu_node *rcu_get_root(void)
+{
+	return &rcu_state.node[0];
+}
+
+static unsigned long *rcu_root_seq(void)
+{
+	struct rcu_node *root = rcu_get_root();
+
+	return &root->gp_seq;
+}
+
 /*
  * Number of grace periods between delays, normalized by the duration of
  * the delay.  The longer the delay, the more the grace periods between
@@ -210,7 +225,7 @@ EXPORT_SYMBOL_GPL(rcu_get_gp_kthreads_prio);
  */
 static int rcu_gp_in_progress(void)
 {
-	return rcu_seq_state(rcu_seq_current(&rcu_state.gp_seq));
+	return rcu_seq_state(rcu_seq_current(rcu_root_seq()));
 }
 
 /*
@@ -504,7 +519,7 @@ static int rcu_pending(int user);
  */
 unsigned long rcu_get_gp_seq(void)
 {
-	return READ_ONCE(rcu_state.gp_seq);
+	return READ_ONCE(*rcu_root_seq());
 }
 EXPORT_SYMBOL_GPL(rcu_get_gp_seq);
 
@@ -520,21 +535,13 @@ unsigned long rcu_exp_batches_completed(void)
 }
 EXPORT_SYMBOL_GPL(rcu_exp_batches_completed);
 
-/*
- * Return the root node of the rcu_state structure.
- */
-static struct rcu_node *rcu_get_root(void)
-{
-	return &rcu_state.node[0];
-}
-
 /*
  * Send along grace-period-related data for rcutorture diagnostics.
  */
 void rcutorture_get_gp_data(int *flags, unsigned long *gp_seq)
 {
 	*flags = READ_ONCE(rcu_state.gp_flags);
-	*gp_seq = rcu_seq_current(&rcu_state.gp_seq);
+	*gp_seq = rcu_seq_current(rcu_root_seq());
 }
 EXPORT_SYMBOL_GPL(rcutorture_get_gp_data);
 
@@ -1010,7 +1017,7 @@ static bool rcu_start_this_gp(struct rcu_node *rnp_start, struct rcu_data *rdp,
 		trace_rcu_this_gp(rnp, rdp, gp_seq_req, TPS("NoGPkthread"));
 		goto unlock_out;
 	}
-	trace_rcu_grace_period(rcu_state.name, data_race(rcu_state.gp_seq), TPS("newreq"));
+	trace_rcu_grace_period(rcu_state.name, data_race(*rcu_root_seq()), TPS("newreq"));
 	ret = true;  /* Caller must wake GP kthread. */
 unlock_out:
 	/* Push furthest requested GP to leaf node and rcu_data structure. */
@@ -1095,7 +1102,7 @@ static void rcu_gp_kthread_wake(void)
 	    !READ_ONCE(rcu_state.gp_flags) || !t)
 		return;
 	WRITE_ONCE(rcu_state.gp_wake_time, jiffies);
-	WRITE_ONCE(rcu_state.gp_wake_seq, READ_ONCE(rcu_state.gp_seq));
+	WRITE_ONCE(rcu_state.gp_wake_seq, READ_ONCE(*rcu_root_seq()));
 	swake_up_one_online(&rcu_state.gp_wq);
 }
 
@@ -1135,7 +1142,7 @@ static bool rcu_accelerate_cbs(struct rcu_node *rnp, struct rcu_data *rdp)
 	 * accelerating callback invocation to an earlier grace-period
 	 * number.
 	 */
-	gp_seq_req = rcu_seq_snap(&rcu_state.gp_seq);
+	gp_seq_req = rcu_seq_snap(rcu_root_seq());
 	if (rcu_segcblist_accelerate(&rdp->cblist, gp_seq_req))
 		ret = rcu_start_this_gp(rnp, rdp, gp_seq_req);
 
@@ -1164,7 +1171,7 @@ static void rcu_accelerate_cbs_unlocked(struct rcu_node *rnp,
 	bool needwake;
 
 	rcu_lockdep_assert_cblist_protected(rdp);
-	c = rcu_seq_snap(&rcu_state.gp_seq);
+	c = rcu_seq_snap(rcu_root_seq());
 	if (!READ_ONCE(rdp->gpwrap) && ULONG_CMP_GE(rdp->gp_seq_needed, c)) {
 		/* Old request still live, so mark recent callbacks. */
 		(void)rcu_segcblist_accelerate(&rdp->cblist, c);
@@ -1249,7 +1256,7 @@ static bool __note_gp_changes(struct rcu_node *rnp, struct rcu_data *rdp)
 
 	raw_lockdep_assert_held_rcu_node(rnp);
 
-	if (rdp->gp_seq == rnp->gp_seq)
+	if (!rcu_seq_changed(rdp->gp_seq, rnp->gp_seq))
 		return false; /* Nothing to do. */
 
 	/* Handle the ends of any preceding grace periods first. */
@@ -1298,7 +1305,7 @@ static void note_gp_changes(struct rcu_data *rdp)
 
 	local_irq_save(flags);
 	rnp = rdp->mynode;
-	if ((rdp->gp_seq == rcu_seq_current(&rnp->gp_seq) &&
+	if ((!rcu_seq_changed(rdp->gp_seq, rcu_seq_current(&rnp->gp_seq)) &&
 	     !unlikely(READ_ONCE(rdp->gpwrap))) || /* w/out lock. */
 	    !raw_spin_trylock_rcu_node(rnp)) { /* irqs already off, so later. */
 		local_irq_restore(flags);
@@ -1341,7 +1348,7 @@ static bool rcu_gp_slow_is_suppressed(void)
 static void rcu_gp_slow(int delay)
 {
 	if (!rcu_gp_slow_is_suppressed() && delay > 0 &&
-	    !(rcu_seq_ctr(rcu_state.gp_seq) % (rcu_num_nodes * PER_RCU_NODE_PERIOD * delay)))
+	    !(rcu_seq_ctr(*rcu_root_seq()) % (rcu_num_nodes * PER_RCU_NODE_PERIOD * delay)))
 		schedule_timeout_idle(delay);
 }
 
@@ -1802,10 +1809,9 @@ static noinline_for_stack bool rcu_gp_init(void)
 	/* Advance to a new grace period and initialize state. */
 	record_gp_stall_check_time();
 	/* Record GP times before starting GP, hence rcu_seq_start(). */
-	rcu_seq_start(&rcu_state.gp_seq);
-	ASSERT_EXCLUSIVE_WRITER(rcu_state.gp_seq);
+	rcu_seq_start(rcu_root_seq());
 	start_new_poll = rcu_sr_normal_gp_init();
-	trace_rcu_grace_period(rcu_state.name, rcu_state.gp_seq, TPS("start"));
+	trace_rcu_grace_period(rcu_state.name, *rcu_root_seq(), TPS("start"));
 	rcu_poll_gp_seq_start(&rcu_state.gp_seq_polled_snap);
 	raw_spin_unlock_irq_rcu_node(rnp);
 
@@ -1902,7 +1908,10 @@ static noinline_for_stack bool rcu_gp_init(void)
 		rdp = this_cpu_ptr(&rcu_data);
 		rcu_preempt_check_blocked_tasks(rnp);
 		rnp->qsmask = rnp->qsmaskinit;
-		WRITE_ONCE(rnp->gp_seq, rcu_state.gp_seq);
+		if (!rnp->parent)
+			rcu_seq_wait_qs(&rnp->gp_seq);
+		else
+			WRITE_ONCE(rnp->gp_seq, *rcu_root_seq());
 		if (rnp == rdp->mynode)
 			(void)__note_gp_changes(rnp, rdp);
 		rcu_preempt_boost_start_gp(rnp);
@@ -2018,7 +2027,7 @@ static noinline_for_stack void rcu_gp_fqs_loop(void)
 			WRITE_ONCE(rcu_state.jiffies_kick_kthreads,
 				   jiffies + (j ? 3 * j : 2));
 		}
-		trace_rcu_grace_period(rcu_state.name, rcu_state.gp_seq,
+		trace_rcu_grace_period(rcu_state.name, *rcu_root_seq(),
 				       TPS("fqswait"));
 		WRITE_ONCE(rcu_state.gp_state, RCU_GP_WAIT_FQS);
 		(void)swait_event_idle_timeout_exclusive(rcu_state.gp_wq,
@@ -2041,7 +2050,7 @@ static noinline_for_stack void rcu_gp_fqs_loop(void)
 		/* If time for quiescent-state forcing, do it. */
 		if (!time_after(rcu_state.jiffies_force_qs, jiffies) ||
 		    (gf & (RCU_GP_FLAG_FQS | RCU_GP_FLAG_OVLD))) {
-			trace_rcu_grace_period(rcu_state.name, rcu_state.gp_seq,
+			trace_rcu_grace_period(rcu_state.name, *rcu_root_seq(),
 					       TPS("fqsstart"));
 			rcu_gp_fqs(first_gp_fqs);
 			gf = 0;
@@ -2049,7 +2058,7 @@ static noinline_for_stack void rcu_gp_fqs_loop(void)
 				first_gp_fqs = false;
 				gf = rcu_state.cbovld ? RCU_GP_FLAG_OVLD : 0;
 			}
-			trace_rcu_grace_period(rcu_state.name, rcu_state.gp_seq,
+			trace_rcu_grace_period(rcu_state.name, *rcu_root_seq(),
 					       TPS("fqsend"));
 			cond_resched_tasks_rcu_qs();
 			WRITE_ONCE(rcu_state.gp_activity, jiffies);
@@ -2060,7 +2069,7 @@ static noinline_for_stack void rcu_gp_fqs_loop(void)
 			cond_resched_tasks_rcu_qs();
 			WRITE_ONCE(rcu_state.gp_activity, jiffies);
 			WARN_ON(signal_pending(current));
-			trace_rcu_grace_period(rcu_state.name, rcu_state.gp_seq,
+			trace_rcu_grace_period(rcu_state.name, *rcu_root_seq(),
 					       TPS("fqswaitsig"));
 			ret = 1; /* Keep old FQS timing. */
 			j = jiffies;
@@ -2081,7 +2090,6 @@ static noinline void rcu_gp_cleanup(void)
 	int cpu;
 	bool needgp = false;
 	unsigned long gp_duration;
-	unsigned long new_gp_seq;
 	bool offloaded;
 	struct rcu_data *rdp;
 	struct rcu_node *rnp = rcu_get_root();
@@ -2103,6 +2111,8 @@ static noinline void rcu_gp_cleanup(void)
 	 * period as completed in all of the rcu_node structures.
 	 */
 	rcu_poll_gp_seq_end(&rcu_state.gp_seq_polled_snap);
+	rcu_seq_end(rcu_root_seq());
+	smp_mb(); // Order against failing poll_state_synchronize_rcu_full().
 	raw_spin_unlock_irq_rcu_node(rnp);
 
 	/*
@@ -2114,16 +2124,12 @@ static noinline void rcu_gp_cleanup(void)
 	 * the rcu_node structures before the beginning of the next grace
 	 * period is recorded in any of the rcu_node structures.
 	 */
-	new_gp_seq = rcu_state.gp_seq;
-	rcu_seq_end(&new_gp_seq);
 	rcu_for_each_node_breadth_first(rnp) {
 		raw_spin_lock_irq_rcu_node(rnp);
 		if (WARN_ON_ONCE(rcu_preempt_blocked_readers_cgp(rnp)))
 			dump_blkd_tasks(rnp, 10);
 		WARN_ON_ONCE(rnp->qsmask);
-		WRITE_ONCE(rnp->gp_seq, new_gp_seq);
-		if (!rnp->parent)
-			smp_mb(); // Order against failing poll_state_synchronize_rcu_full().
+		WRITE_ONCE(rnp->gp_seq, *rcu_root_seq());
 		rdp = this_cpu_ptr(&rcu_data);
 		if (rnp == rdp->mynode)
 			needgp = __note_gp_changes(rnp, rdp) || needgp;
@@ -2146,9 +2152,7 @@ static noinline void rcu_gp_cleanup(void)
 	raw_spin_lock_irq_rcu_node(rnp); /* GP before ->gp_seq update. */
 
 	/* Declare grace period done, trace first to use old GP number. */
-	trace_rcu_grace_period(rcu_state.name, rcu_state.gp_seq, TPS("end"));
-	rcu_seq_end(&rcu_state.gp_seq);
-	ASSERT_EXCLUSIVE_WRITER(rcu_state.gp_seq);
+	trace_rcu_grace_period(rcu_state.name, *rcu_root_seq(), TPS("end"));
 	WRITE_ONCE(rcu_state.gp_state, RCU_GP_IDLE);
 	/* Check for GP requests since above loop. */
 	rdp = this_cpu_ptr(&rcu_data);
@@ -2173,7 +2177,7 @@ static noinline void rcu_gp_cleanup(void)
 
 		WRITE_ONCE(rcu_state.gp_flags, RCU_GP_FLAG_INIT);
 		WRITE_ONCE(rcu_state.gp_req_activity, jiffies);
-		trace_rcu_grace_period(rcu_state.name, rcu_state.gp_seq, TPS("newreq"));
+		trace_rcu_grace_period(rcu_state.name, *rcu_root_seq(), TPS("newreq"));
 	} else {
 
 		// We get here either if there is no need for an
@@ -2204,7 +2208,7 @@ static int __noreturn rcu_gp_kthread(void *unused)
 
 		/* Handle grace-period start. */
 		for (;;) {
-			trace_rcu_grace_period(rcu_state.name, rcu_state.gp_seq,
+			trace_rcu_grace_period(rcu_state.name, *rcu_root_seq(),
 					       TPS("reqwait"));
 			WRITE_ONCE(rcu_state.gp_state, RCU_GP_WAIT_GPS);
 			swait_event_idle_exclusive(rcu_state.gp_wq,
@@ -2218,7 +2222,7 @@ static int __noreturn rcu_gp_kthread(void *unused)
 			cond_resched_tasks_rcu_qs();
 			WRITE_ONCE(rcu_state.gp_activity, jiffies);
 			WARN_ON(signal_pending(current));
-			trace_rcu_grace_period(rcu_state.name, rcu_state.gp_seq,
+			trace_rcu_grace_period(rcu_state.name, *rcu_root_seq(),
 					       TPS("reqwaitsig"));
 		}
 
@@ -3299,9 +3303,10 @@ void synchronize_rcu(void)
 	// these counters.
 	local_irq_save(flags);
 	WARN_ON_ONCE(num_online_cpus() > 1);
-	rcu_state.gp_seq += (1 << RCU_SEQ_CTR_SHIFT);
+	rnp = rcu_get_root();
+	rnp->gp_seq += (1 << RCU_SEQ_CTR_SHIFT);
 	for (rnp = this_cpu_ptr(&rcu_data)->mynode; rnp; rnp = rnp->parent)
-		rnp->gp_seq_needed = rnp->gp_seq = rcu_state.gp_seq;
+		rnp->gp_seq_needed = rnp->gp_seq = *rcu_root_seq();
 	local_irq_restore(flags);
 }
 EXPORT_SYMBOL_GPL(synchronize_rcu);
@@ -3390,7 +3395,7 @@ static void start_poll_synchronize_rcu_common(void)
 	// get a grace period that no one needed.  These accesses are ordered
 	// by smp_mb(), and we are accessing them in the opposite order
 	// from which they are updated at grace-period start, as required.
-	needwake = rcu_start_this_gp(rnp, rdp, rcu_seq_snap(&rcu_state.gp_seq));
+	needwake = rcu_start_this_gp(rnp, rdp, rcu_seq_snap(rcu_root_seq()));
 	raw_spin_unlock_irqrestore_rcu_node(rnp, flags);
 	if (needwake)
 		rcu_gp_kthread_wake();
@@ -3512,11 +3517,9 @@ EXPORT_SYMBOL_GPL(poll_state_synchronize_rcu);
  */
 bool poll_state_synchronize_rcu_full(struct rcu_gp_oldstate *rgosp)
 {
-	struct rcu_node *rnp = rcu_get_root();
-
 	smp_mb(); // Order against root rcu_node structure grace-period cleanup.
 	if (rgosp->rgos_norm == RCU_GET_STATE_COMPLETED ||
-	    rcu_seq_done_exact(&rnp->gp_seq, rgosp->rgos_norm) ||
+	    rcu_seq_done_exact(rcu_root_seq(), rgosp->rgos_norm) ||
 	    rgosp->rgos_exp == RCU_GET_STATE_COMPLETED ||
 	    rcu_seq_done_exact(&rcu_state.expedited_sequence, rgosp->rgos_exp)) {
 		smp_mb(); /* Ensure GP ends before subsequent accesses. */
@@ -4062,9 +4065,9 @@ rcu_boot_init_percpu_data(int cpu)
 	WARN_ON_ONCE(ct->nesting != 1);
 	WARN_ON_ONCE(rcu_watching_snap_in_eqs(ct_rcu_watching_cpu(cpu)));
 	rdp->barrier_seq_snap = rcu_state.barrier_sequence;
-	rdp->rcu_ofl_gp_seq = rcu_state.gp_seq;
+	rdp->rcu_ofl_gp_seq = *rcu_root_seq();
 	rdp->rcu_ofl_gp_state = RCU_GP_CLEANED;
-	rdp->rcu_onl_gp_seq = rcu_state.gp_seq;
+	rdp->rcu_onl_gp_seq = *rcu_root_seq();
 	rdp->rcu_onl_gp_state = RCU_GP_CLEANED;
 	rdp->last_sched_clock = jiffies;
 	rdp->cpu = cpu;
@@ -4271,7 +4274,7 @@ void rcutree_report_cpu_starting(unsigned int cpu)
 	smp_store_release(&rcu_state.ncpus, rcu_state.ncpus + newcpu); /* ^^^ */
 	ASSERT_EXCLUSIVE_WRITER(rcu_state.ncpus);
 	rcu_gpnum_ovf(rnp, rdp); /* Offline-induced counter wrap? */
-	rdp->rcu_onl_gp_seq = READ_ONCE(rcu_state.gp_seq);
+	rdp->rcu_onl_gp_seq = READ_ONCE(*rcu_root_seq());
 	rdp->rcu_onl_gp_state = READ_ONCE(rcu_state.gp_state);
 
 	/* An incoming CPU should never be blocking a grace period. */
@@ -4322,7 +4325,7 @@ void rcutree_report_cpu_dead(void)
 	mask = rdp->grpmask;
 	arch_spin_lock(&rcu_state.ofl_lock);
 	raw_spin_lock_irqsave_rcu_node(rnp, flags); /* Enforce GP memory-order guarantee. */
-	rdp->rcu_ofl_gp_seq = READ_ONCE(rcu_state.gp_seq);
+	rdp->rcu_ofl_gp_seq = READ_ONCE(*rcu_root_seq());
 	rdp->rcu_ofl_gp_state = READ_ONCE(rcu_state.gp_state);
 	if (rnp->qsmask & mask) { /* RCU waiting on outgoing CPU? */
 		/* Report quiescent state -before- changing ->qsmaskinitnext! */
@@ -4534,7 +4537,7 @@ void rcu_scheduler_starting(void)
 	// Fix up the ->gp_seq counters.
 	local_irq_save(flags);
 	rcu_for_each_node_breadth_first(rnp)
-		rnp->gp_seq_needed = rnp->gp_seq = rcu_state.gp_seq;
+		rnp->gp_seq_needed = rnp->gp_seq = *rcu_root_seq();
 	local_irq_restore(flags);
 
 	// Switch out of early boot mode.
@@ -4583,9 +4586,9 @@ static void __init rcu_init_one(void)
 			raw_spin_lock_init(&rnp->fqslock);
 			lockdep_set_class_and_name(&rnp->fqslock,
 						   &rcu_fqs_class[i], fqs[i]);
-			rnp->gp_seq = rcu_state.gp_seq;
-			rnp->gp_seq_needed = rcu_state.gp_seq;
-			rnp->completedqs = rcu_state.gp_seq;
+			rnp->gp_seq = *rcu_root_seq();
+			rnp->gp_seq_needed = *rcu_root_seq();
+			rnp->completedqs = *rcu_root_seq();
 			rnp->qsmask = 0;
 			rnp->qsmaskinit = 0;
 			rnp->grplo = j * cpustride;
diff --git a/kernel/rcu/tree.h b/kernel/rcu/tree.h
index a9a811d9d7a3..9d811270b808 100644
--- a/kernel/rcu/tree.h
+++ b/kernel/rcu/tree.h
@@ -347,7 +347,6 @@ struct rcu_state {
 
 	/* The following fields are guarded by the root rcu_node's lock. */
 
-	unsigned long gp_seq ____cacheline_internodealigned_in_smp;
 						/* Grace-period sequence #. */
 	unsigned long gp_max;			/* Maximum GP duration in */
 						/*  jiffies. */
diff --git a/kernel/rcu/tree_stall.h b/kernel/rcu/tree_stall.h
index 925fcdad5dea..367adb062a6d 100644
--- a/kernel/rcu/tree_stall.h
+++ b/kernel/rcu/tree_stall.h
@@ -344,7 +344,7 @@ static void rcu_dump_cpu_stacks(unsigned long gp_seq)
 	rcu_for_each_leaf_node(rnp) {
 		printk_deferred_enter();
 		for_each_leaf_node_possible_cpu(rnp, cpu) {
-			if (gp_seq != data_race(rcu_state.gp_seq)) {
+			if (gp_seq != data_race(*rcu_root_seq())) {
 				printk_deferred_exit();
 				pr_err("INFO: Stall ended during stack backtracing.\n");
 				return;
@@ -474,7 +474,7 @@ static void print_cpu_stall_info(int cpu)
 	 */
 	touch_nmi_watchdog();
 
-	ticks_value = rcu_seq_ctr(rcu_state.gp_seq - rdp->gp_seq);
+	ticks_value = rcu_seq_ctr(*rcu_root_seq() - rdp->gp_seq);
 	if (ticks_value) {
 		ticks_title = "GPs behind";
 	} else {
@@ -518,7 +518,7 @@ static void rcu_check_gp_kthread_starvation(void)
 		cpu = gpk ? task_cpu(gpk) : -1;
 		pr_err("%s kthread starved for %ld jiffies! g%ld f%#x %s(%d) ->state=%#x ->cpu=%d\n",
 		       rcu_state.name, j,
-		       (long)rcu_seq_current(&rcu_state.gp_seq),
+		       (long)rcu_seq_current(rcu_root_seq()),
 		       data_race(READ_ONCE(rcu_state.gp_flags)),
 		       gp_state_getname(rcu_state.gp_state),
 		       data_race(READ_ONCE(rcu_state.gp_state)),
@@ -561,7 +561,7 @@ static void rcu_check_gp_kthread_expired_fqs_timer(void)
 		cpu = task_cpu(gpk);
 		pr_err("%s kthread timer wakeup didn't happen for %ld jiffies! g%ld f%#x %s(%d) ->state=%#x\n",
 		       rcu_state.name, (jiffies - jiffies_fqs),
-		       (long)rcu_seq_current(&rcu_state.gp_seq),
+		       (long)rcu_seq_current(rcu_root_seq()),
 		       data_race(READ_ONCE(rcu_state.gp_flags)), // Diagnostic read
 		       gp_state_getname(RCU_GP_WAIT_FQS), RCU_GP_WAIT_FQS,
 		       data_race(READ_ONCE(gpk->__state)));
@@ -613,7 +613,7 @@ static void print_other_cpu_stall(unsigned long gp_seq, unsigned long gps)
 		totqlen += rcu_get_n_cbs_cpu(cpu);
 	pr_err("\t(detected by %d, t=%ld jiffies, g=%ld, q=%lu ncpus=%d)\n",
 	       smp_processor_id(), (long)(jiffies - gps),
-	       (long)rcu_seq_current(&rcu_state.gp_seq), totqlen,
+	       (long)rcu_seq_current(rcu_root_seq()), totqlen,
 	       data_race(rcu_state.n_online_cpus)); // Diagnostic read
 	if (ndetected) {
 		rcu_dump_cpu_stacks(gp_seq);
@@ -622,7 +622,7 @@ static void print_other_cpu_stall(unsigned long gp_seq, unsigned long gps)
 		rcu_for_each_leaf_node(rnp)
 			rcu_print_detail_task_stall_rnp(rnp);
 	} else {
-		if (rcu_seq_current(&rcu_state.gp_seq) != gp_seq) {
+		if (rcu_seq_current(rcu_root_seq()) != gp_seq) {
 			pr_err("INFO: Stall ended before state dump start\n");
 		} else {
 			j = jiffies;
@@ -679,7 +679,7 @@ static void print_cpu_stall(unsigned long gp_seq, unsigned long gps)
 		totqlen += rcu_get_n_cbs_cpu(cpu);
 	pr_err("\t(t=%lu jiffies g=%ld q=%lu ncpus=%d)\n",
 		jiffies - gps,
-		(long)rcu_seq_current(&rcu_state.gp_seq), totqlen,
+		(long)rcu_seq_current(rcu_root_seq()), totqlen,
 		data_race(rcu_state.n_online_cpus)); // Diagnostic read
 
 	rcu_check_gp_kthread_expired_fqs_timer();
@@ -743,28 +743,28 @@ static void check_cpu_stall(struct rcu_data *rdp)
 	/*
 	 * Lots of memory barriers to reject false positives.
 	 *
-	 * The idea is to pick up rcu_state.gp_seq, then
+	 * The idea is to pick up rcu_root_seq(), then
 	 * rcu_state.jiffies_stall, then rcu_state.gp_start, and finally
-	 * another copy of rcu_state.gp_seq.  These values are updated in
+	 * another copy of rcu_root_seq().  These values are updated in
 	 * the opposite order with memory barriers (or equivalent) during
 	 * grace-period initialization and cleanup.  Now, a false positive
 	 * can occur if we get an new value of rcu_state.gp_start and a old
 	 * value of rcu_state.jiffies_stall.  But given the memory barriers,
 	 * the only way that this can happen is if one grace period ends
 	 * and another starts between these two fetches.  This is detected
-	 * by comparing the second fetch of rcu_state.gp_seq with the
-	 * previous fetch from rcu_state.gp_seq.
+	 * by comparing the second fetch of rcu_root_seq() with the
+	 * previous fetch from rcu_root_seq().
 	 *
 	 * Given this check, comparisons of jiffies, rcu_state.jiffies_stall,
 	 * and rcu_state.gp_start suffice to forestall false positives.
 	 */
-	gs1 = READ_ONCE(rcu_state.gp_seq);
+	gs1 = READ_ONCE(*rcu_root_seq());
 	smp_rmb(); /* Pick up ->gp_seq first... */
 	js = READ_ONCE(rcu_state.jiffies_stall);
 	smp_rmb(); /* ...then ->jiffies_stall before the rest... */
 	gps = READ_ONCE(rcu_state.gp_start);
 	smp_rmb(); /* ...and finally ->gp_start before ->gp_seq again. */
-	gs2 = READ_ONCE(rcu_state.gp_seq);
+	gs2 = READ_ONCE(*rcu_root_seq());
 	if (gs1 != gs2 ||
 	    ULONG_CMP_LT(j, js) ||
 	    ULONG_CMP_GE(gps, js) ||
@@ -893,12 +893,12 @@ void show_rcu_gp_kthreads(void)
 		data_race(READ_ONCE(rcu_state.gp_state)),
 		t ? data_race(READ_ONCE(t->__state)) : 0x1ffff, t ? t->rt_priority : 0xffU,
 		js, ja, jr, jw, (long)data_race(READ_ONCE(rcu_state.gp_wake_seq)),
-		(long)data_race(READ_ONCE(rcu_state.gp_seq)),
+		(long)data_race(READ_ONCE(*rcu_root_seq())),
 		(long)data_race(READ_ONCE(rcu_get_root()->gp_seq_needed)),
 		data_race(READ_ONCE(rcu_state.gp_max)),
 		data_race(READ_ONCE(rcu_state.gp_flags)));
 	rcu_for_each_node_breadth_first(rnp) {
-		if (ULONG_CMP_GE(READ_ONCE(rcu_state.gp_seq), READ_ONCE(rnp->gp_seq_needed)) &&
+		if (ULONG_CMP_GE(READ_ONCE(*rcu_root_seq()), READ_ONCE(rnp->gp_seq_needed)) &&
 		    !data_race(READ_ONCE(rnp->qsmask)) && !data_race(READ_ONCE(rnp->boost_tasks)) &&
 		    !data_race(READ_ONCE(rnp->exp_tasks)) && !data_race(READ_ONCE(rnp->gp_tasks)))
 			continue;
@@ -917,7 +917,7 @@ void show_rcu_gp_kthreads(void)
 		for_each_leaf_node_possible_cpu(rnp, cpu) {
 			rdp = per_cpu_ptr(&rcu_data, cpu);
 			if (READ_ONCE(rdp->gpwrap) ||
-			    ULONG_CMP_GE(READ_ONCE(rcu_state.gp_seq),
+			    ULONG_CMP_GE(READ_ONCE(*rcu_root_seq()),
 					 READ_ONCE(rdp->gp_seq_needed)))
 				continue;
 			pr_info("\tcpu %d ->gp_seq_needed %ld\n",

Re: [PATCH 1/3] rcu: Replace magic number with meaningful constant in rcu_seq_done_exact()

[Paul E. McKenney]

On Mon, Mar 31, 2025 at 12:10:58AM +0200, Frederic Weisbecker wrote:
> Le Thu, Mar 27, 2025 at 10:09:48AM -0700, Paul E. McKenney a écrit :
> > On Wed, Mar 26, 2025 at 10:42:52PM +0000, Joel Fernandes wrote:
> > > 
> > > 
> > > > On Mar 26, 2025, at 6:33 PM, Paul E. McKenney <paulmck@kernel.org> wrote:
> > > > 
> > > > On Mon, Mar 24, 2025 at 01:01:53PM -0400, Joel Fernandes wrote:
> > > >> The rcu_seq_done_exact() function checks if a grace period has completed by
> > > >> comparing sequence numbers. It includes a guard band to handle sequence number
> > > >> wraparound, which was previously expressed using the magic number calculation
> > > >> '3 * RCU_SEQ_STATE_MASK + 1'.
> > > >> 
> > > >> This magic number is not immediately obvious in terms of what it represents.
> > > >> 
> > > >> Instead, the reason we need this tiny guardband is because of the lag between
> > > >> the setting of rcu_state.gp_seq_polled and root rnp's gp_seq in rcu_gp_init().
> > > >> 
> > > >> This guardband needs to be at least 2 GPs worth of counts, to avoid recognizing
> > > >> the newly started GP as completed immediately, due to the following sequence
> > > >> which arises due to the delay between update of rcu_state.gp_seq_polled and
> > > >> root rnp's gp_seq:
> > > >> 
> > > >> rnp->gp_seq = rcu_state.gp_seq = 0
> > > >> 
> > > >>    CPU 0                                           CPU 1
> > > >>    -----                                           -----
> > > >>    // rcu_state.gp_seq = 1
> > > >>    rcu_seq_start(&rcu_state.gp_seq)
> > > >>                                                    // snap = 8
> > > >>                                                    snap = rcu_seq_snap(&rcu_state.gp_seq)
> > > >>                                                    // Two full GP differences
> > > >>                                                    rcu_seq_done_exact(&rnp->gp_seq, snap)
> > > >>    // rnp->gp_seq = 1
> > > >>    WRITE_ONCE(rnp->gp_seq, rcu_state.gp_seq);
> > > >> 
> > > >> This can happen due to get_state_synchronize_rcu_full() sampling
> > > >> rcu_state.gp_seq_polled, however the poll_state_synchronize_rcu_full()
> > > >> sampling the root rnp's gp_seq. The delay between the update of the 2
> > > >> counters occurs in rcu_gp_init() during which the counters briefly go
> > > >> out of sync.
> > > >> 
> > > >> Make the guardband explictly 2 GPs. This improves code readability and
> > > >> maintainability by making the intent clearer as well.
> > > >> 
> > > >> Suggested-by: Frederic Weisbecker <frederic@kernel.org>
> > > >> Signed-off-by: Joel Fernandes <joelagnelf@nvidia.com>
> > > > 
> > > > One concern is that a small error anywhere in the code could cause this
> > > > minimal guard band to be too small.  This is not a problem for some
> > > > use cases (rcu_barrier() just does an extra operation, and normal grace
> > > > periods are protected from forever-idle CPUs by ->gpwrap), but could be
> > > > an issue on 32-bit systems for user of polled RCU grace periods.
> > > 
> > > Could you provide more details of the usecase (sequence of steps) causing an issue for 32 bit polled RCU users? I am not able to see how this patch can affect them.
> > > 
> > > > 
> > > > In contrast, making the guard band a bit longer than it needs to be
> > > > has little or no downside.
> > > 
> > > Making it 3 GP instead of 2 should be ok with me as long as we document it but at least it will not be a magic number based on an equation. I feel we should not put random magic numbers which is more dangerous since it is hard to explain (and hence debug — just my 2 cents).
> > 
> > Apologies, I was getting ahead of us on this one.
> > 
> > That third (and maybe also a fourth) grace period becomes important if
> > we start getting memory contention on rcu_state.gp_seq, in which case
> > we would want the polled grace periods to look at the leaf rcu_node
> > structure's gp_seq, which would introduce extra slop.
> > 
> > For now, for this patch, assuming you guys promise to remember this
> > should such memory contention prove problematic:
> > 
> > Reviewed-by: Paul E. McKenney <paulmck@kernel.org>
> 
> Taking again a step back on this. This all happens because we have two different
> global gp sequence counter, and the two are used on different occasions, even mixed
> up together since 85aad7cc4178 ("rcu: Fix get_state_synchronize_rcu_full()
> GP-start detection") even though they have different constraints and
> reliability.
> 
> This can be error prone and confusing.

Fair enough, though given the need for the grace-period sequence number
to be replicated across the leaf rcu_node structures, we might well find
that we are facing a law of conservation of confusion...

Nevertheless, I do fully support at least exploring this sort of thing.
After all, once I found something that worked, my incentive to further
improve things was a bit limited.  Especially given that there was never
a shortage of other things demanding improvement.  ;-)

Please see more comments interspersed.

							Thanx, Paul

> So I'm unfortunately asking again if it wouldn't be a good idea to have a single
> global state counter that lives in the root node so that we don't have it
> duplicated in rcu_state.gp_seq.
> 
> This involves some special care:
> 
> 1) As you pointed out, the root can also be the unique leaf. So care must
>    be taken that no QS is reported before new online CPUs have been handled
>    and QS masks initialized. In order for __note_gp_changes() to handle
>    it gracefully we must have two seq states: one to tell the
>    grace period has started (before checking new online CPUs): RCU_STATE_STARTED
>    and one to tell that grace period is ready to receive quiescent states:
>    RCU_STATE_WAIT_QS. Although not strictly required, __note_gp_changes() can
>    ignore simple state changes to RCU_STATE_STARTED in order to spare useless
>    work and rnp locking. No QS will be reported before ->qsmask are initialized
>    in any case.

We have available up to three stages of grace-period processing, which
can be expanded relatively easily.  My concern is that we will be
replacing "which ->gp_seq to use" confusion with "which gp_seq state
to pay attention to" confusion.

> 2) rcu_gp_cleanup() currently ends the grace period top-down through the rnp tree
>    and only then on rcu_state. With this change it's simply top-down. I have audited
>    rcu_seq_snap() and rcu_start_this_gp() uses and I haven't found an obvious
>    issue with that.

That would result in the root rcu_node structure's ->gp_seq declaring the
grace period over before the rest of the rcu_node structures were aware
of it having ended.  I guess that the last leaf rcu_node structure's
->gp_seq field would be what you would look at if you were looking for
the actual grace period end.

Of course, you could argue that the grace period isn't fully over
until all the CPUs are aware of it, but that can take a very long time,
especially if there are CPUs staying idle for seconds (let alone minutes)
at at time.

> The draft below holds 30 minutes of TREE01:
> 
> diff --git a/kernel/rcu/rcu.h b/kernel/rcu/rcu.h
> index feb3ac1dc5d5..c4bb3eb92a58 100644
> --- a/kernel/rcu/rcu.h
> +++ b/kernel/rcu/rcu.h
> @@ -19,6 +19,12 @@
>   * The two least significant bits contain the control flags.
>   * The most significant bits contain the grace-period sequence counter.
>   *
> + * RCU_STATE_STARTED		:	The gp has started but QS are not
> + *					requested yet.
> + *
> + * RCU_STATE_WAIT_QS		:	The gp has started and waits for
> + *					QS to be reported.

Do we want RCU_STATE_IDLE, by analogy with SRCU?

>   * When both control flags are zero, no grace period is in progress.
>   * When either bit is non-zero, a grace period has started and is in
>   * progress. When the grace period completes, the control flags are reset
> @@ -54,6 +60,9 @@
>   *					grace-period sequence number.
>   */
>  
> +#define RCU_STATE_STARTED	0x1
> +#define RCU_STATE_WAIT_QS	0x2
> +
>  /* Low-order bit definition for polled grace-period APIs. */
>  #define RCU_GET_STATE_COMPLETED	0x1
>  
> @@ -92,7 +101,13 @@ static inline void rcu_seq_start(unsigned long *sp)
>  {
>  	WRITE_ONCE(*sp, *sp + 1);
>  	smp_mb(); /* Ensure update-side operation after counter increment. */
> -	WARN_ON_ONCE(rcu_seq_state(*sp) != 1);
> +	WARN_ON_ONCE(rcu_seq_state(*sp) != RCU_STATE_STARTED);
> +}
> +
> +static inline void rcu_seq_wait_qs(unsigned long *sp)
> +{
> +	WRITE_ONCE(*sp, *sp + 1);
> +	WARN_ON_ONCE(rcu_seq_state(*sp) != RCU_STATE_WAIT_QS);

Or we could have the WARN_ON_ONCE() first, and use rcu_seq_set_state().

>  }
>  
>  /* Compute the end-of-grace-period value for the specified sequence number. */
> @@ -135,6 +150,11 @@ static inline unsigned long rcu_seq_current(unsigned long *sp)
>  	return READ_ONCE(*sp);
>  }
>  
> +static inline bool rcu_seq_changed(unsigned long old, unsigned long new)
> +{
> +	return (old & ~RCU_STATE_STARTED) != (new & ~RCU_STATE_STARTED);
> +}
> +
>  /*
>   * Given a snapshot from rcu_seq_snap(), determine whether or not the
>   * corresponding update-side operation has started.
> diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> index 475f31deed14..54480ee9599f 100644
> --- a/kernel/rcu/tree.c
> +++ b/kernel/rcu/tree.c
> @@ -81,9 +81,9 @@ static DEFINE_PER_CPU_SHARED_ALIGNED(struct rcu_data, rcu_data) = {
>  	.gpwrap = true,
>  };
>  static struct rcu_state rcu_state = {
> +	.node[0].gp_seq = (0UL - 300UL) << RCU_SEQ_CTR_SHIFT,
>  	.level = { &rcu_state.node[0] },
>  	.gp_state = RCU_GP_IDLE,
> -	.gp_seq = (0UL - 300UL) << RCU_SEQ_CTR_SHIFT,
>  	.barrier_mutex = __MUTEX_INITIALIZER(rcu_state.barrier_mutex),
>  	.barrier_lock = __RAW_SPIN_LOCK_UNLOCKED(rcu_state.barrier_lock),
>  	.name = RCU_NAME,
> @@ -192,6 +192,21 @@ int rcu_get_gp_kthreads_prio(void)
>  }
>  EXPORT_SYMBOL_GPL(rcu_get_gp_kthreads_prio);
>  
> +/*
> + * Return the root node of the rcu_state structure.
> + */
> +static struct rcu_node *rcu_get_root(void)
> +{
> +	return &rcu_state.node[0];
> +}

OK, just code movement.

> +static unsigned long *rcu_root_seq(void)
> +{
> +	struct rcu_node *root = rcu_get_root();
> +
> +	return &root->gp_seq;
> +}

Adding this function might be useful in any case, even if we keep the
current rcu_state.gp_seq counter.  A single place to add comments that
address the chosen type of confusion, if nothing else.  ;-)

13 calls want the pointer and 13 want the value.  An even split!  Not sure
what lesson to draw from this, though indirecting a function call will
likely cause some confusion.  Some might argue for two access functions,
one for the pointer and the other for the value.  Me, I am just glad that
I included the word "might" in the first sentence of the prior paragraph.

>  /*
>   * Number of grace periods between delays, normalized by the duration of
>   * the delay.  The longer the delay, the more the grace periods between
> @@ -210,7 +225,7 @@ EXPORT_SYMBOL_GPL(rcu_get_gp_kthreads_prio);
>   */
>  static int rcu_gp_in_progress(void)
>  {
> -	return rcu_seq_state(rcu_seq_current(&rcu_state.gp_seq));
> +	return rcu_seq_state(rcu_seq_current(rcu_root_seq()));
>  }
>  
>  /*
> @@ -504,7 +519,7 @@ static int rcu_pending(int user);
>   */
>  unsigned long rcu_get_gp_seq(void)
>  {
> -	return READ_ONCE(rcu_state.gp_seq);
> +	return READ_ONCE(*rcu_root_seq());
>  }
>  EXPORT_SYMBOL_GPL(rcu_get_gp_seq);
>  
> @@ -520,21 +535,13 @@ unsigned long rcu_exp_batches_completed(void)
>  }
>  EXPORT_SYMBOL_GPL(rcu_exp_batches_completed);
>  
> -/*
> - * Return the root node of the rcu_state structure.
> - */
> -static struct rcu_node *rcu_get_root(void)
> -{
> -	return &rcu_state.node[0];
> -}
> -
>  /*
>   * Send along grace-period-related data for rcutorture diagnostics.
>   */
>  void rcutorture_get_gp_data(int *flags, unsigned long *gp_seq)
>  {
>  	*flags = READ_ONCE(rcu_state.gp_flags);
> -	*gp_seq = rcu_seq_current(&rcu_state.gp_seq);
> +	*gp_seq = rcu_seq_current(rcu_root_seq());
>  }
>  EXPORT_SYMBOL_GPL(rcutorture_get_gp_data);
>  
> @@ -1010,7 +1017,7 @@ static bool rcu_start_this_gp(struct rcu_node *rnp_start, struct rcu_data *rdp,
>  		trace_rcu_this_gp(rnp, rdp, gp_seq_req, TPS("NoGPkthread"));
>  		goto unlock_out;
>  	}
> -	trace_rcu_grace_period(rcu_state.name, data_race(rcu_state.gp_seq), TPS("newreq"));
> +	trace_rcu_grace_period(rcu_state.name, data_race(*rcu_root_seq()), TPS("newreq"));
>  	ret = true;  /* Caller must wake GP kthread. */
>  unlock_out:
>  	/* Push furthest requested GP to leaf node and rcu_data structure. */
> @@ -1095,7 +1102,7 @@ static void rcu_gp_kthread_wake(void)
>  	    !READ_ONCE(rcu_state.gp_flags) || !t)
>  		return;
>  	WRITE_ONCE(rcu_state.gp_wake_time, jiffies);
> -	WRITE_ONCE(rcu_state.gp_wake_seq, READ_ONCE(rcu_state.gp_seq));
> +	WRITE_ONCE(rcu_state.gp_wake_seq, READ_ONCE(*rcu_root_seq()));
>  	swake_up_one_online(&rcu_state.gp_wq);
>  }
>  
> @@ -1135,7 +1142,7 @@ static bool rcu_accelerate_cbs(struct rcu_node *rnp, struct rcu_data *rdp)
>  	 * accelerating callback invocation to an earlier grace-period
>  	 * number.
>  	 */
> -	gp_seq_req = rcu_seq_snap(&rcu_state.gp_seq);
> +	gp_seq_req = rcu_seq_snap(rcu_root_seq());
>  	if (rcu_segcblist_accelerate(&rdp->cblist, gp_seq_req))
>  		ret = rcu_start_this_gp(rnp, rdp, gp_seq_req);
>  
> @@ -1164,7 +1171,7 @@ static void rcu_accelerate_cbs_unlocked(struct rcu_node *rnp,
>  	bool needwake;
>  
>  	rcu_lockdep_assert_cblist_protected(rdp);
> -	c = rcu_seq_snap(&rcu_state.gp_seq);
> +	c = rcu_seq_snap(rcu_root_seq());
>  	if (!READ_ONCE(rdp->gpwrap) && ULONG_CMP_GE(rdp->gp_seq_needed, c)) {
>  		/* Old request still live, so mark recent callbacks. */
>  		(void)rcu_segcblist_accelerate(&rdp->cblist, c);
> @@ -1249,7 +1256,7 @@ static bool __note_gp_changes(struct rcu_node *rnp, struct rcu_data *rdp)
>  
>  	raw_lockdep_assert_held_rcu_node(rnp);
>  
> -	if (rdp->gp_seq == rnp->gp_seq)
> +	if (!rcu_seq_changed(rdp->gp_seq, rnp->gp_seq))

The old code would detect a bunch of elapsed grace periods, starting
and ending in the same state.  The new code does not.  Does the new code
correctly handle CPUs that go idle for many grace periods?

>  		return false; /* Nothing to do. */
>  
>  	/* Handle the ends of any preceding grace periods first. */
> @@ -1298,7 +1305,7 @@ static void note_gp_changes(struct rcu_data *rdp)
>  
>  	local_irq_save(flags);
>  	rnp = rdp->mynode;
> -	if ((rdp->gp_seq == rcu_seq_current(&rnp->gp_seq) &&
> +	if ((!rcu_seq_changed(rdp->gp_seq, rcu_seq_current(&rnp->gp_seq)) &&

Ditto.

>  	     !unlikely(READ_ONCE(rdp->gpwrap))) || /* w/out lock. */
>  	    !raw_spin_trylock_rcu_node(rnp)) { /* irqs already off, so later. */
>  		local_irq_restore(flags);
> @@ -1341,7 +1348,7 @@ static bool rcu_gp_slow_is_suppressed(void)
>  static void rcu_gp_slow(int delay)
>  {
>  	if (!rcu_gp_slow_is_suppressed() && delay > 0 &&
> -	    !(rcu_seq_ctr(rcu_state.gp_seq) % (rcu_num_nodes * PER_RCU_NODE_PERIOD * delay)))
> +	    !(rcu_seq_ctr(*rcu_root_seq()) % (rcu_num_nodes * PER_RCU_NODE_PERIOD * delay)))
>  		schedule_timeout_idle(delay);
>  }
>  
> @@ -1802,10 +1809,9 @@ static noinline_for_stack bool rcu_gp_init(void)
>  	/* Advance to a new grace period and initialize state. */
>  	record_gp_stall_check_time();
>  	/* Record GP times before starting GP, hence rcu_seq_start(). */
> -	rcu_seq_start(&rcu_state.gp_seq);
> -	ASSERT_EXCLUSIVE_WRITER(rcu_state.gp_seq);

Why are we dropping the ASSERT_EXCLUSIVE_WRITER()?  As opposed to moving
it to the setting of the rcu_node structures' ->gp_seq updates?

> +	rcu_seq_start(rcu_root_seq());
>  	start_new_poll = rcu_sr_normal_gp_init();
> -	trace_rcu_grace_period(rcu_state.name, rcu_state.gp_seq, TPS("start"));
> +	trace_rcu_grace_period(rcu_state.name, *rcu_root_seq(), TPS("start"));
>  	rcu_poll_gp_seq_start(&rcu_state.gp_seq_polled_snap);
>  	raw_spin_unlock_irq_rcu_node(rnp);
>  
> @@ -1902,7 +1908,10 @@ static noinline_for_stack bool rcu_gp_init(void)
>  		rdp = this_cpu_ptr(&rcu_data);
>  		rcu_preempt_check_blocked_tasks(rnp);
>  		rnp->qsmask = rnp->qsmaskinit;
> -		WRITE_ONCE(rnp->gp_seq, rcu_state.gp_seq);
> +		if (!rnp->parent)
> +			rcu_seq_wait_qs(&rnp->gp_seq);
> +		else
> +			WRITE_ONCE(rnp->gp_seq, *rcu_root_seq());

This asymmetry is necessary because if I understand correctly, the
non-root rcu_node structures still have zero state.  But this is one
argument for use of rcu_seq_set_state(), because then you could just do
rcu_seq_set_state(rnp->gp_seq, RCU_STATE_WAIT_QS) in both cases.

Or am I missing something that prevents this from working?

>  		if (rnp == rdp->mynode)
>  			(void)__note_gp_changes(rnp, rdp);
>  		rcu_preempt_boost_start_gp(rnp);
> @@ -2018,7 +2027,7 @@ static noinline_for_stack void rcu_gp_fqs_loop(void)
>  			WRITE_ONCE(rcu_state.jiffies_kick_kthreads,
>  				   jiffies + (j ? 3 * j : 2));
>  		}
> -		trace_rcu_grace_period(rcu_state.name, rcu_state.gp_seq,
> +		trace_rcu_grace_period(rcu_state.name, *rcu_root_seq(),
>  				       TPS("fqswait"));
>  		WRITE_ONCE(rcu_state.gp_state, RCU_GP_WAIT_FQS);
>  		(void)swait_event_idle_timeout_exclusive(rcu_state.gp_wq,
> @@ -2041,7 +2050,7 @@ static noinline_for_stack void rcu_gp_fqs_loop(void)
>  		/* If time for quiescent-state forcing, do it. */
>  		if (!time_after(rcu_state.jiffies_force_qs, jiffies) ||
>  		    (gf & (RCU_GP_FLAG_FQS | RCU_GP_FLAG_OVLD))) {
> -			trace_rcu_grace_period(rcu_state.name, rcu_state.gp_seq,
> +			trace_rcu_grace_period(rcu_state.name, *rcu_root_seq(),
>  					       TPS("fqsstart"));
>  			rcu_gp_fqs(first_gp_fqs);
>  			gf = 0;
> @@ -2049,7 +2058,7 @@ static noinline_for_stack void rcu_gp_fqs_loop(void)
>  				first_gp_fqs = false;
>  				gf = rcu_state.cbovld ? RCU_GP_FLAG_OVLD : 0;
>  			}
> -			trace_rcu_grace_period(rcu_state.name, rcu_state.gp_seq,
> +			trace_rcu_grace_period(rcu_state.name, *rcu_root_seq(),
>  					       TPS("fqsend"));
>  			cond_resched_tasks_rcu_qs();
>  			WRITE_ONCE(rcu_state.gp_activity, jiffies);
> @@ -2060,7 +2069,7 @@ static noinline_for_stack void rcu_gp_fqs_loop(void)
>  			cond_resched_tasks_rcu_qs();
>  			WRITE_ONCE(rcu_state.gp_activity, jiffies);
>  			WARN_ON(signal_pending(current));
> -			trace_rcu_grace_period(rcu_state.name, rcu_state.gp_seq,
> +			trace_rcu_grace_period(rcu_state.name, *rcu_root_seq(),
>  					       TPS("fqswaitsig"));
>  			ret = 1; /* Keep old FQS timing. */
>  			j = jiffies;
> @@ -2081,7 +2090,6 @@ static noinline void rcu_gp_cleanup(void)
>  	int cpu;
>  	bool needgp = false;
>  	unsigned long gp_duration;
> -	unsigned long new_gp_seq;
>  	bool offloaded;
>  	struct rcu_data *rdp;
>  	struct rcu_node *rnp = rcu_get_root();
> @@ -2103,6 +2111,8 @@ static noinline void rcu_gp_cleanup(void)
>  	 * period as completed in all of the rcu_node structures.
>  	 */
>  	rcu_poll_gp_seq_end(&rcu_state.gp_seq_polled_snap);
> +	rcu_seq_end(rcu_root_seq());
> +	smp_mb(); // Order against failing poll_state_synchronize_rcu_full().
>  	raw_spin_unlock_irq_rcu_node(rnp);
>  
>  	/*
> @@ -2114,16 +2124,12 @@ static noinline void rcu_gp_cleanup(void)
>  	 * the rcu_node structures before the beginning of the next grace
>  	 * period is recorded in any of the rcu_node structures.
>  	 */
> -	new_gp_seq = rcu_state.gp_seq;
> -	rcu_seq_end(&new_gp_seq);
>  	rcu_for_each_node_breadth_first(rnp) {
>  		raw_spin_lock_irq_rcu_node(rnp);
>  		if (WARN_ON_ONCE(rcu_preempt_blocked_readers_cgp(rnp)))
>  			dump_blkd_tasks(rnp, 10);
>  		WARN_ON_ONCE(rnp->qsmask);
> -		WRITE_ONCE(rnp->gp_seq, new_gp_seq);
> -		if (!rnp->parent)
> -			smp_mb(); // Order against failing poll_state_synchronize_rcu_full().

OK, this got moved up.

> +		WRITE_ONCE(rnp->gp_seq, *rcu_root_seq());

The idea being that the root rcu_node ->gp_seq is just overwritten
with itself, correct?

>  		rdp = this_cpu_ptr(&rcu_data);
>  		if (rnp == rdp->mynode)
>  			needgp = __note_gp_changes(rnp, rdp) || needgp;
> @@ -2146,9 +2152,7 @@ static noinline void rcu_gp_cleanup(void)
>  	raw_spin_lock_irq_rcu_node(rnp); /* GP before ->gp_seq update. */
>  
>  	/* Declare grace period done, trace first to use old GP number. */
> -	trace_rcu_grace_period(rcu_state.name, rcu_state.gp_seq, TPS("end"));
> -	rcu_seq_end(&rcu_state.gp_seq);
> -	ASSERT_EXCLUSIVE_WRITER(rcu_state.gp_seq);

Same question on not moving ASSERT_EXCLUSIVE_WRITER() to the rcu_node
structures' ->gp_seq updates.

> +	trace_rcu_grace_period(rcu_state.name, *rcu_root_seq(), TPS("end"));
>  	WRITE_ONCE(rcu_state.gp_state, RCU_GP_IDLE);
>  	/* Check for GP requests since above loop. */
>  	rdp = this_cpu_ptr(&rcu_data);
> @@ -2173,7 +2177,7 @@ static noinline void rcu_gp_cleanup(void)
>  
>  		WRITE_ONCE(rcu_state.gp_flags, RCU_GP_FLAG_INIT);
>  		WRITE_ONCE(rcu_state.gp_req_activity, jiffies);
> -		trace_rcu_grace_period(rcu_state.name, rcu_state.gp_seq, TPS("newreq"));
> +		trace_rcu_grace_period(rcu_state.name, *rcu_root_seq(), TPS("newreq"));
>  	} else {
>  
>  		// We get here either if there is no need for an
> @@ -2204,7 +2208,7 @@ static int __noreturn rcu_gp_kthread(void *unused)
>  
>  		/* Handle grace-period start. */
>  		for (;;) {
> -			trace_rcu_grace_period(rcu_state.name, rcu_state.gp_seq,
> +			trace_rcu_grace_period(rcu_state.name, *rcu_root_seq(),
>  					       TPS("reqwait"));
>  			WRITE_ONCE(rcu_state.gp_state, RCU_GP_WAIT_GPS);
>  			swait_event_idle_exclusive(rcu_state.gp_wq,
> @@ -2218,7 +2222,7 @@ static int __noreturn rcu_gp_kthread(void *unused)
>  			cond_resched_tasks_rcu_qs();
>  			WRITE_ONCE(rcu_state.gp_activity, jiffies);
>  			WARN_ON(signal_pending(current));
> -			trace_rcu_grace_period(rcu_state.name, rcu_state.gp_seq,
> +			trace_rcu_grace_period(rcu_state.name, *rcu_root_seq(),
>  					       TPS("reqwaitsig"));
>  		}
>  
> @@ -3299,9 +3303,10 @@ void synchronize_rcu(void)
>  	// these counters.
>  	local_irq_save(flags);
>  	WARN_ON_ONCE(num_online_cpus() > 1);
> -	rcu_state.gp_seq += (1 << RCU_SEQ_CTR_SHIFT);
> +	rnp = rcu_get_root();
> +	rnp->gp_seq += (1 << RCU_SEQ_CTR_SHIFT);
>  	for (rnp = this_cpu_ptr(&rcu_data)->mynode; rnp; rnp = rnp->parent)
> -		rnp->gp_seq_needed = rnp->gp_seq = rcu_state.gp_seq;
> +		rnp->gp_seq_needed = rnp->gp_seq = *rcu_root_seq();
>  	local_irq_restore(flags);
>  }
>  EXPORT_SYMBOL_GPL(synchronize_rcu);
> @@ -3390,7 +3395,7 @@ static void start_poll_synchronize_rcu_common(void)
>  	// get a grace period that no one needed.  These accesses are ordered
>  	// by smp_mb(), and we are accessing them in the opposite order
>  	// from which they are updated at grace-period start, as required.
> -	needwake = rcu_start_this_gp(rnp, rdp, rcu_seq_snap(&rcu_state.gp_seq));
> +	needwake = rcu_start_this_gp(rnp, rdp, rcu_seq_snap(rcu_root_seq()));
>  	raw_spin_unlock_irqrestore_rcu_node(rnp, flags);
>  	if (needwake)
>  		rcu_gp_kthread_wake();
> @@ -3512,11 +3517,9 @@ EXPORT_SYMBOL_GPL(poll_state_synchronize_rcu);
>   */
>  bool poll_state_synchronize_rcu_full(struct rcu_gp_oldstate *rgosp)
>  {
> -	struct rcu_node *rnp = rcu_get_root();
> -
>  	smp_mb(); // Order against root rcu_node structure grace-period cleanup.
>  	if (rgosp->rgos_norm == RCU_GET_STATE_COMPLETED ||
> -	    rcu_seq_done_exact(&rnp->gp_seq, rgosp->rgos_norm) ||
> +	    rcu_seq_done_exact(rcu_root_seq(), rgosp->rgos_norm) ||
>  	    rgosp->rgos_exp == RCU_GET_STATE_COMPLETED ||
>  	    rcu_seq_done_exact(&rcu_state.expedited_sequence, rgosp->rgos_exp)) {
>  		smp_mb(); /* Ensure GP ends before subsequent accesses. */
> @@ -4062,9 +4065,9 @@ rcu_boot_init_percpu_data(int cpu)
>  	WARN_ON_ONCE(ct->nesting != 1);
>  	WARN_ON_ONCE(rcu_watching_snap_in_eqs(ct_rcu_watching_cpu(cpu)));
>  	rdp->barrier_seq_snap = rcu_state.barrier_sequence;
> -	rdp->rcu_ofl_gp_seq = rcu_state.gp_seq;
> +	rdp->rcu_ofl_gp_seq = *rcu_root_seq();
>  	rdp->rcu_ofl_gp_state = RCU_GP_CLEANED;
> -	rdp->rcu_onl_gp_seq = rcu_state.gp_seq;
> +	rdp->rcu_onl_gp_seq = *rcu_root_seq();
>  	rdp->rcu_onl_gp_state = RCU_GP_CLEANED;
>  	rdp->last_sched_clock = jiffies;
>  	rdp->cpu = cpu;
> @@ -4271,7 +4274,7 @@ void rcutree_report_cpu_starting(unsigned int cpu)
>  	smp_store_release(&rcu_state.ncpus, rcu_state.ncpus + newcpu); /* ^^^ */
>  	ASSERT_EXCLUSIVE_WRITER(rcu_state.ncpus);
>  	rcu_gpnum_ovf(rnp, rdp); /* Offline-induced counter wrap? */
> -	rdp->rcu_onl_gp_seq = READ_ONCE(rcu_state.gp_seq);
> +	rdp->rcu_onl_gp_seq = READ_ONCE(*rcu_root_seq());
>  	rdp->rcu_onl_gp_state = READ_ONCE(rcu_state.gp_state);
>  
>  	/* An incoming CPU should never be blocking a grace period. */
> @@ -4322,7 +4325,7 @@ void rcutree_report_cpu_dead(void)
>  	mask = rdp->grpmask;
>  	arch_spin_lock(&rcu_state.ofl_lock);
>  	raw_spin_lock_irqsave_rcu_node(rnp, flags); /* Enforce GP memory-order guarantee. */
> -	rdp->rcu_ofl_gp_seq = READ_ONCE(rcu_state.gp_seq);
> +	rdp->rcu_ofl_gp_seq = READ_ONCE(*rcu_root_seq());
>  	rdp->rcu_ofl_gp_state = READ_ONCE(rcu_state.gp_state);
>  	if (rnp->qsmask & mask) { /* RCU waiting on outgoing CPU? */
>  		/* Report quiescent state -before- changing ->qsmaskinitnext! */
> @@ -4534,7 +4537,7 @@ void rcu_scheduler_starting(void)
>  	// Fix up the ->gp_seq counters.
>  	local_irq_save(flags);
>  	rcu_for_each_node_breadth_first(rnp)
> -		rnp->gp_seq_needed = rnp->gp_seq = rcu_state.gp_seq;
> +		rnp->gp_seq_needed = rnp->gp_seq = *rcu_root_seq();
>  	local_irq_restore(flags);
>  
>  	// Switch out of early boot mode.
> @@ -4583,9 +4586,9 @@ static void __init rcu_init_one(void)
>  			raw_spin_lock_init(&rnp->fqslock);
>  			lockdep_set_class_and_name(&rnp->fqslock,
>  						   &rcu_fqs_class[i], fqs[i]);
> -			rnp->gp_seq = rcu_state.gp_seq;
> -			rnp->gp_seq_needed = rcu_state.gp_seq;
> -			rnp->completedqs = rcu_state.gp_seq;
> +			rnp->gp_seq = *rcu_root_seq();
> +			rnp->gp_seq_needed = *rcu_root_seq();
> +			rnp->completedqs = *rcu_root_seq();
>  			rnp->qsmask = 0;
>  			rnp->qsmaskinit = 0;
>  			rnp->grplo = j * cpustride;
> diff --git a/kernel/rcu/tree.h b/kernel/rcu/tree.h
> index a9a811d9d7a3..9d811270b808 100644
> --- a/kernel/rcu/tree.h
> +++ b/kernel/rcu/tree.h
> @@ -347,7 +347,6 @@ struct rcu_state {
>  
>  	/* The following fields are guarded by the root rcu_node's lock. */
>  
> -	unsigned long gp_seq ____cacheline_internodealigned_in_smp;
>  						/* Grace-period sequence #. */

Heh!  Does the rcu_node structure's ->gp_seq need alignment?

>  	unsigned long gp_max;			/* Maximum GP duration in */
>  						/*  jiffies. */
> diff --git a/kernel/rcu/tree_stall.h b/kernel/rcu/tree_stall.h
> index 925fcdad5dea..367adb062a6d 100644
> --- a/kernel/rcu/tree_stall.h
> +++ b/kernel/rcu/tree_stall.h
> @@ -344,7 +344,7 @@ static void rcu_dump_cpu_stacks(unsigned long gp_seq)
>  	rcu_for_each_leaf_node(rnp) {
>  		printk_deferred_enter();
>  		for_each_leaf_node_possible_cpu(rnp, cpu) {
> -			if (gp_seq != data_race(rcu_state.gp_seq)) {
> +			if (gp_seq != data_race(*rcu_root_seq())) {
>  				printk_deferred_exit();
>  				pr_err("INFO: Stall ended during stack backtracing.\n");
>  				return;
> @@ -474,7 +474,7 @@ static void print_cpu_stall_info(int cpu)
>  	 */
>  	touch_nmi_watchdog();
>  
> -	ticks_value = rcu_seq_ctr(rcu_state.gp_seq - rdp->gp_seq);
> +	ticks_value = rcu_seq_ctr(*rcu_root_seq() - rdp->gp_seq);
>  	if (ticks_value) {
>  		ticks_title = "GPs behind";
>  	} else {
> @@ -518,7 +518,7 @@ static void rcu_check_gp_kthread_starvation(void)
>  		cpu = gpk ? task_cpu(gpk) : -1;
>  		pr_err("%s kthread starved for %ld jiffies! g%ld f%#x %s(%d) ->state=%#x ->cpu=%d\n",
>  		       rcu_state.name, j,
> -		       (long)rcu_seq_current(&rcu_state.gp_seq),
> +		       (long)rcu_seq_current(rcu_root_seq()),
>  		       data_race(READ_ONCE(rcu_state.gp_flags)),
>  		       gp_state_getname(rcu_state.gp_state),
>  		       data_race(READ_ONCE(rcu_state.gp_state)),
> @@ -561,7 +561,7 @@ static void rcu_check_gp_kthread_expired_fqs_timer(void)
>  		cpu = task_cpu(gpk);
>  		pr_err("%s kthread timer wakeup didn't happen for %ld jiffies! g%ld f%#x %s(%d) ->state=%#x\n",
>  		       rcu_state.name, (jiffies - jiffies_fqs),
> -		       (long)rcu_seq_current(&rcu_state.gp_seq),
> +		       (long)rcu_seq_current(rcu_root_seq()),
>  		       data_race(READ_ONCE(rcu_state.gp_flags)), // Diagnostic read
>  		       gp_state_getname(RCU_GP_WAIT_FQS), RCU_GP_WAIT_FQS,
>  		       data_race(READ_ONCE(gpk->__state)));
> @@ -613,7 +613,7 @@ static void print_other_cpu_stall(unsigned long gp_seq, unsigned long gps)
>  		totqlen += rcu_get_n_cbs_cpu(cpu);
>  	pr_err("\t(detected by %d, t=%ld jiffies, g=%ld, q=%lu ncpus=%d)\n",
>  	       smp_processor_id(), (long)(jiffies - gps),
> -	       (long)rcu_seq_current(&rcu_state.gp_seq), totqlen,
> +	       (long)rcu_seq_current(rcu_root_seq()), totqlen,
>  	       data_race(rcu_state.n_online_cpus)); // Diagnostic read
>  	if (ndetected) {
>  		rcu_dump_cpu_stacks(gp_seq);
> @@ -622,7 +622,7 @@ static void print_other_cpu_stall(unsigned long gp_seq, unsigned long gps)
>  		rcu_for_each_leaf_node(rnp)
>  			rcu_print_detail_task_stall_rnp(rnp);
>  	} else {
> -		if (rcu_seq_current(&rcu_state.gp_seq) != gp_seq) {
> +		if (rcu_seq_current(rcu_root_seq()) != gp_seq) {
>  			pr_err("INFO: Stall ended before state dump start\n");
>  		} else {
>  			j = jiffies;
> @@ -679,7 +679,7 @@ static void print_cpu_stall(unsigned long gp_seq, unsigned long gps)
>  		totqlen += rcu_get_n_cbs_cpu(cpu);
>  	pr_err("\t(t=%lu jiffies g=%ld q=%lu ncpus=%d)\n",
>  		jiffies - gps,
> -		(long)rcu_seq_current(&rcu_state.gp_seq), totqlen,
> +		(long)rcu_seq_current(rcu_root_seq()), totqlen,
>  		data_race(rcu_state.n_online_cpus)); // Diagnostic read
>  
>  	rcu_check_gp_kthread_expired_fqs_timer();
> @@ -743,28 +743,28 @@ static void check_cpu_stall(struct rcu_data *rdp)
>  	/*
>  	 * Lots of memory barriers to reject false positives.
>  	 *
> -	 * The idea is to pick up rcu_state.gp_seq, then
> +	 * The idea is to pick up rcu_root_seq(), then
>  	 * rcu_state.jiffies_stall, then rcu_state.gp_start, and finally
> -	 * another copy of rcu_state.gp_seq.  These values are updated in
> +	 * another copy of rcu_root_seq().  These values are updated in
>  	 * the opposite order with memory barriers (or equivalent) during
>  	 * grace-period initialization and cleanup.  Now, a false positive
>  	 * can occur if we get an new value of rcu_state.gp_start and a old
>  	 * value of rcu_state.jiffies_stall.  But given the memory barriers,
>  	 * the only way that this can happen is if one grace period ends
>  	 * and another starts between these two fetches.  This is detected
> -	 * by comparing the second fetch of rcu_state.gp_seq with the
> -	 * previous fetch from rcu_state.gp_seq.
> +	 * by comparing the second fetch of rcu_root_seq() with the
> +	 * previous fetch from rcu_root_seq().
>  	 *
>  	 * Given this check, comparisons of jiffies, rcu_state.jiffies_stall,
>  	 * and rcu_state.gp_start suffice to forestall false positives.
>  	 */
> -	gs1 = READ_ONCE(rcu_state.gp_seq);
> +	gs1 = READ_ONCE(*rcu_root_seq());
>  	smp_rmb(); /* Pick up ->gp_seq first... */
>  	js = READ_ONCE(rcu_state.jiffies_stall);
>  	smp_rmb(); /* ...then ->jiffies_stall before the rest... */
>  	gps = READ_ONCE(rcu_state.gp_start);
>  	smp_rmb(); /* ...and finally ->gp_start before ->gp_seq again. */
> -	gs2 = READ_ONCE(rcu_state.gp_seq);
> +	gs2 = READ_ONCE(*rcu_root_seq());
>  	if (gs1 != gs2 ||
>  	    ULONG_CMP_LT(j, js) ||
>  	    ULONG_CMP_GE(gps, js) ||
> @@ -893,12 +893,12 @@ void show_rcu_gp_kthreads(void)
>  		data_race(READ_ONCE(rcu_state.gp_state)),
>  		t ? data_race(READ_ONCE(t->__state)) : 0x1ffff, t ? t->rt_priority : 0xffU,
>  		js, ja, jr, jw, (long)data_race(READ_ONCE(rcu_state.gp_wake_seq)),
> -		(long)data_race(READ_ONCE(rcu_state.gp_seq)),
> +		(long)data_race(READ_ONCE(*rcu_root_seq())),
>  		(long)data_race(READ_ONCE(rcu_get_root()->gp_seq_needed)),
>  		data_race(READ_ONCE(rcu_state.gp_max)),
>  		data_race(READ_ONCE(rcu_state.gp_flags)));
>  	rcu_for_each_node_breadth_first(rnp) {
> -		if (ULONG_CMP_GE(READ_ONCE(rcu_state.gp_seq), READ_ONCE(rnp->gp_seq_needed)) &&
> +		if (ULONG_CMP_GE(READ_ONCE(*rcu_root_seq()), READ_ONCE(rnp->gp_seq_needed)) &&
>  		    !data_race(READ_ONCE(rnp->qsmask)) && !data_race(READ_ONCE(rnp->boost_tasks)) &&
>  		    !data_race(READ_ONCE(rnp->exp_tasks)) && !data_race(READ_ONCE(rnp->gp_tasks)))
>  			continue;
> @@ -917,7 +917,7 @@ void show_rcu_gp_kthreads(void)
>  		for_each_leaf_node_possible_cpu(rnp, cpu) {
>  			rdp = per_cpu_ptr(&rcu_data, cpu);
>  			if (READ_ONCE(rdp->gpwrap) ||
> -			    ULONG_CMP_GE(READ_ONCE(rcu_state.gp_seq),
> +			    ULONG_CMP_GE(READ_ONCE(*rcu_root_seq()),
>  					 READ_ONCE(rdp->gp_seq_needed)))
>  				continue;
>  			pr_info("\tcpu %d ->gp_seq_needed %ld\n",

Re: [PATCH 1/3] rcu: Replace magic number with meaningful constant in rcu_seq_done_exact()

[Frederic Weisbecker]

Le Mon, Mar 31, 2025 at 11:28:47AM -0700, Paul E. McKenney a écrit :
> > So I'm unfortunately asking again if it wouldn't be a good idea to have a single
> > global state counter that lives in the root node so that we don't have it
> > duplicated in rcu_state.gp_seq.
> > 
> > This involves some special care:
> > 
> > 1) As you pointed out, the root can also be the unique leaf. So care must
> >    be taken that no QS is reported before new online CPUs have been handled
> >    and QS masks initialized. In order for __note_gp_changes() to handle
> >    it gracefully we must have two seq states: one to tell the
> >    grace period has started (before checking new online CPUs): RCU_STATE_STARTED
> >    and one to tell that grace period is ready to receive quiescent states:
> >    RCU_STATE_WAIT_QS. Although not strictly required, __note_gp_changes() can
> >    ignore simple state changes to RCU_STATE_STARTED in order to spare useless
> >    work and rnp locking. No QS will be reported before ->qsmask are initialized
> >    in any case.
> 
> We have available up to three stages of grace-period processing, which
> can be expanded relatively easily.  My concern is that we will be
> replacing "which ->gp_seq to use" confusion with "which gp_seq state
> to pay attention to" confusion.

True, this comes at the cost of a bit more complex state space.

> 
> > 2) rcu_gp_cleanup() currently ends the grace period top-down through the rnp tree
> >    and only then on rcu_state. With this change it's simply top-down. I have audited
> >    rcu_seq_snap() and rcu_start_this_gp() uses and I haven't found an obvious
> >    issue with that.
> 
> That would result in the root rcu_node structure's ->gp_seq declaring the
> grace period over before the rest of the rcu_node structures were aware
> of it having ended.  I guess that the last leaf rcu_node structure's
> ->gp_seq field would be what you would look at if you were looking for
> the actual grace period end.
> 
> Of course, you could argue that the grace period isn't fully over
> until all the CPUs are aware of it, but that can take a very long time,
> especially if there are CPUs staying idle for seconds (let alone minutes)
> at at time.

Right. And it seems not all parts of RCU agree on a common definition of
a complete grace period. Polling seem to be fine with just the root rnp
to reflect rcu_seq_end() (which happens before the leaves do). Snapshot takers
though rely on the whole tree to reflect the GP end...

As far as I audited the code, mostly rcu_seq_snap() callers, rcu_start_this_gp()
and __note_gp_changes(), it looks fine to consider the GP is ended as soon
as the root reflects it. I may have missed something of course...

> > diff --git a/kernel/rcu/rcu.h b/kernel/rcu/rcu.h
> > index feb3ac1dc5d5..c4bb3eb92a58 100644
> > --- a/kernel/rcu/rcu.h
> > +++ b/kernel/rcu/rcu.h
> > @@ -19,6 +19,12 @@
> >   * The two least significant bits contain the control flags.
> >   * The most significant bits contain the grace-period sequence counter.
> >   *
> > + * RCU_STATE_STARTED		:	The gp has started but QS are not
> > + *					requested yet.
> > + *
> > + * RCU_STATE_WAIT_QS		:	The gp has started and waits for
> > + *					QS to be reported.
> 
> Do we want RCU_STATE_IDLE, by analogy with SRCU?

Makes sense.

> 
> >   * When both control flags are zero, no grace period is in progress.
> >   * When either bit is non-zero, a grace period has started and is in
> >   * progress. When the grace period completes, the control flags are reset
> > @@ -54,6 +60,9 @@
> >   *					grace-period sequence number.
> >   */
> >  
> > +#define RCU_STATE_STARTED	0x1
> > +#define RCU_STATE_WAIT_QS	0x2
> > +
> >  /* Low-order bit definition for polled grace-period APIs. */
> >  #define RCU_GET_STATE_COMPLETED	0x1
> >  
> > @@ -92,7 +101,13 @@ static inline void rcu_seq_start(unsigned long *sp)
> >  {
> >  	WRITE_ONCE(*sp, *sp + 1);
> >  	smp_mb(); /* Ensure update-side operation after counter increment. */
> > -	WARN_ON_ONCE(rcu_seq_state(*sp) != 1);
> > +	WARN_ON_ONCE(rcu_seq_state(*sp) != RCU_STATE_STARTED);
> > +}
> > +
> > +static inline void rcu_seq_wait_qs(unsigned long *sp)
> > +{
> > +	WRITE_ONCE(*sp, *sp + 1);
> > +	WARN_ON_ONCE(rcu_seq_state(*sp) != RCU_STATE_WAIT_QS);
> 
> Or we could have the WARN_ON_ONCE() first, and use rcu_seq_set_state().

Right!

> > @@ -192,6 +192,21 @@ int rcu_get_gp_kthreads_prio(void)
> >  }
> >  EXPORT_SYMBOL_GPL(rcu_get_gp_kthreads_prio);
> >  
> > +/*
> > + * Return the root node of the rcu_state structure.
> > + */
> > +static struct rcu_node *rcu_get_root(void)
> > +{
> > +	return &rcu_state.node[0];
> > +}
> 
> OK, just code movement.

Right, if I proceed with this, a series will (try to) split more gently
for review.

> 
> > +static unsigned long *rcu_root_seq(void)
> > +{
> > +	struct rcu_node *root = rcu_get_root();
> > +
> > +	return &root->gp_seq;
> > +}
> 
> Adding this function might be useful in any case, even if we keep the
> current rcu_state.gp_seq counter.  A single place to add comments that
> address the chosen type of confusion, if nothing else.  ;-)

;-))

> 
> 13 calls want the pointer and 13 want the value.  An even split!

The split is but 13 itself is not even! And I know people who strongly
want it to stay that way!

> Not sure what lesson to draw from this, though indirecting a function call will
> likely cause some confusion.  Some might argue for two access functions,
> one for the pointer and the other for the value.  Me, I am just glad that
> I included the word "might" in the first sentence of the prior paragraph.

Right, I'm not proud of the *rcu_root_seq() things.
I thought about adding rcu_root_seq_current() but I'm not entirely comfortable
with having all readers under READ_ONCE(). I like when our assumptions of non
concurrent plain accesses are defeatedly reported by KCSAN.

> > @@ -1249,7 +1256,7 @@ static bool __note_gp_changes(struct rcu_node *rnp, struct rcu_data *rdp)
> >  
> >  	raw_lockdep_assert_held_rcu_node(rnp);
> >  
> > -	if (rdp->gp_seq == rnp->gp_seq)
> > +	if (!rcu_seq_changed(rdp->gp_seq, rnp->gp_seq))
> 
> The old code would detect a bunch of elapsed grace periods, starting
> and ending in the same state.  The new code does not.  Does the new code
> correctly handle CPUs that go idle for many grace periods?

I'm not sure I understand what you mean. rcu_seq_changed() is a comparison
that simply clears RCU_SEQ_STARTED on both sides such that:

* Different GP numbers with or without RCU_SEQ_STARTED/RCU_SEQ_WAIT_QS
  are always considered a change.

* Same GP numbers but rnp has RCU_SEQ_WAIT_QS while rdp hasn't is considered a
  change

but:

* Same GP numbers but rnp has RCU_SEQ_STARTED while rdp hasn't is _NOT_
  considered a change. This is not for correctness but rather to spare
  rnp locking and useless work. The ->qsmask are 0 anyway so no QS can
  be reported too early.

> > @@ -1802,10 +1809,9 @@ static noinline_for_stack bool rcu_gp_init(void)
> >  	/* Advance to a new grace period and initialize state. */
> >  	record_gp_stall_check_time();
> >  	/* Record GP times before starting GP, hence rcu_seq_start(). */
> > -	rcu_seq_start(&rcu_state.gp_seq);
> > -	ASSERT_EXCLUSIVE_WRITER(rcu_state.gp_seq);
> 
> Why are we dropping the ASSERT_EXCLUSIVE_WRITER()?  As opposed to moving
> it to the setting of the rcu_node structures' ->gp_seq updates?

I must confess I misunderstood that macro and thought it was to assert
the variable has no other writer _at all_. But that seem to apply
scopewise. So yes good point.

> > @@ -1902,7 +1908,10 @@ static noinline_for_stack bool rcu_gp_init(void)
> >  		rdp = this_cpu_ptr(&rcu_data);
> >  		rcu_preempt_check_blocked_tasks(rnp);
> >  		rnp->qsmask = rnp->qsmaskinit;
> > -		WRITE_ONCE(rnp->gp_seq, rcu_state.gp_seq);
> > +		if (!rnp->parent)
> > +			rcu_seq_wait_qs(&rnp->gp_seq);
> > +		else
> > +			WRITE_ONCE(rnp->gp_seq, *rcu_root_seq());
> 
> This asymmetry is necessary because if I understand correctly, the
> non-root rcu_node structures still have zero state.  But this is one
> argument for use of rcu_seq_set_state(), because then you could just do
> rcu_seq_set_state(rnp->gp_seq, RCU_STATE_WAIT_QS) in both cases.
> 
> Or am I missing something that prevents this from working?

Indeed, the sequence counter (but not the state) is expected to be the same on
all nodes, so we can just modify the state as you said. And we can then assert
WARN_ON_ONCE(rnp->gp_seq != *rcu_root_seq())

> > @@ -2114,16 +2124,12 @@ static noinline void rcu_gp_cleanup(void)
> >  	 * the rcu_node structures before the beginning of the next grace
> >  	 * period is recorded in any of the rcu_node structures.
> >  	 */
> > -	new_gp_seq = rcu_state.gp_seq;
> > -	rcu_seq_end(&new_gp_seq);
> >  	rcu_for_each_node_breadth_first(rnp) {
> >  		raw_spin_lock_irq_rcu_node(rnp);
> >  		if (WARN_ON_ONCE(rcu_preempt_blocked_readers_cgp(rnp)))
> >  			dump_blkd_tasks(rnp, 10);
> >  		WARN_ON_ONCE(rnp->qsmask);
> > -		WRITE_ONCE(rnp->gp_seq, new_gp_seq);
> > -		if (!rnp->parent)
> > -			smp_mb(); // Order against failing poll_state_synchronize_rcu_full().
> 
> OK, this got moved up.
> 
> > +		WRITE_ONCE(rnp->gp_seq, *rcu_root_seq());
> 
> The idea being that the root rcu_node ->gp_seq is just overwritten
> with itself, correct?

Right!

> 
> >  		rdp = this_cpu_ptr(&rcu_data);
> >  		if (rnp == rdp->mynode)
> >  			needgp = __note_gp_changes(rnp, rdp) || needgp;
> > @@ -2146,9 +2152,7 @@ static noinline void rcu_gp_cleanup(void)
> >  	raw_spin_lock_irq_rcu_node(rnp); /* GP before ->gp_seq update. */
> >  
> >  	/* Declare grace period done, trace first to use old GP number. */
> > -	trace_rcu_grace_period(rcu_state.name, rcu_state.gp_seq, TPS("end"));
> > -	rcu_seq_end(&rcu_state.gp_seq);
> > -	ASSERT_EXCLUSIVE_WRITER(rcu_state.gp_seq);
> 
> Same question on not moving ASSERT_EXCLUSIVE_WRITER() to the rcu_node
> structures' ->gp_seq updates.

Indeed!

> > diff --git a/kernel/rcu/tree.h b/kernel/rcu/tree.h
> > index a9a811d9d7a3..9d811270b808 100644
> > --- a/kernel/rcu/tree.h
> > +++ b/kernel/rcu/tree.h
> > @@ -347,7 +347,6 @@ struct rcu_state {
> >  
> >  	/* The following fields are guarded by the root rcu_node's lock. */
> >  
> > -	unsigned long gp_seq ____cacheline_internodealigned_in_smp;
> >  						/* Grace-period sequence #. */
> 
> Heh!  Does the rcu_node structure's ->gp_seq need alignment?

Probably but then this will increase the size of all rnp's, right?

Thanks!

Re: [PATCH 1/3] rcu: Replace magic number with meaningful constant in rcu_seq_done_exact()

[Paul E. McKenney]

On Mon, Mar 31, 2025 at 11:14:10PM +0200, Frederic Weisbecker wrote:
> Le Mon, Mar 31, 2025 at 11:28:47AM -0700, Paul E. McKenney a écrit :
> > > So I'm unfortunately asking again if it wouldn't be a good idea to have a single
> > > global state counter that lives in the root node so that we don't have it
> > > duplicated in rcu_state.gp_seq.
> > > 
> > > This involves some special care:
> > > 
> > > 1) As you pointed out, the root can also be the unique leaf. So care must
> > >    be taken that no QS is reported before new online CPUs have been handled
> > >    and QS masks initialized. In order for __note_gp_changes() to handle
> > >    it gracefully we must have two seq states: one to tell the
> > >    grace period has started (before checking new online CPUs): RCU_STATE_STARTED
> > >    and one to tell that grace period is ready to receive quiescent states:
> > >    RCU_STATE_WAIT_QS. Although not strictly required, __note_gp_changes() can
> > >    ignore simple state changes to RCU_STATE_STARTED in order to spare useless
> > >    work and rnp locking. No QS will be reported before ->qsmask are initialized
> > >    in any case.
> > 
> > We have available up to three stages of grace-period processing, which
> > can be expanded relatively easily.  My concern is that we will be
> > replacing "which ->gp_seq to use" confusion with "which gp_seq state
> > to pay attention to" confusion.
> 
> True, this comes at the cost of a bit more complex state space.

Choices, choices!  ;-)

> > > 2) rcu_gp_cleanup() currently ends the grace period top-down through the rnp tree
> > >    and only then on rcu_state. With this change it's simply top-down. I have audited
> > >    rcu_seq_snap() and rcu_start_this_gp() uses and I haven't found an obvious
> > >    issue with that.
> > 
> > That would result in the root rcu_node structure's ->gp_seq declaring the
> > grace period over before the rest of the rcu_node structures were aware
> > of it having ended.  I guess that the last leaf rcu_node structure's
> > ->gp_seq field would be what you would look at if you were looking for
> > the actual grace period end.
> > 
> > Of course, you could argue that the grace period isn't fully over
> > until all the CPUs are aware of it, but that can take a very long time,
> > especially if there are CPUs staying idle for seconds (let alone minutes)
> > at at time.
> 
> Right. And it seems not all parts of RCU agree on a common definition of
> a complete grace period. Polling seem to be fine with just the root rnp
> to reflect rcu_seq_end() (which happens before the leaves do). Snapshot takers
> though rely on the whole tree to reflect the GP end...
> 
> As far as I audited the code, mostly rcu_seq_snap() callers, rcu_start_this_gp()
> and __note_gp_changes(), it looks fine to consider the GP is ended as soon
> as the root reflects it. I may have missed something of course...

The disagreement is a feature, at least up to a point.  That feature
allows CPUs to go idle for long periods without RCU having to bother
them or to mess with their per-CPU data (give or take ->gpwrap).  It also
allows per-rcu_node-leaf locking, which is important on large systems.

Trying to make precisely globally agreed-on beginnings and ends of
RCU grace periods will not end well from performance, scalability,
or real-time-response viewpoints.  ;-)

But simplifications that don't hurt performance, scalability, and
real-time-response are of course welcome.

> > > diff --git a/kernel/rcu/rcu.h b/kernel/rcu/rcu.h
> > > index feb3ac1dc5d5..c4bb3eb92a58 100644
> > > --- a/kernel/rcu/rcu.h
> > > +++ b/kernel/rcu/rcu.h
> > > @@ -19,6 +19,12 @@
> > >   * The two least significant bits contain the control flags.
> > >   * The most significant bits contain the grace-period sequence counter.
> > >   *
> > > + * RCU_STATE_STARTED		:	The gp has started but QS are not
> > > + *					requested yet.
> > > + *
> > > + * RCU_STATE_WAIT_QS		:	The gp has started and waits for
> > > + *					QS to be reported.
> > 
> > Do we want RCU_STATE_IDLE, by analogy with SRCU?
> 
> Makes sense.
> 
> > 
> > >   * When both control flags are zero, no grace period is in progress.
> > >   * When either bit is non-zero, a grace period has started and is in
> > >   * progress. When the grace period completes, the control flags are reset
> > > @@ -54,6 +60,9 @@
> > >   *					grace-period sequence number.
> > >   */
> > >  
> > > +#define RCU_STATE_STARTED	0x1
> > > +#define RCU_STATE_WAIT_QS	0x2
> > > +
> > >  /* Low-order bit definition for polled grace-period APIs. */
> > >  #define RCU_GET_STATE_COMPLETED	0x1
> > >  
> > > @@ -92,7 +101,13 @@ static inline void rcu_seq_start(unsigned long *sp)
> > >  {
> > >  	WRITE_ONCE(*sp, *sp + 1);
> > >  	smp_mb(); /* Ensure update-side operation after counter increment. */
> > > -	WARN_ON_ONCE(rcu_seq_state(*sp) != 1);
> > > +	WARN_ON_ONCE(rcu_seq_state(*sp) != RCU_STATE_STARTED);
> > > +}
> > > +
> > > +static inline void rcu_seq_wait_qs(unsigned long *sp)
> > > +{
> > > +	WRITE_ONCE(*sp, *sp + 1);
> > > +	WARN_ON_ONCE(rcu_seq_state(*sp) != RCU_STATE_WAIT_QS);
> > 
> > Or we could have the WARN_ON_ONCE() first, and use rcu_seq_set_state().
> 
> Right!
> 
> > > @@ -192,6 +192,21 @@ int rcu_get_gp_kthreads_prio(void)
> > >  }
> > >  EXPORT_SYMBOL_GPL(rcu_get_gp_kthreads_prio);
> > >  
> > > +/*
> > > + * Return the root node of the rcu_state structure.
> > > + */
> > > +static struct rcu_node *rcu_get_root(void)
> > > +{
> > > +	return &rcu_state.node[0];
> > > +}
> > 
> > OK, just code movement.
> 
> Right, if I proceed with this, a series will (try to) split more gently
> for review.

Fair enough!

> > > +static unsigned long *rcu_root_seq(void)
> > > +{
> > > +	struct rcu_node *root = rcu_get_root();
> > > +
> > > +	return &root->gp_seq;
> > > +}
> > 
> > Adding this function might be useful in any case, even if we keep the
> > current rcu_state.gp_seq counter.  A single place to add comments that
> > address the chosen type of confusion, if nothing else.  ;-)
> 
> ;-))
> 
> > 
> > 13 calls want the pointer and 13 want the value.  An even split!
> 
> The split is but 13 itself is not even! And I know people who strongly
> want it to stay that way!

Bad regex morning, apparently...

> > Not sure what lesson to draw from this, though indirecting a function call will
> > likely cause some confusion.  Some might argue for two access functions,
> > one for the pointer and the other for the value.  Me, I am just glad that
> > I included the word "might" in the first sentence of the prior paragraph.
> 
> Right, I'm not proud of the *rcu_root_seq() things.
> I thought about adding rcu_root_seq_current() but I'm not entirely comfortable
> with having all readers under READ_ONCE(). I like when our assumptions of non
> concurrent plain accesses are defeatedly reported by KCSAN.

Good point.  I believe that ASSERT_EXCLUSIVE_WRITER() can be used to
make this work, but plain accesses are admittedly much less typing.

And the small number of uses makes three different access functions less
than fully attractive.

> > > @@ -1249,7 +1256,7 @@ static bool __note_gp_changes(struct rcu_node *rnp, struct rcu_data *rdp)
> > >  
> > >  	raw_lockdep_assert_held_rcu_node(rnp);
> > >  
> > > -	if (rdp->gp_seq == rnp->gp_seq)
> > > +	if (!rcu_seq_changed(rdp->gp_seq, rnp->gp_seq))
> > 
> > The old code would detect a bunch of elapsed grace periods, starting
> > and ending in the same state.  The new code does not.  Does the new code
> > correctly handle CPUs that go idle for many grace periods?
> 
> I'm not sure I understand what you mean. rcu_seq_changed() is a comparison
> that simply clears RCU_SEQ_STARTED on both sides such that:
> 
> * Different GP numbers with or without RCU_SEQ_STARTED/RCU_SEQ_WAIT_QS
>   are always considered a change.
> 
> * Same GP numbers but rnp has RCU_SEQ_WAIT_QS while rdp hasn't is considered a
>   change
> 
> but:
> 
> * Same GP numbers but rnp has RCU_SEQ_STARTED while rdp hasn't is _NOT_
>   considered a change. This is not for correctness but rather to spare
>   rnp locking and useless work. The ->qsmask are 0 anyway so no QS can
>   be reported too early.

Idiot here missed the pair of tildes ("~") in that function.

I clearly need to revisit this code with that in mind...

> > > @@ -1802,10 +1809,9 @@ static noinline_for_stack bool rcu_gp_init(void)
> > >  	/* Advance to a new grace period and initialize state. */
> > >  	record_gp_stall_check_time();
> > >  	/* Record GP times before starting GP, hence rcu_seq_start(). */
> > > -	rcu_seq_start(&rcu_state.gp_seq);
> > > -	ASSERT_EXCLUSIVE_WRITER(rcu_state.gp_seq);
> > 
> > Why are we dropping the ASSERT_EXCLUSIVE_WRITER()?  As opposed to moving
> > it to the setting of the rcu_node structures' ->gp_seq updates?
> 
> I must confess I misunderstood that macro and thought it was to assert
> the variable has no other writer _at all_. But that seem to apply
> scopewise. So yes good point.
> 
> > > @@ -1902,7 +1908,10 @@ static noinline_for_stack bool rcu_gp_init(void)
> > >  		rdp = this_cpu_ptr(&rcu_data);
> > >  		rcu_preempt_check_blocked_tasks(rnp);
> > >  		rnp->qsmask = rnp->qsmaskinit;
> > > -		WRITE_ONCE(rnp->gp_seq, rcu_state.gp_seq);
> > > +		if (!rnp->parent)
> > > +			rcu_seq_wait_qs(&rnp->gp_seq);
> > > +		else
> > > +			WRITE_ONCE(rnp->gp_seq, *rcu_root_seq());
> > 
> > This asymmetry is necessary because if I understand correctly, the
> > non-root rcu_node structures still have zero state.  But this is one
> > argument for use of rcu_seq_set_state(), because then you could just do
> > rcu_seq_set_state(rnp->gp_seq, RCU_STATE_WAIT_QS) in both cases.
> > 
> > Or am I missing something that prevents this from working?
> 
> Indeed, the sequence counter (but not the state) is expected to be the same on
> all nodes, so we can just modify the state as you said. And we can then assert
> WARN_ON_ONCE(rnp->gp_seq != *rcu_root_seq())
> 
> > > @@ -2114,16 +2124,12 @@ static noinline void rcu_gp_cleanup(void)
> > >  	 * the rcu_node structures before the beginning of the next grace
> > >  	 * period is recorded in any of the rcu_node structures.
> > >  	 */
> > > -	new_gp_seq = rcu_state.gp_seq;
> > > -	rcu_seq_end(&new_gp_seq);
> > >  	rcu_for_each_node_breadth_first(rnp) {
> > >  		raw_spin_lock_irq_rcu_node(rnp);
> > >  		if (WARN_ON_ONCE(rcu_preempt_blocked_readers_cgp(rnp)))
> > >  			dump_blkd_tasks(rnp, 10);
> > >  		WARN_ON_ONCE(rnp->qsmask);
> > > -		WRITE_ONCE(rnp->gp_seq, new_gp_seq);
> > > -		if (!rnp->parent)
> > > -			smp_mb(); // Order against failing poll_state_synchronize_rcu_full().
> > 
> > OK, this got moved up.
> > 
> > > +		WRITE_ONCE(rnp->gp_seq, *rcu_root_seq());
> > 
> > The idea being that the root rcu_node ->gp_seq is just overwritten
> > with itself, correct?
> 
> Right!
> 
> > 
> > >  		rdp = this_cpu_ptr(&rcu_data);
> > >  		if (rnp == rdp->mynode)
> > >  			needgp = __note_gp_changes(rnp, rdp) || needgp;
> > > @@ -2146,9 +2152,7 @@ static noinline void rcu_gp_cleanup(void)
> > >  	raw_spin_lock_irq_rcu_node(rnp); /* GP before ->gp_seq update. */
> > >  
> > >  	/* Declare grace period done, trace first to use old GP number. */
> > > -	trace_rcu_grace_period(rcu_state.name, rcu_state.gp_seq, TPS("end"));
> > > -	rcu_seq_end(&rcu_state.gp_seq);
> > > -	ASSERT_EXCLUSIVE_WRITER(rcu_state.gp_seq);
> > 
> > Same question on not moving ASSERT_EXCLUSIVE_WRITER() to the rcu_node
> > structures' ->gp_seq updates.
> 
> Indeed!
> 
> > > diff --git a/kernel/rcu/tree.h b/kernel/rcu/tree.h
> > > index a9a811d9d7a3..9d811270b808 100644
> > > --- a/kernel/rcu/tree.h
> > > +++ b/kernel/rcu/tree.h
> > > @@ -347,7 +347,6 @@ struct rcu_state {
> > >  
> > >  	/* The following fields are guarded by the root rcu_node's lock. */
> > >  
> > > -	unsigned long gp_seq ____cacheline_internodealigned_in_smp;
> > >  						/* Grace-period sequence #. */
> > 
> > Heh!  Does the rcu_node structure's ->gp_seq need alignment?
> 
> Probably but then this will increase the size of all rnp's, right?

Indeed, this probably needs actual performance results showing that
it is needed.  My guess is that only systems with a single rcu_node
structure that is both leaf and root would have any chance of noticing.
And those tend to have few CPUs, so they might not care.

							Thanx, Paul

Re: [PATCH 1/3] rcu: Replace magic number with meaningful constant in rcu_seq_done_exact()

[Frederic Weisbecker]

Le Mon, Mar 31, 2025 at 02:29:52PM -0700, Paul E. McKenney a écrit :
> The disagreement is a feature, at least up to a point.  That feature
> allows CPUs to go idle for long periods without RCU having to bother
> them or to mess with their per-CPU data (give or take ->gpwrap).  It also
> allows per-rcu_node-leaf locking, which is important on large systems.
> 
> Trying to make precisely globally agreed-on beginnings and ends of
> RCU grace periods will not end well from performance, scalability,
> or real-time-response viewpoints.  ;-)

The distributed disagreement is definetly a feature. The duplicate root
is more debatable.

> But simplifications that don't hurt performance, scalability, and
> real-time-response are of course welcome.

I'm not even sure my proposal is a simplification. Perhaps it is. Another
hope is that it could avoid future accidents.

> Indeed, this probably needs actual performance results showing that
> it is needed.  My guess is that only systems with a single rcu_node
> structure that is both leaf and root would have any chance of noticing.
> And those tend to have few CPUs, so they might not care.

Do you have any idea for a benchmark to test here?

Thanks.

> 							Thanx, Paul

Re: [PATCH 1/3] rcu: Replace magic number with meaningful constant in rcu_seq_done_exact()

[Paul E. McKenney]

On Tue, Apr 01, 2025 at 03:38:21PM +0200, Frederic Weisbecker wrote:
> Le Mon, Mar 31, 2025 at 02:29:52PM -0700, Paul E. McKenney a écrit :
> > The disagreement is a feature, at least up to a point.  That feature
> > allows CPUs to go idle for long periods without RCU having to bother
> > them or to mess with their per-CPU data (give or take ->gpwrap).  It also
> > allows per-rcu_node-leaf locking, which is important on large systems.
> > 
> > Trying to make precisely globally agreed-on beginnings and ends of
> > RCU grace periods will not end well from performance, scalability,
> > or real-time-response viewpoints.  ;-)
> 
> The distributed disagreement is definetly a feature. The duplicate root
> is more debatable.
> 
> > But simplifications that don't hurt performance, scalability, and
> > real-time-response are of course welcome.
> 
> I'm not even sure my proposal is a simplification. Perhaps it is. Another
> hope is that it could avoid future accidents.
> 
> > Indeed, this probably needs actual performance results showing that
> > it is needed.  My guess is that only systems with a single rcu_node
> > structure that is both leaf and root would have any chance of noticing.
> > And those tend to have few CPUs, so they might not care.
> 
> Do you have any idea for a benchmark to test here?

The best that I can come up with is hammering with a combination of
expedited grace periods and polled grace periods.

							Thanx, Paul

[PATCH 2/3] rcu: Add warning to ensure rcu_seq_done_exact() is working

[Joel Fernandes]

The previous patch improved the rcu_seq_done_exact() function by adding
a meaningful constant for the guardband.

Ensure that this is working for the future by a quick check during
rcu_gp_init().

Signed-off-by: Joel Fernandes <joelagnelf@nvidia.com>
---
 kernel/rcu/tree.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
index 659f83e71048..29ddbcbea25e 100644
--- a/kernel/rcu/tree.c
+++ b/kernel/rcu/tree.c
@@ -1798,6 +1798,7 @@ static noinline_for_stack bool rcu_gp_init(void)
 	struct rcu_data *rdp;
 	struct rcu_node *rnp = rcu_get_root();
 	bool start_new_poll;
+	unsigned long old_gp_seq;
 
 	WRITE_ONCE(rcu_state.gp_activity, jiffies);
 	raw_spin_lock_irq_rcu_node(rnp);
@@ -1825,7 +1826,11 @@ static noinline_for_stack bool rcu_gp_init(void)
 	 */
 	start_new_poll = rcu_sr_normal_gp_init();
 	/* Record GP times before starting GP, hence rcu_seq_start(). */
+	old_gp_seq = rcu_state.gp_seq;
 	rcu_seq_start(&rcu_state.gp_seq);
+	/* Ensure that rcu_seq_done_exact() guardband doesn't give false positives. */
+	WARN_ON_ONCE(rcu_seq_done_exact(&old_gp_seq, rcu_seq_snap(&rcu_state.gp_seq)));
+
 	ASSERT_EXCLUSIVE_WRITER(rcu_state.gp_seq);
 	trace_rcu_grace_period(rcu_state.name, rcu_state.gp_seq, TPS("start"));
 	rcu_poll_gp_seq_start(&rcu_state.gp_seq_polled_snap);
-- 
2.43.0

Re: [PATCH 2/3] rcu: Add warning to ensure rcu_seq_done_exact() is working

[Paul E. McKenney]

On Mon, Mar 24, 2025 at 01:01:54PM -0400, Joel Fernandes wrote:
> The previous patch improved the rcu_seq_done_exact() function by adding
> a meaningful constant for the guardband.
> 
> Ensure that this is working for the future by a quick check during
> rcu_gp_init().
> 
> Signed-off-by: Joel Fernandes <joelagnelf@nvidia.com>

This is a good test for the guardband being way too short.

Are there other tests the should be run, possibly on a separate gp_seq
used only for testing?  Should the test below be under CONFIG_PROVE_RCU?

							Thanx, Paul

> ---
>  kernel/rcu/tree.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> index 659f83e71048..29ddbcbea25e 100644
> --- a/kernel/rcu/tree.c
> +++ b/kernel/rcu/tree.c
> @@ -1798,6 +1798,7 @@ static noinline_for_stack bool rcu_gp_init(void)
>  	struct rcu_data *rdp;
>  	struct rcu_node *rnp = rcu_get_root();
>  	bool start_new_poll;
> +	unsigned long old_gp_seq;
>  
>  	WRITE_ONCE(rcu_state.gp_activity, jiffies);
>  	raw_spin_lock_irq_rcu_node(rnp);
> @@ -1825,7 +1826,11 @@ static noinline_for_stack bool rcu_gp_init(void)
>  	 */
>  	start_new_poll = rcu_sr_normal_gp_init();
>  	/* Record GP times before starting GP, hence rcu_seq_start(). */
> +	old_gp_seq = rcu_state.gp_seq;
>  	rcu_seq_start(&rcu_state.gp_seq);
> +	/* Ensure that rcu_seq_done_exact() guardband doesn't give false positives. */
> +	WARN_ON_ONCE(rcu_seq_done_exact(&old_gp_seq, rcu_seq_snap(&rcu_state.gp_seq)));
> +
>  	ASSERT_EXCLUSIVE_WRITER(rcu_state.gp_seq);
>  	trace_rcu_grace_period(rcu_state.name, rcu_state.gp_seq, TPS("start"));
>  	rcu_poll_gp_seq_start(&rcu_state.gp_seq_polled_snap);
> -- 
> 2.43.0
> 

Re: [PATCH 2/3] rcu: Add warning to ensure rcu_seq_done_exact() is working

[Joel Fernandes]

> On Mar 26, 2025, at 6:36 PM, Paul E. McKenney <paulmck@kernel.org> wrote:
> 
> On Mon, Mar 24, 2025 at 01:01:54PM -0400, Joel Fernandes wrote:
>> The previous patch improved the rcu_seq_done_exact() function by adding
>> a meaningful constant for the guardband.
>> 
>> Ensure that this is working for the future by a quick check during
>> rcu_gp_init().
>> 
>> Signed-off-by: Joel Fernandes <joelagnelf@nvidia.com>
> 
> This is a good test for the guardband being way too short.

Thanks. Let me know if I could add your review tag!

> 
> Are there other tests the should be run, possibly on a separate gp_seq
> used only for testing?  Should the test below be under CONFIG_PROVE_RCU?

Yes, I could move it to PROVE RCU and it should be sufficient for testing.

The other test I was working on is to force the counter wrapping and hence gpwrap which is related. 

Maybe we could also some testing around false negatives not happening too often (for examples with the rcu_seq_done()).

I will add more tests if I come across usecases.

Thanks!

Joel

> 
>                            Thanx, Paul
> 
>> ---
>> kernel/rcu/tree.c | 5 +++++
>> 1 file changed, 5 insertions(+)
>> 
>> diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
>> index 659f83e71048..29ddbcbea25e 100644
>> --- a/kernel/rcu/tree.c
>> +++ b/kernel/rcu/tree.c
>> @@ -1798,6 +1798,7 @@ static noinline_for_stack bool rcu_gp_init(void)
>>    struct rcu_data *rdp;
>>    struct rcu_node *rnp = rcu_get_root();
>>    bool start_new_poll;
>> +    unsigned long old_gp_seq;
>> 
>>    WRITE_ONCE(rcu_state.gp_activity, jiffies);
>>    raw_spin_lock_irq_rcu_node(rnp);
>> @@ -1825,7 +1826,11 @@ static noinline_for_stack bool rcu_gp_init(void)
>>     */
>>    start_new_poll = rcu_sr_normal_gp_init();
>>    /* Record GP times before starting GP, hence rcu_seq_start(). */
>> +    old_gp_seq = rcu_state.gp_seq;
>>    rcu_seq_start(&rcu_state.gp_seq);
>> +    /* Ensure that rcu_seq_done_exact() guardband doesn't give false positives. */
>> +    WARN_ON_ONCE(rcu_seq_done_exact(&old_gp_seq, rcu_seq_snap(&rcu_state.gp_seq)));
>> +
>>    ASSERT_EXCLUSIVE_WRITER(rcu_state.gp_seq);
>>    trace_rcu_grace_period(rcu_state.name, rcu_state.gp_seq, TPS("start"));
>>    rcu_poll_gp_seq_start(&rcu_state.gp_seq_polled_snap);
>> --
>> 2.43.0
>> 

Re: [PATCH 2/3] rcu: Add warning to ensure rcu_seq_done_exact() is working

[Paul E. McKenney]

On Wed, Mar 26, 2025 at 10:50:13PM +0000, Joel Fernandes wrote:
> 
> 
> > On Mar 26, 2025, at 6:36 PM, Paul E. McKenney <paulmck@kernel.org> wrote:
> > 
> > On Mon, Mar 24, 2025 at 01:01:54PM -0400, Joel Fernandes wrote:
> >> The previous patch improved the rcu_seq_done_exact() function by adding
> >> a meaningful constant for the guardband.
> >> 
> >> Ensure that this is working for the future by a quick check during
> >> rcu_gp_init().
> >> 
> >> Signed-off-by: Joel Fernandes <joelagnelf@nvidia.com>
> > 
> > This is a good test for the guardband being way too short.
> 
> Thanks. Let me know if I could add your review tag!
> 
> > Are there other tests the should be run, possibly on a separate gp_seq
> > used only for testing?  Should the test below be under CONFIG_PROVE_RCU?
> 
> Yes, I could move it to PROVE RCU and it should be sufficient for testing.
> 
> The other test I was working on is to force the counter wrapping and hence gpwrap which is related. 

Very good on both counts.

> Maybe we could also some testing around false negatives not happening too often (for examples with the rcu_seq_done()).
> 
> I will add more tests if I come across usecases.

Keep the counter from just after the start (or just before the end)
of the previous grace period and verify that it also has not ended just
after the start of the current grace period?

							Thanx, Paul

> Thanks!
> 
> Joel
> 
> > 
> >                            Thanx, Paul
> > 
> >> ---
> >> kernel/rcu/tree.c | 5 +++++
> >> 1 file changed, 5 insertions(+)
> >> 
> >> diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> >> index 659f83e71048..29ddbcbea25e 100644
> >> --- a/kernel/rcu/tree.c
> >> +++ b/kernel/rcu/tree.c
> >> @@ -1798,6 +1798,7 @@ static noinline_for_stack bool rcu_gp_init(void)
> >>    struct rcu_data *rdp;
> >>    struct rcu_node *rnp = rcu_get_root();
> >>    bool start_new_poll;
> >> +    unsigned long old_gp_seq;
> >> 
> >>    WRITE_ONCE(rcu_state.gp_activity, jiffies);
> >>    raw_spin_lock_irq_rcu_node(rnp);
> >> @@ -1825,7 +1826,11 @@ static noinline_for_stack bool rcu_gp_init(void)
> >>     */
> >>    start_new_poll = rcu_sr_normal_gp_init();
> >>    /* Record GP times before starting GP, hence rcu_seq_start(). */
> >> +    old_gp_seq = rcu_state.gp_seq;
> >>    rcu_seq_start(&rcu_state.gp_seq);
> >> +    /* Ensure that rcu_seq_done_exact() guardband doesn't give false positives. */
> >> +    WARN_ON_ONCE(rcu_seq_done_exact(&old_gp_seq, rcu_seq_snap(&rcu_state.gp_seq)));
> >> +
> >>    ASSERT_EXCLUSIVE_WRITER(rcu_state.gp_seq);
> >>    trace_rcu_grace_period(rcu_state.name, rcu_state.gp_seq, TPS("start"));
> >>    rcu_poll_gp_seq_start(&rcu_state.gp_seq_polled_snap);
> >> --
> >> 2.43.0
> >> 

[PATCH 3/3] rcu: Comment on the extraneous delta test on rcu_seq_done_exact()

[Joel Fernandes]

From: Frederic Weisbecker <frederic@kernel.org>

The numbers used in rcu_seq_done_exact() lack some explanation behind
their magic. Especially after the commit:

    85aad7cc4178 ("rcu: Fix get_state_synchronize_rcu_full() GP-start detection")

which reported a subtle issue where a new GP sequence snapshot was taken
on the root node state while a grace period had already been started and
reflected on the global state sequence but not yet on the root node
sequence, making a polling user waiting on a wrong already started grace
period that would ignore freshly online CPUs.

The fix involved taking the snaphot on the global state sequence and
waiting on the root node sequence. And since a grace period is first
started on the global state and only afterward reflected on the root
node, a snapshot taken on the global state sequence might be two full
grace periods ahead of the root node as in the following example:

rnp->gp_seq = rcu_state.gp_seq = 0

    CPU 0                                           CPU 1
    -----                                           -----
    // rcu_state.gp_seq = 1
    rcu_seq_start(&rcu_state.gp_seq)
                                                    // snap = 8
                                                    snap = rcu_seq_snap(&rcu_state.gp_seq)
                                                    // Two full GP differences
                                                    rcu_seq_done_exact(&rnp->gp_seq, snap)
    // rnp->gp_seq = 1
    WRITE_ONCE(rnp->gp_seq, rcu_state.gp_seq);

Add a comment about those expectations and to clarify the magic within
the relevant function.

Note that the issue arises mainly with the use of rcu_seq_done_exact()
which has a much tigher guardband (of 2 GPs) to ensure the false-negative
window of the API during wraparound is limited to just 2 GPs.
rcu_seq_done() does not have such strict requirements, however its large
false-negative window of ULONG_MAX/2 is not ideal for the polling API.
However, this also means care is needed to ensure the guardband is as
large as needed to avoid the example scenario describe above which a
warning added in an earlier patch does.

[ Comment wordsmithing by Joel ]

Signed-off-by: Frederic Weisbecker <frederic@kernel.org>
Reviewed-by: Paul E. McKenney <paulmck@kernel.org>
Signed-off-by: Joel Fernandes <joelagnelf@nvidia.com>
---
 kernel/rcu/rcu.h | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/kernel/rcu/rcu.h b/kernel/rcu/rcu.h
index 5e1ee570bb27..db63f330768c 100644
--- a/kernel/rcu/rcu.h
+++ b/kernel/rcu/rcu.h
@@ -160,6 +160,15 @@ static inline bool rcu_seq_done(unsigned long *sp, unsigned long s)
  * Given a snapshot from rcu_seq_snap(), determine whether or not a
  * full update-side operation has occurred, but do not allow the
  * (ULONG_MAX / 2) safety-factor/guard-band.
+ *
+ * The token returned by get_state_synchronize_rcu_full() is based on
+ * rcu_state.gp_seq but it is tested in poll_state_synchronize_rcu_full()
+ * against the root rnp->gp_seq. Since rcu_seq_start() is first called
+ * on rcu_state.gp_seq and only later reflected on the root rnp->gp_seq,
+ * it is possible that rcu_seq_snap(rcu_state.gp_seq) returns 2 full grace
+ * periods ahead of the root rnp->gp_seq. To prevent false-positives with the
+ * full polling API that a wrap around instantly completed the GP, when nothing
+ * like that happened, adjust for the 2 GPs in the ULONG_CMP_LT().
  */
 static inline bool rcu_seq_done_exact(unsigned long *sp, unsigned long s)
 {
-- 
2.43.0

Re: [PATCH 3/3] rcu: Comment on the extraneous delta test on rcu_seq_done_exact()

[Paul E. McKenney]

On Mon, Mar 24, 2025 at 01:01:55PM -0400, Joel Fernandes wrote:
> From: Frederic Weisbecker <frederic@kernel.org>
> 
> The numbers used in rcu_seq_done_exact() lack some explanation behind
> their magic. Especially after the commit:
> 
>     85aad7cc4178 ("rcu: Fix get_state_synchronize_rcu_full() GP-start detection")
> 
> which reported a subtle issue where a new GP sequence snapshot was taken
> on the root node state while a grace period had already been started and
> reflected on the global state sequence but not yet on the root node
> sequence, making a polling user waiting on a wrong already started grace
> period that would ignore freshly online CPUs.
> 
> The fix involved taking the snaphot on the global state sequence and
> waiting on the root node sequence. And since a grace period is first
> started on the global state and only afterward reflected on the root
> node, a snapshot taken on the global state sequence might be two full
> grace periods ahead of the root node as in the following example:
> 
> rnp->gp_seq = rcu_state.gp_seq = 0
> 
>     CPU 0                                           CPU 1
>     -----                                           -----
>     // rcu_state.gp_seq = 1
>     rcu_seq_start(&rcu_state.gp_seq)
>                                                     // snap = 8
>                                                     snap = rcu_seq_snap(&rcu_state.gp_seq)
>                                                     // Two full GP differences
>                                                     rcu_seq_done_exact(&rnp->gp_seq, snap)
>     // rnp->gp_seq = 1
>     WRITE_ONCE(rnp->gp_seq, rcu_state.gp_seq);
> 
> Add a comment about those expectations and to clarify the magic within
> the relevant function.
> 
> Note that the issue arises mainly with the use of rcu_seq_done_exact()
> which has a much tigher guardband (of 2 GPs) to ensure the false-negative
> window of the API during wraparound is limited to just 2 GPs.
> rcu_seq_done() does not have such strict requirements, however its large
> false-negative window of ULONG_MAX/2 is not ideal for the polling API.
> However, this also means care is needed to ensure the guardband is as
> large as needed to avoid the example scenario describe above which a
> warning added in an earlier patch does.
> 
> [ Comment wordsmithing by Joel ]
> 
> Signed-off-by: Frederic Weisbecker <frederic@kernel.org>
> Reviewed-by: Paul E. McKenney <paulmck@kernel.org>

Looks good, and I stand by my Reviewed-by.  ;-)

							Thanx, Paul

> Signed-off-by: Joel Fernandes <joelagnelf@nvidia.com>
> ---
>  kernel/rcu/rcu.h | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/kernel/rcu/rcu.h b/kernel/rcu/rcu.h
> index 5e1ee570bb27..db63f330768c 100644
> --- a/kernel/rcu/rcu.h
> +++ b/kernel/rcu/rcu.h
> @@ -160,6 +160,15 @@ static inline bool rcu_seq_done(unsigned long *sp, unsigned long s)
>   * Given a snapshot from rcu_seq_snap(), determine whether or not a
>   * full update-side operation has occurred, but do not allow the
>   * (ULONG_MAX / 2) safety-factor/guard-band.
> + *
> + * The token returned by get_state_synchronize_rcu_full() is based on
> + * rcu_state.gp_seq but it is tested in poll_state_synchronize_rcu_full()
> + * against the root rnp->gp_seq. Since rcu_seq_start() is first called
> + * on rcu_state.gp_seq and only later reflected on the root rnp->gp_seq,
> + * it is possible that rcu_seq_snap(rcu_state.gp_seq) returns 2 full grace
> + * periods ahead of the root rnp->gp_seq. To prevent false-positives with the
> + * full polling API that a wrap around instantly completed the GP, when nothing
> + * like that happened, adjust for the 2 GPs in the ULONG_CMP_LT().
>   */
>  static inline bool rcu_seq_done_exact(unsigned long *sp, unsigned long s)
>  {
> -- 
> 2.43.0
> 

Re: [PATCH 3/3] rcu: Comment on the extraneous delta test on rcu_seq_done_exact()

[Joel Fernandes]

> On Mar 26, 2025, at 6:37 PM, Paul E. McKenney <paulmck@kernel.org> wrote:
> 
> On Mon, Mar 24, 2025 at 01:01:55PM -0400, Joel Fernandes wrote:
>> From: Frederic Weisbecker <frederic@kernel.org>
>> 
>> The numbers used in rcu_seq_done_exact() lack some explanation behind
>> their magic. Especially after the commit:
>> 
>>    85aad7cc4178 ("rcu: Fix get_state_synchronize_rcu_full() GP-start detection")
>> 
>> which reported a subtle issue where a new GP sequence snapshot was taken
>> on the root node state while a grace period had already been started and
>> reflected on the global state sequence but not yet on the root node
>> sequence, making a polling user waiting on a wrong already started grace
>> period that would ignore freshly online CPUs.
>> 
>> The fix involved taking the snaphot on the global state sequence and
>> waiting on the root node sequence. And since a grace period is first
>> started on the global state and only afterward reflected on the root
>> node, a snapshot taken on the global state sequence might be two full
>> grace periods ahead of the root node as in the following example:
>> 
>> rnp->gp_seq = rcu_state.gp_seq = 0
>> 
>>    CPU 0                                           CPU 1
>>    -----                                           -----
>>    // rcu_state.gp_seq = 1
>>    rcu_seq_start(&rcu_state.gp_seq)
>>                                                    // snap = 8
>>                                                    snap = rcu_seq_snap(&rcu_state.gp_seq)
>>                                                    // Two full GP differences
>>                                                    rcu_seq_done_exact(&rnp->gp_seq, snap)
>>    // rnp->gp_seq = 1
>>    WRITE_ONCE(rnp->gp_seq, rcu_state.gp_seq);
>> 
>> Add a comment about those expectations and to clarify the magic within
>> the relevant function.
>> 
>> Note that the issue arises mainly with the use of rcu_seq_done_exact()
>> which has a much tigher guardband (of 2 GPs) to ensure the false-negative
>> window of the API during wraparound is limited to just 2 GPs.
>> rcu_seq_done() does not have such strict requirements, however its large
>> false-negative window of ULONG_MAX/2 is not ideal for the polling API.
>> However, this also means care is needed to ensure the guardband is as
>> large as needed to avoid the example scenario describe above which a
>> warning added in an earlier patch does.
>> 
>> [ Comment wordsmithing by Joel ]
>> 
>> Signed-off-by: Frederic Weisbecker <frederic@kernel.org>
>> Reviewed-by: Paul E. McKenney <paulmck@kernel.org>
> 
> Looks good, and I stand by my Reviewed-by.  ;-)

Thanks, I will queue this one for 6.16.

- Joel


> 
>                            Thanx, Paul
> 
>> Signed-off-by: Joel Fernandes <joelagnelf@nvidia.com>
>> ---
>> kernel/rcu/rcu.h | 9 +++++++++
>> 1 file changed, 9 insertions(+)
>> 
>> diff --git a/kernel/rcu/rcu.h b/kernel/rcu/rcu.h
>> index 5e1ee570bb27..db63f330768c 100644
>> --- a/kernel/rcu/rcu.h
>> +++ b/kernel/rcu/rcu.h
>> @@ -160,6 +160,15 @@ static inline bool rcu_seq_done(unsigned long *sp, unsigned long s)
>>  * Given a snapshot from rcu_seq_snap(), determine whether or not a
>>  * full update-side operation has occurred, but do not allow the
>>  * (ULONG_MAX / 2) safety-factor/guard-band.
>> + *
>> + * The token returned by get_state_synchronize_rcu_full() is based on
>> + * rcu_state.gp_seq but it is tested in poll_state_synchronize_rcu_full()
>> + * against the root rnp->gp_seq. Since rcu_seq_start() is first called
>> + * on rcu_state.gp_seq and only later reflected on the root rnp->gp_seq,
>> + * it is possible that rcu_seq_snap(rcu_state.gp_seq) returns 2 full grace
>> + * periods ahead of the root rnp->gp_seq. To prevent false-positives with the
>> + * full polling API that a wrap around instantly completed the GP, when nothing
>> + * like that happened, adjust for the 2 GPs in the ULONG_CMP_LT().
>>  */
>> static inline bool rcu_seq_done_exact(unsigned long *sp, unsigned long s)
>> {
>> --
>> 2.43.0
>>