[PATCHv2 0/2] srcu: Optimize when srcu_gp_start_if_needed() holds read lock

[PATCHv2 0/2] srcu: Optimize when srcu_gp_start_if_needed() holds read lock

[Pingfan Liu]

This series optimizes and remove some code under the condition that
srcu_gp_start_if_needed() holds the srcu read lock.

v1 -> v2:
  add debug check in [1/2]
  drop original "[PATCH 2/2] srcu: Remove needless updating of
srcu_have_cbs in srcu_gp_end()"

Pingfan Liu (2):
  srcu: Remove needless rcu_seq_done() check while holding read lock
  srcu: Eliminate the case that snp_seq bigger than snap in
    srcu_funnel_gp_start()

Cc: Lai Jiangshan <jiangshanlai@gmail.com>
Cc: "Paul E. McKenney" <paulmck@kernel.org>
Cc: Frederic Weisbecker <frederic@kernel.org>
Cc: Josh Triplett <josh@joshtriplett.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
To: rcu@vger.kernel.org

 include/linux/rcupdate.h |  1 +
 kernel/rcu/srcutree.c    | 27 +++++++++++++++------------
 2 files changed, 16 insertions(+), 12 deletions(-)

-- 
2.31.1

[PATCHv2 1/2] srcu: Remove needless rcu_seq_done() check while holding read lock

[Pingfan Liu]

As the code changes, now, srcu_gp_start_if_needed() holds the srcu read
lock, gets the gp_seq snap and call srcu_funnel_gp_start() passing that
snap value.

As the rcu_seq_snap() promises "a full grace period has elapsed since
the current time".  In srcu_funnel_gp_start(), the statement
  rcu_seq_done(&ssp->srcu_gp_seq, s)
always return false.

The same condition stands for srcu_funnel_exp_start(). Hence removing
all the checks of rcu_seq_done().

Signed-off-by: Pingfan Liu <kernelfans@gmail.com>
Cc: Lai Jiangshan <jiangshanlai@gmail.com>
Cc: "Paul E. McKenney" <paulmck@kernel.org>
Cc: Frederic Weisbecker <frederic@kernel.org>
Cc: Josh Triplett <josh@joshtriplett.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
To: rcu@vger.kernel.org
---
 kernel/rcu/srcutree.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
