* [PATCHv2 0/2] srcu: Optimize when srcu_gp_start_if_needed() holds read lock
@ 2022-11-23 13:56 Pingfan Liu
2022-11-23 13:56 ` [PATCHv2 1/2] srcu: Remove needless rcu_seq_done() check while holding " Pingfan Liu
2022-11-23 13:56 ` [PATCHv2 2/2] srcu: Eliminate the case that snp_seq bigger than snap in srcu_funnel_gp_start() Pingfan Liu
0 siblings, 2 replies; 20+ messages in thread
From: Pingfan Liu @ 2022-11-23 13:56 UTC (permalink / raw)
To: rcu
Cc: Pingfan Liu, Lai Jiangshan, Paul E. McKenney, Frederic Weisbecker,
Josh Triplett, Steven Rostedt, Mathieu Desnoyers
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCHv2 1/2] srcu: Remove needless rcu_seq_done() check while holding read lock
2022-11-23 13:56 [PATCHv2 0/2] srcu: Optimize when srcu_gp_start_if_needed() holds read lock Pingfan Liu
@ 2022-11-23 13:56 ` Pingfan Liu
2022-11-28 23:57 ` Paul E. McKenney
2022-11-23 13:56 ` [PATCHv2 2/2] srcu: Eliminate the case that snp_seq bigger than snap in srcu_funnel_gp_start() Pingfan Liu
1 sibling, 1 reply; 20+ messages in thread
From: Pingfan Liu @ 2022-11-23 13:56 UTC (permalink / raw)
To: rcu
Cc: Pingfan Liu, Lai Jiangshan, Paul E. McKenney, Frederic Weisbecker,
Josh Triplett, Steven Rostedt, Mathieu Desnoyers
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
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCHv2 2/2] srcu: Eliminate the case that snp_seq bigger than snap in srcu_funnel_gp_start()
2022-11-23 13:56 [PATCHv2 0/2] srcu: Optimize when srcu_gp_start_if_needed() holds read lock Pingfan Liu
2022-11-23 13:56 ` [PATCHv2 1/2] srcu: Remove needless rcu_seq_done() check while holding " Pingfan Liu
@ 2022-11-23 13:56 ` Pingfan Liu
2022-11-23 19:20 ` Paul E. McKenney
2022-11-25 12:46 ` [PATCHv3] " Pingfan Liu
1 sibling, 2 replies; 20+ messages in thread
From: Pingfan Liu @ 2022-11-23 13:56 UTC (permalink / raw)
To: rcu
Cc: Pingfan Liu, Lai Jiangshan, Paul E. McKenney, Frederic Weisbecker,
Josh Triplett, Steven Rostedt, Mathieu Desnoyers
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
^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCHv2 2/2] srcu: Eliminate the case that snp_seq bigger than snap in srcu_funnel_gp_start()
2022-11-23 13:56 ` [PATCHv2 2/2] srcu: Eliminate the case that snp_seq bigger than snap in srcu_funnel_gp_start() Pingfan Liu
@ 2022-11-23 19:20 ` Paul E. McKenney
2022-11-24 3:33 ` Pingfan Liu
2022-11-25 12:46 ` [PATCHv3] " Pingfan Liu
1 sibling, 1 reply; 20+ messages in thread
From: Paul E. McKenney @ 2022-11-23 19:20 UTC (permalink / raw)
To: Pingfan Liu
Cc: rcu, Lai Jiangshan, Frederic Weisbecker, Josh Triplett,
Steven Rostedt, Mathieu Desnoyers
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
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCHv2 2/2] srcu: Eliminate the case that snp_seq bigger than snap in srcu_funnel_gp_start()
2022-11-23 19:20 ` Paul E. McKenney
@ 2022-11-24 3:33 ` Pingfan Liu
2022-11-24 4:06 ` Pingfan Liu
0 siblings, 1 reply; 20+ messages in thread
From: Pingfan Liu @ 2022-11-24 3:33 UTC (permalink / raw)
To: Paul E. McKenney
Cc: rcu, Lai Jiangshan, Frederic Weisbecker, Josh Triplett,
Steven Rostedt, Mathieu Desnoyers
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
> >
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCHv2 2/2] srcu: Eliminate the case that snp_seq bigger than snap in srcu_funnel_gp_start()
2022-11-24 3:33 ` Pingfan Liu
@ 2022-11-24 4:06 ` Pingfan Liu
2022-11-24 5:24 ` Paul E. McKenney
0 siblings, 1 reply; 20+ messages in thread
From: Pingfan Liu @ 2022-11-24 4:06 UTC (permalink / raw)
To: Paul E. McKenney
Cc: rcu, Lai Jiangshan, Frederic Weisbecker, Josh Triplett,
Steven Rostedt, Mathieu Desnoyers
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
> > >
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCHv2 2/2] srcu: Eliminate the case that snp_seq bigger than snap in srcu_funnel_gp_start()
2022-11-24 4:06 ` Pingfan Liu
@ 2022-11-24 5:24 ` Paul E. McKenney
2022-11-24 16:19 ` Paul E. McKenney
2022-11-25 12:32 ` Pingfan Liu
0 siblings, 2 replies; 20+ messages in thread
From: Paul E. McKenney @ 2022-11-24 5:24 UTC (permalink / raw)
To: Pingfan Liu
Cc: rcu, Lai Jiangshan, Frederic Weisbecker, Josh Triplett,
Steven Rostedt, Mathieu Desnoyers
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
> > > >
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCHv2 2/2] srcu: Eliminate the case that snp_seq bigger than snap in srcu_funnel_gp_start()
2022-11-24 5:24 ` Paul E. McKenney
@ 2022-11-24 16:19 ` Paul E. McKenney
2022-11-25 12:37 ` Pingfan Liu
2022-11-25 12:32 ` Pingfan Liu
1 sibling, 1 reply; 20+ messages in thread
From: Paul E. McKenney @ 2022-11-24 16:19 UTC (permalink / raw)
To: Pingfan Liu
Cc: rcu, Lai Jiangshan, Frederic Weisbecker, Josh Triplett,
Steven Rostedt, Mathieu Desnoyers
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
> > > > >
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCHv2 2/2] srcu: Eliminate the case that snp_seq bigger than snap in srcu_funnel_gp_start()
2022-11-24 5:24 ` Paul E. McKenney
2022-11-24 16:19 ` Paul E. McKenney
@ 2022-11-25 12:32 ` Pingfan Liu
1 sibling, 0 replies; 20+ messages in thread
From: Pingfan Liu @ 2022-11-25 12:32 UTC (permalink / raw)
To: Paul E. McKenney
Cc: rcu, Lai Jiangshan, Frederic Weisbecker, Josh Triplett,
Steven Rostedt, Mathieu Desnoyers
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
> > > > >
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCHv2 2/2] srcu: Eliminate the case that snp_seq bigger than snap in srcu_funnel_gp_start()
2022-11-24 16:19 ` Paul E. McKenney
@ 2022-11-25 12:37 ` Pingfan Liu
2022-11-26 18:03 ` Paul E. McKenney
0 siblings, 1 reply; 20+ messages in thread
From: Pingfan Liu @ 2022-11-25 12:37 UTC (permalink / raw)
To: Paul E. McKenney
Cc: rcu, Lai Jiangshan, Frederic Weisbecker, Josh Triplett,
Steven Rostedt, Mathieu Desnoyers
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
> > > > > >
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCHv3] srcu: Eliminate the case that snp_seq bigger than snap in srcu_funnel_gp_start()
2022-11-23 13:56 ` [PATCHv2 2/2] srcu: Eliminate the case that snp_seq bigger than snap in srcu_funnel_gp_start() Pingfan Liu
2022-11-23 19:20 ` Paul E. McKenney
@ 2022-11-25 12:46 ` Pingfan Liu
1 sibling, 0 replies; 20+ messages in thread
From: Pingfan Liu @ 2022-11-25 12:46 UTC (permalink / raw)
To: rcu
Cc: Pingfan Liu, Lai Jiangshan, Paul E. McKenney, Frederic Weisbecker,
Josh Triplett, Steven Rostedt, Mathieu Desnoyers
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
^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCHv2 2/2] srcu: Eliminate the case that snp_seq bigger than snap in srcu_funnel_gp_start()
2022-11-25 12:37 ` Pingfan Liu
@ 2022-11-26 18:03 ` Paul E. McKenney
2022-11-26 18:07 ` Paul E. McKenney
0 siblings, 1 reply; 20+ messages in thread
From: Paul E. McKenney @ 2022-11-26 18:03 UTC (permalink / raw)
To: Pingfan Liu
Cc: rcu, Lai Jiangshan, Frederic Weisbecker, Josh Triplett,
Steven Rostedt, Mathieu Desnoyers
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
> > > > > > >
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCHv2 2/2] srcu: Eliminate the case that snp_seq bigger than snap in srcu_funnel_gp_start()
2022-11-26 18:03 ` Paul E. McKenney
@ 2022-11-26 18:07 ` Paul E. McKenney
2022-11-27 14:46 ` Joel Fernandes
2022-11-27 15:01 ` Pingfan Liu
0 siblings, 2 replies; 20+ messages in thread
From: Paul E. McKenney @ 2022-11-26 18:07 UTC (permalink / raw)
To: Pingfan Liu
Cc: rcu, Lai Jiangshan, Frederic Weisbecker, Josh Triplett,
Steven Rostedt, Mathieu Desnoyers
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
> > > > > > > >
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCHv2 2/2] srcu: Eliminate the case that snp_seq bigger than snap in srcu_funnel_gp_start()
2022-11-26 18:07 ` Paul E. McKenney
@ 2022-11-27 14:46 ` Joel Fernandes
2022-11-27 17:55 ` Paul E. McKenney
2022-11-27 15:01 ` Pingfan Liu
1 sibling, 1 reply; 20+ messages in thread
From: Joel Fernandes @ 2022-11-27 14:46 UTC (permalink / raw)
To: paulmck
Cc: Pingfan Liu, rcu, Lai Jiangshan, Frederic Weisbecker,
Josh Triplett, Steven Rostedt, Mathieu Desnoyers
> 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
>>>>>>>>>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCHv2 2/2] srcu: Eliminate the case that snp_seq bigger than snap in srcu_funnel_gp_start()
2022-11-26 18:07 ` Paul E. McKenney
2022-11-27 14:46 ` Joel Fernandes
@ 2022-11-27 15:01 ` Pingfan Liu
2022-11-27 18:00 ` Paul E. McKenney
1 sibling, 1 reply; 20+ messages in thread
From: Pingfan Liu @ 2022-11-27 15:01 UTC (permalink / raw)
To: Paul E. McKenney
Cc: rcu, Lai Jiangshan, Frederic Weisbecker, Josh Triplett,
Steven Rostedt, Mathieu Desnoyers
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
> > > > > > > > >
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCHv2 2/2] srcu: Eliminate the case that snp_seq bigger than snap in srcu_funnel_gp_start()
2022-11-27 14:46 ` Joel Fernandes
@ 2022-11-27 17:55 ` Paul E. McKenney
0 siblings, 0 replies; 20+ messages in thread
From: Paul E. McKenney @ 2022-11-27 17:55 UTC (permalink / raw)
To: Joel Fernandes
Cc: Pingfan Liu, rcu, Lai Jiangshan, Frederic Weisbecker,
Josh Triplett, Steven Rostedt, Mathieu Desnoyers
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
> >>>>>>>>>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCHv2 2/2] srcu: Eliminate the case that snp_seq bigger than snap in srcu_funnel_gp_start()
2022-11-27 15:01 ` Pingfan Liu
@ 2022-11-27 18:00 ` Paul E. McKenney
2022-11-28 7:59 ` Pingfan Liu
0 siblings, 1 reply; 20+ messages in thread
From: Paul E. McKenney @ 2022-11-27 18:00 UTC (permalink / raw)
To: Pingfan Liu
Cc: rcu, Lai Jiangshan, Frederic Weisbecker, Josh Triplett,
Steven Rostedt, Mathieu Desnoyers
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
> > > > > > > > > >
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCHv2 2/2] srcu: Eliminate the case that snp_seq bigger than snap in srcu_funnel_gp_start()
2022-11-27 18:00 ` Paul E. McKenney
@ 2022-11-28 7:59 ` Pingfan Liu
2022-11-29 0:37 ` Paul E. McKenney
0 siblings, 1 reply; 20+ messages in thread
From: Pingfan Liu @ 2022-11-28 7:59 UTC (permalink / raw)
To: Paul E. McKenney
Cc: rcu, Lai Jiangshan, Frederic Weisbecker, Josh Triplett,
Steven Rostedt, Mathieu Desnoyers
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
> > > > > > > > > > >
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCHv2 1/2] srcu: Remove needless rcu_seq_done() check while holding read lock
2022-11-23 13:56 ` [PATCHv2 1/2] srcu: Remove needless rcu_seq_done() check while holding " Pingfan Liu
@ 2022-11-28 23:57 ` Paul E. McKenney
0 siblings, 0 replies; 20+ messages in thread
From: Paul E. McKenney @ 2022-11-28 23:57 UTC (permalink / raw)
To: Pingfan Liu
Cc: rcu, Lai Jiangshan, Frederic Weisbecker, Josh Triplett,
Steven Rostedt, Mathieu Desnoyers
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);
^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCHv2 2/2] srcu: Eliminate the case that snp_seq bigger than snap in srcu_funnel_gp_start()
2022-11-28 7:59 ` Pingfan Liu
@ 2022-11-29 0:37 ` Paul E. McKenney
0 siblings, 0 replies; 20+ messages in thread
From: Paul E. McKenney @ 2022-11-29 0:37 UTC (permalink / raw)
To: Pingfan Liu
Cc: rcu, Lai Jiangshan, Frederic Weisbecker, Josh Triplett,
Steven Rostedt, Mathieu Desnoyers
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
> > > > > > > > > > > >
^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2022-11-29 0:37 UTC | newest]
Thread overview: 20+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-11-23 13:56 [PATCHv2 0/2] srcu: Optimize when srcu_gp_start_if_needed() holds read lock Pingfan Liu
2022-11-23 13:56 ` [PATCHv2 1/2] srcu: Remove needless rcu_seq_done() check while holding " Pingfan Liu
2022-11-28 23:57 ` Paul E. McKenney
2022-11-23 13:56 ` [PATCHv2 2/2] srcu: Eliminate the case that snp_seq bigger than snap in srcu_funnel_gp_start() Pingfan Liu
2022-11-23 19:20 ` Paul E. McKenney
2022-11-24 3:33 ` Pingfan Liu
2022-11-24 4:06 ` Pingfan Liu
2022-11-24 5:24 ` Paul E. McKenney
2022-11-24 16:19 ` Paul E. McKenney
2022-11-25 12:37 ` Pingfan Liu
2022-11-26 18:03 ` Paul E. McKenney
2022-11-26 18:07 ` Paul E. McKenney
2022-11-27 14:46 ` Joel Fernandes
2022-11-27 17:55 ` Paul E. McKenney
2022-11-27 15:01 ` Pingfan Liu
2022-11-27 18:00 ` Paul E. McKenney
2022-11-28 7:59 ` Pingfan Liu
2022-11-29 0:37 ` Paul E. McKenney
2022-11-25 12:32 ` Pingfan Liu
2022-11-25 12:46 ` [PATCHv3] " Pingfan Liu
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.