[PATCH rcu 0/6] Grace-period memory-barrier adjustments for v6.11

[PATCH rcu 0/6] Grace-period memory-barrier adjustments for v6.11

[Paul E. McKenney]

Hello!

This series removes redundant memory barriers from the grace-period
code paths:

1.	Remove full ordering on second EQS snapshot, courtesy of Frederic
	Weisbecker.

2.	Remove superfluous full memory barrier upon first EQS snapshot,
	courtesy of Frederic Weisbecker.

3.	rcu/exp: Remove superfluous full memory barrier upon first EQS
	snapshot, courtesy of Frederic Weisbecker.

4.	Remove full memory barrier on boot time eqs sanity check,
	courtesy of Frederic Weisbecker.

5.	Remove full memory barrier on RCU stall printout, courtesy of
	Frederic Weisbecker.

6.	rcu/exp: Remove redundant full memory barrier at the end of GP,
	courtesy of Frederic Weisbecker.

						Thanx, Paul

------------------------------------------------------------------------

 b/Documentation/RCU/Design/Memory-Ordering/Tree-RCU-Memory-Ordering.rst |    6 +--
 b/kernel/rcu/tree.c                                                     |    2 -
 b/kernel/rcu/tree_exp.h                                                 |    8 +++-
 b/kernel/rcu/tree_stall.h                                               |    4 +-
 kernel/rcu/tree.c                                                       |   19 +++-------
 kernel/rcu/tree_exp.h                                                   |    8 +++-
 6 files changed, 26 insertions(+), 21 deletions(-)

[PATCH rcu 1/6] rcu: Remove full ordering on second EQS snapshot

[Paul E. McKenney]

From: Frederic Weisbecker <frederic@kernel.org>

When the grace period kthread checks the extended quiescent state
counter of a CPU, full ordering is necessary to ensure that either:

* If the GP kthread observes the remote target in an extended quiescent
  state, then that target must observe all accesses prior to the current
  grace period, including the current grace period sequence number, once
  it exits that extended quiescent state. Also the GP kthread must
  observe all accesses performed by the target prior it entering in
  EQS.

or:

* If the GP kthread observes the remote target NOT in an extended
  quiescent state, then the target further entering in an extended
  quiescent state must observe all accesses prior to the current
  grace period, including the current grace period sequence number, once
  it enters that extended quiescent state. Also the GP kthread later
  observing that EQS must also observe all accesses performed by the
  target prior it entering in EQS.

This ordering is explicitly performed both on the first EQS snapshot
and on the second one as well through the combination of a preceding
full barrier followed by an acquire read. However the second snapshot's
full memory barrier is redundant and not needed to enforce the above
guarantees:

    GP kthread                  Remote target
    ----                        -----
    // Access prior GP
    WRITE_ONCE(A, 1)
    // first snapshot
    smp_mb()
    x = smp_load_acquire(EQS)
                               // Access prior GP
                               WRITE_ONCE(B, 1)
                               // EQS enter
                               // implied full barrier by atomic_add_return()
                               atomic_add_return(RCU_DYNTICKS_IDX, EQS)
                               // implied full barrier by atomic_add_return()
                               READ_ONCE(A)
    // second snapshot
    y = smp_load_acquire(EQS)
    z = READ_ONCE(B)

If the GP kthread above fails to observe the remote target in EQS
(x not in EQS), the remote target will observe A == 1 after further
entering in EQS. Then the second snapshot taken by the GP kthread only
need to be an acquire read in order to observe z == 1.

Therefore remove the needless full memory barrier on second snapshot.

Signed-off-by: Frederic Weisbecker <frederic@kernel.org>
Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
---
 kernel/rcu/tree.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
index 28c7031711a3f..f07b8bff4621b 100644
--- a/kernel/rcu/tree.c
+++ b/kernel/rcu/tree.c
@@ -321,7 +321,7 @@ static bool rcu_dynticks_in_eqs(int snap)
  */
 static bool rcu_dynticks_in_eqs_since(struct rcu_data *rdp, int snap)
 {
-	return snap != rcu_dynticks_snap(rdp->cpu);
+	return snap != ct_dynticks_cpu_acquire(rdp->cpu);
 }
 
 /*
-- 
2.40.1

[PATCH rcu 2/6] rcu: Remove superfluous full memory barrier upon first EQS snapshot

[Paul E. McKenney]

From: Frederic Weisbecker <frederic@kernel.org>

When the grace period kthread checks the extended quiescent state
counter of a CPU, full ordering is necessary to ensure that either:

* If the GP kthread observes the remote target in an extended quiescent
  state, then that target must observe all accesses prior to the current
  grace period, including the current grace period sequence number, once
  it exits that extended quiescent state.

or:

* If the GP kthread observes the remote target NOT in an extended
  quiescent state, then the target further entering in an extended
  quiescent state must observe all accesses prior to the current
  grace period, including the current grace period sequence number, once
  it enters that extended quiescent state.

This ordering is enforced through a full memory barrier placed right
before taking the first EQS snapshot. However this is superfluous
because the snapshot is taken while holding the target's rnp lock which
provides the necessary ordering through its chain of
smp_mb__after_unlock_lock().

Remove the needless explicit barrier before the snapshot and put a
comment about the implicit barrier newly relied upon here.

Signed-off-by: Frederic Weisbecker <frederic@kernel.org>
Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
---
 .../Design/Memory-Ordering/Tree-RCU-Memory-Ordering.rst    | 6 +++---
 kernel/rcu/tree.c                                          | 7 ++++++-
 2 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/Documentation/RCU/Design/Memory-Ordering/Tree-RCU-Memory-Ordering.rst b/Documentation/RCU/Design/Memory-Ordering/Tree-RCU-Memory-Ordering.rst
index 5750f125361b0..728b1e690c646 100644
--- a/Documentation/RCU/Design/Memory-Ordering/Tree-RCU-Memory-Ordering.rst
+++ b/Documentation/RCU/Design/Memory-Ordering/Tree-RCU-Memory-Ordering.rst
@@ -149,9 +149,9 @@ This case is handled by calls to the strongly ordered
 ``atomic_add_return()`` read-modify-write atomic operation that
 is invoked within ``rcu_dynticks_eqs_enter()`` at idle-entry
 time and within ``rcu_dynticks_eqs_exit()`` at idle-exit time.
-The grace-period kthread invokes ``rcu_dynticks_snap()`` and
-``rcu_dynticks_in_eqs_since()`` (both of which invoke
-an ``atomic_add_return()`` of zero) to detect idle CPUs.
+The grace-period kthread invokes first ``ct_dynticks_cpu_acquire()``
+(preceded by a full memory barrier) and ``rcu_dynticks_in_eqs_since()``
+(both of which rely on acquire semantics) to detect idle CPUs.
 
 +-----------------------------------------------------------------------+
 | **Quick Quiz**:                                                       |
diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
index f07b8bff4621b..1a6ef9c5c949e 100644
--- a/kernel/rcu/tree.c
+++ b/kernel/rcu/tree.c
@@ -769,7 +769,12 @@ static void rcu_gpnum_ovf(struct rcu_node *rnp, struct rcu_data *rdp)
  */
 static int dyntick_save_progress_counter(struct rcu_data *rdp)
 {
-	rdp->dynticks_snap = rcu_dynticks_snap(rdp->cpu);
+	/*
+	 * Full ordering against accesses prior current GP and also against
+	 * current GP sequence number is enforced by current rnp locking
+	 * with chained smp_mb__after_unlock_lock().
+	 */
+	rdp->dynticks_snap = ct_dynticks_cpu_acquire(rdp->cpu);
 	if (rcu_dynticks_in_eqs(rdp->dynticks_snap)) {
 		trace_rcu_fqs(rcu_state.name, rdp->gp_seq, rdp->cpu, TPS("dti"));
 		rcu_gpnum_ovf(rdp->mynode, rdp);
-- 
2.40.1

[PATCH rcu 3/6] rcu/exp: Remove superfluous full memory barrier upon first EQS snapshot

[Paul E. McKenney]

From: Frederic Weisbecker <frederic@kernel.org>

When the grace period kthread checks the extended quiescent state
counter of a CPU, full ordering is necessary to ensure that either:

* If the GP kthread observes the remote target in an extended quiescent
  state, then that target must observe all accesses prior to the current
  grace period, including the current grace period sequence number, once
  it exits that extended quiescent state.

or:

* If the GP kthread observes the remote target NOT in an extended
  quiescent state, then the target further entering in an extended
  quiescent state must observe all accesses prior to the current
  grace period, including the current grace period sequence number, once
  it enters that extended quiescent state.

This ordering is enforced through a full memory barrier placed right
before taking the first EQS snapshot. However this is superfluous
because the snapshot is taken while holding the target's rnp lock which
provides the necessary ordering through its chain of
smp_mb__after_unlock_lock().

Remove the needless explicit barrier before the snapshot and put a
comment about the implicit barrier newly relied upon here.

Signed-off-by: Frederic Weisbecker <frederic@kernel.org>
Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
---
 kernel/rcu/tree_exp.h | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/kernel/rcu/tree_exp.h b/kernel/rcu/tree_exp.h
index 8a1d9c8bd9f74..bec24ea6777e8 100644
--- a/kernel/rcu/tree_exp.h
+++ b/kernel/rcu/tree_exp.h
@@ -357,7 +357,13 @@ static void __sync_rcu_exp_select_node_cpus(struct rcu_exp_work *rewp)
 		    !(rnp->qsmaskinitnext & mask)) {
 			mask_ofl_test |= mask;
 		} else {
-			snap = rcu_dynticks_snap(cpu);
+			/*
+			 * Full ordering against accesses prior current GP and
+			 * also against current GP sequence number is enforced
+			 * by current rnp locking with chained
+			 * smp_mb__after_unlock_lock().
+			 */
+			snap = ct_dynticks_cpu_acquire(cpu);
 			if (rcu_dynticks_in_eqs(snap))
 				mask_ofl_test |= mask;
 			else
-- 
2.40.1

[PATCH rcu 4/6] rcu: Remove full memory barrier on boot time eqs sanity check

[Paul E. McKenney]

From: Frederic Weisbecker <frederic@kernel.org>

When the boot CPU initializes the per-CPU data on behalf of all possible
CPUs, a sanity check is performed on each of them to make sure none is
initialized in an extended quiescent state.

This check involves a full memory barrier which is useless at this early
boot stage.

Do a plain access instead.

Signed-off-by: Frederic Weisbecker <frederic@kernel.org>
Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
---
 kernel/rcu/tree.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
index 1a6ef9c5c949e..9fa6d2b557d6c 100644
--- a/kernel/rcu/tree.c
+++ b/kernel/rcu/tree.c
@@ -4772,7 +4772,7 @@ rcu_boot_init_percpu_data(int cpu)
 	rdp->grpmask = leaf_node_cpu_bit(rdp->mynode, cpu);
 	INIT_WORK(&rdp->strict_work, strict_work_handler);
 	WARN_ON_ONCE(ct->dynticks_nesting != 1);
-	WARN_ON_ONCE(rcu_dynticks_in_eqs(rcu_dynticks_snap(cpu)));
+	WARN_ON_ONCE(rcu_dynticks_in_eqs(ct_dynticks_cpu(cpu)));
 	rdp->barrier_seq_snap = rcu_state.barrier_sequence;
 	rdp->rcu_ofl_gp_seq = rcu_state.gp_seq;
 	rdp->rcu_ofl_gp_state = RCU_GP_CLEANED;
-- 
2.40.1

[PATCH rcu 5/6] rcu: Remove full memory barrier on RCU stall printout

[Paul E. McKenney]

From: Frederic Weisbecker <frederic@kernel.org>

RCU stall printout fetches the EQS state of a CPU with a preceding full
memory barrier. However there is nothing to order this read against at
this debugging stage. It is inherently racy when performed remotely.

Do a plain read instead.

This was the last user of rcu_dynticks_snap().

Signed-off-by: Frederic Weisbecker <frederic@kernel.org>
Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
---
 kernel/rcu/tree.c       | 10 ----------
 kernel/rcu/tree_stall.h |  4 ++--
 2 files changed, 2 insertions(+), 12 deletions(-)

diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
index 9fa6d2b557d6c..db9ca6c0c9b30 100644
--- a/kernel/rcu/tree.c
+++ b/kernel/rcu/tree.c
@@ -295,16 +295,6 @@ static void rcu_dynticks_eqs_online(void)
 	ct_state_inc(RCU_DYNTICKS_IDX);
 }
 
-/*
- * Snapshot the ->dynticks counter with full ordering so as to allow
- * stable comparison of this counter with past and future snapshots.
- */
-static int rcu_dynticks_snap(int cpu)
-{
-	smp_mb();  // Fundamental RCU ordering guarantee.
-	return ct_dynticks_cpu_acquire(cpu);
-}
-
 /*
  * Return true if the snapshot returned from rcu_dynticks_snap()
  * indicates that RCU is in an extended quiescent state.
diff --git a/kernel/rcu/tree_stall.h b/kernel/rcu/tree_stall.h
index 460efecd077be..4b0e9d7c4c68e 100644
--- a/kernel/rcu/tree_stall.h
+++ b/kernel/rcu/tree_stall.h
@@ -501,7 +501,7 @@ static void print_cpu_stall_info(int cpu)
 	}
 	delta = rcu_seq_ctr(rdp->mynode->gp_seq - rdp->rcu_iw_gp_seq);
 	falsepositive = rcu_is_gp_kthread_starving(NULL) &&
-			rcu_dynticks_in_eqs(rcu_dynticks_snap(cpu));
+			rcu_dynticks_in_eqs(ct_dynticks_cpu(cpu));
 	rcuc_starved = rcu_is_rcuc_kthread_starving(rdp, &j);
 	if (rcuc_starved)
 		// Print signed value, as negative values indicate a probable bug.
@@ -515,7 +515,7 @@ static void print_cpu_stall_info(int cpu)
 			rdp->rcu_iw_pending ? (int)min(delta, 9UL) + '0' :
 				"!."[!delta],
 	       ticks_value, ticks_title,
-	       rcu_dynticks_snap(cpu) & 0xffff,
+	       ct_dynticks_cpu(cpu) & 0xffff,
 	       ct_dynticks_nesting_cpu(cpu), ct_dynticks_nmi_nesting_cpu(cpu),
 	       rdp->softirq_snap, kstat_softirqs_cpu(RCU_SOFTIRQ, cpu),
 	       data_race(rcu_state.n_force_qs) - rcu_state.n_force_qs_gpstart,
-- 
2.40.1

[PATCH rcu 6/6] rcu/exp: Remove redundant full memory barrier at the end of GP

[Paul E. McKenney]

From: Frederic Weisbecker <frederic@kernel.org>

A full memory barrier is necessary at the end of the expedited grace
period to order:

1) The grace period completion (pictured by the GP sequence
   number) with all preceding accesses. This pairs with rcu_seq_end()
   performed by the concurrent kworker.

2) The grace period completion and subsequent post-GP update side
   accesses. Pairs again against rcu_seq_end().

This full barrier is already provided by the final sync_exp_work_done()
test, making the subsequent explicit one redundant. Remove it and
improve comments.

Signed-off-by: Frederic Weisbecker <frederic@kernel.org>
Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
---
 kernel/rcu/tree_exp.h | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/kernel/rcu/tree_exp.h b/kernel/rcu/tree_exp.h
index bec24ea6777e8..721cb93b1fece 100644
--- a/kernel/rcu/tree_exp.h
+++ b/kernel/rcu/tree_exp.h
@@ -265,7 +265,12 @@ static bool sync_exp_work_done(unsigned long s)
 {
 	if (rcu_exp_gp_seq_done(s)) {
 		trace_rcu_exp_grace_period(rcu_state.name, s, TPS("done"));
-		smp_mb(); /* Ensure test happens before caller kfree(). */
+		/*
+		 * Order GP completion with preceding accesses. Order also GP
+		 * completion with post GP update side accesses. Pairs with
+		 * rcu_seq_end().
+		 */
+		smp_mb();
 		return true;
 	}
 	return false;