index 1c304fec89c0..2fc0e775ade4 100644
--- a/kernel/rcu/srcutree.c
+++ b/kernel/rcu/srcutree.c
@@ -851,8 +851,7 @@ static void srcu_funnel_exp_start(struct srcu_struct *ssp, struct srcu_node *snp
 	if (snp)
 		for (; snp != NULL; snp = snp->srcu_parent) {
 			sgsne = READ_ONCE(snp->srcu_gp_seq_needed_exp);
-			if (rcu_seq_done(&ssp->srcu_gp_seq, s) ||
-			    (!srcu_invl_snp_seq(sgsne) && ULONG_CMP_GE(sgsne, s)))
+			if (!srcu_invl_snp_seq(sgsne) && ULONG_CMP_GE(sgsne, s))
 				return;
 			spin_lock_irqsave_rcu_node(snp, flags);
 			sgsne = snp->srcu_gp_seq_needed_exp;
@@ -878,6 +877,9 @@ static void srcu_funnel_exp_start(struct srcu_struct *ssp, struct srcu_node *snp
  *
  * Note that this function also does the work of srcu_funnel_exp_start(),
  * in some cases by directly invoking it.
+ *
+ * The srcu read lock should be hold around this function. And s is a seq snap
+ * after holding that lock.
  */
 static void srcu_funnel_gp_start(struct srcu_struct *ssp, struct srcu_data *sdp,
 				 unsigned long s, bool do_norm)
@@ -895,11 +897,12 @@ static void srcu_funnel_gp_start(struct srcu_struct *ssp, struct srcu_data *sdp,
 	else
 		snp_leaf = sdp->mynode;
 
+	BUG_ON(!srcu_read_lock_held(ssp));
+	WARN_ON_ONCE(rcu_seq_done(&ssp->srcu_gp_seq, s));
+
 	if (snp_leaf)
 		/* Each pass through the loop does one level of the srcu_node tree. */
 		for (snp = snp_leaf; snp != NULL; snp = snp->srcu_parent) {
-			if (rcu_seq_done(&ssp->srcu_gp_seq, s) && snp != snp_leaf)
-				return; /* GP already done and CBs recorded. */
 			spin_lock_irqsave_rcu_node(snp, flags);
 			snp_seq = snp->srcu_have_cbs[idx];
 			if (!srcu_invl_snp_seq(snp_seq) && ULONG_CMP_GE(snp_seq, s)) {
@@ -935,9 +938,8 @@ static void srcu_funnel_gp_start(struct srcu_struct *ssp, struct srcu_data *sdp,
 	if (!do_norm && ULONG_CMP_LT(ssp->srcu_gp_seq_needed_exp, s))
 		WRITE_ONCE(ssp->srcu_gp_seq_needed_exp, s);
 
-	/* If grace period not already done and none in progress, start it. */
-	if (!rcu_seq_done(&ssp->srcu_gp_seq, s) &&
-	    rcu_seq_state(ssp->srcu_gp_seq) == SRCU_STATE_IDLE) {
+	/* If grace period not already in progress, start it. */
+	if (rcu_seq_state(ssp->srcu_gp_seq) == SRCU_STATE_IDLE) {
 		WARN_ON_ONCE(ULONG_CMP_GE(ssp->srcu_gp_seq, ssp->srcu_gp_seq_needed));
 		srcu_gp_start(ssp);
 
-- 
2.31.1

Re: [PATCHv2 1/2] srcu: Remove needless rcu_seq_done() check while holding read lock

[Paul E. McKenney]

On Wed, Nov 23, 2022 at 09:56:37PM +0800, Pingfan Liu wrote:
> As the code changes, now, srcu_gp_start_if_needed() holds the srcu read
> lock, gets the gp_seq snap and call srcu_funnel_gp_start() passing that
> snap value.
> 
> As the rcu_seq_snap() promises "a full grace period has elapsed since
> the current time".  In srcu_funnel_gp_start(), the statement
>   rcu_seq_done(&ssp->srcu_gp_seq, s)
> always return false.
> 
> The same condition stands for srcu_funnel_exp_start(). Hence removing
> all the checks of rcu_seq_done().
> 
> Signed-off-by: Pingfan Liu <kernelfans@gmail.com>
> Cc: Lai Jiangshan <jiangshanlai@gmail.com>
> Cc: "Paul E. McKenney" <paulmck@kernel.org>
> Cc: Frederic Weisbecker <frederic@kernel.org>
> Cc: Josh Triplett <josh@joshtriplett.org>
> Cc: Steven Rostedt <rostedt@goodmis.org>
> Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> To: rcu@vger.kernel.org

Thank you!  I have queued this with some wordsmithing and modifications,
as shown below.  Please check these.

Also, please test future patches with the debug options shown in
Documentation/process/submit-checklist.rst.  In this case, testing with
CONFIG_PROVE_RCU=y would have located a bug with this repeat-by:

tools/testing/selftests/rcutorture/bin/kvm.sh --allcpus --duration 2m --configs "SRCU-P SRCU-N" --trust-make

							Thanx, Paul

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

commit 17a874a71faed9e38b69c6ec7bae6aa309a2530e
Author: Pingfan Liu <kernelfans@gmail.com>
Date:   Wed Nov 23 21:56:37 2022 +0800

    srcu: Remove needless rcu_seq_done() check while holding read lock
    
    The srcu_gp_start_if_needed() function now read-holds the srcu_struct
    whose grace period is being started, which means that the corresponding
    SRCU grace period cannot end.  This in turn means that the SRCU
    grace-period sequence number returned by rcu_seq_snap() cannot expire
    during this time.  And that means that the calls to rcu_seq_done() in
    srcu_funnel_exp_start() and srcu_funnel_gp_start() can never return true.
    
    This commit therefore removes these rcu_seq_done() checks, but adds checks
    in kernels built with CONFIG_PROVE_RCU=y that splats if rcu_seq_done()
    does somehow return true.
    
    [ paulmck: Rearrange checks to handle kernels built with lockdep. ]
    
    Signed-off-by: Pingfan Liu <kernelfans@gmail.com>
    Cc: Lai Jiangshan <jiangshanlai@gmail.com>
    Cc: Frederic Weisbecker <frederic@kernel.org>
    Cc: Josh Triplett <josh@joshtriplett.org>
    Cc: Steven Rostedt <rostedt@goodmis.org>
    Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
    To: rcu@vger.kernel.org
    Signed-off-by: Paul E. McKenney <paulmck@kernel.org>

diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
index 6af0312005801..b25f77df9e5e7 100644
--- a/kernel/rcu/srcutree.c
+++ b/kernel/rcu/srcutree.c
@@ -915,8 +915,9 @@ static void srcu_funnel_exp_start(struct srcu_struct *ssp, struct srcu_node *snp
 	if (snp)
 		for (; snp != NULL; snp = snp->srcu_parent) {
 			sgsne = READ_ONCE(snp->srcu_gp_seq_needed_exp);
-			if (rcu_seq_done(&ssp->srcu_gp_seq, s) ||
-			    (!srcu_invl_snp_seq(sgsne) && ULONG_CMP_GE(sgsne, s)))
+			WARN_ON_ONCE(IS_ENABLED(CONFIG_PROVE_RCU) &&
+				     rcu_seq_done(&ssp->srcu_gp_seq, s));
+			if (!srcu_invl_snp_seq(sgsne) && ULONG_CMP_GE(sgsne, s))
 				return;
 			spin_lock_irqsave_rcu_node(snp, flags);
 			sgsne = snp->srcu_gp_seq_needed_exp;
@@ -942,6 +943,9 @@ static void srcu_funnel_exp_start(struct srcu_struct *ssp, struct srcu_node *snp
  *
  * Note that this function also does the work of srcu_funnel_exp_start(),
  * in some cases by directly invoking it.
+ *
+ * The srcu read lock should be hold around this function. And s is a seq snap
+ * after holding that lock.
  */
 static void srcu_funnel_gp_start(struct srcu_struct *ssp, struct srcu_data *sdp,
 				 unsigned long s, bool do_norm)
@@ -962,8 +966,8 @@ static void srcu_funnel_gp_start(struct srcu_struct *ssp, struct srcu_data *sdp,
 	if (snp_leaf)
 		/* Each pass through the loop does one level of the srcu_node tree. */
 		for (snp = snp_leaf; snp != NULL; snp = snp->srcu_parent) {
-			if (rcu_seq_done(&ssp->srcu_gp_seq, s) && snp != snp_leaf)
-				return; /* GP already done and CBs recorded. */
+			WARN_ON_ONCE(IS_ENABLED(CONFIG_PROVE_RCU) &&
+				     rcu_seq_done(&ssp->srcu_gp_seq, s));
 			spin_lock_irqsave_rcu_node(snp, flags);
 			snp_seq = snp->srcu_have_cbs[idx];
 			if (!srcu_invl_snp_seq(snp_seq) && ULONG_CMP_GE(snp_seq, s)) {
@@ -999,9 +1003,9 @@ static void srcu_funnel_gp_start(struct srcu_struct *ssp, struct srcu_data *sdp,
 	if (!do_norm && ULONG_CMP_LT(ssp->srcu_gp_seq_needed_exp, s))
 		WRITE_ONCE(ssp->srcu_gp_seq_needed_exp, s);
 
-	/* If grace period not already done and none in progress, start it. */
-	if (!rcu_seq_done(&ssp->srcu_gp_seq, s) &&
-	    rcu_seq_state(ssp->srcu_gp_seq) == SRCU_STATE_IDLE) {
+	/* If grace period not already in progress, start it. */
+	WARN_ON_ONCE(IS_ENABLED(CONFIG_PROVE_RCU) && rcu_seq_done(&ssp->srcu_gp_seq, s));
+	if (rcu_seq_state(ssp->srcu_gp_seq) == SRCU_STATE_IDLE) {
 		WARN_ON_ONCE(ULONG_CMP_GE(ssp->srcu_gp_seq, ssp->srcu_gp_seq_needed));
 		srcu_gp_start(ssp);
 

[PATCHv2 2/2] srcu: Eliminate the case that snp_seq bigger than snap in srcu_funnel_gp_start()

[Pingfan Liu]

Since the srcu read lock is still held during srcu_funnel_gp_start(),
the seq snap should be the largest number for the slot
srcu_have_cbs[idx].

Signed-off-by: Pingfan Liu <kernelfans@gmail.com>
Cc: Lai Jiangshan <jiangshanlai@gmail.com>
Cc: "Paul E. McKenney" <paulmck@kernel.org>
Cc: Frederic Weisbecker <frederic@kernel.org>
Cc: Josh Triplett <josh@joshtriplett.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
To: rcu@vger.kernel.org
---
 include/linux/rcupdate.h |  1 +
 kernel/rcu/srcutree.c    | 11 ++++++-----
 2 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
index 08605ce7379d..a09007236660 100644
--- a/include/linux/rcupdate.h
+++ b/include/linux/rcupdate.h
@@ -32,6 +32,7 @@
 #include <linux/context_tracking_irq.h>
 
 #define ULONG_CMP_GE(a, b)	(ULONG_MAX / 2 >= (a) - (b))
+#define ULONG_CMP_GT(a, b)	(ULONG_MAX / 2 > (a) - (b))
 #define ULONG_CMP_LT(a, b)	(ULONG_MAX / 2 < (a) - (b))
 #define ulong2long(a)		(*(long *)(&(a)))
 #define USHORT_CMP_GE(a, b)	(USHRT_MAX / 2 >= (unsigned short)((a) - (b)))
diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
index 2fc0e775ade4..41902b823687 100644
--- a/kernel/rcu/srcutree.c
+++ b/kernel/rcu/srcutree.c
@@ -905,14 +905,15 @@ static void srcu_funnel_gp_start(struct srcu_struct *ssp, struct srcu_data *sdp,
 		for (snp = snp_leaf; snp != NULL; snp = snp->srcu_parent) {
 			spin_lock_irqsave_rcu_node(snp, flags);
 			snp_seq = snp->srcu_have_cbs[idx];
-			if (!srcu_invl_snp_seq(snp_seq) && ULONG_CMP_GE(snp_seq, s)) {
+			/*
+			 * s should be the biggest in the current slot. Hence only LE is
+			 * valid
+			 */
+			BUG_ON(ULONG_CMP_GT(snp_seq, s));
+			if (!srcu_invl_snp_seq(snp_seq) && (snp_seq == s)) {
 				if (snp == snp_leaf && snp_seq == s)
 					snp->srcu_data_have_cbs[idx] |= sdp->grpmask;
 				spin_unlock_irqrestore_rcu_node(snp, flags);
-				if (snp == snp_leaf && snp_seq != s) {
-					srcu_schedule_cbs_sdp(sdp, do_norm ? SRCU_INTERVAL : 0);
-					return;
-				}
 				if (!do_norm)
 					srcu_funnel_exp_start(ssp, snp, s);
 				return;
-- 
2.31.1

Re: [PATCHv2 2/2] srcu: Eliminate the case that snp_seq bigger than snap in srcu_funnel_gp_start()

[Paul E. McKenney]

On Wed, Nov 23, 2022 at 09:56:38PM +0800, Pingfan Liu wrote:
> Since the srcu read lock is still held during srcu_funnel_gp_start(),
> the seq snap should be the largest number for the slot
> srcu_have_cbs[idx].
> 
> Signed-off-by: Pingfan Liu <kernelfans@gmail.com>
> Cc: Lai Jiangshan <jiangshanlai@gmail.com>
> Cc: "Paul E. McKenney" <paulmck@kernel.org>
> Cc: Frederic Weisbecker <frederic@kernel.org>
> Cc: Josh Triplett <josh@joshtriplett.org>
> Cc: Steven Rostedt <rostedt@goodmis.org>
> Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> To: rcu@vger.kernel.org
> ---
>  include/linux/rcupdate.h |  1 +
>  kernel/rcu/srcutree.c    | 11 ++++++-----
>  2 files changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
> index 08605ce7379d..a09007236660 100644
> --- a/include/linux/rcupdate.h
> +++ b/include/linux/rcupdate.h
> @@ -32,6 +32,7 @@
>  #include <linux/context_tracking_irq.h>
>  
>  #define ULONG_CMP_GE(a, b)	(ULONG_MAX / 2 >= (a) - (b))
> +#define ULONG_CMP_GT(a, b)	(ULONG_MAX / 2 > (a) - (b))

Please see below...

>  #define ULONG_CMP_LT(a, b)	(ULONG_MAX / 2 < (a) - (b))
>  #define ulong2long(a)		(*(long *)(&(a)))
>  #define USHORT_CMP_GE(a, b)	(USHRT_MAX / 2 >= (unsigned short)((a) - (b)))
> diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
> index 2fc0e775ade4..41902b823687 100644
> --- a/kernel/rcu/srcutree.c
> +++ b/kernel/rcu/srcutree.c
> @@ -905,14 +905,15 @@ static void srcu_funnel_gp_start(struct srcu_struct *ssp, struct srcu_data *sdp,
>  		for (snp = snp_leaf; snp != NULL; snp = snp->srcu_parent) {
>  			spin_lock_irqsave_rcu_node(snp, flags);
>  			snp_seq = snp->srcu_have_cbs[idx];
> -			if (!srcu_invl_snp_seq(snp_seq) && ULONG_CMP_GE(snp_seq, s)) {
> +			/*
> +			 * s should be the biggest in the current slot. Hence only LE is
> +			 * valid
> +			 */
> +			BUG_ON(ULONG_CMP_GT(snp_seq, s));

Why not this?  (Plus adjusting the comment above, of course.)

			WARN_ON_ONCE(ULONG_CMP_LT(s, snp_seq));

That way we don't need ULONG_CMP_GT().  Plus if we are confused about
s being the biggest in the current slot, we get a splat and can debug
further.  We both might be quite sure that we are not confused, but
that is exactly when we are most prone to making mistakes.  ;-)

I also ask that you run with this check for some time.  After all, if
the assumption is incorrect, the resulting SRCU hangs off in various
systems around the world will not be so much fun to debug.

Given that this is slowpath code, it is much better to take an extra
compare and branch than to introduce even an extremely small risk of
hanging SRCU.

							Thanx, Paul

> +			if (!srcu_invl_snp_seq(snp_seq) && (snp_seq == s)) {
>  				if (snp == snp_leaf && snp_seq == s)
>  					snp->srcu_data_have_cbs[idx] |= sdp->grpmask;
>  				spin_unlock_irqrestore_rcu_node(snp, flags);
> -				if (snp == snp_leaf && snp_seq != s) {
> -					srcu_schedule_cbs_sdp(sdp, do_norm ? SRCU_INTERVAL : 0);
> -					return;
> -				}
>  				if (!do_norm)
>  					srcu_funnel_exp_start(ssp, snp, s);
>  				return;
> -- 
> 2.31.1
> 

Re: [PATCHv2 2/2] srcu: Eliminate the case that snp_seq bigger than snap in srcu_funnel_gp_start()

[Pingfan Liu]

On Wed, Nov 23, 2022 at 11:20:46AM -0800, Paul E. McKenney wrote:
> On Wed, Nov 23, 2022 at 09:56:38PM +0800, Pingfan Liu wrote:
> > Since the srcu read lock is still held during srcu_funnel_gp_start(),
> > the seq snap should be the largest number for the slot
> > srcu_have_cbs[idx].
> > 
> > Signed-off-by: Pingfan Liu <kernelfans@gmail.com>
> > Cc: Lai Jiangshan <jiangshanlai@gmail.com>
> > Cc: "Paul E. McKenney" <paulmck@kernel.org>
> > Cc: Frederic Weisbecker <frederic@kernel.org>
> > Cc: Josh Triplett <josh@joshtriplett.org>
> > Cc: Steven Rostedt <rostedt@goodmis.org>
> > Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> > To: rcu@vger.kernel.org
> > ---
> >  include/linux/rcupdate.h |  1 +
> >  kernel/rcu/srcutree.c    | 11 ++++++-----
> >  2 files changed, 7 insertions(+), 5 deletions(-)
> > 
> > diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
> > index 08605ce7379d..a09007236660 100644
> > --- a/include/linux/rcupdate.h
> > +++ b/include/linux/rcupdate.h
> > @@ -32,6 +32,7 @@
> >  #include <linux/context_tracking_irq.h>
> >  
> >  #define ULONG_CMP_GE(a, b)	(ULONG_MAX / 2 >= (a) - (b))
> > +#define ULONG_CMP_GT(a, b)	(ULONG_MAX / 2 > (a) - (b))
> 
> Please see below...
> 
> >  #define ULONG_CMP_LT(a, b)	(ULONG_MAX / 2 < (a) - (b))
> >  #define ulong2long(a)		(*(long *)(&(a)))
> >  #define USHORT_CMP_GE(a, b)	(USHRT_MAX / 2 >= (unsigned short)((a) - (b)))
> > diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
> > index 2fc0e775ade4..41902b823687 100644
> > --- a/kernel/rcu/srcutree.c
> > +++ b/kernel/rcu/srcutree.c
> > @@ -905,14 +905,15 @@ static void srcu_funnel_gp_start(struct srcu_struct *ssp, struct srcu_data *sdp,
> >  		for (snp = snp_leaf; snp != NULL; snp = snp->srcu_parent) {
> >  			spin_lock_irqsave_rcu_node(snp, flags);
> >  			snp_seq = snp->srcu_have_cbs[idx];
> > -			if (!srcu_invl_snp_seq(snp_seq) && ULONG_CMP_GE(snp_seq, s)) {
> > +			/*
> > +			 * s should be the biggest in the current slot. Hence only LE is
> > +			 * valid
> > +			 */
> > +			BUG_ON(ULONG_CMP_GT(snp_seq, s));
> 
> Why not this?  (Plus adjusting the comment above, of course.)
> 
> 			WARN_ON_ONCE(ULONG_CMP_LT(s, snp_seq));
> 

A neat solution!

And what about the comment
			/* s should be the biggest in the current slot. */

> That way we don't need ULONG_CMP_GT().  Plus if we are confused about
> s being the biggest in the current slot, we get a splat and can debug
> further.  We both might be quite sure that we are not confused, but
> that is exactly when we are most prone to making mistakes.  ;-)
> 

Same feeling :)

> I also ask that you run with this check for some time.  After all, if

Sure. I will try it immediately.

> the assumption is incorrect, the resulting SRCU hangs off in various
> systems around the world will not be so much fun to debug.
> 
> Given that this is slowpath code, it is much better to take an extra
> compare and branch than to introduce even an extremely small risk of
> hanging SRCU.
> 

Agree.


Thanks,

	Pingfan

> 							Thanx, Paul
> 
> > +			if (!srcu_invl_snp_seq(snp_seq) && (snp_seq == s)) {
> >  				if (snp == snp_leaf && snp_seq == s)
> >  					snp->srcu_data_have_cbs[idx] |= sdp->grpmask;
> >  				spin_unlock_irqrestore_rcu_node(snp, flags);
> > -				if (snp == snp_leaf && snp_seq != s) {
> > -					srcu_schedule_cbs_sdp(sdp, do_norm ? SRCU_INTERVAL : 0);
> > -					return;
> > -				}
> >  				if (!do_norm)
> >  					srcu_funnel_exp_start(ssp, snp, s);
> >  				return;
> > -- 
> > 2.31.1
> > 

Re: [PATCHv2 2/2] srcu: Eliminate the case that snp_seq bigger than snap in srcu_funnel_gp_start()

[Pingfan Liu]

On Thu, Nov 24, 2022 at 11:33 AM Pingfan Liu <kernelfans@gmail.com> wrote:
>
> On Wed, Nov 23, 2022 at 11:20:46AM -0800, Paul E. McKenney wrote:
> > On Wed, Nov 23, 2022 at 09:56:38PM +0800, Pingfan Liu wrote:
> > > Since the srcu read lock is still held during srcu_funnel_gp_start(),
> > > the seq snap should be the largest number for the slot
> > > srcu_have_cbs[idx].
> > >
> > > Signed-off-by: Pingfan Liu <kernelfans@gmail.com>
> > > Cc: Lai Jiangshan <jiangshanlai@gmail.com>
> > > Cc: "Paul E. McKenney" <paulmck@kernel.org>
> > > Cc: Frederic Weisbecker <frederic@kernel.org>
> > > Cc: Josh Triplett <josh@joshtriplett.org>
> > > Cc: Steven Rostedt <rostedt@goodmis.org>
> > > Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> > > To: rcu@vger.kernel.org
> > > ---
> > >  include/linux/rcupdate.h |  1 +
> > >  kernel/rcu/srcutree.c    | 11 ++++++-----
> > >  2 files changed, 7 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
> > > index 08605ce7379d..a09007236660 100644
> > > --- a/include/linux/rcupdate.h
> > > +++ b/include/linux/rcupdate.h
> > > @@ -32,6 +32,7 @@
> > >  #include <linux/context_tracking_irq.h>
> > >
> > >  #define ULONG_CMP_GE(a, b) (ULONG_MAX / 2 >= (a) - (b))
> > > +#define ULONG_CMP_GT(a, b) (ULONG_MAX / 2 > (a) - (b))
> >
> > Please see below...
> >
> > >  #define ULONG_CMP_LT(a, b) (ULONG_MAX / 2 < (a) - (b))
> > >  #define ulong2long(a)              (*(long *)(&(a)))
> > >  #define USHORT_CMP_GE(a, b)        (USHRT_MAX / 2 >= (unsigned short)((a) - (b)))
> > > diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
> > > index 2fc0e775ade4..41902b823687 100644
> > > --- a/kernel/rcu/srcutree.c
> > > +++ b/kernel/rcu/srcutree.c
> > > @@ -905,14 +905,15 @@ static void srcu_funnel_gp_start(struct srcu_struct *ssp, struct srcu_data *sdp,
> > >             for (snp = snp_leaf; snp != NULL; snp = snp->srcu_parent) {
> > >                     spin_lock_irqsave_rcu_node(snp, flags);
> > >                     snp_seq = snp->srcu_have_cbs[idx];
> > > -                   if (!srcu_invl_snp_seq(snp_seq) && ULONG_CMP_GE(snp_seq, s)) {
> > > +                   /*
> > > +                    * s should be the biggest in the current slot. Hence only LE is
> > > +                    * valid
> > > +                    */
> > > +                   BUG_ON(ULONG_CMP_GT(snp_seq, s));
> >
> > Why not this?  (Plus adjusting the comment above, of course.)
> >
> >                       WARN_ON_ONCE(ULONG_CMP_LT(s, snp_seq));
> >
>
> A neat solution!
>
> And what about the comment
>                         /* s should be the biggest in the current slot. */
>
> > That way we don't need ULONG_CMP_GT().  Plus if we are confused about
> > s being the biggest in the current slot, we get a splat and can debug
> > further.  We both might be quite sure that we are not confused, but
> > that is exactly when we are most prone to making mistakes.  ;-)
> >
>
> Same feeling :)
>
> > I also ask that you run with this check for some time.  After all, if
>

Forget to ask if  the test
  "tools/testing/selftests/rcutorture/bin/kvm.sh --allcpus --duration
10h --configs 18*SRCU-P"
satisfies your requirement?

Thanks

    Pingfan
> Sure. I will try it immediately.
>
> > the assumption is incorrect, the resulting SRCU hangs off in various
> > systems around the world will not be so much fun to debug.
> >
> > Given that this is slowpath code, it is much better to take an extra
> > compare and branch than to introduce even an extremely small risk of
> > hanging SRCU.
> >
>
> Agree.
>
>
> Thanks,
>
>         Pingfan
>
> >                                                       Thanx, Paul
> >
> > > +                   if (!srcu_invl_snp_seq(snp_seq) && (snp_seq == s)) {
> > >                             if (snp == snp_leaf && snp_seq == s)
> > >                                     snp->srcu_data_have_cbs[idx] |= sdp->grpmask;
> > >                             spin_unlock_irqrestore_rcu_node(snp, flags);
> > > -                           if (snp == snp_leaf && snp_seq != s) {
> > > -                                   srcu_schedule_cbs_sdp(sdp, do_norm ? SRCU_INTERVAL : 0);
> > > -                                   return;
> > > -                           }
> > >                             if (!do_norm)
> > >                                     srcu_funnel_exp_start(ssp, snp, s);
> > >                             return;
> > > --
> > > 2.31.1
> > >

Re: [PATCHv2 2/2] srcu: Eliminate the case that snp_seq bigger than snap in srcu_funnel_gp_start()

[Paul E. McKenney]

On Thu, Nov 24, 2022 at 12:06:39PM +0800, Pingfan Liu wrote:
> On Thu, Nov 24, 2022 at 11:33 AM Pingfan Liu <kernelfans@gmail.com> wrote:
> >
> > On Wed, Nov 23, 2022 at 11:20:46AM -0800, Paul E. McKenney wrote:
> > > On Wed, Nov 23, 2022 at 09:56:38PM +0800, Pingfan Liu wrote:
> > > > Since the srcu read lock is still held during srcu_funnel_gp_start(),
> > > > the seq snap should be the largest number for the slot
> > > > srcu_have_cbs[idx].
> > > >
> > > > Signed-off-by: Pingfan Liu <kernelfans@gmail.com>
> > > > Cc: Lai Jiangshan <jiangshanlai@gmail.com>
> > > > Cc: "Paul E. McKenney" <paulmck@kernel.org>
> > > > Cc: Frederic Weisbecker <frederic@kernel.org>
> > > > Cc: Josh Triplett <josh@joshtriplett.org>
> > > > Cc: Steven Rostedt <rostedt@goodmis.org>
> > > > Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> > > > To: rcu@vger.kernel.org
> > > > ---
> > > >  include/linux/rcupdate.h |  1 +
> > > >  kernel/rcu/srcutree.c    | 11 ++++++-----
> > > >  2 files changed, 7 insertions(+), 5 deletions(-)
> > > >
> > > > diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
> > > > index 08605ce7379d..a09007236660 100644
> > > > --- a/include/linux/rcupdate.h
> > > > +++ b/include/linux/rcupdate.h
> > > > @@ -32,6 +32,7 @@
> > > >  #include <linux/context_tracking_irq.h>
> > > >
> > > >  #define ULONG_CMP_GE(a, b) (ULONG_MAX / 2 >= (a) - (b))
> > > > +#define ULONG_CMP_GT(a, b) (ULONG_MAX / 2 > (a) - (b))
> > >
> > > Please see below...
> > >
> > > >  #define ULONG_CMP_LT(a, b) (ULONG_MAX / 2 < (a) - (b))
> > > >  #define ulong2long(a)              (*(long *)(&(a)))
> > > >  #define USHORT_CMP_GE(a, b)        (USHRT_MAX / 2 >= (unsigned short)((a) - (b)))
> > > > diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
> > > > index 2fc0e775ade4..41902b823687 100644
> > > > --- a/kernel/rcu/srcutree.c
> > > > +++ b/kernel/rcu/srcutree.c
> > > > @@ -905,14 +905,15 @@ static void srcu_funnel_gp_start(struct srcu_struct *ssp, struct srcu_data *sdp,
> > > >             for (snp = snp_leaf; snp != NULL; snp = snp->srcu_parent) {
> > > >                     spin_lock_irqsave_rcu_node(snp, flags);
> > > >                     snp_seq = snp->srcu_have_cbs[idx];
> > > > -                   if (!srcu_invl_snp_seq(snp_seq) && ULONG_CMP_GE(snp_seq, s)) {
> > > > +                   /*
> > > > +                    * s should be the biggest in the current slot. Hence only LE is
> > > > +                    * valid
> > > > +                    */
> > > > +                   BUG_ON(ULONG_CMP_GT(snp_seq, s));
> > >
> > > Why not this?  (Plus adjusting the comment above, of course.)
> > >
> > >                       WARN_ON_ONCE(ULONG_CMP_LT(s, snp_seq));
> > >
> >
> > A neat solution!
> >
> > And what about the comment
> >                         /* s should be the biggest in the current slot. */
> >
> > > That way we don't need ULONG_CMP_GT().  Plus if we are confused about
> > > s being the biggest in the current slot, we get a splat and can debug
> > > further.  We both might be quite sure that we are not confused, but
> > > that is exactly when we are most prone to making mistakes.  ;-)
> > >
> >
> > Same feeling :)
> >
> > > I also ask that you run with this check for some time.  After all, if
> 
> Forget to ask if  the test
>   "tools/testing/selftests/rcutorture/bin/kvm.sh --allcpus --duration
> 10h --configs 18*SRCU-P"
> satisfies your requirement?

I am thinking more in terms of adding the WARN_ON_ONCE(), letting it be
in -next and possibly mainline for a couple of years, and then if there
are no splats, start feeling more confident in the asserted relationship.

If there was a significant performance, scalability, energy-efficiency, or
simplification benefit, I would feel justified in being more aggressive.

But I am not seeing a significant benefit.

							Thanx, Paul

> Thanks
> 
>     Pingfan
> > Sure. I will try it immediately.
> >
> > > the assumption is incorrect, the resulting SRCU hangs off in various
> > > systems around the world will not be so much fun to debug.
> > >
> > > Given that this is slowpath code, it is much better to take an extra
> > > compare and branch than to introduce even an extremely small risk of
> > > hanging SRCU.
> > >
> >
> > Agree.
> >
> >
> > Thanks,
> >
> >         Pingfan
> >
> > >                                                       Thanx, Paul
> > >
> > > > +                   if (!srcu_invl_snp_seq(snp_seq) && (snp_seq == s)) {
> > > >                             if (snp == snp_leaf && snp_seq == s)
> > > >                                     snp->srcu_data_have_cbs[idx] |= sdp->grpmask;
> > > >                             spin_unlock_irqrestore_rcu_node(snp, flags);
> > > > -                           if (snp == snp_leaf && snp_seq != s) {
> > > > -                                   srcu_schedule_cbs_sdp(sdp, do_norm ? SRCU_INTERVAL : 0);
> > > > -                                   return;
> > > > -                           }
> > > >                             if (!do_norm)
> > > >                                     srcu_funnel_exp_start(ssp, snp, s);
> > > >                             return;
> > > > --
> > > > 2.31.1
> > > >

Re: [PATCHv2 2/2] srcu: Eliminate the case that snp_seq bigger than snap in srcu_funnel_gp_start()

[Paul E. McKenney]

On Wed, Nov 23, 2022 at 09:24:17PM -0800, Paul E. McKenney wrote:
> On Thu, Nov 24, 2022 at 12:06:39PM +0800, Pingfan Liu wrote:
> > On Thu, Nov 24, 2022 at 11:33 AM Pingfan Liu <kernelfans@gmail.com> wrote:
> > >
> > > On Wed, Nov 23, 2022 at 11:20:46AM -0800, Paul E. McKenney wrote:
> > > > On Wed, Nov 23, 2022 at 09:56:38PM +0800, Pingfan Liu wrote:
> > > > > Since the srcu read lock is still held during srcu_funnel_gp_start(),
> > > > > the seq snap should be the largest number for the slot
> > > > > srcu_have_cbs[idx].
> > > > >
> > > > > Signed-off-by: Pingfan Liu <kernelfans@gmail.com>
> > > > > Cc: Lai Jiangshan <jiangshanlai@gmail.com>
> > > > > Cc: "Paul E. McKenney" <paulmck@kernel.org>
> > > > > Cc: Frederic Weisbecker <frederic@kernel.org>
> > > > > Cc: Josh Triplett <josh@joshtriplett.org>
> > > > > Cc: Steven Rostedt <rostedt@goodmis.org>
> > > > > Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> > > > > To: rcu@vger.kernel.org
> > > > > ---
> > > > >  include/linux/rcupdate.h |  1 +
> > > > >  kernel/rcu/srcutree.c    | 11 ++++++-----
> > > > >  2 files changed, 7 insertions(+), 5 deletions(-)
> > > > >
> > > > > diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
> > > > > index 08605ce7379d..a09007236660 100644
> > > > > --- a/include/linux/rcupdate.h
> > > > > +++ b/include/linux/rcupdate.h
> > > > > @@ -32,6 +32,7 @@
> > > > >  #include <linux/context_tracking_irq.h>
> > > > >
> > > > >  #define ULONG_CMP_GE(a, b) (ULONG_MAX / 2 >= (a) - (b))
> > > > > +#define ULONG_CMP_GT(a, b) (ULONG_MAX / 2 > (a) - (b))
> > > >
> > > > Please see below...
> > > >
> > > > >  #define ULONG_CMP_LT(a, b) (ULONG_MAX / 2 < (a) - (b))
> > > > >  #define ulong2long(a)              (*(long *)(&(a)))
> > > > >  #define USHORT_CMP_GE(a, b)        (USHRT_MAX / 2 >= (unsigned short)((a) - (b)))
> > > > > diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
> > > > > index 2fc0e775ade4..41902b823687 100644
> > > > > --- a/kernel/rcu/srcutree.c
> > > > > +++ b/kernel/rcu/srcutree.c
> > > > > @@ -905,14 +905,15 @@ static void srcu_funnel_gp_start(struct srcu_struct *ssp, struct srcu_data *sdp,
> > > > >             for (snp = snp_leaf; snp != NULL; snp = snp->srcu_parent) {
> > > > >                     spin_lock_irqsave_rcu_node(snp, flags);
> > > > >                     snp_seq = snp->srcu_have_cbs[idx];
> > > > > -                   if (!srcu_invl_snp_seq(snp_seq) && ULONG_CMP_GE(snp_seq, s)) {
> > > > > +                   /*
> > > > > +                    * s should be the biggest in the current slot. Hence only LE is
> > > > > +                    * valid
> > > > > +                    */
> > > > > +                   BUG_ON(ULONG_CMP_GT(snp_seq, s));
> > > >
> > > > Why not this?  (Plus adjusting the comment above, of course.)
> > > >
> > > >                       WARN_ON_ONCE(ULONG_CMP_LT(s, snp_seq));
> > > >
> > >
> > > A neat solution!
> > >
> > > And what about the comment
> > >                         /* s should be the biggest in the current slot. */
> > >
> > > > That way we don't need ULONG_CMP_GT().  Plus if we are confused about
> > > > s being the biggest in the current slot, we get a splat and can debug
> > > > further.  We both might be quite sure that we are not confused, but
> > > > that is exactly when we are most prone to making mistakes.  ;-)
> > > >
> > >
> > > Same feeling :)
> > >
> > > > I also ask that you run with this check for some time.  After all, if
> > 
> > Forget to ask if  the test
> >   "tools/testing/selftests/rcutorture/bin/kvm.sh --allcpus --duration
> > 10h --configs 18*SRCU-P"
> > satisfies your requirement?
> 
> I am thinking more in terms of adding the WARN_ON_ONCE(), letting it be
> in -next and possibly mainline for a couple of years, and then if there
> are no splats, start feeling more confident in the asserted relationship.
> 
> If there was a significant performance, scalability, energy-efficiency, or
> simplification benefit, I would feel justified in being more aggressive.
> 
> But I am not seeing a significant benefit.

Ah, and before I forget -again-, have you thought through the counter-wrap
scenarios?  Please keep in mind that on a 32-bit system, those counters
can wrap quite quickly compared to typical uptimes.

							Thanx, Paul

> > Thanks
> > 
> >     Pingfan
> > > Sure. I will try it immediately.
> > >
> > > > the assumption is incorrect, the resulting SRCU hangs off in various
> > > > systems around the world will not be so much fun to debug.
> > > >
> > > > Given that this is slowpath code, it is much better to take an extra
> > > > compare and branch than to introduce even an extremely small risk of
> > > > hanging SRCU.
> > > >
> > >
> > > Agree.
> > >
> > >
> > > Thanks,
> > >
> > >         Pingfan
> > >
> > > >                                                       Thanx, Paul
> > > >
> > > > > +                   if (!srcu_invl_snp_seq(snp_seq) && (snp_seq == s)) {
> > > > >                             if (snp == snp_leaf && snp_seq == s)
> > > > >                                     snp->srcu_data_have_cbs[idx] |= sdp->grpmask;
> > > > >                             spin_unlock_irqrestore_rcu_node(snp, flags);
> > > > > -                           if (snp == snp_leaf && snp_seq != s) {
> > > > > -                                   srcu_schedule_cbs_sdp(sdp, do_norm ? SRCU_INTERVAL : 0);
> > > > > -                                   return;
> > > > > -                           }
> > > > >                             if (!do_norm)
> > > > >                                     srcu_funnel_exp_start(ssp, snp, s);
> > > > >                             return;
> > > > > --
> > > > > 2.31.1
> > > > >

Re: [PATCHv2 2/2] srcu: Eliminate the case that snp_seq bigger than snap in srcu_funnel_gp_start()

[Pingfan Liu]

On Thu, Nov 24, 2022 at 08:19:26AM -0800, Paul E. McKenney wrote:
> On Wed, Nov 23, 2022 at 09:24:17PM -0800, Paul E. McKenney wrote:
> > On Thu, Nov 24, 2022 at 12:06:39PM +0800, Pingfan Liu wrote:
> > > On Thu, Nov 24, 2022 at 11:33 AM Pingfan Liu <kernelfans@gmail.com> wrote:
> > > >
> > > > On Wed, Nov 23, 2022 at 11:20:46AM -0800, Paul E. McKenney wrote:
> > > > > On Wed, Nov 23, 2022 at 09:56:38PM +0800, Pingfan Liu wrote:
> > > > > > Since the srcu read lock is still held during srcu_funnel_gp_start(),
> > > > > > the seq snap should be the largest number for the slot
> > > > > > srcu_have_cbs[idx].
> > > > > >
> > > > > > Signed-off-by: Pingfan Liu <kernelfans@gmail.com>
> > > > > > Cc: Lai Jiangshan <jiangshanlai@gmail.com>
> > > > > > Cc: "Paul E. McKenney" <paulmck@kernel.org>
> > > > > > Cc: Frederic Weisbecker <frederic@kernel.org>
> > > > > > Cc: Josh Triplett <josh@joshtriplett.org>
> > > > > > Cc: Steven Rostedt <rostedt@goodmis.org>
> > > > > > Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> > > > > > To: rcu@vger.kernel.org
> > > > > > ---
> > > > > >  include/linux/rcupdate.h |  1 +
> > > > > >  kernel/rcu/srcutree.c    | 11 ++++++-----
> > > > > >  2 files changed, 7 insertions(+), 5 deletions(-)
> > > > > >
> > > > > > diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
> > > > > > index 08605ce7379d..a09007236660 100644
> > > > > > --- a/include/linux/rcupdate.h
> > > > > > +++ b/include/linux/rcupdate.h
> > > > > > @@ -32,6 +32,7 @@
> > > > > >  #include <linux/context_tracking_irq.h>
> > > > > >
> > > > > >  #define ULONG_CMP_GE(a, b) (ULONG_MAX / 2 >= (a) - (b))
> > > > > > +#define ULONG_CMP_GT(a, b) (ULONG_MAX / 2 > (a) - (b))
> > > > >
> > > > > Please see below...
> > > > >
> > > > > >  #define ULONG_CMP_LT(a, b) (ULONG_MAX / 2 < (a) - (b))
> > > > > >  #define ulong2long(a)              (*(long *)(&(a)))
> > > > > >  #define USHORT_CMP_GE(a, b)        (USHRT_MAX / 2 >= (unsigned short)((a) - (b)))
> > > > > > diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
> > > > > > index 2fc0e775ade4..41902b823687 100644
> > > > > > --- a/kernel/rcu/srcutree.c
> > > > > > +++ b/kernel/rcu/srcutree.c
> > > > > > @@ -905,14 +905,15 @@ static void srcu_funnel_gp_start(struct srcu_struct *ssp, struct srcu_data *sdp,
> > > > > >             for (snp = snp_leaf; snp != NULL; snp = snp->srcu_parent) {
> > > > > >                     spin_lock_irqsave_rcu_node(snp, flags);
> > > > > >                     snp_seq = snp->srcu_have_cbs[idx];
> > > > > > -                   if (!srcu_invl_snp_seq(snp_seq) && ULONG_CMP_GE(snp_seq, s)) {
> > > > > > +                   /*
> > > > > > +                    * s should be the biggest in the current slot. Hence only LE is
> > > > > > +                    * valid
> > > > > > +                    */
> > > > > > +                   BUG_ON(ULONG_CMP_GT(snp_seq, s));
> > > > >
> > > > > Why not this?  (Plus adjusting the comment above, of course.)
> > > > >
> > > > >                       WARN_ON_ONCE(ULONG_CMP_LT(s, snp_seq));
> > > > >
> > > >
> > > > A neat solution!
> > > >
> > > > And what about the comment
> > > >                         /* s should be the biggest in the current slot. */
> > > >
> > > > > That way we don't need ULONG_CMP_GT().  Plus if we are confused about
> > > > > s being the biggest in the current slot, we get a splat and can debug
> > > > > further.  We both might be quite sure that we are not confused, but
> > > > > that is exactly when we are most prone to making mistakes.  ;-)
> > > > >
> > > >
> > > > Same feeling :)
> > > >
> > > > > I also ask that you run with this check for some time.  After all, if
> > > 
> > > Forget to ask if  the test
> > >   "tools/testing/selftests/rcutorture/bin/kvm.sh --allcpus --duration
> > > 10h --configs 18*SRCU-P"
> > > satisfies your requirement?
> > 
> > I am thinking more in terms of adding the WARN_ON_ONCE(), letting it be
> > in -next and possibly mainline for a couple of years, and then if there
> > are no splats, start feeling more confident in the asserted relationship.
> > 
> > If there was a significant performance, scalability, energy-efficiency, or
> > simplification benefit, I would feel justified in being more aggressive.
> > 
> > But I am not seeing a significant benefit.
> 
> Ah, and before I forget -again-, have you thought through the counter-wrap
> scenarios?  Please keep in mind that on a 32-bit system, those counters
> can wrap quite quickly compared to typical uptimes.
> 

I think 32-bit has not significant different since the statement
  WARN_ON_ONCE(ULONG_CMP_LT(s, snp_seq));

where 's < snp_seq' is not associated with bits. Am I missing anything?

Thanks,

	Pingfan

> 							Thanx, Paul
> 
> > > Thanks
> > > 
> > >     Pingfan
> > > > Sure. I will try it immediately.
> > > >
> > > > > the assumption is incorrect, the resulting SRCU hangs off in various
> > > > > systems around the world will not be so much fun to debug.
> > > > >
> > > > > Given that this is slowpath code, it is much better to take an extra
> > > > > compare and branch than to introduce even an extremely small risk of
> > > > > hanging SRCU.
> > > > >
> > > >
> > > > Agree.
> > > >
> > > >
> > > > Thanks,
> > > >
> > > >         Pingfan
> > > >
> > > > >                                                       Thanx, Paul
> > > > >
> > > > > > +                   if (!srcu_invl_snp_seq(snp_seq) && (snp_seq == s)) {
> > > > > >                             if (snp == snp_leaf && snp_seq == s)
> > > > > >                                     snp->srcu_data_have_cbs[idx] |= sdp->grpmask;
> > > > > >                             spin_unlock_irqrestore_rcu_node(snp, flags);
> > > > > > -                           if (snp == snp_leaf && snp_seq != s) {
> > > > > > -                                   srcu_schedule_cbs_sdp(sdp, do_norm ? SRCU_INTERVAL : 0);
> > > > > > -                                   return;
> > > > > > -                           }
> > > > > >                             if (!do_norm)
> > > > > >                                     srcu_funnel_exp_start(ssp, snp, s);
> > > > > >                             return;
> > > > > > --
> > > > > > 2.31.1
> > > > > >

Re: [PATCHv2 2/2] srcu: Eliminate the case that snp_seq bigger than snap in srcu_funnel_gp_start()

[Paul E. McKenney]

On Fri, Nov 25, 2022 at 08:37:03PM +0800, Pingfan Liu wrote:
> On Thu, Nov 24, 2022 at 08:19:26AM -0800, Paul E. McKenney wrote:
> > On Wed, Nov 23, 2022 at 09:24:17PM -0800, Paul E. McKenney wrote:
> > > On Thu, Nov 24, 2022 at 12:06:39PM +0800, Pingfan Liu wrote:
> > > > On Thu, Nov 24, 2022 at 11:33 AM Pingfan Liu <kernelfans@gmail.com> wrote:
> > > > > On Wed, Nov 23, 2022 at 11:20:46AM -0800, Paul E. McKenney wrote:
> > > > > > On Wed, Nov 23, 2022 at 09:56:38PM +0800, Pingfan Liu wrote:
> > > > > > > Since the srcu read lock is still held during srcu_funnel_gp_start(),
> > > > > > > the seq snap should be the largest number for the slot
> > > > > > > srcu_have_cbs[idx].
> > > > > > >
> > > > > > > Signed-off-by: Pingfan Liu <kernelfans@gmail.com>
> > > > > > > Cc: Lai Jiangshan <jiangshanlai@gmail.com>
> > > > > > > Cc: "Paul E. McKenney" <paulmck@kernel.org>
> > > > > > > Cc: Frederic Weisbecker <frederic@kernel.org>
> > > > > > > Cc: Josh Triplett <josh@joshtriplett.org>
> > > > > > > Cc: Steven Rostedt <rostedt@goodmis.org>
> > > > > > > Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> > > > > > > To: rcu@vger.kernel.org
> > > > > > > ---
> > > > > > >  include/linux/rcupdate.h |  1 +
> > > > > > >  kernel/rcu/srcutree.c    | 11 ++++++-----
> > > > > > >  2 files changed, 7 insertions(+), 5 deletions(-)
> > > > > > >
> > > > > > > diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
> > > > > > > index 08605ce7379d..a09007236660 100644
> > > > > > > --- a/include/linux/rcupdate.h
> > > > > > > +++ b/include/linux/rcupdate.h
> > > > > > > @@ -32,6 +32,7 @@
> > > > > > >  #include <linux/context_tracking_irq.h>
> > > > > > >
> > > > > > >  #define ULONG_CMP_GE(a, b) (ULONG_MAX / 2 >= (a) - (b))
> > > > > > > +#define ULONG_CMP_GT(a, b) (ULONG_MAX / 2 > (a) - (b))
> > > > > >
> > > > > > Please see below...
> > > > > >
> > > > > > >  #define ULONG_CMP_LT(a, b) (ULONG_MAX / 2 < (a) - (b))
> > > > > > >  #define ulong2long(a)              (*(long *)(&(a)))
> > > > > > >  #define USHORT_CMP_GE(a, b)        (USHRT_MAX / 2 >= (unsigned short)((a) - (b)))
> > > > > > > diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
> > > > > > > index 2fc0e775ade4..41902b823687 100644
> > > > > > > --- a/kernel/rcu/srcutree.c
> > > > > > > +++ b/kernel/rcu/srcutree.c
> > > > > > > @@ -905,14 +905,15 @@ static void srcu_funnel_gp_start(struct srcu_struct *ssp, struct srcu_data *sdp,
> > > > > > >             for (snp = snp_leaf; snp != NULL; snp = snp->srcu_parent) {
> > > > > > >                     spin_lock_irqsave_rcu_node(snp, flags);
> > > > > > >                     snp_seq = snp->srcu_have_cbs[idx];
> > > > > > > -                   if (!srcu_invl_snp_seq(snp_seq) && ULONG_CMP_GE(snp_seq, s)) {
> > > > > > > +                   /*
> > > > > > > +                    * s should be the biggest in the current slot. Hence only LE is
> > > > > > > +                    * valid
> > > > > > > +                    */
> > > > > > > +                   BUG_ON(ULONG_CMP_GT(snp_seq, s));
> > > > > >
> > > > > > Why not this?  (Plus adjusting the comment above, of course.)
> > > > > >
> > > > > >                       WARN_ON_ONCE(ULONG_CMP_LT(s, snp_seq));
> > > > > >
> > > > >
> > > > > A neat solution!
> > > > >
> > > > > And what about the comment
> > > > >                         /* s should be the biggest in the current slot. */
> > > > >
> > > > > > That way we don't need ULONG_CMP_GT().  Plus if we are confused about
> > > > > > s being the biggest in the current slot, we get a splat and can debug
> > > > > > further.  We both might be quite sure that we are not confused, but
> > > > > > that is exactly when we are most prone to making mistakes.  ;-)
> > > > > >
> > > > >
> > > > > Same feeling :)
> > > > >
> > > > > > I also ask that you run with this check for some time.  After all, if
> > > > 
> > > > Forget to ask if  the test
> > > >   "tools/testing/selftests/rcutorture/bin/kvm.sh --allcpus --duration
> > > > 10h --configs 18*SRCU-P"
> > > > satisfies your requirement?
> > > 
> > > I am thinking more in terms of adding the WARN_ON_ONCE(), letting it be
> > > in -next and possibly mainline for a couple of years, and then if there
> > > are no splats, start feeling more confident in the asserted relationship.
> > > 
> > > If there was a significant performance, scalability, energy-efficiency, or
> > > simplification benefit, I would feel justified in being more aggressive.
> > > 
> > > But I am not seeing a significant benefit.
> > 
> > Ah, and before I forget -again-, have you thought through the counter-wrap
> > scenarios?  Please keep in mind that on a 32-bit system, those counters
> > can wrap quite quickly compared to typical uptimes.
> 
> I think 32-bit has not significant different since the statement
>   WARN_ON_ONCE(ULONG_CMP_LT(s, snp_seq));
> 
> where 's < snp_seq' is not associated with bits. Am I missing anything?

Your point is that long before there was any danger of wrap, this
WARN_ON_ONCE() would trigger?

If so, the point is to avoid that WARN_ON_ONCE() from triggering.

Or am I missing your point?

							Thanx, Paul

> Thanks,
> 
> 	Pingfan
> 
> > 							Thanx, Paul
> > 
> > > > Thanks
> > > > 
> > > >     Pingfan
> > > > > Sure. I will try it immediately.
> > > > >
> > > > > > the assumption is incorrect, the resulting SRCU hangs off in various
> > > > > > systems around the world will not be so much fun to debug.
> > > > > >
> > > > > > Given that this is slowpath code, it is much better to take an extra
> > > > > > compare and branch than to introduce even an extremely small risk of
> > > > > > hanging SRCU.
> > > > > >
> > > > >
> > > > > Agree.
> > > > >
> > > > >
> > > > > Thanks,
> > > > >
> > > > >         Pingfan
> > > > >
> > > > > >                                                       Thanx, Paul
> > > > > >
> > > > > > > +                   if (!srcu_invl_snp_seq(snp_seq) && (snp_seq == s)) {
> > > > > > >                             if (snp == snp_leaf && snp_seq == s)
> > > > > > >                                     snp->srcu_data_have_cbs[idx] |= sdp->grpmask;
> > > > > > >                             spin_unlock_irqrestore_rcu_node(snp, flags);
> > > > > > > -                           if (snp == snp_leaf && snp_seq != s) {
> > > > > > > -                                   srcu_schedule_cbs_sdp(sdp, do_norm ? SRCU_INTERVAL : 0);
> > > > > > > -                                   return;
> > > > > > > -                           }
> > > > > > >                             if (!do_norm)
> > > > > > >                                     srcu_funnel_exp_start(ssp, snp, s);
> > > > > > >                             return;
> > > > > > > --
> > > > > > > 2.31.1
> > > > > > >

Re: [PATCHv2 2/2] srcu: Eliminate the case that snp_seq bigger than snap in srcu_funnel_gp_start()

[Paul E. McKenney]

On Sat, Nov 26, 2022 at 10:03:52AM -0800, Paul E. McKenney wrote:
> On Fri, Nov 25, 2022 at 08:37:03PM +0800, Pingfan Liu wrote:
> > On Thu, Nov 24, 2022 at 08:19:26AM -0800, Paul E. McKenney wrote:
> > > On Wed, Nov 23, 2022 at 09:24:17PM -0800, Paul E. McKenney wrote:
> > > > On Thu, Nov 24, 2022 at 12:06:39PM +0800, Pingfan Liu wrote:
> > > > > On Thu, Nov 24, 2022 at 11:33 AM Pingfan Liu <kernelfans@gmail.com> wrote:
> > > > > > On Wed, Nov 23, 2022 at 11:20:46AM -0800, Paul E. McKenney wrote:
> > > > > > > On Wed, Nov 23, 2022 at 09:56:38PM +0800, Pingfan Liu wrote:
> > > > > > > > Since the srcu read lock is still held during srcu_funnel_gp_start(),
> > > > > > > > the seq snap should be the largest number for the slot
> > > > > > > > srcu_have_cbs[idx].
> > > > > > > >
> > > > > > > > Signed-off-by: Pingfan Liu <kernelfans@gmail.com>
> > > > > > > > Cc: Lai Jiangshan <jiangshanlai@gmail.com>
> > > > > > > > Cc: "Paul E. McKenney" <paulmck@kernel.org>
> > > > > > > > Cc: Frederic Weisbecker <frederic@kernel.org>
> > > > > > > > Cc: Josh Triplett <josh@joshtriplett.org>
> > > > > > > > Cc: Steven Rostedt <rostedt@goodmis.org>
> > > > > > > > Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> > > > > > > > To: rcu@vger.kernel.org
> > > > > > > > ---
> > > > > > > >  include/linux/rcupdate.h |  1 +
> > > > > > > >  kernel/rcu/srcutree.c    | 11 ++++++-----
> > > > > > > >  2 files changed, 7 insertions(+), 5 deletions(-)
> > > > > > > >
> > > > > > > > diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
> > > > > > > > index 08605ce7379d..a09007236660 100644
> > > > > > > > --- a/include/linux/rcupdate.h
> > > > > > > > +++ b/include/linux/rcupdate.h
> > > > > > > > @@ -32,6 +32,7 @@
> > > > > > > >  #include <linux/context_tracking_irq.h>
> > > > > > > >
> > > > > > > >  #define ULONG_CMP_GE(a, b) (ULONG_MAX / 2 >= (a) - (b))
> > > > > > > > +#define ULONG_CMP_GT(a, b) (ULONG_MAX / 2 > (a) - (b))
> > > > > > >
> > > > > > > Please see below...
> > > > > > >
> > > > > > > >  #define ULONG_CMP_LT(a, b) (ULONG_MAX / 2 < (a) - (b))
> > > > > > > >  #define ulong2long(a)              (*(long *)(&(a)))
> > > > > > > >  #define USHORT_CMP_GE(a, b)        (USHRT_MAX / 2 >= (unsigned short)((a) - (b)))
> > > > > > > > diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
> > > > > > > > index 2fc0e775ade4..41902b823687 100644
> > > > > > > > --- a/kernel/rcu/srcutree.c
> > > > > > > > +++ b/kernel/rcu/srcutree.c
> > > > > > > > @@ -905,14 +905,15 @@ static void srcu_funnel_gp_start(struct srcu_struct *ssp, struct srcu_data *sdp,
> > > > > > > >             for (snp = snp_leaf; snp != NULL; snp = snp->srcu_parent) {
> > > > > > > >                     spin_lock_irqsave_rcu_node(snp, flags);
> > > > > > > >                     snp_seq = snp->srcu_have_cbs[idx];
> > > > > > > > -                   if (!srcu_invl_snp_seq(snp_seq) && ULONG_CMP_GE(snp_seq, s)) {
> > > > > > > > +                   /*
> > > > > > > > +                    * s should be the biggest in the current slot. Hence only LE is
> > > > > > > > +                    * valid
> > > > > > > > +                    */
> > > > > > > > +                   BUG_ON(ULONG_CMP_GT(snp_seq, s));
> > > > > > >
> > > > > > > Why not this?  (Plus adjusting the comment above, of course.)
> > > > > > >
> > > > > > >                       WARN_ON_ONCE(ULONG_CMP_LT(s, snp_seq));
> > > > > > >
> > > > > >
> > > > > > A neat solution!
> > > > > >
> > > > > > And what about the comment
> > > > > >                         /* s should be the biggest in the current slot. */
> > > > > >
> > > > > > > That way we don't need ULONG_CMP_GT().  Plus if we are confused about
> > > > > > > s being the biggest in the current slot, we get a splat and can debug
> > > > > > > further.  We both might be quite sure that we are not confused, but
> > > > > > > that is exactly when we are most prone to making mistakes.  ;-)
> > > > > > >
> > > > > >
> > > > > > Same feeling :)
> > > > > >
> > > > > > > I also ask that you run with this check for some time.  After all, if
> > > > > 
> > > > > Forget to ask if  the test
> > > > >   "tools/testing/selftests/rcutorture/bin/kvm.sh --allcpus --duration
> > > > > 10h --configs 18*SRCU-P"
> > > > > satisfies your requirement?
> > > > 
> > > > I am thinking more in terms of adding the WARN_ON_ONCE(), letting it be
> > > > in -next and possibly mainline for a couple of years, and then if there
> > > > are no splats, start feeling more confident in the asserted relationship.
> > > > 
> > > > If there was a significant performance, scalability, energy-efficiency, or
> > > > simplification benefit, I would feel justified in being more aggressive.
> > > > 
> > > > But I am not seeing a significant benefit.
> > > 
> > > Ah, and before I forget -again-, have you thought through the counter-wrap
> > > scenarios?  Please keep in mind that on a 32-bit system, those counters
> > > can wrap quite quickly compared to typical uptimes.
> > 
> > I think 32-bit has not significant different since the statement
> >   WARN_ON_ONCE(ULONG_CMP_LT(s, snp_seq));
> > 
> > where 's < snp_seq' is not associated with bits. Am I missing anything?
> 
> Your point is that long before there was any danger of wrap, this
> WARN_ON_ONCE() would trigger?
> 
> If so, the point is to avoid that WARN_ON_ONCE() from triggering.
> 
> Or am I missing your point?

For example, are you instead saying that the WARN_ON_ONCE() proposed in
your patch will detect the problem either way?  If so, what I am asking
is whether your earlier analysis considered wrap without the addition
of that WARN_ON_ONCE().

							Thanx, Paul

> > Thanks,
> > 
> > 	Pingfan
> > 
> > > 							Thanx, Paul
> > > 
> > > > > Thanks
> > > > > 
> > > > >     Pingfan
> > > > > > Sure. I will try it immediately.
> > > > > >
> > > > > > > the assumption is incorrect, the resulting SRCU hangs off in various
> > > > > > > systems around the world will not be so much fun to debug.
> > > > > > >
> > > > > > > Given that this is slowpath code, it is much better to take an extra
> > > > > > > compare and branch than to introduce even an extremely small risk of
> > > > > > > hanging SRCU.
> > > > > > >
> > > > > >
> > > > > > Agree.
> > > > > >
> > > > > >
> > > > > > Thanks,
> > > > > >
> > > > > >         Pingfan
> > > > > >
> > > > > > >                                                       Thanx, Paul
> > > > > > >
> > > > > > > > +                   if (!srcu_invl_snp_seq(snp_seq) && (snp_seq == s)) {
> > > > > > > >                             if (snp == snp_leaf && snp_seq == s)
> > > > > > > >                                     snp->srcu_data_have_cbs[idx] |= sdp->grpmask;
> > > > > > > >                             spin_unlock_irqrestore_rcu_node(snp, flags);
> > > > > > > > -                           if (snp == snp_leaf && snp_seq != s) {
> > > > > > > > -                                   srcu_schedule_cbs_sdp(sdp, do_norm ? SRCU_INTERVAL : 0);
> > > > > > > > -                                   return;
> > > > > > > > -                           }
> > > > > > > >                             if (!do_norm)
> > > > > > > >                                     srcu_funnel_exp_start(ssp, snp, s);
> > > > > > > >                             return;
> > > > > > > > --
> > > > > > > > 2.31.1
> > > > > > > >

Re: [PATCHv2 2/2] srcu: Eliminate the case that snp_seq bigger than snap in srcu_funnel_gp_start()

[Joel Fernandes]

> On Nov 26, 2022, at 1:07 PM, Paul E. McKenney <paulmck@kernel.org> wrote:
> 
> On Sat, Nov 26, 2022 at 10:03:52AM -0800, Paul E. McKenney wrote:
>>> On Fri, Nov 25, 2022 at 08:37:03PM +0800, Pingfan Liu wrote:
>>> On Thu, Nov 24, 2022 at 08:19:26AM -0800, Paul E. McKenney wrote:
>>>> On Wed, Nov 23, 2022 at 09:24:17PM -0800, Paul E. McKenney wrote:
>>>>> On Thu, Nov 24, 2022 at 12:06:39PM +0800, Pingfan Liu wrote:
>>>>>> On Thu, Nov 24, 2022 at 11:33 AM Pingfan Liu <kernelfans@gmail.com> wrote:
>>>>>>> On Wed, Nov 23, 2022 at 11:20:46AM -0800, Paul E. McKenney wrote:
>>>>>>>> On Wed, Nov 23, 2022 at 09:56:38PM +0800, Pingfan Liu wrote:
>>>>>>>>> Since the srcu read lock is still held during srcu_funnel_gp_start(),
>>>>>>>>> the seq snap should be the largest number for the slot
>>>>>>>>> srcu_have_cbs[idx].
>>>>>>>>> 
>>>>>>>>> Signed-off-by: Pingfan Liu <kernelfans@gmail.com>
>>>>>>>>> Cc: Lai Jiangshan <jiangshanlai@gmail.com>
>>>>>>>>> Cc: "Paul E. McKenney" <paulmck@kernel.org>
>>>>>>>>> Cc: Frederic Weisbecker <frederic@kernel.org>
>>>>>>>>> Cc: Josh Triplett <josh@joshtriplett.org>
>>>>>>>>> Cc: Steven Rostedt <rostedt@goodmis.org>
>>>>>>>>> Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
>>>>>>>>> To: rcu@vger.kernel.org
>>>>>>>>> ---
>>>>>>>>> include/linux/rcupdate.h |  1 +
>>>>>>>>> kernel/rcu/srcutree.c    | 11 ++++++-----
>>>>>>>>> 2 files changed, 7 insertions(+), 5 deletions(-)
>>>>>>>>> 
>>>>>>>>> diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
>>>>>>>>> index 08605ce7379d..a09007236660 100644
>>>>>>>>> --- a/include/linux/rcupdate.h
>>>>>>>>> +++ b/include/linux/rcupdate.h
>>>>>>>>> @@ -32,6 +32,7 @@
>>>>>>>>> #include <linux/context_tracking_irq.h>
>>>>>>>>> 
>>>>>>>>> #define ULONG_CMP_GE(a, b) (ULONG_MAX / 2 >= (a) - (b))
>>>>>>>>> +#define ULONG_CMP_GT(a, b) (ULONG_MAX / 2 > (a) - (b))
>>>>>>>> 
>>>>>>>> Please see below...
>>>>>>>> 
>>>>>>>>> #define ULONG_CMP_LT(a, b) (ULONG_MAX / 2 < (a) - (b))
>>>>>>>>> #define ulong2long(a)              (*(long *)(&(a)))
>>>>>>>>> #define USHORT_CMP_GE(a, b)        (USHRT_MAX / 2 >= (unsigned short)((a) - (b)))
>>>>>>>>> diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
>>>>>>>>> index 2fc0e775ade4..41902b823687 100644
>>>>>>>>> --- a/kernel/rcu/srcutree.c
>>>>>>>>> +++ b/kernel/rcu/srcutree.c
>>>>>>>>> @@ -905,14 +905,15 @@ static void srcu_funnel_gp_start(struct srcu_struct *ssp, struct srcu_data *sdp,
>>>>>>>>>            for (snp = snp_leaf; snp != NULL; snp = snp->srcu_parent) {
>>>>>>>>>                    spin_lock_irqsave_rcu_node(snp, flags);
>>>>>>>>>                    snp_seq = snp->srcu_have_cbs[idx];
>>>>>>>>> -                   if (!srcu_invl_snp_seq(snp_seq) && ULONG_CMP_GE(snp_seq, s)) {
>>>>>>>>> +                   /*
>>>>>>>>> +                    * s should be the biggest in the current slot. Hence only LE is
>>>>>>>>> +                    * valid
>>>>>>>>> +                    */
>>>>>>>>> +                   BUG_ON(ULONG_CMP_GT(snp_seq, s));
>>>>>>>> 
>>>>>>>> Why not this?  (Plus adjusting the comment above, of course.)
>>>>>>>> 
>>>>>>>>                      WARN_ON_ONCE(ULONG_CMP_LT(s, snp_seq));
>>>>>>>> 
>>>>>>> 
>>>>>>> A neat solution!
>>>>>>> 
>>>>>>> And what about the comment
>>>>>>>                        /* s should be the biggest in the current slot. */
>>>>>>> 
>>>>>>>> That way we don't need ULONG_CMP_GT().  Plus if we are confused about
>>>>>>>> s being the biggest in the current slot, we get a splat and can debug
>>>>>>>> further.  We both might be quite sure that we are not confused, but
>>>>>>>> that is exactly when we are most prone to making mistakes.  ;-)
>>>>>>>> 
>>>>>>> 
>>>>>>> Same feeling :)
>>>>>>> 
>>>>>>>> I also ask that you run with this check for some time.  After all, if
>>>>>> 
>>>>>> Forget to ask if  the test
>>>>>>  "tools/testing/selftests/rcutorture/bin/kvm.sh --allcpus --duration
>>>>>> 10h --configs 18*SRCU-P"
>>>>>> satisfies your requirement?
>>>>> 
>>>>> I am thinking more in terms of adding the WARN_ON_ONCE(), letting it be
>>>>> in -next and possibly mainline for a couple of years, and then if there
>>>>> are no splats, start feeling more confident in the asserted relationship.
>>>>> 
>>>>> If there was a significant performance, scalability, energy-efficiency, or
>>>>> simplification benefit, I would feel justified in being more aggressive.
>>>>> 
>>>>> But I am not seeing a significant benefit.
>>>> 
>>>> Ah, and before I forget -again-, have you thought through the counter-wrap
>>>> scenarios?  Please keep in mind that on a 32-bit system, those counters
>>>> can wrap quite quickly compared to typical uptimes.
>>> 
>>> I think 32-bit has not significant different since the statement
>>>  WARN_ON_ONCE(ULONG_CMP_LT(s, snp_seq));
>>> 
>>> where 's < snp_seq' is not associated with bits. Am I missing anything?
>> 
>> Your point is that long before there was any danger of wrap, this
>> WARN_ON_ONCE() would trigger?
>> 
>> If so, the point is to avoid that WARN_ON_ONCE() from triggering.
>> 
>> Or am I missing your point?
> 
> For example, are you instead saying that the WARN_ON_ONCE() proposed in
> your patch will detect the problem either way?  If so, what I am asking
> is whether your earlier analysis considered wrap without the addition
> of that WARN_ON_ONCE().

Do we want to make some rules on when to delete WARNs and fold in the change, such as a future date after scanning the list for reports? Such date can be embedded in a comment on top of the warning.

Thanks,

    - Joel

>                            Thanx, Paul
> 
>>> Thanks,
>>> 
>>>    Pingfan
>>> 
>>>>                            Thanx, Paul
>>>> 
>>>>>> Thanks
>>>>>> 
>>>>>>    Pingfan
>>>>>>> Sure. I will try it immediately.
>>>>>>> 
>>>>>>>> the assumption is incorrect, the resulting SRCU hangs off in various
>>>>>>>> systems around the world will not be so much fun to debug.
>>>>>>>> 
>>>>>>>> Given that this is slowpath code, it is much better to take an extra
>>>>>>>> compare and branch than to introduce even an extremely small risk of
>>>>>>>> hanging SRCU.
>>>>>>>> 
>>>>>>> 
>>>>>>> Agree.
>>>>>>> 
>>>>>>> 
>>>>>>> Thanks,
>>>>>>> 
>>>>>>>        Pingfan
>>>>>>> 
>>>>>>>>                                                      Thanx, Paul
>>>>>>>> 
>>>>>>>>> +                   if (!srcu_invl_snp_seq(snp_seq) && (snp_seq == s)) {
>>>>>>>>>                            if (snp == snp_leaf && snp_seq == s)
>>>>>>>>>                                    snp->srcu_data_have_cbs[idx] |= sdp->grpmask;
>>>>>>>>>                            spin_unlock_irqrestore_rcu_node(snp, flags);
>>>>>>>>> -                           if (snp == snp_leaf && snp_seq != s) {
>>>>>>>>> -                                   srcu_schedule_cbs_sdp(sdp, do_norm ? SRCU_INTERVAL : 0);
>>>>>>>>> -                                   return;
>>>>>>>>> -                           }
>>>>>>>>>                            if (!do_norm)
>>>>>>>>>                                    srcu_funnel_exp_start(ssp, snp, s);
>>>>>>>>>                            return;
>>>>>>>>> --
>>>>>>>>> 2.31.1
>>>>>>>>> 

Re: [PATCHv2 2/2] srcu: Eliminate the case that snp_seq bigger than snap in srcu_funnel_gp_start()

[Paul E. McKenney]

On Sun, Nov 27, 2022 at 09:46:51AM -0500, Joel Fernandes wrote:
> > On Nov 26, 2022, at 1:07 PM, Paul E. McKenney <paulmck@kernel.org> wrote:
> > On Sat, Nov 26, 2022 at 10:03:52AM -0800, Paul E. McKenney wrote:
> >>> On Fri, Nov 25, 2022 at 08:37:03PM +0800, Pingfan Liu wrote:
> >>> On Thu, Nov 24, 2022 at 08:19:26AM -0800, Paul E. McKenney wrote:
> >>>> On Wed, Nov 23, 2022 at 09:24:17PM -0800, Paul E. McKenney wrote:
> >>>>> On Thu, Nov 24, 2022 at 12:06:39PM +0800, Pingfan Liu wrote:
> >>>>>> On Thu, Nov 24, 2022 at 11:33 AM Pingfan Liu <kernelfans@gmail.com> wrote:
> >>>>>>> On Wed, Nov 23, 2022 at 11:20:46AM -0800, Paul E. McKenney wrote:
> >>>>>>>> On Wed, Nov 23, 2022 at 09:56:38PM +0800, Pingfan Liu wrote:
> >>>>>>>>> Since the srcu read lock is still held during srcu_funnel_gp_start(),
> >>>>>>>>> the seq snap should be the largest number for the slot
> >>>>>>>>> srcu_have_cbs[idx].
> >>>>>>>>> 
> >>>>>>>>> Signed-off-by: Pingfan Liu <kernelfans@gmail.com>
> >>>>>>>>> Cc: Lai Jiangshan <jiangshanlai@gmail.com>
> >>>>>>>>> Cc: "Paul E. McKenney" <paulmck@kernel.org>
> >>>>>>>>> Cc: Frederic Weisbecker <frederic@kernel.org>
> >>>>>>>>> Cc: Josh Triplett <josh@joshtriplett.org>
> >>>>>>>>> Cc: Steven Rostedt <rostedt@goodmis.org>
> >>>>>>>>> Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> >>>>>>>>> To: rcu@vger.kernel.org
> >>>>>>>>> ---
> >>>>>>>>> include/linux/rcupdate.h |  1 +
> >>>>>>>>> kernel/rcu/srcutree.c    | 11 ++++++-----
> >>>>>>>>> 2 files changed, 7 insertions(+), 5 deletions(-)
> >>>>>>>>> 
> >>>>>>>>> diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
> >>>>>>>>> index 08605ce7379d..a09007236660 100644
> >>>>>>>>> --- a/include/linux/rcupdate.h
> >>>>>>>>> +++ b/include/linux/rcupdate.h
> >>>>>>>>> @@ -32,6 +32,7 @@
> >>>>>>>>> #include <linux/context_tracking_irq.h>
> >>>>>>>>> 
> >>>>>>>>> #define ULONG_CMP_GE(a, b) (ULONG_MAX / 2 >= (a) - (b))
> >>>>>>>>> +#define ULONG_CMP_GT(a, b) (ULONG_MAX / 2 > (a) - (b))
> >>>>>>>> 
> >>>>>>>> Please see below...
> >>>>>>>> 
> >>>>>>>>> #define ULONG_CMP_LT(a, b) (ULONG_MAX / 2 < (a) - (b))
> >>>>>>>>> #define ulong2long(a)              (*(long *)(&(a)))
> >>>>>>>>> #define USHORT_CMP_GE(a, b)        (USHRT_MAX / 2 >= (unsigned short)((a) - (b)))
> >>>>>>>>> diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
> >>>>>>>>> index 2fc0e775ade4..41902b823687 100644
> >>>>>>>>> --- a/kernel/rcu/srcutree.c
> >>>>>>>>> +++ b/kernel/rcu/srcutree.c
> >>>>>>>>> @@ -905,14 +905,15 @@ static void srcu_funnel_gp_start(struct srcu_struct *ssp, struct srcu_data *sdp,
> >>>>>>>>>            for (snp = snp_leaf; snp != NULL; snp = snp->srcu_parent) {
> >>>>>>>>>                    spin_lock_irqsave_rcu_node(snp, flags);
> >>>>>>>>>                    snp_seq = snp->srcu_have_cbs[idx];
> >>>>>>>>> -                   if (!srcu_invl_snp_seq(snp_seq) && ULONG_CMP_GE(snp_seq, s)) {
> >>>>>>>>> +                   /*
> >>>>>>>>> +                    * s should be the biggest in the current slot. Hence only LE is
> >>>>>>>>> +                    * valid
> >>>>>>>>> +                    */
> >>>>>>>>> +                   BUG_ON(ULONG_CMP_GT(snp_seq, s));
> >>>>>>>> 
> >>>>>>>> Why not this?  (Plus adjusting the comment above, of course.)
> >>>>>>>> 
> >>>>>>>>                      WARN_ON_ONCE(ULONG_CMP_LT(s, snp_seq));
> >>>>>>>> 
> >>>>>>> 
> >>>>>>> A neat solution!
> >>>>>>> 
> >>>>>>> And what about the comment
> >>>>>>>                        /* s should be the biggest in the current slot. */
> >>>>>>> 
> >>>>>>>> That way we don't need ULONG_CMP_GT().  Plus if we are confused about
> >>>>>>>> s being the biggest in the current slot, we get a splat and can debug
> >>>>>>>> further.  We both might be quite sure that we are not confused, but
> >>>>>>>> that is exactly when we are most prone to making mistakes.  ;-)
> >>>>>>>> 
> >>>>>>> 
> >>>>>>> Same feeling :)
> >>>>>>> 
> >>>>>>>> I also ask that you run with this check for some time.  After all, if
> >>>>>> 
> >>>>>> Forget to ask if  the test
> >>>>>>  "tools/testing/selftests/rcutorture/bin/kvm.sh --allcpus --duration
> >>>>>> 10h --configs 18*SRCU-P"
> >>>>>> satisfies your requirement?
> >>>>> 
> >>>>> I am thinking more in terms of adding the WARN_ON_ONCE(), letting it be
> >>>>> in -next and possibly mainline for a couple of years, and then if there
> >>>>> are no splats, start feeling more confident in the asserted relationship.
> >>>>> 
> >>>>> If there was a significant performance, scalability, energy-efficiency, or
> >>>>> simplification benefit, I would feel justified in being more aggressive.
> >>>>> 
> >>>>> But I am not seeing a significant benefit.
> >>>> 
> >>>> Ah, and before I forget -again-, have you thought through the counter-wrap
> >>>> scenarios?  Please keep in mind that on a 32-bit system, those counters
> >>>> can wrap quite quickly compared to typical uptimes.
> >>> 
> >>> I think 32-bit has not significant different since the statement
> >>>  WARN_ON_ONCE(ULONG_CMP_LT(s, snp_seq));
> >>> 
> >>> where 's < snp_seq' is not associated with bits. Am I missing anything?
> >> 
> >> Your point is that long before there was any danger of wrap, this
> >> WARN_ON_ONCE() would trigger?
> >> 
> >> If so, the point is to avoid that WARN_ON_ONCE() from triggering.
> >> 
> >> Or am I missing your point?
> > 
> > For example, are you instead saying that the WARN_ON_ONCE() proposed in
> > your patch will detect the problem either way?  If so, what I am asking
> > is whether your earlier analysis considered wrap without the addition
> > of that WARN_ON_ONCE().
> 
> Do we want to make some rules on when to delete WARNs and fold in the
> change, such as a future date after scanning the list for reports? Such
> date can be embedded in a comment on top of the warning.

Possibly, but this question is about adding a warning.  If wrap was
not considered, let's consider wrap and see if removing the store is
appropriate.  Only if removing the store is appropriate does it make
sense to add the warning.

							Thanx, Paul

> Thanks,
> 
>     - Joel
> 
> >                            Thanx, Paul
> > 
> >>> Thanks,
> >>> 
> >>>    Pingfan
> >>> 
> >>>>                            Thanx, Paul
> >>>> 
> >>>>>> Thanks
> >>>>>> 
> >>>>>>    Pingfan
> >>>>>>> Sure. I will try it immediately.
> >>>>>>> 
> >>>>>>>> the assumption is incorrect, the resulting SRCU hangs off in various
> >>>>>>>> systems around the world will not be so much fun to debug.
> >>>>>>>> 
> >>>>>>>> Given that this is slowpath code, it is much better to take an extra
> >>>>>>>> compare and branch than to introduce even an extremely small risk of
> >>>>>>>> hanging SRCU.
> >>>>>>>> 
> >>>>>>> 
> >>>>>>> Agree.
> >>>>>>> 
> >>>>>>> 
> >>>>>>> Thanks,
> >>>>>>> 
> >>>>>>>        Pingfan
> >>>>>>> 
> >>>>>>>>                                                      Thanx, Paul
> >>>>>>>> 
> >>>>>>>>> +                   if (!srcu_invl_snp_seq(snp_seq) && (snp_seq == s)) {
> >>>>>>>>>                            if (snp == snp_leaf && snp_seq == s)
> >>>>>>>>>                                    snp->srcu_data_have_cbs[idx] |= sdp->grpmask;
> >>>>>>>>>                            spin_unlock_irqrestore_rcu_node(snp, flags);
> >>>>>>>>> -                           if (snp == snp_leaf && snp_seq != s) {
> >>>>>>>>> -                                   srcu_schedule_cbs_sdp(sdp, do_norm ? SRCU_INTERVAL : 0);
> >>>>>>>>> -                                   return;
> >>>>>>>>> -                           }
> >>>>>>>>>                            if (!do_norm)
> >>>>>>>>>                                    srcu_funnel_exp_start(ssp, snp, s);
> >>>>>>>>>                            return;
> >>>>>>>>> --
> >>>>>>>>> 2.31.1
> >>>>>>>>> 

Re: [PATCHv2 2/2] srcu: Eliminate the case that snp_seq bigger than snap in srcu_funnel_gp_start()

[Pingfan Liu]

On Sat, Nov 26, 2022 at 10:07:04AM -0800, Paul E. McKenney wrote:
> On Sat, Nov 26, 2022 at 10:03:52AM -0800, Paul E. McKenney wrote:
> > On Fri, Nov 25, 2022 at 08:37:03PM +0800, Pingfan Liu wrote:
> > > On Thu, Nov 24, 2022 at 08:19:26AM -0800, Paul E. McKenney wrote:
> > > > On Wed, Nov 23, 2022 at 09:24:17PM -0800, Paul E. McKenney wrote:
> > > > > On Thu, Nov 24, 2022 at 12:06:39PM +0800, Pingfan Liu wrote:
> > > > > > On Thu, Nov 24, 2022 at 11:33 AM Pingfan Liu <kernelfans@gmail.com> wrote:
> > > > > > > On Wed, Nov 23, 2022 at 11:20:46AM -0800, Paul E. McKenney wrote:
> > > > > > > > On Wed, Nov 23, 2022 at 09:56:38PM +0800, Pingfan Liu wrote:
> > > > > > > > > Since the srcu read lock is still held during srcu_funnel_gp_start(),
> > > > > > > > > the seq snap should be the largest number for the slot
> > > > > > > > > srcu_have_cbs[idx].
> > > > > > > > >
> > > > > > > > > Signed-off-by: Pingfan Liu <kernelfans@gmail.com>
> > > > > > > > > Cc: Lai Jiangshan <jiangshanlai@gmail.com>
> > > > > > > > > Cc: "Paul E. McKenney" <paulmck@kernel.org>
> > > > > > > > > Cc: Frederic Weisbecker <frederic@kernel.org>
> > > > > > > > > Cc: Josh Triplett <josh@joshtriplett.org>
> > > > > > > > > Cc: Steven Rostedt <rostedt@goodmis.org>
> > > > > > > > > Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> > > > > > > > > To: rcu@vger.kernel.org
> > > > > > > > > ---
> > > > > > > > >  include/linux/rcupdate.h |  1 +
> > > > > > > > >  kernel/rcu/srcutree.c    | 11 ++++++-----
> > > > > > > > >  2 files changed, 7 insertions(+), 5 deletions(-)
> > > > > > > > >
> > > > > > > > > diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
> > > > > > > > > index 08605ce7379d..a09007236660 100644
> > > > > > > > > --- a/include/linux/rcupdate.h
> > > > > > > > > +++ b/include/linux/rcupdate.h
> > > > > > > > > @@ -32,6 +32,7 @@
> > > > > > > > >  #include <linux/context_tracking_irq.h>
> > > > > > > > >
> > > > > > > > >  #define ULONG_CMP_GE(a, b) (ULONG_MAX / 2 >= (a) - (b))
> > > > > > > > > +#define ULONG_CMP_GT(a, b) (ULONG_MAX / 2 > (a) - (b))
> > > > > > > >
> > > > > > > > Please see below...
> > > > > > > >
> > > > > > > > >  #define ULONG_CMP_LT(a, b) (ULONG_MAX / 2 < (a) - (b))
> > > > > > > > >  #define ulong2long(a)              (*(long *)(&(a)))
> > > > > > > > >  #define USHORT_CMP_GE(a, b)        (USHRT_MAX / 2 >= (unsigned short)((a) - (b)))
> > > > > > > > > diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
> > > > > > > > > index 2fc0e775ade4..41902b823687 100644
> > > > > > > > > --- a/kernel/rcu/srcutree.c
> > > > > > > > > +++ b/kernel/rcu/srcutree.c
> > > > > > > > > @@ -905,14 +905,15 @@ static void srcu_funnel_gp_start(struct srcu_struct *ssp, struct srcu_data *sdp,
> > > > > > > > >             for (snp = snp_leaf; snp != NULL; snp = snp->srcu_parent) {
> > > > > > > > >                     spin_lock_irqsave_rcu_node(snp, flags);
> > > > > > > > >                     snp_seq = snp->srcu_have_cbs[idx];
> > > > > > > > > -                   if (!srcu_invl_snp_seq(snp_seq) && ULONG_CMP_GE(snp_seq, s)) {
> > > > > > > > > +                   /*
> > > > > > > > > +                    * s should be the biggest in the current slot. Hence only LE is
> > > > > > > > > +                    * valid
> > > > > > > > > +                    */
> > > > > > > > > +                   BUG_ON(ULONG_CMP_GT(snp_seq, s));
> > > > > > > >
> > > > > > > > Why not this?  (Plus adjusting the comment above, of course.)
> > > > > > > >
> > > > > > > >                       WARN_ON_ONCE(ULONG_CMP_LT(s, snp_seq));
> > > > > > > >
> > > > > > >
> > > > > > > A neat solution!
> > > > > > >
> > > > > > > And what about the comment
> > > > > > >                         /* s should be the biggest in the current slot. */
> > > > > > >
> > > > > > > > That way we don't need ULONG_CMP_GT().  Plus if we are confused about
> > > > > > > > s being the biggest in the current slot, we get a splat and can debug
> > > > > > > > further.  We both might be quite sure that we are not confused, but
> > > > > > > > that is exactly when we are most prone to making mistakes.  ;-)
> > > > > > > >
> > > > > > >
> > > > > > > Same feeling :)
> > > > > > >
> > > > > > > > I also ask that you run with this check for some time.  After all, if
> > > > > > 
> > > > > > Forget to ask if  the test
> > > > > >   "tools/testing/selftests/rcutorture/bin/kvm.sh --allcpus --duration
> > > > > > 10h --configs 18*SRCU-P"
> > > > > > satisfies your requirement?
> > > > > 
> > > > > I am thinking more in terms of adding the WARN_ON_ONCE(), letting it be
> > > > > in -next and possibly mainline for a couple of years, and then if there
> > > > > are no splats, start feeling more confident in the asserted relationship.
> > > > > 
> > > > > If there was a significant performance, scalability, energy-efficiency, or
> > > > > simplification benefit, I would feel justified in being more aggressive.
> > > > > 
> > > > > But I am not seeing a significant benefit.
> > > > 
> > > > Ah, and before I forget -again-, have you thought through the counter-wrap
> > > > scenarios?  Please keep in mind that on a 32-bit system, those counters
> > > > can wrap quite quickly compared to typical uptimes.
> > > 
> > > I think 32-bit has not significant different since the statement
> > >   WARN_ON_ONCE(ULONG_CMP_LT(s, snp_seq));
> > > 
> > > where 's < snp_seq' is not associated with bits. Am I missing anything?
> > 
> > Your point is that long before there was any danger of wrap, this
> > WARN_ON_ONCE() would trigger?
> > 
> > If so, the point is to avoid that WARN_ON_ONCE() from triggering.
> > 

Oops! I failed to figure out the scene you pointed out, where snp_x did
not have call_srcu event for a very long time, then comes the wrap, and
when s is close enough to snp_x's snp_seq, ULONG_CMP_LT(s, snp_seq) will
raise false alarm.

Yes, this is a bug.

> > Or am I missing your point?

Sorry that I missed your point and confused you.

Can I remedy it using the root snp? Because the root snp's snp_seq advances
each GP. And it is the biggest one among snp-s.

> 
> For example, are you instead saying that the WARN_ON_ONCE() proposed in
> your patch will detect the problem either way?  If so, what I am asking
> is whether your earlier analysis considered wrap without the addition
> of that WARN_ON_ONCE().
> 

Have I answer all of your concerns?


Thanks,

	Pingfan

> 							Thanx, Paul
> 
> > > Thanks,
> > > 
> > > 	Pingfan
> > > 
> > > > 							Thanx, Paul
> > > > 
> > > > > > Thanks
> > > > > > 
> > > > > >     Pingfan
> > > > > > > Sure. I will try it immediately.
> > > > > > >
> > > > > > > > the assumption is incorrect, the resulting SRCU hangs off in various
> > > > > > > > systems around the world will not be so much fun to debug.
> > > > > > > >
> > > > > > > > Given that this is slowpath code, it is much better to take an extra
> > > > > > > > compare and branch than to introduce even an extremely small risk of
> > > > > > > > hanging SRCU.
> > > > > > > >
> > > > > > >
> > > > > > > Agree.
> > > > > > >
> > > > > > >
> > > > > > > Thanks,
> > > > > > >
> > > > > > >         Pingfan
> > > > > > >
> > > > > > > >                                                       Thanx, Paul
> > > > > > > >
> > > > > > > > > +                   if (!srcu_invl_snp_seq(snp_seq) && (snp_seq == s)) {
> > > > > > > > >                             if (snp == snp_leaf && snp_seq == s)
> > > > > > > > >                                     snp->srcu_data_have_cbs[idx] |= sdp->grpmask;
> > > > > > > > >                             spin_unlock_irqrestore_rcu_node(snp, flags);
> > > > > > > > > -                           if (snp == snp_leaf && snp_seq != s) {
> > > > > > > > > -                                   srcu_schedule_cbs_sdp(sdp, do_norm ? SRCU_INTERVAL : 0);
> > > > > > > > > -                                   return;
> > > > > > > > > -                           }
> > > > > > > > >                             if (!do_norm)
> > > > > > > > >                                     srcu_funnel_exp_start(ssp, snp, s);
> > > > > > > > >                             return;
> > > > > > > > > --
> > > > > > > > > 2.31.1
> > > > > > > > >

Re: [PATCHv2 2/2] srcu: Eliminate the case that snp_seq bigger than snap in srcu_funnel_gp_start()

[Paul E. McKenney]

On Sun, Nov 27, 2022 at 11:01:17PM +0800, Pingfan Liu wrote:
> On Sat, Nov 26, 2022 at 10:07:04AM -0800, Paul E. McKenney wrote:
> > On Sat, Nov 26, 2022 at 10:03:52AM -0800, Paul E. McKenney wrote:
> > > On Fri, Nov 25, 2022 at 08:37:03PM +0800, Pingfan Liu wrote:
> > > > On Thu, Nov 24, 2022 at 08:19:26AM -0800, Paul E. McKenney wrote:
> > > > > On Wed, Nov 23, 2022 at 09:24:17PM -0800, Paul E. McKenney wrote:
> > > > > > On Thu, Nov 24, 2022 at 12:06:39PM +0800, Pingfan Liu wrote:
> > > > > > > On Thu, Nov 24, 2022 at 11:33 AM Pingfan Liu <kernelfans@gmail.com> wrote:
> > > > > > > > On Wed, Nov 23, 2022 at 11:20:46AM -0800, Paul E. McKenney wrote:
> > > > > > > > > On Wed, Nov 23, 2022 at 09:56:38PM +0800, Pingfan Liu wrote:
> > > > > > > > > > Since the srcu read lock is still held during srcu_funnel_gp_start(),
> > > > > > > > > > the seq snap should be the largest number for the slot
> > > > > > > > > > srcu_have_cbs[idx].
> > > > > > > > > >
> > > > > > > > > > Signed-off-by: Pingfan Liu <kernelfans@gmail.com>
> > > > > > > > > > Cc: Lai Jiangshan <jiangshanlai@gmail.com>
> > > > > > > > > > Cc: "Paul E. McKenney" <paulmck@kernel.org>
> > > > > > > > > > Cc: Frederic Weisbecker <frederic@kernel.org>
> > > > > > > > > > Cc: Josh Triplett <josh@joshtriplett.org>
> > > > > > > > > > Cc: Steven Rostedt <rostedt@goodmis.org>
> > > > > > > > > > Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> > > > > > > > > > To: rcu@vger.kernel.org
> > > > > > > > > > ---
> > > > > > > > > >  include/linux/rcupdate.h |  1 +
> > > > > > > > > >  kernel/rcu/srcutree.c    | 11 ++++++-----
> > > > > > > > > >  2 files changed, 7 insertions(+), 5 deletions(-)
> > > > > > > > > >
> > > > > > > > > > diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
> > > > > > > > > > index 08605ce7379d..a09007236660 100644
> > > > > > > > > > --- a/include/linux/rcupdate.h
> > > > > > > > > > +++ b/include/linux/rcupdate.h
> > > > > > > > > > @@ -32,6 +32,7 @@
> > > > > > > > > >  #include <linux/context_tracking_irq.h>
> > > > > > > > > >
> > > > > > > > > >  #define ULONG_CMP_GE(a, b) (ULONG_MAX / 2 >= (a) - (b))
> > > > > > > > > > +#define ULONG_CMP_GT(a, b) (ULONG_MAX / 2 > (a) - (b))
> > > > > > > > >
> > > > > > > > > Please see below...
> > > > > > > > >
> > > > > > > > > >  #define ULONG_CMP_LT(a, b) (ULONG_MAX / 2 < (a) - (b))
> > > > > > > > > >  #define ulong2long(a)              (*(long *)(&(a)))
> > > > > > > > > >  #define USHORT_CMP_GE(a, b)        (USHRT_MAX / 2 >= (unsigned short)((a) - (b)))
> > > > > > > > > > diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
> > > > > > > > > > index 2fc0e775ade4..41902b823687 100644
> > > > > > > > > > --- a/kernel/rcu/srcutree.c
> > > > > > > > > > +++ b/kernel/rcu/srcutree.c
> > > > > > > > > > @@ -905,14 +905,15 @@ static void srcu_funnel_gp_start(struct srcu_struct *ssp, struct srcu_data *sdp,
> > > > > > > > > >             for (snp = snp_leaf; snp != NULL; snp = snp->srcu_parent) {
> > > > > > > > > >                     spin_lock_irqsave_rcu_node(snp, flags);
> > > > > > > > > >                     snp_seq = snp->srcu_have_cbs[idx];
> > > > > > > > > > -                   if (!srcu_invl_snp_seq(snp_seq) && ULONG_CMP_GE(snp_seq, s)) {
> > > > > > > > > > +                   /*
> > > > > > > > > > +                    * s should be the biggest in the current slot. Hence only LE is
> > > > > > > > > > +                    * valid
> > > > > > > > > > +                    */
> > > > > > > > > > +                   BUG_ON(ULONG_CMP_GT(snp_seq, s));
> > > > > > > > >
> > > > > > > > > Why not this?  (Plus adjusting the comment above, of course.)
> > > > > > > > >
> > > > > > > > >                       WARN_ON_ONCE(ULONG_CMP_LT(s, snp_seq));
> > > > > > > > >
> > > > > > > >
> > > > > > > > A neat solution!
> > > > > > > >
> > > > > > > > And what about the comment
> > > > > > > >                         /* s should be the biggest in the current slot. */
> > > > > > > >
> > > > > > > > > That way we don't need ULONG_CMP_GT().  Plus if we are confused about
> > > > > > > > > s being the biggest in the current slot, we get a splat and can debug
> > > > > > > > > further.  We both might be quite sure that we are not confused, but
> > > > > > > > > that is exactly when we are most prone to making mistakes.  ;-)
> > > > > > > > >
> > > > > > > >
> > > > > > > > Same feeling :)
> > > > > > > >
> > > > > > > > > I also ask that you run with this check for some time.  After all, if
> > > > > > > 
> > > > > > > Forget to ask if  the test
> > > > > > >   "tools/testing/selftests/rcutorture/bin/kvm.sh --allcpus --duration
> > > > > > > 10h --configs 18*SRCU-P"
> > > > > > > satisfies your requirement?
> > > > > > 
> > > > > > I am thinking more in terms of adding the WARN_ON_ONCE(), letting it be
> > > > > > in -next and possibly mainline for a couple of years, and then if there
> > > > > > are no splats, start feeling more confident in the asserted relationship.
> > > > > > 
> > > > > > If there was a significant performance, scalability, energy-efficiency, or
> > > > > > simplification benefit, I would feel justified in being more aggressive.
> > > > > > 
> > > > > > But I am not seeing a significant benefit.
> > > > > 
> > > > > Ah, and before I forget -again-, have you thought through the counter-wrap
> > > > > scenarios?  Please keep in mind that on a 32-bit system, those counters
> > > > > can wrap quite quickly compared to typical uptimes.
> > > > 
> > > > I think 32-bit has not significant different since the statement
> > > >   WARN_ON_ONCE(ULONG_CMP_LT(s, snp_seq));
> > > > 
> > > > where 's < snp_seq' is not associated with bits. Am I missing anything?
> > > 
> > > Your point is that long before there was any danger of wrap, this
> > > WARN_ON_ONCE() would trigger?
> > > 
> > > If so, the point is to avoid that WARN_ON_ONCE() from triggering.
> 
> Oops! I failed to figure out the scene you pointed out, where snp_x did
> not have call_srcu event for a very long time, then comes the wrap, and
> when s is close enough to snp_x's snp_seq, ULONG_CMP_LT(s, snp_seq) will
> raise false alarm.
> 
> Yes, this is a bug.

Very good, you did get there.  ;-)

> > > Or am I missing your point?
> 
> Sorry that I missed your point and confused you.
> 
> Can I remedy it using the root snp? Because the root snp's snp_seq advances
> each GP. And it is the biggest one among snp-s.

Yes, the root srcu_node structure is always there, but it will tend to be
less warm in the cache, and thus will tend to be more expensive to access.

So, does keeping the current unconditional store solve the wrap problem?

> > For example, are you instead saying that the WARN_ON_ONCE() proposed in
> > your patch will detect the problem either way?  If so, what I am asking
> > is whether your earlier analysis considered wrap without the addition
> > of that WARN_ON_ONCE().
> 
> Have I answer all of your concerns?

Quite possibly.

However, to quote George Bernard Shaw, "The single biggest problem in
communication is the illusion that it has taken place."

So I guess we will see!  ;-)

							Thanx, Paul

> Thanks,
> 
> 	Pingfan
> 
> > 							Thanx, Paul
> > 
> > > > Thanks,
> > > > 
> > > > 	Pingfan
> > > > 
> > > > > 							Thanx, Paul
> > > > > 
> > > > > > > Thanks
> > > > > > > 
> > > > > > >     Pingfan
> > > > > > > > Sure. I will try it immediately.
> > > > > > > >
> > > > > > > > > the assumption is incorrect, the resulting SRCU hangs off in various
> > > > > > > > > systems around the world will not be so much fun to debug.
> > > > > > > > >
> > > > > > > > > Given that this is slowpath code, it is much better to take an extra
> > > > > > > > > compare and branch than to introduce even an extremely small risk of
> > > > > > > > > hanging SRCU.
> > > > > > > > >
> > > > > > > >
> > > > > > > > Agree.
> > > > > > > >
> > > > > > > >
> > > > > > > > Thanks,
> > > > > > > >
> > > > > > > >         Pingfan
> > > > > > > >
> > > > > > > > >                                                       Thanx, Paul
> > > > > > > > >
> > > > > > > > > > +                   if (!srcu_invl_snp_seq(snp_seq) && (snp_seq == s)) {
> > > > > > > > > >                             if (snp == snp_leaf && snp_seq == s)
> > > > > > > > > >                                     snp->srcu_data_have_cbs[idx] |= sdp->grpmask;
> > > > > > > > > >                             spin_unlock_irqrestore_rcu_node(snp, flags);
> > > > > > > > > > -                           if (snp == snp_leaf && snp_seq != s) {
> > > > > > > > > > -                                   srcu_schedule_cbs_sdp(sdp, do_norm ? SRCU_INTERVAL : 0);
> > > > > > > > > > -                                   return;
> > > > > > > > > > -                           }
> > > > > > > > > >                             if (!do_norm)
> > > > > > > > > >                                     srcu_funnel_exp_start(ssp, snp, s);
> > > > > > > > > >                             return;
> > > > > > > > > > --
> > > > > > > > > > 2.31.1
> > > > > > > > > >

Re: [PATCHv2 2/2] srcu: Eliminate the case that snp_seq bigger than snap in srcu_funnel_gp_start()

[Pingfan Liu]

On Sun, Nov 27, 2022 at 10:00:16AM -0800, Paul E. McKenney wrote:
> On Sun, Nov 27, 2022 at 11:01:17PM +0800, Pingfan Liu wrote:
> > On Sat, Nov 26, 2022 at 10:07:04AM -0800, Paul E. McKenney wrote:
> > > On Sat, Nov 26, 2022 at 10:03:52AM -0800, Paul E. McKenney wrote:
> > > > On Fri, Nov 25, 2022 at 08:37:03PM +0800, Pingfan Liu wrote:
> > > > > On Thu, Nov 24, 2022 at 08:19:26AM -0800, Paul E. McKenney wrote:
> > > > > > On Wed, Nov 23, 2022 at 09:24:17PM -0800, Paul E. McKenney wrote:
> > > > > > > On Thu, Nov 24, 2022 at 12:06:39PM +0800, Pingfan Liu wrote:
> > > > > > > > On Thu, Nov 24, 2022 at 11:33 AM Pingfan Liu <kernelfans@gmail.com> wrote:
> > > > > > > > > On Wed, Nov 23, 2022 at 11:20:46AM -0800, Paul E. McKenney wrote:
> > > > > > > > > > On Wed, Nov 23, 2022 at 09:56:38PM +0800, Pingfan Liu wrote:
> > > > > > > > > > > Since the srcu read lock is still held during srcu_funnel_gp_start(),
> > > > > > > > > > > the seq snap should be the largest number for the slot
> > > > > > > > > > > srcu_have_cbs[idx].
> > > > > > > > > > >
> > > > > > > > > > > Signed-off-by: Pingfan Liu <kernelfans@gmail.com>
> > > > > > > > > > > Cc: Lai Jiangshan <jiangshanlai@gmail.com>
> > > > > > > > > > > Cc: "Paul E. McKenney" <paulmck@kernel.org>
> > > > > > > > > > > Cc: Frederic Weisbecker <frederic@kernel.org>
> > > > > > > > > > > Cc: Josh Triplett <josh@joshtriplett.org>
> > > > > > > > > > > Cc: Steven Rostedt <rostedt@goodmis.org>
> > > > > > > > > > > Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> > > > > > > > > > > To: rcu@vger.kernel.org
> > > > > > > > > > > ---
> > > > > > > > > > >  include/linux/rcupdate.h |  1 +
> > > > > > > > > > >  kernel/rcu/srcutree.c    | 11 ++++++-----
> > > > > > > > > > >  2 files changed, 7 insertions(+), 5 deletions(-)
> > > > > > > > > > >
> > > > > > > > > > > diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
> > > > > > > > > > > index 08605ce7379d..a09007236660 100644
> > > > > > > > > > > --- a/include/linux/rcupdate.h
> > > > > > > > > > > +++ b/include/linux/rcupdate.h
> > > > > > > > > > > @@ -32,6 +32,7 @@
> > > > > > > > > > >  #include <linux/context_tracking_irq.h>
> > > > > > > > > > >
> > > > > > > > > > >  #define ULONG_CMP_GE(a, b) (ULONG_MAX / 2 >= (a) - (b))
> > > > > > > > > > > +#define ULONG_CMP_GT(a, b) (ULONG_MAX / 2 > (a) - (b))
> > > > > > > > > >
> > > > > > > > > > Please see below...
> > > > > > > > > >
> > > > > > > > > > >  #define ULONG_CMP_LT(a, b) (ULONG_MAX / 2 < (a) - (b))
> > > > > > > > > > >  #define ulong2long(a)              (*(long *)(&(a)))
> > > > > > > > > > >  #define USHORT_CMP_GE(a, b)        (USHRT_MAX / 2 >= (unsigned short)((a) - (b)))
> > > > > > > > > > > diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
> > > > > > > > > > > index 2fc0e775ade4..41902b823687 100644
> > > > > > > > > > > --- a/kernel/rcu/srcutree.c
> > > > > > > > > > > +++ b/kernel/rcu/srcutree.c
> > > > > > > > > > > @@ -905,14 +905,15 @@ static void srcu_funnel_gp_start(struct srcu_struct *ssp, struct srcu_data *sdp,
> > > > > > > > > > >             for (snp = snp_leaf; snp != NULL; snp = snp->srcu_parent) {
> > > > > > > > > > >                     spin_lock_irqsave_rcu_node(snp, flags);
> > > > > > > > > > >                     snp_seq = snp->srcu_have_cbs[idx];
> > > > > > > > > > > -                   if (!srcu_invl_snp_seq(snp_seq) && ULONG_CMP_GE(snp_seq, s)) {
> > > > > > > > > > > +                   /*
> > > > > > > > > > > +                    * s should be the biggest in the current slot. Hence only LE is
> > > > > > > > > > > +                    * valid
> > > > > > > > > > > +                    */
> > > > > > > > > > > +                   BUG_ON(ULONG_CMP_GT(snp_seq, s));
> > > > > > > > > >
> > > > > > > > > > Why not this?  (Plus adjusting the comment above, of course.)
> > > > > > > > > >
> > > > > > > > > >                       WARN_ON_ONCE(ULONG_CMP_LT(s, snp_seq));
> > > > > > > > > >
> > > > > > > > >
> > > > > > > > > A neat solution!
> > > > > > > > >
> > > > > > > > > And what about the comment
> > > > > > > > >                         /* s should be the biggest in the current slot. */
> > > > > > > > >
> > > > > > > > > > That way we don't need ULONG_CMP_GT().  Plus if we are confused about
> > > > > > > > > > s being the biggest in the current slot, we get a splat and can debug
> > > > > > > > > > further.  We both might be quite sure that we are not confused, but
> > > > > > > > > > that is exactly when we are most prone to making mistakes.  ;-)
> > > > > > > > > >
> > > > > > > > >
> > > > > > > > > Same feeling :)
> > > > > > > > >
> > > > > > > > > > I also ask that you run with this check for some time.  After all, if
> > > > > > > > 
> > > > > > > > Forget to ask if  the test
> > > > > > > >   "tools/testing/selftests/rcutorture/bin/kvm.sh --allcpus --duration
> > > > > > > > 10h --configs 18*SRCU-P"
> > > > > > > > satisfies your requirement?
> > > > > > > 
> > > > > > > I am thinking more in terms of adding the WARN_ON_ONCE(), letting it be
> > > > > > > in -next and possibly mainline for a couple of years, and then if there
> > > > > > > are no splats, start feeling more confident in the asserted relationship.
> > > > > > > 
> > > > > > > If there was a significant performance, scalability, energy-efficiency, or
> > > > > > > simplification benefit, I would feel justified in being more aggressive.
> > > > > > > 
> > > > > > > But I am not seeing a significant benefit.
> > > > > > 
> > > > > > Ah, and before I forget -again-, have you thought through the counter-wrap
> > > > > > scenarios?  Please keep in mind that on a 32-bit system, those counters
> > > > > > can wrap quite quickly compared to typical uptimes.
> > > > > 
> > > > > I think 32-bit has not significant different since the statement
> > > > >   WARN_ON_ONCE(ULONG_CMP_LT(s, snp_seq));
> > > > > 
> > > > > where 's < snp_seq' is not associated with bits. Am I missing anything?
> > > > 
> > > > Your point is that long before there was any danger of wrap, this
> > > > WARN_ON_ONCE() would trigger?
> > > > 
> > > > If so, the point is to avoid that WARN_ON_ONCE() from triggering.
> > 
> > Oops! I failed to figure out the scene you pointed out, where snp_x did
> > not have call_srcu event for a very long time, then comes the wrap, and
> > when s is close enough to snp_x's snp_seq, ULONG_CMP_LT(s, snp_seq) will
> > raise false alarm.
> > 
> > Yes, this is a bug.
> 
> Very good, you did get there.  ;-)
> 
> > > > Or am I missing your point?
> > 
> > Sorry that I missed your point and confused you.
> > 
> > Can I remedy it using the root snp? Because the root snp's snp_seq advances
> > each GP. And it is the biggest one among snp-s.
> 
> Yes, the root srcu_node structure is always there, but it will tend to be
> less warm in the cache, and thus will tend to be more expensive to access.
> 
> So, does keeping the current unconditional store solve the wrap problem?
> 

I think you are talking about the "snp->srcu_have_cbs[idx] = gpseq" in
srcu_gp_end(). Yes, it can serve the purpose. And I can also apply wrap
check on that logic later.

So if I keep that store, is PATCHv3 good enough?

Thanks,

	Pingfan

> > > For example, are you instead saying that the WARN_ON_ONCE() proposed in
> > > your patch will detect the problem either way?  If so, what I am asking
> > > is whether your earlier analysis considered wrap without the addition
> > > of that WARN_ON_ONCE().
> > 
> > Have I answer all of your concerns?
> 
> Quite possibly.
> 
> However, to quote George Bernard Shaw, "The single biggest problem in
> communication is the illusion that it has taken place."
> 
> So I guess we will see!  ;-)
> 
> 							Thanx, Paul
> 
> > Thanks,
> > 
> > 	Pingfan
> > 
> > > 							Thanx, Paul
> > > 
> > > > > Thanks,
> > > > > 
> > > > > 	Pingfan
> > > > > 
> > > > > > 							Thanx, Paul
> > > > > > 
> > > > > > > > Thanks
> > > > > > > > 
> > > > > > > >     Pingfan
> > > > > > > > > Sure. I will try it immediately.
> > > > > > > > >
> > > > > > > > > > the assumption is incorrect, the resulting SRCU hangs off in various
> > > > > > > > > > systems around the world will not be so much fun to debug.
> > > > > > > > > >
> > > > > > > > > > Given that this is slowpath code, it is much better to take an extra
> > > > > > > > > > compare and branch than to introduce even an extremely small risk of
> > > > > > > > > > hanging SRCU.
> > > > > > > > > >
> > > > > > > > >
> > > > > > > > > Agree.
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > Thanks,
> > > > > > > > >
> > > > > > > > >         Pingfan
> > > > > > > > >
> > > > > > > > > >                                                       Thanx, Paul
> > > > > > > > > >
> > > > > > > > > > > +                   if (!srcu_invl_snp_seq(snp_seq) && (snp_seq == s)) {
> > > > > > > > > > >                             if (snp == snp_leaf && snp_seq == s)
> > > > > > > > > > >                                     snp->srcu_data_have_cbs[idx] |= sdp->grpmask;
> > > > > > > > > > >                             spin_unlock_irqrestore_rcu_node(snp, flags);
> > > > > > > > > > > -                           if (snp == snp_leaf && snp_seq != s) {
> > > > > > > > > > > -                                   srcu_schedule_cbs_sdp(sdp, do_norm ? SRCU_INTERVAL : 0);
> > > > > > > > > > > -                                   return;
> > > > > > > > > > > -                           }
> > > > > > > > > > >                             if (!do_norm)
> > > > > > > > > > >                                     srcu_funnel_exp_start(ssp, snp, s);
> > > > > > > > > > >                             return;
> > > > > > > > > > > --
> > > > > > > > > > > 2.31.1
> > > > > > > > > > >

Re: [PATCHv2 2/2] srcu: Eliminate the case that snp_seq bigger than snap in srcu_funnel_gp_start()

[Paul E. McKenney]

On Mon, Nov 28, 2022 at 03:59:39PM +0800, Pingfan Liu wrote:
> On Sun, Nov 27, 2022 at 10:00:16AM -0800, Paul E. McKenney wrote:
> > On Sun, Nov 27, 2022 at 11:01:17PM +0800, Pingfan Liu wrote:
> > > On Sat, Nov 26, 2022 at 10:07:04AM -0800, Paul E. McKenney wrote:
> > > > On Sat, Nov 26, 2022 at 10:03:52AM -0800, Paul E. McKenney wrote:
> > > > > On Fri, Nov 25, 2022 at 08:37:03PM +0800, Pingfan Liu wrote:
> > > > > > On Thu, Nov 24, 2022 at 08:19:26AM -0800, Paul E. McKenney wrote:
> > > > > > > On Wed, Nov 23, 2022 at 09:24:17PM -0800, Paul E. McKenney wrote:
> > > > > > > > On Thu, Nov 24, 2022 at 12:06:39PM +0800, Pingfan Liu wrote:
> > > > > > > > > On Thu, Nov 24, 2022 at 11:33 AM Pingfan Liu <kernelfans@gmail.com> wrote:
> > > > > > > > > > On Wed, Nov 23, 2022 at 11:20:46AM -0800, Paul E. McKenney wrote:
> > > > > > > > > > > On Wed, Nov 23, 2022 at 09:56:38PM +0800, Pingfan Liu wrote:
> > > > > > > > > > > > Since the srcu read lock is still held during srcu_funnel_gp_start(),
> > > > > > > > > > > > the seq snap should be the largest number for the slot
> > > > > > > > > > > > srcu_have_cbs[idx].
> > > > > > > > > > > >
> > > > > > > > > > > > Signed-off-by: Pingfan Liu <kernelfans@gmail.com>
> > > > > > > > > > > > Cc: Lai Jiangshan <jiangshanlai@gmail.com>
> > > > > > > > > > > > Cc: "Paul E. McKenney" <paulmck@kernel.org>
> > > > > > > > > > > > Cc: Frederic Weisbecker <frederic@kernel.org>
> > > > > > > > > > > > Cc: Josh Triplett <josh@joshtriplett.org>
> > > > > > > > > > > > Cc: Steven Rostedt <rostedt@goodmis.org>
> > > > > > > > > > > > Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> > > > > > > > > > > > To: rcu@vger.kernel.org
> > > > > > > > > > > > ---
> > > > > > > > > > > >  include/linux/rcupdate.h |  1 +
> > > > > > > > > > > >  kernel/rcu/srcutree.c    | 11 ++++++-----
> > > > > > > > > > > >  2 files changed, 7 insertions(+), 5 deletions(-)
> > > > > > > > > > > >
> > > > > > > > > > > > diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
> > > > > > > > > > > > index 08605ce7379d..a09007236660 100644
> > > > > > > > > > > > --- a/include/linux/rcupdate.h
> > > > > > > > > > > > +++ b/include/linux/rcupdate.h
> > > > > > > > > > > > @@ -32,6 +32,7 @@
> > > > > > > > > > > >  #include <linux/context_tracking_irq.h>
> > > > > > > > > > > >
> > > > > > > > > > > >  #define ULONG_CMP_GE(a, b) (ULONG_MAX / 2 >= (a) - (b))
> > > > > > > > > > > > +#define ULONG_CMP_GT(a, b) (ULONG_MAX / 2 > (a) - (b))
> > > > > > > > > > >
> > > > > > > > > > > Please see below...
> > > > > > > > > > >
> > > > > > > > > > > >  #define ULONG_CMP_LT(a, b) (ULONG_MAX / 2 < (a) - (b))
> > > > > > > > > > > >  #define ulong2long(a)              (*(long *)(&(a)))
> > > > > > > > > > > >  #define USHORT_CMP_GE(a, b)        (USHRT_MAX / 2 >= (unsigned short)((a) - (b)))
> > > > > > > > > > > > diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
> > > > > > > > > > > > index 2fc0e775ade4..41902b823687 100644
> > > > > > > > > > > > --- a/kernel/rcu/srcutree.c
> > > > > > > > > > > > +++ b/kernel/rcu/srcutree.c
> > > > > > > > > > > > @@ -905,14 +905,15 @@ static void srcu_funnel_gp_start(struct srcu_struct *ssp, struct srcu_data *sdp,
> > > > > > > > > > > >             for (snp = snp_leaf; snp != NULL; snp = snp->srcu_parent) {
> > > > > > > > > > > >                     spin_lock_irqsave_rcu_node(snp, flags);
> > > > > > > > > > > >                     snp_seq = snp->srcu_have_cbs[idx];
> > > > > > > > > > > > -                   if (!srcu_invl_snp_seq(snp_seq) && ULONG_CMP_GE(snp_seq, s)) {
> > > > > > > > > > > > +                   /*
> > > > > > > > > > > > +                    * s should be the biggest in the current slot. Hence only LE is
> > > > > > > > > > > > +                    * valid
> > > > > > > > > > > > +                    */
> > > > > > > > > > > > +                   BUG_ON(ULONG_CMP_GT(snp_seq, s));
> > > > > > > > > > >
> > > > > > > > > > > Why not this?  (Plus adjusting the comment above, of course.)
> > > > > > > > > > >
> > > > > > > > > > >                       WARN_ON_ONCE(ULONG_CMP_LT(s, snp_seq));
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > A neat solution!
> > > > > > > > > >
> > > > > > > > > > And what about the comment
> > > > > > > > > >                         /* s should be the biggest in the current slot. */
> > > > > > > > > >
> > > > > > > > > > > That way we don't need ULONG_CMP_GT().  Plus if we are confused about
> > > > > > > > > > > s being the biggest in the current slot, we get a splat and can debug
> > > > > > > > > > > further.  We both might be quite sure that we are not confused, but
> > > > > > > > > > > that is exactly when we are most prone to making mistakes.  ;-)
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > Same feeling :)
> > > > > > > > > >
> > > > > > > > > > > I also ask that you run with this check for some time.  After all, if
> > > > > > > > > 
> > > > > > > > > Forget to ask if  the test
> > > > > > > > >   "tools/testing/selftests/rcutorture/bin/kvm.sh --allcpus --duration
> > > > > > > > > 10h --configs 18*SRCU-P"
> > > > > > > > > satisfies your requirement?
> > > > > > > > 
> > > > > > > > I am thinking more in terms of adding the WARN_ON_ONCE(), letting it be
> > > > > > > > in -next and possibly mainline for a couple of years, and then if there
> > > > > > > > are no splats, start feeling more confident in the asserted relationship.
> > > > > > > > 
> > > > > > > > If there was a significant performance, scalability, energy-efficiency, or
> > > > > > > > simplification benefit, I would feel justified in being more aggressive.
> > > > > > > > 
> > > > > > > > But I am not seeing a significant benefit.
> > > > > > > 
> > > > > > > Ah, and before I forget -again-, have you thought through the counter-wrap
> > > > > > > scenarios?  Please keep in mind that on a 32-bit system, those counters
> > > > > > > can wrap quite quickly compared to typical uptimes.
> > > > > > 
> > > > > > I think 32-bit has not significant different since the statement
> > > > > >   WARN_ON_ONCE(ULONG_CMP_LT(s, snp_seq));
> > > > > > 
> > > > > > where 's < snp_seq' is not associated with bits. Am I missing anything?
> > > > > 
> > > > > Your point is that long before there was any danger of wrap, this
> > > > > WARN_ON_ONCE() would trigger?
> > > > > 
> > > > > If so, the point is to avoid that WARN_ON_ONCE() from triggering.
> > > 
> > > Oops! I failed to figure out the scene you pointed out, where snp_x did
> > > not have call_srcu event for a very long time, then comes the wrap, and
> > > when s is close enough to snp_x's snp_seq, ULONG_CMP_LT(s, snp_seq) will
> > > raise false alarm.
> > > 
> > > Yes, this is a bug.
> > 
> > Very good, you did get there.  ;-)
> > 
> > > > > Or am I missing your point?
> > > 
> > > Sorry that I missed your point and confused you.
> > > 
> > > Can I remedy it using the root snp? Because the root snp's snp_seq advances
> > > each GP. And it is the biggest one among snp-s.
> > 
> > Yes, the root srcu_node structure is always there, but it will tend to be
> > less warm in the cache, and thus will tend to be more expensive to access.
> > 
> > So, does keeping the current unconditional store solve the wrap problem?
> > 
> 
> I think you are talking about the "snp->srcu_have_cbs[idx] = gpseq" in
> srcu_gp_end(). Yes, it can serve the purpose. And I can also apply wrap
> check on that logic later.
> 
> So if I keep that store, is PATCHv3 good enough?

It certainly needs an improved commit log.  For example, removing that
call to srcu_schedule_cbs_sdp() can increase apparent SRCU grace-period
latency.  I could easily believe that this is not a problem, but the
commit log at the very least needs to mention this possibility.

I also need to see at least one ack, given that this patch is removing
software-engineering redundancy.

							Thanx, Paul

> Thanks,
> 
> 	Pingfan
> 
> > > > For example, are you instead saying that the WARN_ON_ONCE() proposed in
> > > > your patch will detect the problem either way?  If so, what I am asking
> > > > is whether your earlier analysis considered wrap without the addition
> > > > of that WARN_ON_ONCE().
> > > 
> > > Have I answer all of your concerns?
> > 
> > Quite possibly.
> > 
> > However, to quote George Bernard Shaw, "The single biggest problem in
> > communication is the illusion that it has taken place."
> > 
> > So I guess we will see!  ;-)
> > 
> > 							Thanx, Paul
> > 
> > > Thanks,
> > > 
> > > 	Pingfan
> > > 
> > > > 							Thanx, Paul
> > > > 
> > > > > > Thanks,
> > > > > > 
> > > > > > 	Pingfan
> > > > > > 
> > > > > > > 							Thanx, Paul
> > > > > > > 
> > > > > > > > > Thanks
> > > > > > > > > 
> > > > > > > > >     Pingfan
> > > > > > > > > > Sure. I will try it immediately.
> > > > > > > > > >
> > > > > > > > > > > the assumption is incorrect, the resulting SRCU hangs off in various
> > > > > > > > > > > systems around the world will not be so much fun to debug.
> > > > > > > > > > >
> > > > > > > > > > > Given that this is slowpath code, it is much better to take an extra
> > > > > > > > > > > compare and branch than to introduce even an extremely small risk of
> > > > > > > > > > > hanging SRCU.
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > Agree.
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > Thanks,
> > > > > > > > > >
> > > > > > > > > >         Pingfan
> > > > > > > > > >
> > > > > > > > > > >                                                       Thanx, Paul
> > > > > > > > > > >
> > > > > > > > > > > > +                   if (!srcu_invl_snp_seq(snp_seq) && (snp_seq == s)) {
> > > > > > > > > > > >                             if (snp == snp_leaf && snp_seq == s)
> > > > > > > > > > > >                                     snp->srcu_data_have_cbs[idx] |= sdp->grpmask;
> > > > > > > > > > > >                             spin_unlock_irqrestore_rcu_node(snp, flags);
> > > > > > > > > > > > -                           if (snp == snp_leaf && snp_seq != s) {
> > > > > > > > > > > > -                                   srcu_schedule_cbs_sdp(sdp, do_norm ? SRCU_INTERVAL : 0);
> > > > > > > > > > > > -                                   return;
> > > > > > > > > > > > -                           }
> > > > > > > > > > > >                             if (!do_norm)
> > > > > > > > > > > >                                     srcu_funnel_exp_start(ssp, snp, s);
> > > > > > > > > > > >                             return;
> > > > > > > > > > > > --
> > > > > > > > > > > > 2.31.1
> > > > > > > > > > > >

Re: [PATCHv2 2/2] srcu: Eliminate the case that snp_seq bigger than snap in srcu_funnel_gp_start()

[Pingfan Liu]

On Wed, Nov 23, 2022 at 09:24:17PM -0800, Paul E. McKenney wrote:
> On Thu, Nov 24, 2022 at 12:06:39PM +0800, Pingfan Liu wrote:
> > On Thu, Nov 24, 2022 at 11:33 AM Pingfan Liu <kernelfans@gmail.com> wrote:
> > >
> > > On Wed, Nov 23, 2022 at 11:20:46AM -0800, Paul E. McKenney wrote:
> > > > On Wed, Nov 23, 2022 at 09:56:38PM +0800, Pingfan Liu wrote:
> > > > > Since the srcu read lock is still held during srcu_funnel_gp_start(),
> > > > > the seq snap should be the largest number for the slot
> > > > > srcu_have_cbs[idx].
> > > > >
> > > > > Signed-off-by: Pingfan Liu <kernelfans@gmail.com>
> > > > > Cc: Lai Jiangshan <jiangshanlai@gmail.com>
> > > > > Cc: "Paul E. McKenney" <paulmck@kernel.org>
> > > > > Cc: Frederic Weisbecker <frederic@kernel.org>
> > > > > Cc: Josh Triplett <josh@joshtriplett.org>
> > > > > Cc: Steven Rostedt <rostedt@goodmis.org>
> > > > > Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> > > > > To: rcu@vger.kernel.org
> > > > > ---
> > > > >  include/linux/rcupdate.h |  1 +
> > > > >  kernel/rcu/srcutree.c    | 11 ++++++-----
> > > > >  2 files changed, 7 insertions(+), 5 deletions(-)
> > > > >
> > > > > diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
> > > > > index 08605ce7379d..a09007236660 100644
> > > > > --- a/include/linux/rcupdate.h
> > > > > +++ b/include/linux/rcupdate.h
> > > > > @@ -32,6 +32,7 @@
> > > > >  #include <linux/context_tracking_irq.h>
> > > > >
> > > > >  #define ULONG_CMP_GE(a, b) (ULONG_MAX / 2 >= (a) - (b))
> > > > > +#define ULONG_CMP_GT(a, b) (ULONG_MAX / 2 > (a) - (b))
> > > >
> > > > Please see below...
> > > >
> > > > >  #define ULONG_CMP_LT(a, b) (ULONG_MAX / 2 < (a) - (b))
> > > > >  #define ulong2long(a)              (*(long *)(&(a)))
> > > > >  #define USHORT_CMP_GE(a, b)        (USHRT_MAX / 2 >= (unsigned short)((a) - (b)))
> > > > > diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
> > > > > index 2fc0e775ade4..41902b823687 100644
> > > > > --- a/kernel/rcu/srcutree.c
> > > > > +++ b/kernel/rcu/srcutree.c
> > > > > @@ -905,14 +905,15 @@ static void srcu_funnel_gp_start(struct srcu_struct *ssp, struct srcu_data *sdp,
> > > > >             for (snp = snp_leaf; snp != NULL; snp = snp->srcu_parent) {
> > > > >                     spin_lock_irqsave_rcu_node(snp, flags);
> > > > >                     snp_seq = snp->srcu_have_cbs[idx];
> > > > > -                   if (!srcu_invl_snp_seq(snp_seq) && ULONG_CMP_GE(snp_seq, s)) {
> > > > > +                   /*
> > > > > +                    * s should be the biggest in the current slot. Hence only LE is
> > > > > +                    * valid
> > > > > +                    */
> > > > > +                   BUG_ON(ULONG_CMP_GT(snp_seq, s));
> > > >
> > > > Why not this?  (Plus adjusting the comment above, of course.)
> > > >
> > > >                       WARN_ON_ONCE(ULONG_CMP_LT(s, snp_seq));
> > > >
> > >
> > > A neat solution!
> > >
> > > And what about the comment
> > >                         /* s should be the biggest in the current slot. */
> > >
> > > > That way we don't need ULONG_CMP_GT().  Plus if we are confused about
> > > > s being the biggest in the current slot, we get a splat and can debug
> > > > further.  We both might be quite sure that we are not confused, but
> > > > that is exactly when we are most prone to making mistakes.  ;-)
> > > >
> > >
> > > Same feeling :)
> > >
> > > > I also ask that you run with this check for some time.  After all, if
> > 
> > Forget to ask if  the test
> >   "tools/testing/selftests/rcutorture/bin/kvm.sh --allcpus --duration
> > 10h --configs 18*SRCU-P"
> > satisfies your requirement?
> 
> I am thinking more in terms of adding the WARN_ON_ONCE(), letting it be
> in -next and possibly mainline for a couple of years, and then if there
> are no splats, start feeling more confident in the asserted relationship.
> 

Sounds reasonable.

> If there was a significant performance, scalability, energy-efficiency, or
> simplification benefit, I would feel justified in being more aggressive.
> 
> But I am not seeing a significant benefit.
> 

No, not a significant benefit.

I will send out V3 for this patch


Thanks,

	Pingfan

> 							Thanx, Paul
> 
> > Thanks
> > 
> >     Pingfan
> > > Sure. I will try it immediately.
> > >
> > > > the assumption is incorrect, the resulting SRCU hangs off in various
> > > > systems around the world will not be so much fun to debug.
> > > >
> > > > Given that this is slowpath code, it is much better to take an extra
> > > > compare and branch than to introduce even an extremely small risk of
> > > > hanging SRCU.
> > > >
> > >
> > > Agree.
> > >
> > >
> > > Thanks,
> > >
> > >         Pingfan
> > >
> > > >                                                       Thanx, Paul
> > > >
> > > > > +                   if (!srcu_invl_snp_seq(snp_seq) && (snp_seq == s)) {
> > > > >                             if (snp == snp_leaf && snp_seq == s)
> > > > >                                     snp->srcu_data_have_cbs[idx] |= sdp->grpmask;
> > > > >                             spin_unlock_irqrestore_rcu_node(snp, flags);
> > > > > -                           if (snp == snp_leaf && snp_seq != s) {
> > > > > -                                   srcu_schedule_cbs_sdp(sdp, do_norm ? SRCU_INTERVAL : 0);
> > > > > -                                   return;
> > > > > -                           }
> > > > >                             if (!do_norm)
> > > > >                                     srcu_funnel_exp_start(ssp, snp, s);
> > > > >                             return;
> > > > > --
> > > > > 2.31.1
> > > > >

[PATCHv3] srcu: Eliminate the case that snp_seq bigger than snap in srcu_funnel_gp_start()

[Pingfan Liu]

Since the srcu read lock is still held during srcu_funnel_gp_start(),
the seq snap should be the largest number for the slot
srcu_have_cbs[idx].

Test info
Running the following test without any failure
  tools/testing/selftests/rcutorture/bin/kvm.sh --allcpus --duration 10h --configs 18*SRCU-P
And
  cd res && find . -name  console.log -exec grep WARNING {} \;
prints nothing

Signed-off-by: Pingfan Liu <kernelfans@gmail.com>
Cc: Lai Jiangshan <jiangshanlai@gmail.com>
Cc: "Paul E. McKenney" <paulmck@kernel.org>
Cc: Frederic Weisbecker <frederic@kernel.org>
Cc: Josh Triplett <josh@joshtriplett.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
To: rcu@vger.kernel.org
---
 kernel/rcu/srcutree.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
index 7e450bb4ddcf..687f243dd2e2 100644
--- a/kernel/rcu/srcutree.c
+++ b/kernel/rcu/srcutree.c
@@ -905,14 +905,12 @@ static void srcu_funnel_gp_start(struct srcu_struct *ssp, struct srcu_data *sdp,
 		for (snp = snp_leaf; snp != NULL; snp = snp->srcu_parent) {
 			spin_lock_irqsave_rcu_node(snp, flags);
 			snp_seq = snp->srcu_have_cbs[idx];
-			if (!srcu_invl_snp_seq(snp_seq) && ULONG_CMP_GE(snp_seq, s)) {
+			/* s should be the biggest in the current slot. */
+			WARN_ON_ONCE(ULONG_CMP_LT(s, snp_seq));
+			if (!srcu_invl_snp_seq(snp_seq) && (snp_seq == s)) {
 				if (snp == snp_leaf && snp_seq == s)
 					snp->srcu_data_have_cbs[idx] |= sdp->grpmask;
 				spin_unlock_irqrestore_rcu_node(snp, flags);
-				if (snp == snp_leaf && snp_seq != s) {
-					srcu_schedule_cbs_sdp(sdp, do_norm ? SRCU_INTERVAL : 0);
-					return;
-				}
 				if (!do_norm)
 					srcu_funnel_exp_start(ssp, snp, s);
 				return;
-- 
2.31.1