@@ -959,7 +964,6 @@ void synchronize_rcu_expedited(void)
 	rnp = rcu_get_root();
 	wait_event(rnp->exp_wq[rcu_seq_ctr(s) & 0x3],
 		   sync_exp_work_done(s));
-	smp_mb(); /* Work actions happen before return. */
 
 	/* Let the next expedited grace period start. */
 	mutex_unlock(&rcu_state.exp_mutex);
-- 
2.40.1

Re: [PATCH rcu 1/6] rcu: Remove full ordering on second EQS snapshot

[Frederic Weisbecker]

Le Tue, Jun 04, 2024 at 03:26:47PM -0700, Paul E. McKenney a écrit :
> From: Frederic Weisbecker <frederic@kernel.org>
> 
> When the grace period kthread checks the extended quiescent state
> counter of a CPU, full ordering is necessary to ensure that either:
> 
> * If the GP kthread observes the remote target in an extended quiescent
>   state, then that target must observe all accesses prior to the current
>   grace period, including the current grace period sequence number, once
>   it exits that extended quiescent state. Also the GP kthread must
>   observe all accesses performed by the target prior it entering in
>   EQS.
> 
> or:
> 
> * If the GP kthread observes the remote target NOT in an extended
>   quiescent state, then the target further entering in an extended
>   quiescent state must observe all accesses prior to the current
>   grace period, including the current grace period sequence number, once
>   it enters that extended quiescent state. Also the GP kthread later
>   observing that EQS must also observe all accesses performed by the
>   target prior it entering in EQS.
> 
> This ordering is explicitly performed both on the first EQS snapshot
> and on the second one as well through the combination of a preceding
> full barrier followed by an acquire read. However the second snapshot's
> full memory barrier is redundant and not needed to enforce the above
> guarantees:
> 
>     GP kthread                  Remote target
>     ----                        -----
>     // Access prior GP
>     WRITE_ONCE(A, 1)
>     // first snapshot
>     smp_mb()
>     x = smp_load_acquire(EQS)
>                                // Access prior GP
>                                WRITE_ONCE(B, 1)
>                                // EQS enter
>                                // implied full barrier by atomic_add_return()
>                                atomic_add_return(RCU_DYNTICKS_IDX, EQS)
>                                // implied full barrier by atomic_add_return()
>                                READ_ONCE(A)
>     // second snapshot
>     y = smp_load_acquire(EQS)
>     z = READ_ONCE(B)
> 
> If the GP kthread above fails to observe the remote target in EQS
> (x not in EQS), the remote target will observe A == 1 after further
> entering in EQS. Then the second snapshot taken by the GP kthread only
> need to be an acquire read in order to observe z == 1.
> 
> Therefore remove the needless full memory barrier on second snapshot.
> 
> Signed-off-by: Frederic Weisbecker <frederic@kernel.org>
> Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
> ---
>  kernel/rcu/tree.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> index 28c7031711a3f..f07b8bff4621b 100644
> --- a/kernel/rcu/tree.c
> +++ b/kernel/rcu/tree.c
> @@ -321,7 +321,7 @@ static bool rcu_dynticks_in_eqs(int snap)
>   */
>  static bool rcu_dynticks_in_eqs_since(struct rcu_data *rdp, int snap)
>  {
> -	return snap != rcu_dynticks_snap(rdp->cpu);
> +	return snap != ct_dynticks_cpu_acquire(rdp->cpu);

I guess I'm going to add a comment here to elaborate on the fact
it relies on the ordering enforced before the first snapshot. Would
you prefer a delta patch or an updated patch?

Thanks.

>  }
>  
>  /*
> -- 
> 2.40.1
> 
> 

Re: [PATCH rcu 1/6] rcu: Remove full ordering on second EQS snapshot

[Paul E. McKenney]

On Wed, Jun 05, 2024 at 02:21:13PM +0200, Frederic Weisbecker wrote:
> Le Tue, Jun 04, 2024 at 03:26:47PM -0700, Paul E. McKenney a écrit :
> > From: Frederic Weisbecker <frederic@kernel.org>
> > 
> > When the grace period kthread checks the extended quiescent state
> > counter of a CPU, full ordering is necessary to ensure that either:
> > 
> > * If the GP kthread observes the remote target in an extended quiescent
> >   state, then that target must observe all accesses prior to the current
> >   grace period, including the current grace period sequence number, once
> >   it exits that extended quiescent state. Also the GP kthread must
> >   observe all accesses performed by the target prior it entering in
> >   EQS.
> > 
> > or:
> > 
> > * If the GP kthread observes the remote target NOT in an extended
> >   quiescent state, then the target further entering in an extended
> >   quiescent state must observe all accesses prior to the current
> >   grace period, including the current grace period sequence number, once
> >   it enters that extended quiescent state. Also the GP kthread later
> >   observing that EQS must also observe all accesses performed by the
> >   target prior it entering in EQS.
> > 
> > This ordering is explicitly performed both on the first EQS snapshot
> > and on the second one as well through the combination of a preceding
> > full barrier followed by an acquire read. However the second snapshot's
> > full memory barrier is redundant and not needed to enforce the above
> > guarantees:
> > 
> >     GP kthread                  Remote target
> >     ----                        -----
> >     // Access prior GP
> >     WRITE_ONCE(A, 1)
> >     // first snapshot
> >     smp_mb()
> >     x = smp_load_acquire(EQS)
> >                                // Access prior GP
> >                                WRITE_ONCE(B, 1)
> >                                // EQS enter
> >                                // implied full barrier by atomic_add_return()
> >                                atomic_add_return(RCU_DYNTICKS_IDX, EQS)
> >                                // implied full barrier by atomic_add_return()
> >                                READ_ONCE(A)
> >     // second snapshot
> >     y = smp_load_acquire(EQS)
> >     z = READ_ONCE(B)
> > 
> > If the GP kthread above fails to observe the remote target in EQS
> > (x not in EQS), the remote target will observe A == 1 after further
> > entering in EQS. Then the second snapshot taken by the GP kthread only
> > need to be an acquire read in order to observe z == 1.
> > 
> > Therefore remove the needless full memory barrier on second snapshot.
> > 
> > Signed-off-by: Frederic Weisbecker <frederic@kernel.org>
> > Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
> > ---
> >  kernel/rcu/tree.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> > index 28c7031711a3f..f07b8bff4621b 100644
> > --- a/kernel/rcu/tree.c
> > +++ b/kernel/rcu/tree.c
> > @@ -321,7 +321,7 @@ static bool rcu_dynticks_in_eqs(int snap)
> >   */
> >  static bool rcu_dynticks_in_eqs_since(struct rcu_data *rdp, int snap)
> >  {
> > -	return snap != rcu_dynticks_snap(rdp->cpu);
> > +	return snap != ct_dynticks_cpu_acquire(rdp->cpu);
> 
> I guess I'm going to add a comment here to elaborate on the fact
> it relies on the ordering enforced before the first snapshot. Would
> you prefer a delta patch or an updated patch?

Either works, just tell me which you are doing when you submit the patch.
Either way, I will arrange for there to be a single combined commit.

							Thanx, Paul

Re: [PATCH rcu 0/6] Grace-period memory-barrier adjustments for v6.11

[Boqun Feng]

On Tue, Jun 04, 2024 at 03:26:46PM -0700, Paul E. McKenney wrote:
> Hello!
> 
> This series removes redundant memory barriers from the grace-period
> code paths:
> 
> 1.	Remove full ordering on second EQS snapshot, courtesy of Frederic
> 	Weisbecker.
> 
> 2.	Remove superfluous full memory barrier upon first EQS snapshot,
> 	courtesy of Frederic Weisbecker.
> 
> 3.	rcu/exp: Remove superfluous full memory barrier upon first EQS
> 	snapshot, courtesy of Frederic Weisbecker.
> 
> 4.	Remove full memory barrier on boot time eqs sanity check,
> 	courtesy of Frederic Weisbecker.
> 
> 5.	Remove full memory barrier on RCU stall printout, courtesy of
> 	Frederic Weisbecker.
> 
> 6.	rcu/exp: Remove redundant full memory barrier at the end of GP,
> 	courtesy of Frederic Weisbecker.
> 

Reviewed-by: Boqun Feng <boqun.feng@gmail.com>

Regards,
Boqun

> 						Thanx, Paul
> 
> ------------------------------------------------------------------------
> 
>  b/Documentation/RCU/Design/Memory-Ordering/Tree-RCU-Memory-Ordering.rst |    6 +--
>  b/kernel/rcu/tree.c                                                     |    2 -
>  b/kernel/rcu/tree_exp.h                                                 |    8 +++-
>  b/kernel/rcu/tree_stall.h                                               |    4 +-
>  kernel/rcu/tree.c                                                       |   19 +++-------
>  kernel/rcu/tree_exp.h                                                   |    8 +++-
>  6 files changed, 26 insertions(+), 21 deletions(-)
> 

Re: [PATCH rcu 2/6] rcu: Remove superfluous full memory barrier upon first EQS snapshot

[Neeraj upadhyay]

On Wed, Jun 5, 2024 at 3:58 AM Paul E. McKenney <paulmck@kernel.org> wrote:
>
> From: Frederic Weisbecker <frederic@kernel.org>
>
> When the grace period kthread checks the extended quiescent state
> counter of a CPU, full ordering is necessary to ensure that either:
>
> * If the GP kthread observes the remote target in an extended quiescent
>   state, then that target must observe all accesses prior to the current
>   grace period, including the current grace period sequence number, once
>   it exits that extended quiescent state.
>
> or:
>
> * If the GP kthread observes the remote target NOT in an extended
>   quiescent state, then the target further entering in an extended
>   quiescent state must observe all accesses prior to the current
>   grace period, including the current grace period sequence number, once
>   it enters that extended quiescent state.
>
> This ordering is enforced through a full memory barrier placed right
> before taking the first EQS snapshot. However this is superfluous
> because the snapshot is taken while holding the target's rnp lock which
> provides the necessary ordering through its chain of
> smp_mb__after_unlock_lock().
>
> Remove the needless explicit barrier before the snapshot and put a
> comment about the implicit barrier newly relied upon here.
>
> Signed-off-by: Frederic Weisbecker <frederic@kernel.org>
> Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
> ---
>  .../Design/Memory-Ordering/Tree-RCU-Memory-Ordering.rst    | 6 +++---
>  kernel/rcu/tree.c                                          | 7 ++++++-
>  2 files changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/RCU/Design/Memory-Ordering/Tree-RCU-Memory-Ordering.rst b/Documentation/RCU/Design/Memory-Ordering/Tree-RCU-Memory-Ordering.rst
> index 5750f125361b0..728b1e690c646 100644
> --- a/Documentation/RCU/Design/Memory-Ordering/Tree-RCU-Memory-Ordering.rst
> +++ b/Documentation/RCU/Design/Memory-Ordering/Tree-RCU-Memory-Ordering.rst
> @@ -149,9 +149,9 @@ This case is handled by calls to the strongly ordered
>  ``atomic_add_return()`` read-modify-write atomic operation that
>  is invoked within ``rcu_dynticks_eqs_enter()`` at idle-entry
>  time and within ``rcu_dynticks_eqs_exit()`` at idle-exit time.
> -The grace-period kthread invokes ``rcu_dynticks_snap()`` and
> -``rcu_dynticks_in_eqs_since()`` (both of which invoke
> -an ``atomic_add_return()`` of zero) to detect idle CPUs.
> +The grace-period kthread invokes first ``ct_dynticks_cpu_acquire()``
> +(preceded by a full memory barrier) and ``rcu_dynticks_in_eqs_since()``
> +(both of which rely on acquire semantics) to detect idle CPUs.
>
>  +-----------------------------------------------------------------------+
>  | **Quick Quiz**:                                                       |
> diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> index f07b8bff4621b..1a6ef9c5c949e 100644
> --- a/kernel/rcu/tree.c
> +++ b/kernel/rcu/tree.c
> @@ -769,7 +769,12 @@ static void rcu_gpnum_ovf(struct rcu_node *rnp, struct rcu_data *rdp)
>   */
>  static int dyntick_save_progress_counter(struct rcu_data *rdp)
>  {
> -       rdp->dynticks_snap = rcu_dynticks_snap(rdp->cpu);
> +       /*
> +        * Full ordering against accesses prior current GP and also against
> +        * current GP sequence number is enforced by current rnp locking
> +        * with chained smp_mb__after_unlock_lock().
> +        */

It might be worth mentioning that this chained smp_mb__after_unlock_lock()
is provided by rnp leaf node locking in rcu_gp_init() and rcu_gp_fqs_loop() ?


Thanks
Neeraj

> +       rdp->dynticks_snap = ct_dynticks_cpu_acquire(rdp->cpu);
>         if (rcu_dynticks_in_eqs(rdp->dynticks_snap)) {
>                 trace_rcu_fqs(rcu_state.name, rdp->gp_seq, rdp->cpu, TPS("dti"));
>                 rcu_gpnum_ovf(rdp->mynode, rdp);
> --
> 2.40.1
>
>

Re: [PATCH rcu 3/6] rcu/exp: Remove superfluous full memory barrier upon first EQS snapshot

[Neeraj upadhyay]

On Wed, Jun 5, 2024 at 3:58 AM Paul E. McKenney <paulmck@kernel.org> wrote:
>
> From: Frederic Weisbecker <frederic@kernel.org>
>
> When the grace period kthread checks the extended quiescent state
> counter of a CPU, full ordering is necessary to ensure that either:
>
> * If the GP kthread observes the remote target in an extended quiescent
>   state, then that target must observe all accesses prior to the current
>   grace period, including the current grace period sequence number, once
>   it exits that extended quiescent state.
>
> or:
>
> * If the GP kthread observes the remote target NOT in an extended
>   quiescent state, then the target further entering in an extended
>   quiescent state must observe all accesses prior to the current
>   grace period, including the current grace period sequence number, once
>   it enters that extended quiescent state.
>
> This ordering is enforced through a full memory barrier placed right
> before taking the first EQS snapshot. However this is superfluous
> because the snapshot is taken while holding the target's rnp lock which
> provides the necessary ordering through its chain of
> smp_mb__after_unlock_lock().
>
> Remove the needless explicit barrier before the snapshot and put a
> comment about the implicit barrier newly relied upon here.
>
> Signed-off-by: Frederic Weisbecker <frederic@kernel.org>
> Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
> ---
>  kernel/rcu/tree_exp.h | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/rcu/tree_exp.h b/kernel/rcu/tree_exp.h
> index 8a1d9c8bd9f74..bec24ea6777e8 100644
> --- a/kernel/rcu/tree_exp.h
> +++ b/kernel/rcu/tree_exp.h
> @@ -357,7 +357,13 @@ static void __sync_rcu_exp_select_node_cpus(struct rcu_exp_work *rewp)
>                     !(rnp->qsmaskinitnext & mask)) {
>                         mask_ofl_test |= mask;
>                 } else {
> -                       snap = rcu_dynticks_snap(cpu);
> +                       /*
> +                        * Full ordering against accesses prior current GP and
> +                        * also against current GP sequence number is enforced
> +                        * by current rnp locking with chained
> +                        * smp_mb__after_unlock_lock().

Again, worth mentioning the chaining sites sync_exp_reset_tree() and
this function?


Thanks
Neeraj

> +                        */
> +                       snap = ct_dynticks_cpu_acquire(cpu);
>                         if (rcu_dynticks_in_eqs(snap))
>                                 mask_ofl_test |= mask;
>                         else
> --
> 2.40.1
>
>

Re: [PATCH rcu 0/6] Grace-period memory-barrier adjustments for v6.11

[Neeraj Upadhyay]

On Tue, Jun 04, 2024 at 03:26:46PM -0700, Paul E. McKenney wrote:
> Hello!
> 
> This series removes redundant memory barriers from the grace-period
> code paths:
> 
> 1.	Remove full ordering on second EQS snapshot, courtesy of Frederic
> 	Weisbecker.
> 
> 2.	Remove superfluous full memory barrier upon first EQS snapshot,
> 	courtesy of Frederic Weisbecker.
> 
> 3.	rcu/exp: Remove superfluous full memory barrier upon first EQS
> 	snapshot, courtesy of Frederic Weisbecker.
> 
> 4.	Remove full memory barrier on boot time eqs sanity check,
> 	courtesy of Frederic Weisbecker.
> 
> 5.	Remove full memory barrier on RCU stall printout, courtesy of
> 	Frederic Weisbecker.
> 
> 6.	rcu/exp: Remove redundant full memory barrier at the end of GP,
> 	courtesy of Frederic Weisbecker.
> 

Reviewed-by: Neeraj Upadhyay <neeraj.upadhyay@kernel.org>

> 						Thanx, Paul
> 
> ------------------------------------------------------------------------
> 
>  b/Documentation/RCU/Design/Memory-Ordering/Tree-RCU-Memory-Ordering.rst |    6 +--
>  b/kernel/rcu/tree.c                                                     |    2 -
>  b/kernel/rcu/tree_exp.h                                                 |    8 +++-
>  b/kernel/rcu/tree_stall.h                                               |    4 +-
>  kernel/rcu/tree.c                                                       |   19 +++-------
>  kernel/rcu/tree_exp.h                                                   |    8 +++-
>  6 files changed, 26 insertions(+), 21 deletions(-)

Re: [PATCH rcu 3/6] rcu/exp: Remove superfluous full memory barrier upon first EQS snapshot

[Paul E. McKenney]

On Wed, Jun 12, 2024 at 02:14:14PM +0530, Neeraj upadhyay wrote:
> On Wed, Jun 5, 2024 at 3:58 AM Paul E. McKenney <paulmck@kernel.org> wrote:
> >
> > From: Frederic Weisbecker <frederic@kernel.org>
> >
> > When the grace period kthread checks the extended quiescent state
> > counter of a CPU, full ordering is necessary to ensure that either:
> >
> > * If the GP kthread observes the remote target in an extended quiescent
> >   state, then that target must observe all accesses prior to the current
> >   grace period, including the current grace period sequence number, once
> >   it exits that extended quiescent state.
> >
> > or:
> >
> > * If the GP kthread observes the remote target NOT in an extended
> >   quiescent state, then the target further entering in an extended
> >   quiescent state must observe all accesses prior to the current
> >   grace period, including the current grace period sequence number, once
> >   it enters that extended quiescent state.
> >
> > This ordering is enforced through a full memory barrier placed right
> > before taking the first EQS snapshot. However this is superfluous
> > because the snapshot is taken while holding the target's rnp lock which
> > provides the necessary ordering through its chain of
> > smp_mb__after_unlock_lock().
> >
> > Remove the needless explicit barrier before the snapshot and put a
> > comment about the implicit barrier newly relied upon here.
> >
> > Signed-off-by: Frederic Weisbecker <frederic@kernel.org>
> > Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
> > ---
> >  kernel/rcu/tree_exp.h | 8 +++++++-
> >  1 file changed, 7 insertions(+), 1 deletion(-)
> >
> > diff --git a/kernel/rcu/tree_exp.h b/kernel/rcu/tree_exp.h
> > index 8a1d9c8bd9f74..bec24ea6777e8 100644
> > --- a/kernel/rcu/tree_exp.h
> > +++ b/kernel/rcu/tree_exp.h
> > @@ -357,7 +357,13 @@ static void __sync_rcu_exp_select_node_cpus(struct rcu_exp_work *rewp)
> >                     !(rnp->qsmaskinitnext & mask)) {
> >                         mask_ofl_test |= mask;
> >                 } else {
> > -                       snap = rcu_dynticks_snap(cpu);
> > +                       /*
> > +                        * Full ordering against accesses prior current GP and
> > +                        * also against current GP sequence number is enforced
> > +                        * by current rnp locking with chained
> > +                        * smp_mb__after_unlock_lock().
> 
> Again, worth mentioning the chaining sites sync_exp_reset_tree() and
> this function?

It might well be in both cases.  Could you and Frederic propose
agreed-upon appropriate changes (including the null change, if
appropriate)?

							Thanx, Paul

> Thanks
> Neeraj
> 
> > +                        */
> > +                       snap = ct_dynticks_cpu_acquire(cpu);
> >                         if (rcu_dynticks_in_eqs(snap))
> >                                 mask_ofl_test |= mask;
> >                         else
> > --
> > 2.40.1
> >
> >

Re: [PATCH rcu 0/6] Grace-period memory-barrier adjustments for v6.11

[Paul E. McKenney]

On Wed, Jun 12, 2024 at 03:12:07PM +0530, Neeraj Upadhyay wrote:
> On Tue, Jun 04, 2024 at 03:26:46PM -0700, Paul E. McKenney wrote:
> > Hello!
> > 
> > This series removes redundant memory barriers from the grace-period
> > code paths:
> > 
> > 1.	Remove full ordering on second EQS snapshot, courtesy of Frederic
> > 	Weisbecker.
> > 
> > 2.	Remove superfluous full memory barrier upon first EQS snapshot,
> > 	courtesy of Frederic Weisbecker.
> > 
> > 3.	rcu/exp: Remove superfluous full memory barrier upon first EQS
> > 	snapshot, courtesy of Frederic Weisbecker.
> > 
> > 4.	Remove full memory barrier on boot time eqs sanity check,
> > 	courtesy of Frederic Weisbecker.
> > 
> > 5.	Remove full memory barrier on RCU stall printout, courtesy of
> > 	Frederic Weisbecker.
> > 
> > 6.	rcu/exp: Remove redundant full memory barrier at the end of GP,
> > 	courtesy of Frederic Weisbecker.
> > 
> 
> Reviewed-by: Neeraj Upadhyay <neeraj.upadhyay@kernel.org>

I will apply yours and Boqun's tag on the next rebase, thank you both!

						Thanx, Paul

> > ------------------------------------------------------------------------
> > 
> >  b/Documentation/RCU/Design/Memory-Ordering/Tree-RCU-Memory-Ordering.rst |    6 +--
> >  b/kernel/rcu/tree.c                                                     |    2 -
> >  b/kernel/rcu/tree_exp.h                                                 |    8 +++-
> >  b/kernel/rcu/tree_stall.h                                               |    4 +-
> >  kernel/rcu/tree.c                                                       |   19 +++-------
> >  kernel/rcu/tree_exp.h                                                   |    8 +++-
> >  6 files changed, 26 insertions(+), 21 deletions(-)

Re: [PATCH rcu 2/6] rcu: Remove superfluous full memory barrier upon first EQS snapshot

[Frederic Weisbecker]

Le Wed, Jun 12, 2024 at 01:57:20PM +0530, Neeraj upadhyay a écrit :
> On Wed, Jun 5, 2024 at 3:58 AM Paul E. McKenney <paulmck@kernel.org> wrote:
> >
> > From: Frederic Weisbecker <frederic@kernel.org>
> >
> > When the grace period kthread checks the extended quiescent state
> > counter of a CPU, full ordering is necessary to ensure that either:
> >
> > * If the GP kthread observes the remote target in an extended quiescent
> >   state, then that target must observe all accesses prior to the current
> >   grace period, including the current grace period sequence number, once
> >   it exits that extended quiescent state.
> >
> > or:
> >
> > * If the GP kthread observes the remote target NOT in an extended
> >   quiescent state, then the target further entering in an extended
> >   quiescent state must observe all accesses prior to the current
> >   grace period, including the current grace period sequence number, once
> >   it enters that extended quiescent state.
> >
> > This ordering is enforced through a full memory barrier placed right
> > before taking the first EQS snapshot. However this is superfluous
> > because the snapshot is taken while holding the target's rnp lock which
> > provides the necessary ordering through its chain of
> > smp_mb__after_unlock_lock().
> >
> > Remove the needless explicit barrier before the snapshot and put a
> > comment about the implicit barrier newly relied upon here.
> >
> > Signed-off-by: Frederic Weisbecker <frederic@kernel.org>
> > Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
> > ---
> >  .../Design/Memory-Ordering/Tree-RCU-Memory-Ordering.rst    | 6 +++---
> >  kernel/rcu/tree.c                                          | 7 ++++++-
> >  2 files changed, 9 insertions(+), 4 deletions(-)
> >
> > diff --git a/Documentation/RCU/Design/Memory-Ordering/Tree-RCU-Memory-Ordering.rst b/Documentation/RCU/Design/Memory-Ordering/Tree-RCU-Memory-Ordering.rst
> > index 5750f125361b0..728b1e690c646 100644
> > --- a/Documentation/RCU/Design/Memory-Ordering/Tree-RCU-Memory-Ordering.rst
> > +++ b/Documentation/RCU/Design/Memory-Ordering/Tree-RCU-Memory-Ordering.rst
> > @@ -149,9 +149,9 @@ This case is handled by calls to the strongly ordered
> >  ``atomic_add_return()`` read-modify-write atomic operation that
> >  is invoked within ``rcu_dynticks_eqs_enter()`` at idle-entry
> >  time and within ``rcu_dynticks_eqs_exit()`` at idle-exit time.
> > -The grace-period kthread invokes ``rcu_dynticks_snap()`` and
> > -``rcu_dynticks_in_eqs_since()`` (both of which invoke
> > -an ``atomic_add_return()`` of zero) to detect idle CPUs.
> > +The grace-period kthread invokes first ``ct_dynticks_cpu_acquire()``
> > +(preceded by a full memory barrier) and ``rcu_dynticks_in_eqs_since()``
> > +(both of which rely on acquire semantics) to detect idle CPUs.
> >
> >  +-----------------------------------------------------------------------+
> >  | **Quick Quiz**:                                                       |
> > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> > index f07b8bff4621b..1a6ef9c5c949e 100644
> > --- a/kernel/rcu/tree.c
> > +++ b/kernel/rcu/tree.c
> > @@ -769,7 +769,12 @@ static void rcu_gpnum_ovf(struct rcu_node *rnp, struct rcu_data *rdp)
> >   */
> >  static int dyntick_save_progress_counter(struct rcu_data *rdp)
> >  {
> > -       rdp->dynticks_snap = rcu_dynticks_snap(rdp->cpu);
> > +       /*
> > +        * Full ordering against accesses prior current GP and also against
> > +        * current GP sequence number is enforced by current rnp locking
> > +        * with chained smp_mb__after_unlock_lock().
> > +        */
> 
> It might be worth mentioning that this chained smp_mb__after_unlock_lock()
> is provided by rnp leaf node locking in rcu_gp_init() and rcu_gp_fqs_loop() ?

Right!

How about this?

    /*
     * Full ordering against accesses prior current GP and also against
     * current GP sequence number is enforced by rcu_seq_start() implicit
     * barrier and even further by smp_mb__after_unlock_lock() barriers
     * chained all the way throughout the rnp locking tree since rcu_gp_init()
     * and up to the current leaf rnp locking.
     */

Thanks.

Re: [PATCH rcu 3/6] rcu/exp: Remove superfluous full memory barrier upon first EQS snapshot

[Frederic Weisbecker]

Le Wed, Jun 12, 2024 at 02:14:14PM +0530, Neeraj upadhyay a écrit :
> On Wed, Jun 5, 2024 at 3:58 AM Paul E. McKenney <paulmck@kernel.org> wrote:
> >
> > From: Frederic Weisbecker <frederic@kernel.org>
> >
> > When the grace period kthread checks the extended quiescent state
> > counter of a CPU, full ordering is necessary to ensure that either:
> >
> > * If the GP kthread observes the remote target in an extended quiescent
> >   state, then that target must observe all accesses prior to the current
> >   grace period, including the current grace period sequence number, once
> >   it exits that extended quiescent state.
> >
> > or:
> >
> > * If the GP kthread observes the remote target NOT in an extended
> >   quiescent state, then the target further entering in an extended
> >   quiescent state must observe all accesses prior to the current
> >   grace period, including the current grace period sequence number, once
> >   it enters that extended quiescent state.
> >
> > This ordering is enforced through a full memory barrier placed right
> > before taking the first EQS snapshot. However this is superfluous
> > because the snapshot is taken while holding the target's rnp lock which
> > provides the necessary ordering through its chain of
> > smp_mb__after_unlock_lock().
> >
> > Remove the needless explicit barrier before the snapshot and put a
> > comment about the implicit barrier newly relied upon here.
> >
> > Signed-off-by: Frederic Weisbecker <frederic@kernel.org>
> > Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
> > ---
> >  kernel/rcu/tree_exp.h | 8 +++++++-
> >  1 file changed, 7 insertions(+), 1 deletion(-)
> >
> > diff --git a/kernel/rcu/tree_exp.h b/kernel/rcu/tree_exp.h
> > index 8a1d9c8bd9f74..bec24ea6777e8 100644
> > --- a/kernel/rcu/tree_exp.h
> > +++ b/kernel/rcu/tree_exp.h
> > @@ -357,7 +357,13 @@ static void __sync_rcu_exp_select_node_cpus(struct rcu_exp_work *rewp)
> >                     !(rnp->qsmaskinitnext & mask)) {
> >                         mask_ofl_test |= mask;
> >                 } else {
> > -                       snap = rcu_dynticks_snap(cpu);
> > +                       /*
> > +                        * Full ordering against accesses prior current GP and
> > +                        * also against current GP sequence number is enforced
> > +                        * by current rnp locking with chained
> > +                        * smp_mb__after_unlock_lock().
> 
> Again, worth mentioning the chaining sites sync_exp_reset_tree() and
> this function?

How about this?

    /*
     * Full ordering against accesses prior current GP and also against
     * current GP sequence number is enforced by rcu_seq_start() implicit
     * barrier, relayed by kworkers locking and even further by
     * smp_mb__after_unlock_lock() barriers chained all the way throughout
     * the rnp locking tree since sync_exp_reset_tree() and up to the current
     * leaf rnp locking.
     */

Thanks.

> 
> 
> Thanks
> Neeraj
> 
> > +                        */
> > +                       snap = ct_dynticks_cpu_acquire(cpu);
> >                         if (rcu_dynticks_in_eqs(snap))
> >                                 mask_ofl_test |= mask;
> >                         else
> > --
> > 2.40.1
> >
> >

Re: [PATCH rcu 1/6] rcu: Remove full ordering on second EQS snapshot

[Frederic Weisbecker]

Le Wed, Jun 05, 2024 at 11:44:42AM -0700, Paul E. McKenney a écrit :
> On Wed, Jun 05, 2024 at 02:21:13PM +0200, Frederic Weisbecker wrote:
> > Le Tue, Jun 04, 2024 at 03:26:47PM -0700, Paul E. McKenney a écrit :
> > > From: Frederic Weisbecker <frederic@kernel.org>
> > > 
> > > When the grace period kthread checks the extended quiescent state
> > > counter of a CPU, full ordering is necessary to ensure that either:
> > > 
> > > * If the GP kthread observes the remote target in an extended quiescent
> > >   state, then that target must observe all accesses prior to the current
> > >   grace period, including the current grace period sequence number, once
> > >   it exits that extended quiescent state. Also the GP kthread must
> > >   observe all accesses performed by the target prior it entering in
> > >   EQS.
> > > 
> > > or:
> > > 
> > > * If the GP kthread observes the remote target NOT in an extended
> > >   quiescent state, then the target further entering in an extended
> > >   quiescent state must observe all accesses prior to the current
> > >   grace period, including the current grace period sequence number, once
> > >   it enters that extended quiescent state. Also the GP kthread later
> > >   observing that EQS must also observe all accesses performed by the
> > >   target prior it entering in EQS.
> > > 
> > > This ordering is explicitly performed both on the first EQS snapshot
> > > and on the second one as well through the combination of a preceding
> > > full barrier followed by an acquire read. However the second snapshot's
> > > full memory barrier is redundant and not needed to enforce the above
> > > guarantees:
> > > 
> > >     GP kthread                  Remote target
> > >     ----                        -----
> > >     // Access prior GP
> > >     WRITE_ONCE(A, 1)
> > >     // first snapshot
> > >     smp_mb()
> > >     x = smp_load_acquire(EQS)
> > >                                // Access prior GP
> > >                                WRITE_ONCE(B, 1)
> > >                                // EQS enter
> > >                                // implied full barrier by atomic_add_return()
> > >                                atomic_add_return(RCU_DYNTICKS_IDX, EQS)
> > >                                // implied full barrier by atomic_add_return()
> > >                                READ_ONCE(A)
> > >     // second snapshot
> > >     y = smp_load_acquire(EQS)
> > >     z = READ_ONCE(B)
> > > 
> > > If the GP kthread above fails to observe the remote target in EQS
> > > (x not in EQS), the remote target will observe A == 1 after further
> > > entering in EQS. Then the second snapshot taken by the GP kthread only
> > > need to be an acquire read in order to observe z == 1.
> > > 
> > > Therefore remove the needless full memory barrier on second snapshot.
> > > 
> > > Signed-off-by: Frederic Weisbecker <frederic@kernel.org>
> > > Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
> > > ---
> > >  kernel/rcu/tree.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> > > index 28c7031711a3f..f07b8bff4621b 100644
> > > --- a/kernel/rcu/tree.c
> > > +++ b/kernel/rcu/tree.c
> > > @@ -321,7 +321,7 @@ static bool rcu_dynticks_in_eqs(int snap)
> > >   */
> > >  static bool rcu_dynticks_in_eqs_since(struct rcu_data *rdp, int snap)
> > >  {
> > > -	return snap != rcu_dynticks_snap(rdp->cpu);
> > > +	return snap != ct_dynticks_cpu_acquire(rdp->cpu);
> > 
> > I guess I'm going to add a comment here to elaborate on the fact
> > it relies on the ordering enforced before the first snapshot. Would
> > you prefer a delta patch or an updated patch?
> 
> Either works, just tell me which you are doing when you submit the patch.
> Either way, I will arrange for there to be a single combined commit.

Ok before I resend, how does the following comment look like?

/*
 * The first failing snapshot is already ordered against the accesses
 * performed by the remote CPU after it exiting idle.
 *
 * The second snapshot therefore only needs to order against accesses
 * performed by the remote CPU prior it entering idle and therefore can
 * solely on acquire semantics.
 */

Thanks.

> 
> 							Thanx, Paul
> 

Re: [PATCH rcu 1/6] rcu: Remove full ordering on second EQS snapshot

[Paul E. McKenney]

On Wed, Jun 26, 2024 at 05:03:02PM +0200, Frederic Weisbecker wrote:
> Le Wed, Jun 05, 2024 at 11:44:42AM -0700, Paul E. McKenney a écrit :
> > On Wed, Jun 05, 2024 at 02:21:13PM +0200, Frederic Weisbecker wrote:
> > > Le Tue, Jun 04, 2024 at 03:26:47PM -0700, Paul E. McKenney a écrit :
> > > > From: Frederic Weisbecker <frederic@kernel.org>
> > > > 
> > > > When the grace period kthread checks the extended quiescent state
> > > > counter of a CPU, full ordering is necessary to ensure that either:
> > > > 
> > > > * If the GP kthread observes the remote target in an extended quiescent
> > > >   state, then that target must observe all accesses prior to the current
> > > >   grace period, including the current grace period sequence number, once
> > > >   it exits that extended quiescent state. Also the GP kthread must
> > > >   observe all accesses performed by the target prior it entering in
> > > >   EQS.
> > > > 
> > > > or:
> > > > 
> > > > * If the GP kthread observes the remote target NOT in an extended
> > > >   quiescent state, then the target further entering in an extended
> > > >   quiescent state must observe all accesses prior to the current
> > > >   grace period, including the current grace period sequence number, once
> > > >   it enters that extended quiescent state. Also the GP kthread later
> > > >   observing that EQS must also observe all accesses performed by the
> > > >   target prior it entering in EQS.
> > > > 
> > > > This ordering is explicitly performed both on the first EQS snapshot
> > > > and on the second one as well through the combination of a preceding
> > > > full barrier followed by an acquire read. However the second snapshot's
> > > > full memory barrier is redundant and not needed to enforce the above
> > > > guarantees:
> > > > 
> > > >     GP kthread                  Remote target
> > > >     ----                        -----
> > > >     // Access prior GP
> > > >     WRITE_ONCE(A, 1)
> > > >     // first snapshot
> > > >     smp_mb()
> > > >     x = smp_load_acquire(EQS)
> > > >                                // Access prior GP
> > > >                                WRITE_ONCE(B, 1)
> > > >                                // EQS enter
> > > >                                // implied full barrier by atomic_add_return()
> > > >                                atomic_add_return(RCU_DYNTICKS_IDX, EQS)
> > > >                                // implied full barrier by atomic_add_return()
> > > >                                READ_ONCE(A)
> > > >     // second snapshot
> > > >     y = smp_load_acquire(EQS)
> > > >     z = READ_ONCE(B)
> > > > 
> > > > If the GP kthread above fails to observe the remote target in EQS
> > > > (x not in EQS), the remote target will observe A == 1 after further
> > > > entering in EQS. Then the second snapshot taken by the GP kthread only
> > > > need to be an acquire read in order to observe z == 1.
> > > > 
> > > > Therefore remove the needless full memory barrier on second snapshot.
> > > > 
> > > > Signed-off-by: Frederic Weisbecker <frederic@kernel.org>
> > > > Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
> > > > ---
> > > >  kernel/rcu/tree.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> > > > index 28c7031711a3f..f07b8bff4621b 100644
> > > > --- a/kernel/rcu/tree.c
> > > > +++ b/kernel/rcu/tree.c
> > > > @@ -321,7 +321,7 @@ static bool rcu_dynticks_in_eqs(int snap)
> > > >   */
> > > >  static bool rcu_dynticks_in_eqs_since(struct rcu_data *rdp, int snap)
> > > >  {
> > > > -	return snap != rcu_dynticks_snap(rdp->cpu);
> > > > +	return snap != ct_dynticks_cpu_acquire(rdp->cpu);
> > > 
> > > I guess I'm going to add a comment here to elaborate on the fact
> > > it relies on the ordering enforced before the first snapshot. Would
> > > you prefer a delta patch or an updated patch?
> > 
> > Either works, just tell me which you are doing when you submit the patch.
> > Either way, I will arrange for there to be a single combined commit.
> 
> Ok before I resend, how does the following comment look like?
> 
> /*
>  * The first failing snapshot is already ordered against the accesses
>  * performed by the remote CPU after it exiting idle.

s/exiting/exits/

>  * The second snapshot therefore only needs to order against accesses
>  * performed by the remote CPU prior it entering idle and therefore can
>  * solely on acquire semantics.
>  */

s/prior it entering/prior to entering/
s/solely/rely solely/

Other than those nits, looks good to me!  

								Thanx, Paul

Re: [PATCH rcu 1/6] rcu: Remove full ordering on second EQS snapshot

[Frederic Weisbecker]

Le Wed, Jun 26, 2024 at 08:32:45AM -0700, Paul E. McKenney a écrit :
> On Wed, Jun 26, 2024 at 05:03:02PM +0200, Frederic Weisbecker wrote:
> > Le Wed, Jun 05, 2024 at 11:44:42AM -0700, Paul E. McKenney a écrit :
> > > On Wed, Jun 05, 2024 at 02:21:13PM +0200, Frederic Weisbecker wrote:
> > > > Le Tue, Jun 04, 2024 at 03:26:47PM -0700, Paul E. McKenney a écrit :
> > > > > From: Frederic Weisbecker <frederic@kernel.org>
> > > > > 
> > > > > When the grace period kthread checks the extended quiescent state
> > > > > counter of a CPU, full ordering is necessary to ensure that either:
> > > > > 
> > > > > * If the GP kthread observes the remote target in an extended quiescent
> > > > >   state, then that target must observe all accesses prior to the current
> > > > >   grace period, including the current grace period sequence number, once
> > > > >   it exits that extended quiescent state. Also the GP kthread must
> > > > >   observe all accesses performed by the target prior it entering in
> > > > >   EQS.
> > > > > 
> > > > > or:
> > > > > 
> > > > > * If the GP kthread observes the remote target NOT in an extended
> > > > >   quiescent state, then the target further entering in an extended
> > > > >   quiescent state must observe all accesses prior to the current
> > > > >   grace period, including the current grace period sequence number, once
> > > > >   it enters that extended quiescent state. Also the GP kthread later
> > > > >   observing that EQS must also observe all accesses performed by the
> > > > >   target prior it entering in EQS.
> > > > > 
> > > > > This ordering is explicitly performed both on the first EQS snapshot
> > > > > and on the second one as well through the combination of a preceding
> > > > > full barrier followed by an acquire read. However the second snapshot's
> > > > > full memory barrier is redundant and not needed to enforce the above
> > > > > guarantees:
> > > > > 
> > > > >     GP kthread                  Remote target
> > > > >     ----                        -----
> > > > >     // Access prior GP
> > > > >     WRITE_ONCE(A, 1)
> > > > >     // first snapshot
> > > > >     smp_mb()
> > > > >     x = smp_load_acquire(EQS)
> > > > >                                // Access prior GP
> > > > >                                WRITE_ONCE(B, 1)
> > > > >                                // EQS enter
> > > > >                                // implied full barrier by atomic_add_return()
> > > > >                                atomic_add_return(RCU_DYNTICKS_IDX, EQS)
> > > > >                                // implied full barrier by atomic_add_return()
> > > > >                                READ_ONCE(A)
> > > > >     // second snapshot
> > > > >     y = smp_load_acquire(EQS)
> > > > >     z = READ_ONCE(B)
> > > > > 
> > > > > If the GP kthread above fails to observe the remote target in EQS
> > > > > (x not in EQS), the remote target will observe A == 1 after further
> > > > > entering in EQS. Then the second snapshot taken by the GP kthread only
> > > > > need to be an acquire read in order to observe z == 1.
> > > > > 
> > > > > Therefore remove the needless full memory barrier on second snapshot.
> > > > > 
> > > > > Signed-off-by: Frederic Weisbecker <frederic@kernel.org>
> > > > > Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
> > > > > ---
> > > > >  kernel/rcu/tree.c | 2 +-
> > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> > > > > index 28c7031711a3f..f07b8bff4621b 100644
> > > > > --- a/kernel/rcu/tree.c
> > > > > +++ b/kernel/rcu/tree.c
> > > > > @@ -321,7 +321,7 @@ static bool rcu_dynticks_in_eqs(int snap)
> > > > >   */
> > > > >  static bool rcu_dynticks_in_eqs_since(struct rcu_data *rdp, int snap)
> > > > >  {
> > > > > -	return snap != rcu_dynticks_snap(rdp->cpu);
> > > > > +	return snap != ct_dynticks_cpu_acquire(rdp->cpu);
> > > > 
> > > > I guess I'm going to add a comment here to elaborate on the fact
> > > > it relies on the ordering enforced before the first snapshot. Would
> > > > you prefer a delta patch or an updated patch?
> > > 
> > > Either works, just tell me which you are doing when you submit the patch.
> > > Either way, I will arrange for there to be a single combined commit.
> > 
> > Ok before I resend, how does the following comment look like?
> > 
> > /*
> >  * The first failing snapshot is already ordered against the accesses
> >  * performed by the remote CPU after it exiting idle.
> 
> s/exiting/exits/
> 
> >  * The second snapshot therefore only needs to order against accesses
> >  * performed by the remote CPU prior it entering idle and therefore can
> >  * solely on acquire semantics.
> >  */
> 
> s/prior it entering/prior to entering/
> s/solely/rely solely/
> 
> Other than those nits, looks good to me!

Thanks a lot!

I'll resend with these changes.

> 
> 								Thanx, Paul

Re: [PATCH rcu 2/6] rcu: Remove superfluous full memory barrier upon first EQS snapshot

[Neeraj upadhyay]

On Wed, Jun 26, 2024 at 7:43 PM Frederic Weisbecker <frederic@kernel.org> wrote:
>
> Le Wed, Jun 12, 2024 at 01:57:20PM +0530, Neeraj upadhyay a écrit :
> > On Wed, Jun 5, 2024 at 3:58 AM Paul E. McKenney <paulmck@kernel.org> wrote:
> > >
> > > From: Frederic Weisbecker <frederic@kernel.org>
> > >
> > > When the grace period kthread checks the extended quiescent state
> > > counter of a CPU, full ordering is necessary to ensure that either:
> > >
> > > * If the GP kthread observes the remote target in an extended quiescent
> > >   state, then that target must observe all accesses prior to the current
> > >   grace period, including the current grace period sequence number, once
> > >   it exits that extended quiescent state.
> > >
> > > or:
> > >
> > > * If the GP kthread observes the remote target NOT in an extended
> > >   quiescent state, then the target further entering in an extended
> > >   quiescent state must observe all accesses prior to the current
> > >   grace period, including the current grace period sequence number, once
> > >   it enters that extended quiescent state.
> > >
> > > This ordering is enforced through a full memory barrier placed right
> > > before taking the first EQS snapshot. However this is superfluous
> > > because the snapshot is taken while holding the target's rnp lock which
> > > provides the necessary ordering through its chain of
> > > smp_mb__after_unlock_lock().
> > >
> > > Remove the needless explicit barrier before the snapshot and put a
> > > comment about the implicit barrier newly relied upon here.
> > >
> > > Signed-off-by: Frederic Weisbecker <frederic@kernel.org>
> > > Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
> > > ---
> > >  .../Design/Memory-Ordering/Tree-RCU-Memory-Ordering.rst    | 6 +++---
> > >  kernel/rcu/tree.c                                          | 7 ++++++-
> > >  2 files changed, 9 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/Documentation/RCU/Design/Memory-Ordering/Tree-RCU-Memory-Ordering.rst b/Documentation/RCU/Design/Memory-Ordering/Tree-RCU-Memory-Ordering.rst
> > > index 5750f125361b0..728b1e690c646 100644
> > > --- a/Documentation/RCU/Design/Memory-Ordering/Tree-RCU-Memory-Ordering.rst
> > > +++ b/Documentation/RCU/Design/Memory-Ordering/Tree-RCU-Memory-Ordering.rst
> > > @@ -149,9 +149,9 @@ This case is handled by calls to the strongly ordered
> > >  ``atomic_add_return()`` read-modify-write atomic operation that
> > >  is invoked within ``rcu_dynticks_eqs_enter()`` at idle-entry
> > >  time and within ``rcu_dynticks_eqs_exit()`` at idle-exit time.
> > > -The grace-period kthread invokes ``rcu_dynticks_snap()`` and
> > > -``rcu_dynticks_in_eqs_since()`` (both of which invoke
> > > -an ``atomic_add_return()`` of zero) to detect idle CPUs.
> > > +The grace-period kthread invokes first ``ct_dynticks_cpu_acquire()``
> > > +(preceded by a full memory barrier) and ``rcu_dynticks_in_eqs_since()``
> > > +(both of which rely on acquire semantics) to detect idle CPUs.
> > >
> > >  +-----------------------------------------------------------------------+
> > >  | **Quick Quiz**:                                                       |
> > > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> > > index f07b8bff4621b..1a6ef9c5c949e 100644
> > > --- a/kernel/rcu/tree.c
> > > +++ b/kernel/rcu/tree.c
> > > @@ -769,7 +769,12 @@ static void rcu_gpnum_ovf(struct rcu_node *rnp, struct rcu_data *rdp)
> > >   */
> > >  static int dyntick_save_progress_counter(struct rcu_data *rdp)
> > >  {
> > > -       rdp->dynticks_snap = rcu_dynticks_snap(rdp->cpu);
> > > +       /*
> > > +        * Full ordering against accesses prior current GP and also against
> > > +        * current GP sequence number is enforced by current rnp locking
> > > +        * with chained smp_mb__after_unlock_lock().
> > > +        */
> >
> > It might be worth mentioning that this chained smp_mb__after_unlock_lock()
> > is provided by rnp leaf node locking in rcu_gp_init() and rcu_gp_fqs_loop() ?
>
> Right!
>
> How about this?
>

Looks good to me, thanks! Minor comment (ditto for the other patch) below


>     /*
>      * Full ordering against accesses prior current GP and also against

Nit: "prior to current GP" ?


Thanks
Neeraj

>      * current GP sequence number is enforced by rcu_seq_start() implicit
>      * barrier and even further by smp_mb__after_unlock_lock() barriers
>      * chained all the way throughout the rnp locking tree since rcu_gp_init()
>      * and up to the current leaf rnp locking.
>      */
>
> Thanks.

Re: [PATCH rcu 3/6] rcu/exp: Remove superfluous full memory barrier upon first EQS snapshot

[Neeraj upadhyay]

On Wed, Jun 26, 2024 at 7:58 PM Frederic Weisbecker <frederic@kernel.org> wrote:
>
> Le Wed, Jun 12, 2024 at 02:14:14PM +0530, Neeraj upadhyay a écrit :
> > On Wed, Jun 5, 2024 at 3:58 AM Paul E. McKenney <paulmck@kernel.org> wrote:
> > >
> > > From: Frederic Weisbecker <frederic@kernel.org>
> > >
> > > When the grace period kthread checks the extended quiescent state
> > > counter of a CPU, full ordering is necessary to ensure that either:
> > >
> > > * If the GP kthread observes the remote target in an extended quiescent
> > >   state, then that target must observe all accesses prior to the current
> > >   grace period, including the current grace period sequence number, once
> > >   it exits that extended quiescent state.
> > >
> > > or:
> > >
> > > * If the GP kthread observes the remote target NOT in an extended
> > >   quiescent state, then the target further entering in an extended
> > >   quiescent state must observe all accesses prior to the current
> > >   grace period, including the current grace period sequence number, once
> > >   it enters that extended quiescent state.
> > >
> > > This ordering is enforced through a full memory barrier placed right
> > > before taking the first EQS snapshot. However this is superfluous
> > > because the snapshot is taken while holding the target's rnp lock which
> > > provides the necessary ordering through its chain of
> > > smp_mb__after_unlock_lock().
> > >
> > > Remove the needless explicit barrier before the snapshot and put a
> > > comment about the implicit barrier newly relied upon here.
> > >
> > > Signed-off-by: Frederic Weisbecker <frederic@kernel.org>
> > > Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
> > > ---
> > >  kernel/rcu/tree_exp.h | 8 +++++++-
> > >  1 file changed, 7 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/kernel/rcu/tree_exp.h b/kernel/rcu/tree_exp.h
> > > index 8a1d9c8bd9f74..bec24ea6777e8 100644
> > > --- a/kernel/rcu/tree_exp.h
> > > +++ b/kernel/rcu/tree_exp.h
> > > @@ -357,7 +357,13 @@ static void __sync_rcu_exp_select_node_cpus(struct rcu_exp_work *rewp)
> > >                     !(rnp->qsmaskinitnext & mask)) {
> > >                         mask_ofl_test |= mask;
> > >                 } else {
> > > -                       snap = rcu_dynticks_snap(cpu);
> > > +                       /*
> > > +                        * Full ordering against accesses prior current GP and
> > > +                        * also against current GP sequence number is enforced
> > > +                        * by current rnp locking with chained
> > > +                        * smp_mb__after_unlock_lock().
> >
> > Again, worth mentioning the chaining sites sync_exp_reset_tree() and
> > this function?
>
> How about this?
>

Looks good to me, thanks!


- Neeraj

>     /*
>      * Full ordering against accesses prior current GP and also against
>      * current GP sequence number is enforced by rcu_seq_start() implicit
>      * barrier, relayed by kworkers locking and even further by
>      * smp_mb__after_unlock_lock() barriers chained all the way throughout
>      * the rnp locking tree since sync_exp_reset_tree() and up to the current
>      * leaf rnp locking.
>      */
>
> Thanks.
>
> >
> >
> > Thanks
> > Neeraj
> >
> > > +                        */
> > > +                       snap = ct_dynticks_cpu_acquire(cpu);
> > >                         if (rcu_dynticks_in_eqs(snap))
> > >                                 mask_ofl_test |= mask;
> > >                         else
> > > --
> > > 2.40.1
> > >
> > >

Re: [PATCH rcu 2/6] rcu: Remove superfluous full memory barrier upon first EQS snapshot

[Frederic Weisbecker]

Le Wed, Jun 26, 2024 at 10:49:05PM +0530, Neeraj upadhyay a écrit :
> On Wed, Jun 26, 2024 at 7:43 PM Frederic Weisbecker <frederic@kernel.org> wrote:
> >
> > Le Wed, Jun 12, 2024 at 01:57:20PM +0530, Neeraj upadhyay a écrit :
> > > On Wed, Jun 5, 2024 at 3:58 AM Paul E. McKenney <paulmck@kernel.org> wrote:
> > > >
> > > > From: Frederic Weisbecker <frederic@kernel.org>
> > > >
> > > > When the grace period kthread checks the extended quiescent state
> > > > counter of a CPU, full ordering is necessary to ensure that either:
> > > >
> > > > * If the GP kthread observes the remote target in an extended quiescent
> > > >   state, then that target must observe all accesses prior to the current
> > > >   grace period, including the current grace period sequence number, once
> > > >   it exits that extended quiescent state.
> > > >
> > > > or:
> > > >
> > > > * If the GP kthread observes the remote target NOT in an extended
> > > >   quiescent state, then the target further entering in an extended
> > > >   quiescent state must observe all accesses prior to the current
> > > >   grace period, including the current grace period sequence number, once
> > > >   it enters that extended quiescent state.
> > > >
> > > > This ordering is enforced through a full memory barrier placed right
> > > > before taking the first EQS snapshot. However this is superfluous
> > > > because the snapshot is taken while holding the target's rnp lock which
> > > > provides the necessary ordering through its chain of
> > > > smp_mb__after_unlock_lock().
> > > >
> > > > Remove the needless explicit barrier before the snapshot and put a
> > > > comment about the implicit barrier newly relied upon here.
> > > >
> > > > Signed-off-by: Frederic Weisbecker <frederic@kernel.org>
> > > > Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
> > > > ---
> > > >  .../Design/Memory-Ordering/Tree-RCU-Memory-Ordering.rst    | 6 +++---
> > > >  kernel/rcu/tree.c                                          | 7 ++++++-
> > > >  2 files changed, 9 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/Documentation/RCU/Design/Memory-Ordering/Tree-RCU-Memory-Ordering.rst b/Documentation/RCU/Design/Memory-Ordering/Tree-RCU-Memory-Ordering.rst
> > > > index 5750f125361b0..728b1e690c646 100644
> > > > --- a/Documentation/RCU/Design/Memory-Ordering/Tree-RCU-Memory-Ordering.rst
> > > > +++ b/Documentation/RCU/Design/Memory-Ordering/Tree-RCU-Memory-Ordering.rst
> > > > @@ -149,9 +149,9 @@ This case is handled by calls to the strongly ordered
> > > >  ``atomic_add_return()`` read-modify-write atomic operation that
> > > >  is invoked within ``rcu_dynticks_eqs_enter()`` at idle-entry
> > > >  time and within ``rcu_dynticks_eqs_exit()`` at idle-exit time.
> > > > -The grace-period kthread invokes ``rcu_dynticks_snap()`` and
> > > > -``rcu_dynticks_in_eqs_since()`` (both of which invoke
> > > > -an ``atomic_add_return()`` of zero) to detect idle CPUs.
> > > > +The grace-period kthread invokes first ``ct_dynticks_cpu_acquire()``
> > > > +(preceded by a full memory barrier) and ``rcu_dynticks_in_eqs_since()``
> > > > +(both of which rely on acquire semantics) to detect idle CPUs.
> > > >
> > > >  +-----------------------------------------------------------------------+
> > > >  | **Quick Quiz**:                                                       |
> > > > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> > > > index f07b8bff4621b..1a6ef9c5c949e 100644
> > > > --- a/kernel/rcu/tree.c
> > > > +++ b/kernel/rcu/tree.c
> > > > @@ -769,7 +769,12 @@ static void rcu_gpnum_ovf(struct rcu_node *rnp, struct rcu_data *rdp)
> > > >   */
> > > >  static int dyntick_save_progress_counter(struct rcu_data *rdp)
> > > >  {
> > > > -       rdp->dynticks_snap = rcu_dynticks_snap(rdp->cpu);
> > > > +       /*
> > > > +        * Full ordering against accesses prior current GP and also against
> > > > +        * current GP sequence number is enforced by current rnp locking
> > > > +        * with chained smp_mb__after_unlock_lock().
> > > > +        */
> > >
> > > It might be worth mentioning that this chained smp_mb__after_unlock_lock()
> > > is provided by rnp leaf node locking in rcu_gp_init() and rcu_gp_fqs_loop() ?
> >
> > Right!
> >
> > How about this?
> >
> 
> Looks good to me, thanks! Minor comment (ditto for the other patch) below
> 
> 
> >     /*
> >      * Full ordering against accesses prior current GP and also against
> 
> Nit: "prior to current GP" ?

Thanks. On a second thought and just to make sure we don't forget why we did
what we did after a few years, I expanded some more, still ok with the following?

	/*
	 * Full ordering between remote CPU's post idle accesses and updater's
	 * accesses prior to current GP (and also the started GP sequence number)
	 * is enforced by rcu_seq_start() implicit barrier and even further by
	 * smp_mb__after_unlock_lock() barriers chained all the way throughout the
	 * rnp locking tree since rcu_gp_init() and up to the current leaf rnp
	 * locking.
	 *
	 * Ordering between remote CPU's pre idle accesses and post grace period's
	 * accesses is enforced by the below acquire semantic.
	 */

Thanks.

Re: [PATCH rcu 3/6] rcu/exp: Remove superfluous full memory barrier upon first EQS snapshot

[Frederic Weisbecker]

Le Wed, Jun 26, 2024 at 10:49:58PM +0530, Neeraj upadhyay a écrit :
> On Wed, Jun 26, 2024 at 7:58 PM Frederic Weisbecker <frederic@kernel.org> wrote:
> >
> > Le Wed, Jun 12, 2024 at 02:14:14PM +0530, Neeraj upadhyay a écrit :
> > > On Wed, Jun 5, 2024 at 3:58 AM Paul E. McKenney <paulmck@kernel.org> wrote:
> > > >
> > > > From: Frederic Weisbecker <frederic@kernel.org>
> > > >
> > > > When the grace period kthread checks the extended quiescent state
> > > > counter of a CPU, full ordering is necessary to ensure that either:
> > > >
> > > > * If the GP kthread observes the remote target in an extended quiescent
> > > >   state, then that target must observe all accesses prior to the current
> > > >   grace period, including the current grace period sequence number, once
> > > >   it exits that extended quiescent state.
> > > >
> > > > or:
> > > >
> > > > * If the GP kthread observes the remote target NOT in an extended
> > > >   quiescent state, then the target further entering in an extended
> > > >   quiescent state must observe all accesses prior to the current
> > > >   grace period, including the current grace period sequence number, once
> > > >   it enters that extended quiescent state.
> > > >
> > > > This ordering is enforced through a full memory barrier placed right
> > > > before taking the first EQS snapshot. However this is superfluous
> > > > because the snapshot is taken while holding the target's rnp lock which
> > > > provides the necessary ordering through its chain of
> > > > smp_mb__after_unlock_lock().
> > > >
> > > > Remove the needless explicit barrier before the snapshot and put a
> > > > comment about the implicit barrier newly relied upon here.
> > > >
> > > > Signed-off-by: Frederic Weisbecker <frederic@kernel.org>
> > > > Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
> > > > ---
> > > >  kernel/rcu/tree_exp.h | 8 +++++++-
> > > >  1 file changed, 7 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/kernel/rcu/tree_exp.h b/kernel/rcu/tree_exp.h
> > > > index 8a1d9c8bd9f74..bec24ea6777e8 100644
> > > > --- a/kernel/rcu/tree_exp.h
> > > > +++ b/kernel/rcu/tree_exp.h
> > > > @@ -357,7 +357,13 @@ static void __sync_rcu_exp_select_node_cpus(struct rcu_exp_work *rewp)
> > > >                     !(rnp->qsmaskinitnext & mask)) {
> > > >                         mask_ofl_test |= mask;
> > > >                 } else {
> > > > -                       snap = rcu_dynticks_snap(cpu);
> > > > +                       /*
> > > > +                        * Full ordering against accesses prior current GP and
> > > > +                        * also against current GP sequence number is enforced
> > > > +                        * by current rnp locking with chained
> > > > +                        * smp_mb__after_unlock_lock().
> > >
> > > Again, worth mentioning the chaining sites sync_exp_reset_tree() and
> > > this function?
> >
> > How about this?
> >
> 
> Looks good to me, thanks!

And similar to the previous one, a last minute edition:

			/*
			 * Full ordering between remote CPU's post idle accesses
			 * and updater's accesses prior to current GP (and also
			 * the started GP sequence number) is enforced by
			 * rcu_seq_start() implicit barrier, relayed by kworkers
			 * locking and even further by smp_mb__after_unlock_lock()
			 * barriers chained all the way throughout the rnp locking
			 * tree since sync_exp_reset_tree() and up to the current
			 * leaf rnp locking.
			 *
			 * Ordering between remote CPU's pre idle accesses and
			 * post grace period updater's accesses is enforced by the
			 * below acquire semantic.
			 */

Still ok?

Thanks.

Re: [PATCH rcu 2/6] rcu: Remove superfluous full memory barrier upon first EQS snapshot

[Neeraj upadhyay]

On Thu, Jun 27, 2024 at 3:33 AM Frederic Weisbecker <frederic@kernel.org> wrote:
>
> Le Wed, Jun 26, 2024 at 10:49:05PM +0530, Neeraj upadhyay a écrit :
> > On Wed, Jun 26, 2024 at 7:43 PM Frederic Weisbecker <frederic@kernel.org> wrote:
> > >
> > > Le Wed, Jun 12, 2024 at 01:57:20PM +0530, Neeraj upadhyay a écrit :
> > > > On Wed, Jun 5, 2024 at 3:58 AM Paul E. McKenney <paulmck@kernel.org> wrote:
> > > > >
> > > > > From: Frederic Weisbecker <frederic@kernel.org>
> > > > >
> > > > > When the grace period kthread checks the extended quiescent state
> > > > > counter of a CPU, full ordering is necessary to ensure that either:
> > > > >
> > > > > * If the GP kthread observes the remote target in an extended quiescent
> > > > >   state, then that target must observe all accesses prior to the current
> > > > >   grace period, including the current grace period sequence number, once
> > > > >   it exits that extended quiescent state.
> > > > >
> > > > > or:
> > > > >
> > > > > * If the GP kthread observes the remote target NOT in an extended
> > > > >   quiescent state, then the target further entering in an extended
> > > > >   quiescent state must observe all accesses prior to the current
> > > > >   grace period, including the current grace period sequence number, once
> > > > >   it enters that extended quiescent state.
> > > > >
> > > > > This ordering is enforced through a full memory barrier placed right
> > > > > before taking the first EQS snapshot. However this is superfluous
> > > > > because the snapshot is taken while holding the target's rnp lock which
> > > > > provides the necessary ordering through its chain of
> > > > > smp_mb__after_unlock_lock().
> > > > >
> > > > > Remove the needless explicit barrier before the snapshot and put a
> > > > > comment about the implicit barrier newly relied upon here.
> > > > >
> > > > > Signed-off-by: Frederic Weisbecker <frederic@kernel.org>
> > > > > Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
> > > > > ---
> > > > >  .../Design/Memory-Ordering/Tree-RCU-Memory-Ordering.rst    | 6 +++---
> > > > >  kernel/rcu/tree.c                                          | 7 ++++++-
> > > > >  2 files changed, 9 insertions(+), 4 deletions(-)
> > > > >
> > > > > diff --git a/Documentation/RCU/Design/Memory-Ordering/Tree-RCU-Memory-Ordering.rst b/Documentation/RCU/Design/Memory-Ordering/Tree-RCU-Memory-Ordering.rst
> > > > > index 5750f125361b0..728b1e690c646 100644
> > > > > --- a/Documentation/RCU/Design/Memory-Ordering/Tree-RCU-Memory-Ordering.rst
> > > > > +++ b/Documentation/RCU/Design/Memory-Ordering/Tree-RCU-Memory-Ordering.rst
> > > > > @@ -149,9 +149,9 @@ This case is handled by calls to the strongly ordered
> > > > >  ``atomic_add_return()`` read-modify-write atomic operation that
> > > > >  is invoked within ``rcu_dynticks_eqs_enter()`` at idle-entry
> > > > >  time and within ``rcu_dynticks_eqs_exit()`` at idle-exit time.
> > > > > -The grace-period kthread invokes ``rcu_dynticks_snap()`` and
> > > > > -``rcu_dynticks_in_eqs_since()`` (both of which invoke
> > > > > -an ``atomic_add_return()`` of zero) to detect idle CPUs.
> > > > > +The grace-period kthread invokes first ``ct_dynticks_cpu_acquire()``
> > > > > +(preceded by a full memory barrier) and ``rcu_dynticks_in_eqs_since()``
> > > > > +(both of which rely on acquire semantics) to detect idle CPUs.
> > > > >
> > > > >  +-----------------------------------------------------------------------+
> > > > >  | **Quick Quiz**:                                                       |
> > > > > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> > > > > index f07b8bff4621b..1a6ef9c5c949e 100644
> > > > > --- a/kernel/rcu/tree.c
> > > > > +++ b/kernel/rcu/tree.c
> > > > > @@ -769,7 +769,12 @@ static void rcu_gpnum_ovf(struct rcu_node *rnp, struct rcu_data *rdp)
> > > > >   */
> > > > >  static int dyntick_save_progress_counter(struct rcu_data *rdp)
> > > > >  {
> > > > > -       rdp->dynticks_snap = rcu_dynticks_snap(rdp->cpu);
> > > > > +       /*
> > > > > +        * Full ordering against accesses prior current GP and also against
> > > > > +        * current GP sequence number is enforced by current rnp locking
> > > > > +        * with chained smp_mb__after_unlock_lock().
> > > > > +        */
> > > >
> > > > It might be worth mentioning that this chained smp_mb__after_unlock_lock()
> > > > is provided by rnp leaf node locking in rcu_gp_init() and rcu_gp_fqs_loop() ?
> > >
> > > Right!
> > >
> > > How about this?
> > >
> >
> > Looks good to me, thanks! Minor comment (ditto for the other patch) below
> >
> >
> > >     /*
> > >      * Full ordering against accesses prior current GP and also against
> >
> > Nit: "prior to current GP" ?
>
> Thanks. On a second thought and just to make sure we don't forget why we did
> what we did after a few years, I expanded some more, still ok with the following?
>

Yes, looks good!


Thanks
Neeraj

>         /*
>          * Full ordering between remote CPU's post idle accesses and updater's
>          * accesses prior to current GP (and also the started GP sequence number)
>          * is enforced by rcu_seq_start() implicit barrier and even further by
>          * smp_mb__after_unlock_lock() barriers chained all the way throughout the
>          * rnp locking tree since rcu_gp_init() and up to the current leaf rnp
>          * locking.
>          *
>          * Ordering between remote CPU's pre idle accesses and post grace period's
>          * accesses is enforced by the below acquire semantic.
>          */
>
> Thanks.

Re: [PATCH rcu 3/6] rcu/exp: Remove superfluous full memory barrier upon first EQS snapshot

[Neeraj upadhyay]

On Thu, Jun 27, 2024 at 3:42 AM Frederic Weisbecker <frederic@kernel.org> wrote:
>
> Le Wed, Jun 26, 2024 at 10:49:58PM +0530, Neeraj upadhyay a écrit :
> > On Wed, Jun 26, 2024 at 7:58 PM Frederic Weisbecker <frederic@kernel.org> wrote:
> > >
> > > Le Wed, Jun 12, 2024 at 02:14:14PM +0530, Neeraj upadhyay a écrit :
> > > > On Wed, Jun 5, 2024 at 3:58 AM Paul E. McKenney <paulmck@kernel.org> wrote:
> > > > >
> > > > > From: Frederic Weisbecker <frederic@kernel.org>
> > > > >
> > > > > When the grace period kthread checks the extended quiescent state
> > > > > counter of a CPU, full ordering is necessary to ensure that either:
> > > > >
> > > > > * If the GP kthread observes the remote target in an extended quiescent
> > > > >   state, then that target must observe all accesses prior to the current
> > > > >   grace period, including the current grace period sequence number, once
> > > > >   it exits that extended quiescent state.
> > > > >
> > > > > or:
> > > > >
> > > > > * If the GP kthread observes the remote target NOT in an extended
> > > > >   quiescent state, then the target further entering in an extended
> > > > >   quiescent state must observe all accesses prior to the current
> > > > >   grace period, including the current grace period sequence number, once
> > > > >   it enters that extended quiescent state.
> > > > >
> > > > > This ordering is enforced through a full memory barrier placed right
> > > > > before taking the first EQS snapshot. However this is superfluous
> > > > > because the snapshot is taken while holding the target's rnp lock which
> > > > > provides the necessary ordering through its chain of
> > > > > smp_mb__after_unlock_lock().
> > > > >
> > > > > Remove the needless explicit barrier before the snapshot and put a
> > > > > comment about the implicit barrier newly relied upon here.
> > > > >
> > > > > Signed-off-by: Frederic Weisbecker <frederic@kernel.org>
> > > > > Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
> > > > > ---
> > > > >  kernel/rcu/tree_exp.h | 8 +++++++-
> > > > >  1 file changed, 7 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/kernel/rcu/tree_exp.h b/kernel/rcu/tree_exp.h
> > > > > index 8a1d9c8bd9f74..bec24ea6777e8 100644
> > > > > --- a/kernel/rcu/tree_exp.h
> > > > > +++ b/kernel/rcu/tree_exp.h
> > > > > @@ -357,7 +357,13 @@ static void __sync_rcu_exp_select_node_cpus(struct rcu_exp_work *rewp)
> > > > >                     !(rnp->qsmaskinitnext & mask)) {
> > > > >                         mask_ofl_test |= mask;
> > > > >                 } else {
> > > > > -                       snap = rcu_dynticks_snap(cpu);
> > > > > +                       /*
> > > > > +                        * Full ordering against accesses prior current GP and
> > > > > +                        * also against current GP sequence number is enforced
> > > > > +                        * by current rnp locking with chained
> > > > > +                        * smp_mb__after_unlock_lock().
> > > >
> > > > Again, worth mentioning the chaining sites sync_exp_reset_tree() and
> > > > this function?
> > >
> > > How about this?
> > >
> >
> > Looks good to me, thanks!
>
> And similar to the previous one, a last minute edition:
>
>                         /*
>                          * Full ordering between remote CPU's post idle accesses
>                          * and updater's accesses prior to current GP (and also
>                          * the started GP sequence number) is enforced by
>                          * rcu_seq_start() implicit barrier, relayed by kworkers
>                          * locking and even further by smp_mb__after_unlock_lock()
>                          * barriers chained all the way throughout the rnp locking
>                          * tree since sync_exp_reset_tree() and up to the current
>                          * leaf rnp locking.
>                          *
>                          * Ordering between remote CPU's pre idle accesses and
>                          * post grace period updater's accesses is enforced by the
>                          * below acquire semantic.
>                          */
>
> Still ok?
>

Yes, looks good, thanks.


Thanks
Neeraj

> Thanks.

Re: [PATCH rcu 2/6] rcu: Remove superfluous full memory barrier upon first EQS snapshot

[Neeraj upadhyay]

On Thu, Jun 27, 2024 at 7:46 AM Neeraj upadhyay <neeraj.iitr10@gmail.com> wrote:
>
> On Thu, Jun 27, 2024 at 3:33 AM Frederic Weisbecker <frederic@kernel.org> wrote:
> >
> > Le Wed, Jun 26, 2024 at 10:49:05PM +0530, Neeraj upadhyay a écrit :
> > > On Wed, Jun 26, 2024 at 7:43 PM Frederic Weisbecker <frederic@kernel.org> wrote:
> > > >
> > > > Le Wed, Jun 12, 2024 at 01:57:20PM +0530, Neeraj upadhyay a écrit :
> > > > > On Wed, Jun 5, 2024 at 3:58 AM Paul E. McKenney <paulmck@kernel.org> wrote:
> > > > > >
> > > > > > From: Frederic Weisbecker <frederic@kernel.org>
> > > > > >
> > > > > > When the grace period kthread checks the extended quiescent state
> > > > > > counter of a CPU, full ordering is necessary to ensure that either:
> > > > > >
> > > > > > * If the GP kthread observes the remote target in an extended quiescent
> > > > > >   state, then that target must observe all accesses prior to the current
> > > > > >   grace period, including the current grace period sequence number, once
> > > > > >   it exits that extended quiescent state.
> > > > > >
> > > > > > or:
> > > > > >
> > > > > > * If the GP kthread observes the remote target NOT in an extended
> > > > > >   quiescent state, then the target further entering in an extended
> > > > > >   quiescent state must observe all accesses prior to the current
> > > > > >   grace period, including the current grace period sequence number, once
> > > > > >   it enters that extended quiescent state.
> > > > > >
> > > > > > This ordering is enforced through a full memory barrier placed right
> > > > > > before taking the first EQS snapshot. However this is superfluous
> > > > > > because the snapshot is taken while holding the target's rnp lock which
> > > > > > provides the necessary ordering through its chain of
> > > > > > smp_mb__after_unlock_lock().
> > > > > >
> > > > > > Remove the needless explicit barrier before the snapshot and put a
> > > > > > comment about the implicit barrier newly relied upon here.
> > > > > >
> > > > > > Signed-off-by: Frederic Weisbecker <frederic@kernel.org>
> > > > > > Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
> > > > > > ---
> > > > > >  .../Design/Memory-Ordering/Tree-RCU-Memory-Ordering.rst    | 6 +++---
> > > > > >  kernel/rcu/tree.c                                          | 7 ++++++-
> > > > > >  2 files changed, 9 insertions(+), 4 deletions(-)
> > > > > >
> > > > > > diff --git a/Documentation/RCU/Design/Memory-Ordering/Tree-RCU-Memory-Ordering.rst b/Documentation/RCU/Design/Memory-Ordering/Tree-RCU-Memory-Ordering.rst
> > > > > > index 5750f125361b0..728b1e690c646 100644
> > > > > > --- a/Documentation/RCU/Design/Memory-Ordering/Tree-RCU-Memory-Ordering.rst
> > > > > > +++ b/Documentation/RCU/Design/Memory-Ordering/Tree-RCU-Memory-Ordering.rst
> > > > > > @@ -149,9 +149,9 @@ This case is handled by calls to the strongly ordered
> > > > > >  ``atomic_add_return()`` read-modify-write atomic operation that
> > > > > >  is invoked within ``rcu_dynticks_eqs_enter()`` at idle-entry
> > > > > >  time and within ``rcu_dynticks_eqs_exit()`` at idle-exit time.
> > > > > > -The grace-period kthread invokes ``rcu_dynticks_snap()`` and
> > > > > > -``rcu_dynticks_in_eqs_since()`` (both of which invoke
> > > > > > -an ``atomic_add_return()`` of zero) to detect idle CPUs.
> > > > > > +The grace-period kthread invokes first ``ct_dynticks_cpu_acquire()``
> > > > > > +(preceded by a full memory barrier) and ``rcu_dynticks_in_eqs_since()``
> > > > > > +(both of which rely on acquire semantics) to detect idle CPUs.
> > > > > >
> > > > > >  +-----------------------------------------------------------------------+
> > > > > >  | **Quick Quiz**:                                                       |
> > > > > > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> > > > > > index f07b8bff4621b..1a6ef9c5c949e 100644
> > > > > > --- a/kernel/rcu/tree.c
> > > > > > +++ b/kernel/rcu/tree.c
> > > > > > @@ -769,7 +769,12 @@ static void rcu_gpnum_ovf(struct rcu_node *rnp, struct rcu_data *rdp)
> > > > > >   */
> > > > > >  static int dyntick_save_progress_counter(struct rcu_data *rdp)
> > > > > >  {
> > > > > > -       rdp->dynticks_snap = rcu_dynticks_snap(rdp->cpu);
> > > > > > +       /*
> > > > > > +        * Full ordering against accesses prior current GP and also against
> > > > > > +        * current GP sequence number is enforced by current rnp locking
> > > > > > +        * with chained smp_mb__after_unlock_lock().
> > > > > > +        */
> > > > >
> > > > > It might be worth mentioning that this chained smp_mb__after_unlock_lock()
> > > > > is provided by rnp leaf node locking in rcu_gp_init() and rcu_gp_fqs_loop() ?
> > > >
> > > > Right!
> > > >
> > > > How about this?
> > > >
> > >
> > > Looks good to me, thanks! Minor comment (ditto for the other patch) below
> > >
> > >
> > > >     /*
> > > >      * Full ordering against accesses prior current GP and also against
> > >
> > > Nit: "prior to current GP" ?
> >
> > Thanks. On a second thought and just to make sure we don't forget why we did
> > what we did after a few years, I expanded some more, still ok with the following?
> >
>
> Yes, looks good!
>

So, I rechecked this after reviewing the other one. One comment below:

>
> Thanks
> Neeraj
>
> >         /*
> >          * Full ordering between remote CPU's post idle accesses and updater's
> >          * accesses prior to current GP (and also the started GP sequence number)
> >          * is enforced by rcu_seq_start() implicit barrier and even further by
> >          * smp_mb__after_unlock_lock() barriers chained all the way throughout the
> >          * rnp locking tree since rcu_gp_init() and up to the current leaf rnp
> >          * locking.
> >          *
> >          * Ordering between remote CPU's pre idle accesses and post grace period's
> >          * accesses is enforced by the below acquire semantic.

Maybe say "post grace period updater's accesses" as in the other change?

(I had to refer to your sequence flow in PATCH 1/6, between GP kthread
and remote CPU
to visualize this :) )


Thanks
Neeraj

> >          */
> >
> > Thanks.

Re: [PATCH rcu 2/6] rcu: Remove superfluous full memory barrier upon first EQS snapshot

[Frederic Weisbecker]

Le Thu, Jun 27, 2024 at 08:10:07AM +0530, Neeraj upadhyay a écrit :
> On Thu, Jun 27, 2024 at 7:46 AM Neeraj upadhyay <neeraj.iitr10@gmail.com> wrote:
> >
> > On Thu, Jun 27, 2024 at 3:33 AM Frederic Weisbecker <frederic@kernel.org> wrote:
> > >
> > > Le Wed, Jun 26, 2024 at 10:49:05PM +0530, Neeraj upadhyay a écrit :
> > > > On Wed, Jun 26, 2024 at 7:43 PM Frederic Weisbecker <frederic@kernel.org> wrote:
> > > > >
> > > > > Le Wed, Jun 12, 2024 at 01:57:20PM +0530, Neeraj upadhyay a écrit :
> > > > > > On Wed, Jun 5, 2024 at 3:58 AM Paul E. McKenney <paulmck@kernel.org> wrote:
> > > > > > >
> > > > > > > From: Frederic Weisbecker <frederic@kernel.org>
> > > > > > >
> > > > > > > When the grace period kthread checks the extended quiescent state
> > > > > > > counter of a CPU, full ordering is necessary to ensure that either:
> > > > > > >
> > > > > > > * If the GP kthread observes the remote target in an extended quiescent
> > > > > > >   state, then that target must observe all accesses prior to the current
> > > > > > >   grace period, including the current grace period sequence number, once
> > > > > > >   it exits that extended quiescent state.
> > > > > > >
> > > > > > > or:
> > > > > > >
> > > > > > > * If the GP kthread observes the remote target NOT in an extended
> > > > > > >   quiescent state, then the target further entering in an extended
> > > > > > >   quiescent state must observe all accesses prior to the current
> > > > > > >   grace period, including the current grace period sequence number, once
> > > > > > >   it enters that extended quiescent state.
> > > > > > >
> > > > > > > This ordering is enforced through a full memory barrier placed right
> > > > > > > before taking the first EQS snapshot. However this is superfluous
> > > > > > > because the snapshot is taken while holding the target's rnp lock which
> > > > > > > provides the necessary ordering through its chain of
> > > > > > > smp_mb__after_unlock_lock().
> > > > > > >
> > > > > > > Remove the needless explicit barrier before the snapshot and put a
> > > > > > > comment about the implicit barrier newly relied upon here.
> > > > > > >
> > > > > > > Signed-off-by: Frederic Weisbecker <frederic@kernel.org>
> > > > > > > Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
> > > > > > > ---
> > > > > > >  .../Design/Memory-Ordering/Tree-RCU-Memory-Ordering.rst    | 6 +++---
> > > > > > >  kernel/rcu/tree.c                                          | 7 ++++++-
> > > > > > >  2 files changed, 9 insertions(+), 4 deletions(-)
> > > > > > >
> > > > > > > diff --git a/Documentation/RCU/Design/Memory-Ordering/Tree-RCU-Memory-Ordering.rst b/Documentation/RCU/Design/Memory-Ordering/Tree-RCU-Memory-Ordering.rst
> > > > > > > index 5750f125361b0..728b1e690c646 100644
> > > > > > > --- a/Documentation/RCU/Design/Memory-Ordering/Tree-RCU-Memory-Ordering.rst
> > > > > > > +++ b/Documentation/RCU/Design/Memory-Ordering/Tree-RCU-Memory-Ordering.rst
> > > > > > > @@ -149,9 +149,9 @@ This case is handled by calls to the strongly ordered
> > > > > > >  ``atomic_add_return()`` read-modify-write atomic operation that
> > > > > > >  is invoked within ``rcu_dynticks_eqs_enter()`` at idle-entry
> > > > > > >  time and within ``rcu_dynticks_eqs_exit()`` at idle-exit time.
> > > > > > > -The grace-period kthread invokes ``rcu_dynticks_snap()`` and
> > > > > > > -``rcu_dynticks_in_eqs_since()`` (both of which invoke
> > > > > > > -an ``atomic_add_return()`` of zero) to detect idle CPUs.
> > > > > > > +The grace-period kthread invokes first ``ct_dynticks_cpu_acquire()``
> > > > > > > +(preceded by a full memory barrier) and ``rcu_dynticks_in_eqs_since()``
> > > > > > > +(both of which rely on acquire semantics) to detect idle CPUs.
> > > > > > >
> > > > > > >  +-----------------------------------------------------------------------+
> > > > > > >  | **Quick Quiz**:                                                       |
> > > > > > > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> > > > > > > index f07b8bff4621b..1a6ef9c5c949e 100644
> > > > > > > --- a/kernel/rcu/tree.c
> > > > > > > +++ b/kernel/rcu/tree.c
> > > > > > > @@ -769,7 +769,12 @@ static void rcu_gpnum_ovf(struct rcu_node *rnp, struct rcu_data *rdp)
> > > > > > >   */
> > > > > > >  static int dyntick_save_progress_counter(struct rcu_data *rdp)
> > > > > > >  {
> > > > > > > -       rdp->dynticks_snap = rcu_dynticks_snap(rdp->cpu);
> > > > > > > +       /*
> > > > > > > +        * Full ordering against accesses prior current GP and also against
> > > > > > > +        * current GP sequence number is enforced by current rnp locking
> > > > > > > +        * with chained smp_mb__after_unlock_lock().
> > > > > > > +        */
> > > > > >
> > > > > > It might be worth mentioning that this chained smp_mb__after_unlock_lock()
> > > > > > is provided by rnp leaf node locking in rcu_gp_init() and rcu_gp_fqs_loop() ?
> > > > >
> > > > > Right!
> > > > >
> > > > > How about this?
> > > > >
> > > >
> > > > Looks good to me, thanks! Minor comment (ditto for the other patch) below
> > > >
> > > >
> > > > >     /*
> > > > >      * Full ordering against accesses prior current GP and also against
> > > >
> > > > Nit: "prior to current GP" ?
> > >
> > > Thanks. On a second thought and just to make sure we don't forget why we did
> > > what we did after a few years, I expanded some more, still ok with the following?
> > >
> >
> > Yes, looks good!
> >
> 
> So, I rechecked this after reviewing the other one. One comment below:
> 
> >
> > Thanks
> > Neeraj
> >
> > >         /*
> > >          * Full ordering between remote CPU's post idle accesses and updater's
> > >          * accesses prior to current GP (and also the started GP sequence number)
> > >          * is enforced by rcu_seq_start() implicit barrier and even further by
> > >          * smp_mb__after_unlock_lock() barriers chained all the way throughout the
> > >          * rnp locking tree since rcu_gp_init() and up to the current leaf rnp
> > >          * locking.
> > >          *
> > >          * Ordering between remote CPU's pre idle accesses and post grace period's
> > >          * accesses is enforced by the below acquire semantic.
> 
> Maybe say "post grace period updater's accesses" as in the other change?

Exactly! Good catch, I updated.

> 
> (I had to refer to your sequence flow in PATCH 1/6, between GP kthread
> and remote CPU
> to visualize this :) )

:)

Thanks!

> 
> 
> Thanks
> Neeraj
> 
> > >          */
> > >
> > > Thanks.

[PATCH rcu 1/6 v2] rcu: Remove full ordering on second EQS snapshot

[Frederic Weisbecker]

When the grace period kthread checks the extended quiescent state
counter of a CPU, full ordering is necessary to ensure that either:

* If the GP kthread observes the remote target in an extended quiescent
  state, then that target must observe all accesses prior to the current
  grace period, including the current grace period sequence number, once
  it exits that extended quiescent state. Also the GP kthread must
  observe all accesses performed by the target prior it entering in
  EQS.

or:

* If the GP kthread observes the remote target NOT in an extended
  quiescent state, then the target further entering in an extended
  quiescent state must observe all accesses prior to the current
  grace period, including the current grace period sequence number, once
  it enters that extended quiescent state. Also the GP kthread later
  observing that EQS must also observe all accesses performed by the
  target prior it entering in EQS.

This ordering is explicitly performed both on the first EQS snapshot
and on the second one as well through the combination of a preceding
full barrier followed by an acquire read. However the second snapshot's
full memory barrier is redundant and not needed to enforce the above
guarantees:

    GP kthread                  Remote target
    ----                        -----
    // Access prior GP
    WRITE_ONCE(A, 1)
    // first snapshot
    smp_mb()
    x = smp_load_acquire(EQS)
                               // Access prior GP
                               WRITE_ONCE(B, 1)
                               // EQS enter
                               // implied full barrier by atomic_add_return()
                               atomic_add_return(RCU_DYNTICKS_IDX, EQS)
                               // implied full barrier by atomic_add_return()
                               READ_ONCE(A)
    // second snapshot
    y = smp_load_acquire(EQS)
    z = READ_ONCE(B)

If the GP kthread above fails to observe the remote target in EQS
(x not in EQS), the remote target will observe A == 1 after further
entering in EQS. Then the second snapshot taken by the GP kthread only
need to be an acquire read in order to observe z == 1.

Therefore remove the needless full memory barrier on second snapshot.

Signed-off-by: Frederic Weisbecker <frederic@kernel.org>
Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
Reviewed-by: Boqun Feng <boqun.feng@gmail.com>
Reviewed-by: Neeraj Upadhyay <neeraj.upadhyay@kernel.org>
---
 kernel/rcu/tree.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
index 28c7031711a3..b7d943e98a9a 100644
--- a/kernel/rcu/tree.c
+++ b/kernel/rcu/tree.c
@@ -321,7 +321,15 @@ static bool rcu_dynticks_in_eqs(int snap)
  */
 static bool rcu_dynticks_in_eqs_since(struct rcu_data *rdp, int snap)
 {
-	return snap != rcu_dynticks_snap(rdp->cpu);
+	/*
+	 * The first failing snapshot is already ordered against the accesses
+	 * performed by the remote CPU after it exits idle.
+	 *
+	 * The second snapshot therefore only needs to order against accesses
+	 * performed by the remote CPU prior to entering idle and therefore can
+	 * rely solely on acquire semantics.
+	 */
+	return snap != ct_dynticks_cpu_acquire(rdp->cpu);
 }
 
 /*
-- 
2.45.2

[PATCH rcu 2/6 v2] rcu: Remove superfluous full memory barrier upon first EQS snapshot

[Frederic Weisbecker]

When the grace period kthread checks the extended quiescent state
counter of a CPU, full ordering is necessary to ensure that either:

* If the GP kthread observes the remote target in an extended quiescent
  state, then that target must observe all accesses prior to the current
  grace period, including the current grace period sequence number, once
  it exits that extended quiescent state.

or:

* If the GP kthread observes the remote target NOT in an extended
  quiescent state, then the target further entering in an extended
  quiescent state must observe all accesses prior to the current
  grace period, including the current grace period sequence number, once
  it enters that extended quiescent state.

This ordering is enforced through a full memory barrier placed right
before taking the first EQS snapshot. However this is superfluous
because the snapshot is taken while holding the target's rnp lock which
provides the necessary ordering through its chain of
smp_mb__after_unlock_lock().

Remove the needless explicit barrier before the snapshot and put a
comment about the implicit barrier newly relied upon here.

Signed-off-by: Frederic Weisbecker <frederic@kernel.org>
Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
Reviewed-by: Boqun Feng <boqun.feng@gmail.com>
Reviewed-by: Neeraj Upadhyay <neeraj.upadhyay@kernel.org>
---
 .../Memory-Ordering/Tree-RCU-Memory-Ordering.rst    |  6 +++---
 kernel/rcu/tree.c                                   | 13 ++++++++++++-
 2 files changed, 15 insertions(+), 4 deletions(-)

diff --git a/Documentation/RCU/Design/Memory-Ordering/Tree-RCU-Memory-Ordering.rst b/Documentation/RCU/Design/Memory-Ordering/Tree-RCU-Memory-Ordering.rst
index 5750f125361b..728b1e690c64 100644
--- a/Documentation/RCU/Design/Memory-Ordering/Tree-RCU-Memory-Ordering.rst
+++ b/Documentation/RCU/Design/Memory-Ordering/Tree-RCU-Memory-Ordering.rst
@@ -149,9 +149,9 @@ This case is handled by calls to the strongly ordered
 ``atomic_add_return()`` read-modify-write atomic operation that
 is invoked within ``rcu_dynticks_eqs_enter()`` at idle-entry
 time and within ``rcu_dynticks_eqs_exit()`` at idle-exit time.
-The grace-period kthread invokes ``rcu_dynticks_snap()`` and
-``rcu_dynticks_in_eqs_since()`` (both of which invoke
-an ``atomic_add_return()`` of zero) to detect idle CPUs.
+The grace-period kthread invokes first ``ct_dynticks_cpu_acquire()``
+(preceded by a full memory barrier) and ``rcu_dynticks_in_eqs_since()``
+(both of which rely on acquire semantics) to detect idle CPUs.
 
 +-----------------------------------------------------------------------+
 | **Quick Quiz**:                                                       |
diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
index b7d943e98a9a..da5718da280d 100644
--- a/kernel/rcu/tree.c
+++ b/kernel/rcu/tree.c
@@ -777,7 +777,18 @@ static void rcu_gpnum_ovf(struct rcu_node *rnp, struct rcu_data *rdp)
  */
 static int dyntick_save_progress_counter(struct rcu_data *rdp)
 {
-	rdp->dynticks_snap = rcu_dynticks_snap(rdp->cpu);
+	/*
+	 * Full ordering between remote CPU's post idle accesses and updater's
+	 * accesses prior to current GP (and also the started GP sequence number)
+	 * is enforced by rcu_seq_start() implicit barrier and even further by
+	 * smp_mb__after_unlock_lock() barriers chained all the way throughout the
+	 * rnp locking tree since rcu_gp_init() and up to the current leaf rnp
+	 * locking.
+	 *
+	 * Ordering between remote CPU's pre idle accesses and post grace period
+	 * updater's accesses is enforced by the below acquire semantic.
+	 */
+	rdp->dynticks_snap = ct_dynticks_cpu_acquire(rdp->cpu);
 	if (rcu_dynticks_in_eqs(rdp->dynticks_snap)) {
 		trace_rcu_fqs(rcu_state.name, rdp->gp_seq, rdp->cpu, TPS("dti"));
 		rcu_gpnum_ovf(rdp->mynode, rdp);
-- 
2.45.2

[PATCH rcu 3/6 v2] rcu/exp: Remove superfluous full memory barrier upon first EQS snapshot

[Frederic Weisbecker]

When the grace period kthread checks the extended quiescent state
counter of a CPU, full ordering is necessary to ensure that either:

* If the GP kthread observes the remote target in an extended quiescent
  state, then that target must observe all accesses prior to the current
  grace period, including the current grace period sequence number, once
  it exits that extended quiescent state.

or:

* If the GP kthread observes the remote target NOT in an extended
  quiescent state, then the target further entering in an extended
  quiescent state must observe all accesses prior to the current
  grace period, including the current grace period sequence number, once
  it enters that extended quiescent state.

This ordering is enforced through a full memory barrier placed right
before taking the first EQS snapshot. However this is superfluous
because the snapshot is taken while holding the target's rnp lock which
provides the necessary ordering through its chain of
smp_mb__after_unlock_lock().

Remove the needless explicit barrier before the snapshot and put a
comment about the implicit barrier newly relied upon here.

Signed-off-by: Frederic Weisbecker <frederic@kernel.org>
Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
Reviewed-by: Boqun Feng <boqun.feng@gmail.com>
Reviewed-by: Neeraj Upadhyay <neeraj.upadhyay@kernel.org>
---
 kernel/rcu/tree_exp.h | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/kernel/rcu/tree_exp.h b/kernel/rcu/tree_exp.h
index 8a1d9c8bd9f7..1dbad2442e8d 100644
--- a/kernel/rcu/tree_exp.h
+++ b/kernel/rcu/tree_exp.h
@@ -357,7 +357,21 @@ static void __sync_rcu_exp_select_node_cpus(struct rcu_exp_work *rewp)
 		    !(rnp->qsmaskinitnext & mask)) {
 			mask_ofl_test |= mask;
 		} else {
-			snap = rcu_dynticks_snap(cpu);
+			/*
+			 * Full ordering between remote CPU's post idle accesses
+			 * and updater's accesses prior to current GP (and also
+			 * the started GP sequence number) is enforced by
+			 * rcu_seq_start() implicit barrier, relayed by kworkers
+			 * locking and even further by smp_mb__after_unlock_lock()
+			 * barriers chained all the way throughout the rnp locking
+			 * tree since sync_exp_reset_tree() and up to the current
+			 * leaf rnp locking.
+			 *
+			 * Ordering between remote CPU's pre idle accesses and
+			 * post grace period updater's accesses is enforced by the
+			 * below acquire semantic.
+			 */
+			snap = ct_dynticks_cpu_acquire(cpu);
 			if (rcu_dynticks_in_eqs(snap))
 				mask_ofl_test |= mask;
 			else
-- 
2.45.2

Re: [PATCH rcu 3/6 v2] rcu/exp: Remove superfluous full memory barrier upon first EQS snapshot

[Paul E. McKenney]

On Thu, Jun 27, 2024 at 01:36:58PM +0200, Frederic Weisbecker wrote:
> When the grace period kthread checks the extended quiescent state
> counter of a CPU, full ordering is necessary to ensure that either:

Just to make sure that I understand...

I need to replace these commits in -rcu:

da979d0162fc6 rcu: Remove full ordering on second EQS snapshot
6411f4185f657 rcu: Remove superfluous full memory barrier upon first EQS snapshot
dec56ca5f1c34 rcu/exp: Remove superfluous full memory barrier upon first EQS snapshot

With these three patches, and keep these three commits as they are?

d43a302fc08a5 rcu: Remove full memory barrier on boot time eqs sanity check
b1c36aa90cbf1 rcu: Remove full memory barrier on RCU stall printout
64d68f1d53f77 rcu/exp: Remove redundant full memory barrier at the end of GP

							Thanx, Paul

> * If the GP kthread observes the remote target in an extended quiescent
>   state, then that target must observe all accesses prior to the current
>   grace period, including the current grace period sequence number, once
>   it exits that extended quiescent state.
> 
> or:
> 
> * If the GP kthread observes the remote target NOT in an extended
>   quiescent state, then the target further entering in an extended
>   quiescent state must observe all accesses prior to the current
>   grace period, including the current grace period sequence number, once
>   it enters that extended quiescent state.
> 
> This ordering is enforced through a full memory barrier placed right
> before taking the first EQS snapshot. However this is superfluous
> because the snapshot is taken while holding the target's rnp lock which
> provides the necessary ordering through its chain of
> smp_mb__after_unlock_lock().
> 
> Remove the needless explicit barrier before the snapshot and put a
> comment about the implicit barrier newly relied upon here.
> 
> Signed-off-by: Frederic Weisbecker <frederic@kernel.org>
> Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
> Reviewed-by: Boqun Feng <boqun.feng@gmail.com>
> Reviewed-by: Neeraj Upadhyay <neeraj.upadhyay@kernel.org>
> ---
>  kernel/rcu/tree_exp.h | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/rcu/tree_exp.h b/kernel/rcu/tree_exp.h
> index 8a1d9c8bd9f7..1dbad2442e8d 100644
> --- a/kernel/rcu/tree_exp.h
> +++ b/kernel/rcu/tree_exp.h
> @@ -357,7 +357,21 @@ static void __sync_rcu_exp_select_node_cpus(struct rcu_exp_work *rewp)
>  		    !(rnp->qsmaskinitnext & mask)) {
>  			mask_ofl_test |= mask;
>  		} else {
> -			snap = rcu_dynticks_snap(cpu);
> +			/*
> +			 * Full ordering between remote CPU's post idle accesses
> +			 * and updater's accesses prior to current GP (and also
> +			 * the started GP sequence number) is enforced by
> +			 * rcu_seq_start() implicit barrier, relayed by kworkers
> +			 * locking and even further by smp_mb__after_unlock_lock()
> +			 * barriers chained all the way throughout the rnp locking
> +			 * tree since sync_exp_reset_tree() and up to the current
> +			 * leaf rnp locking.
> +			 *
> +			 * Ordering between remote CPU's pre idle accesses and
> +			 * post grace period updater's accesses is enforced by the
> +			 * below acquire semantic.
> +			 */
> +			snap = ct_dynticks_cpu_acquire(cpu);
>  			if (rcu_dynticks_in_eqs(snap))
>  				mask_ofl_test |= mask;
>  			else
> -- 
> 2.45.2
> 

Re: [PATCH rcu 3/6 v2] rcu/exp: Remove superfluous full memory barrier upon first EQS snapshot

[Frederic Weisbecker]

Le Thu, Jun 27, 2024 at 11:53:29AM -0700, Paul E. McKenney a écrit :
> On Thu, Jun 27, 2024 at 01:36:58PM +0200, Frederic Weisbecker wrote:
> > When the grace period kthread checks the extended quiescent state
> > counter of a CPU, full ordering is necessary to ensure that either:
> 
> Just to make sure that I understand...
> 
> I need to replace these commits in -rcu:
> 
> da979d0162fc6 rcu: Remove full ordering on second EQS snapshot
> 6411f4185f657 rcu: Remove superfluous full memory barrier upon first EQS snapshot
> dec56ca5f1c34 rcu/exp: Remove superfluous full memory barrier upon first EQS snapshot
> 
> With these three patches, and keep these three commits as they are?
> 
> d43a302fc08a5 rcu: Remove full memory barrier on boot time eqs sanity check
> b1c36aa90cbf1 rcu: Remove full memory barrier on RCU stall printout
> 64d68f1d53f77 rcu/exp: Remove redundant full memory barrier at the end of GP

Exactly! Those were the precisions I forgot to mention.

Thanks.

Re: [PATCH rcu 3/6 v2] rcu/exp: Remove superfluous full memory barrier upon first EQS snapshot

[Paul E. McKenney]

On Fri, Jun 28, 2024 at 01:20:55PM +0200, Frederic Weisbecker wrote:
> Le Thu, Jun 27, 2024 at 11:53:29AM -0700, Paul E. McKenney a écrit :
> > On Thu, Jun 27, 2024 at 01:36:58PM +0200, Frederic Weisbecker wrote:
> > > When the grace period kthread checks the extended quiescent state
> > > counter of a CPU, full ordering is necessary to ensure that either:
> > 
> > Just to make sure that I understand...
> > 
> > I need to replace these commits in -rcu:
> > 
> > da979d0162fc6 rcu: Remove full ordering on second EQS snapshot
> > 6411f4185f657 rcu: Remove superfluous full memory barrier upon first EQS snapshot
> > dec56ca5f1c34 rcu/exp: Remove superfluous full memory barrier upon first EQS snapshot
> > 
> > With these three patches, and keep these three commits as they are?
> > 
> > d43a302fc08a5 rcu: Remove full memory barrier on boot time eqs sanity check
> > b1c36aa90cbf1 rcu: Remove full memory barrier on RCU stall printout
> > 64d68f1d53f77 rcu/exp: Remove redundant full memory barrier at the end of GP
> 
> Exactly! Those were the precisions I forgot to mention.

Done, and started testing.  If all goes well, this might make tomorrow's
-next.

							Thanx, Paul