* [PATCH 0/2] srcu: Optimize when srcu_gp_start_if_needed() holds
@ 2022-11-20 3:40 Pingfan Liu
2022-11-20 3:40 ` [PATCH 1/2] srcu: Remove needless rcu_seq_done() check while holding read lock Pingfan Liu
` (2 more replies)
0 siblings, 3 replies; 15+ messages in thread
From: Pingfan Liu @ 2022-11-20 3:40 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.
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
Pingfan Liu (2):
srcu: Remove needless rcu_seq_done() check while holding read lock
srcu: Remove needless updating of srcu_have_cbs in srcu_gp_end()
kernel/rcu/srcutree.c | 15 ++++++---------
1 file changed, 6 insertions(+), 9 deletions(-)
--
2.31.1
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 1/2] srcu: Remove needless rcu_seq_done() check while holding read lock
2022-11-20 3:40 [PATCH 0/2] srcu: Optimize when srcu_gp_start_if_needed() holds Pingfan Liu
@ 2022-11-20 3:40 ` Pingfan Liu
2022-11-20 5:00 ` Zhang, Qiang1
2022-11-22 1:13 ` Paul E. McKenney
2022-11-20 3:40 ` [PATCH 2/2] srcu: Remove needless updating of srcu_have_cbs in srcu_gp_end() Pingfan Liu
2022-11-20 15:20 ` [PATCH] srcu: Eliminate the case that snp_seq bigger than snap in srcu_funnel_gp_start() Pingfan Liu
2 siblings, 2 replies; 15+ messages in thread
From: Pingfan Liu @ 2022-11-20 3:40 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().
Test info:
Running "tools/testing/selftests/rcutorture/bin/kvm.sh --allcpus --duration 10h --configs 18*SRCU-P"
without any failure.
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 | 13 ++++++-------
1 file changed, 6 insertions(+), 7 deletions(-)
diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
index 1c304fec89c0..7df59fc8073e 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)
@@ -898,8 +900,6 @@ 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. */
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 +935,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] 15+ messages in thread
* [PATCH 2/2] srcu: Remove needless updating of srcu_have_cbs in srcu_gp_end()
2022-11-20 3:40 [PATCH 0/2] srcu: Optimize when srcu_gp_start_if_needed() holds Pingfan Liu
2022-11-20 3:40 ` [PATCH 1/2] srcu: Remove needless rcu_seq_done() check while holding read lock Pingfan Liu
@ 2022-11-20 3:40 ` Pingfan Liu
2022-11-22 1:19 ` Paul E. McKenney
2022-11-20 15:20 ` [PATCH] srcu: Eliminate the case that snp_seq bigger than snap in srcu_funnel_gp_start() Pingfan Liu
2 siblings, 1 reply; 15+ messages in thread
From: Pingfan Liu @ 2022-11-20 3:40 UTC (permalink / raw)
To: rcu
Cc: Pingfan Liu, Lai Jiangshan, Paul E. McKenney,
Frederic Weisbecker, Josh Triplett, Steven Rostedt,
Mathieu Desnoyers
At present, snp->srcu_have_cbs[idx] is updated by either
srcu_funnel_gp_start() or srcu_gp_end().
But as the code changes, now, srcu_funnel_gp_start() is called with srcu
read lock held. And its input parameter s, s = rcu_seq_snap(&ssp->srcu_gp_seq),
whose counter field always proceeds that of the srcu_gp_seq by one or two.
If the seq snap only proceeds by one, the state machine should be in
state SRCU_STATE_IDLE, the held srcu read lock will prevent the state
machine from moving ahead. So when srcu_gp_end() updates
snp->srcu_have_cbs[idx], the idx must be the past idx for
srcu_funnel_gp_start() that is cared by nobody.
So removing the unnecessary updating in srcu_gp_end().
Test info:
Running "tools/testing/selftests/rcutorture/bin/kvm.sh --allcpus --duration 10h --configs 18*SRCU-P"
without any failure.
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 | 2 --
1 file changed, 2 deletions(-)
diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
index 7df59fc8073e..c54d6c04751f 100644
--- a/kernel/rcu/srcutree.c
+++ b/kernel/rcu/srcutree.c
@@ -783,8 +783,6 @@ static void srcu_gp_end(struct srcu_struct *ssp)
last_lvl = snp >= ssp->level[rcu_num_lvls - 1];
if (last_lvl)
cbs = ss_state < SRCU_SIZE_BIG || snp->srcu_have_cbs[idx] == gpseq;
- snp->srcu_have_cbs[idx] = gpseq;
- rcu_seq_set_state(&snp->srcu_have_cbs[idx], 1);
sgsne = snp->srcu_gp_seq_needed_exp;
if (srcu_invl_snp_seq(sgsne) || ULONG_CMP_LT(sgsne, gpseq))
WRITE_ONCE(snp->srcu_gp_seq_needed_exp, gpseq);
--
2.31.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* RE: [PATCH 1/2] srcu: Remove needless rcu_seq_done() check while holding read lock
2022-11-20 3:40 ` [PATCH 1/2] srcu: Remove needless rcu_seq_done() check while holding read lock Pingfan Liu
@ 2022-11-20 5:00 ` Zhang, Qiang1
2022-11-20 15:26 ` Pingfan Liu
2022-11-22 1:13 ` Paul E. McKenney
1 sibling, 1 reply; 15+ messages in thread
From: Zhang, Qiang1 @ 2022-11-20 5:00 UTC (permalink / raw)
To: Pingfan Liu, rcu
Cc: 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.
Hi Pingfan
Please correct me if I understand your commit message incorrectly. because the srcu_gp_start_if_needed() is
protected by srcu read lock, and the rcu_seq_snap() seq is a at the end of the current or next srcu grace period,
so as long as we are still in the srcu_gp_start_if_needed(), it also means that we are in SRCU critical section.
the current or next srcu grace period will not end, it also means that we can not invoke rcu_seq_end() to update
'ssp->secu_gp_seq' , so the rcu_seq_done(&ssp->srcu_gp_seq, s) is always return false.
Thanks
Zqiang
>
>The same condition stands for srcu_funnel_exp_start(). Hence removing
>all the checks of rcu_seq_done().
>
>Test info:
> Running "tools/testing/selftests/rcutorture/bin/kvm.sh --allcpus --duration 10h --configs 18*SRCU-P"
>without any failure.
>
>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 | 13 ++++++-------
1 file changed, 6 insertions(+), 7 deletions(-)
diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
index 1c304fec89c0..7df59fc8073e 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)
@@ -898,8 +900,6 @@ 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. */
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 +935,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] 15+ messages in thread
* [PATCH] srcu: Eliminate the case that snp_seq bigger than snap in srcu_funnel_gp_start()
2022-11-20 3:40 [PATCH 0/2] srcu: Optimize when srcu_gp_start_if_needed() holds Pingfan Liu
2022-11-20 3:40 ` [PATCH 1/2] srcu: Remove needless rcu_seq_done() check while holding read lock Pingfan Liu
2022-11-20 3:40 ` [PATCH 2/2] srcu: Remove needless updating of srcu_have_cbs in srcu_gp_end() Pingfan Liu
@ 2022-11-20 15:20 ` Pingfan Liu
2022-11-20 15:23 ` Pingfan Liu
2 siblings, 1 reply; 15+ messages in thread
From: Pingfan Liu @ 2022-11-20 15:20 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 | 10 +++++-----
2 files changed, 6 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 c54d6c04751f..057752db1125 100644
--- a/kernel/rcu/srcutree.c
+++ b/kernel/rcu/srcutree.c
@@ -900,14 +900,14 @@ 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] 15+ messages in thread
* Re: [PATCH] srcu: Eliminate the case that snp_seq bigger than snap in srcu_funnel_gp_start()
2022-11-20 15:20 ` [PATCH] srcu: Eliminate the case that snp_seq bigger than snap in srcu_funnel_gp_start() Pingfan Liu
@ 2022-11-20 15:23 ` Pingfan Liu
2022-11-22 1:20 ` Paul E. McKenney
0 siblings, 1 reply; 15+ messages in thread
From: Pingfan Liu @ 2022-11-20 15:23 UTC (permalink / raw)
To: rcu
Cc: Lai Jiangshan, Paul E. McKenney, Frederic Weisbecker,
Josh Triplett, Steven Rostedt, Mathieu Desnoyers
Sorry for the fragment, but I am just aware of this and think it is
better to fold into the same series.
Thanks,
Pingfan
On Sun, Nov 20, 2022 at 11:20 PM Pingfan Liu <kernelfans@gmail.com> 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 | 10 +++++-----
> 2 files changed, 6 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 c54d6c04751f..057752db1125 100644
> --- a/kernel/rcu/srcutree.c
> +++ b/kernel/rcu/srcutree.c
> @@ -900,14 +900,14 @@ 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 [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] srcu: Remove needless rcu_seq_done() check while holding read lock
2022-11-20 5:00 ` Zhang, Qiang1
@ 2022-11-20 15:26 ` Pingfan Liu
0 siblings, 0 replies; 15+ messages in thread
From: Pingfan Liu @ 2022-11-20 15:26 UTC (permalink / raw)
To: Zhang, Qiang1
Cc: rcu, Lai Jiangshan, Paul E. McKenney, Frederic Weisbecker,
Josh Triplett, Steven Rostedt, Mathieu Desnoyers
On Sun, Nov 20, 2022 at 05:00:43AM +0000, Zhang, Qiang1 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.
>
> Hi Pingfan
>
> Please correct me if I understand your commit message incorrectly. because the srcu_gp_start_if_needed() is
> protected by srcu read lock, and the rcu_seq_snap() seq is a at the end of the current or next srcu grace period,
> so as long as we are still in the srcu_gp_start_if_needed(), it also means that we are in SRCU critical section.
> the current or next srcu grace period will not end, it also means that we can not invoke rcu_seq_end() to update
> 'ssp->secu_gp_seq' , so the rcu_seq_done(&ssp->srcu_gp_seq, s) is always return false.
>
Yes, you totally got it.
Thanks
Pingfan
> Thanks
> Zqiang
>
> >
> >The same condition stands for srcu_funnel_exp_start(). Hence removing
> >all the checks of rcu_seq_done().
> >
> >Test info:
> > Running "tools/testing/selftests/rcutorture/bin/kvm.sh --allcpus --duration 10h --configs 18*SRCU-P"
> >without any failure.
> >
> >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 | 13 ++++++-------
> 1 file changed, 6 insertions(+), 7 deletions(-)
>
> diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
> index 1c304fec89c0..7df59fc8073e 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)
> @@ -898,8 +900,6 @@ 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. */
> 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 +935,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 [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] srcu: Remove needless rcu_seq_done() check while holding read lock
2022-11-20 3:40 ` [PATCH 1/2] srcu: Remove needless rcu_seq_done() check while holding read lock Pingfan Liu
2022-11-20 5:00 ` Zhang, Qiang1
@ 2022-11-22 1:13 ` Paul E. McKenney
2022-11-22 9:50 ` Pingfan Liu
1 sibling, 1 reply; 15+ messages in thread
From: Paul E. McKenney @ 2022-11-22 1:13 UTC (permalink / raw)
To: Pingfan Liu
Cc: rcu, Lai Jiangshan, Frederic Weisbecker, Josh Triplett,
Steven Rostedt, Mathieu Desnoyers
On Sun, Nov 20, 2022 at 11:40:13AM +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().
>
> Test info:
> Running "tools/testing/selftests/rcutorture/bin/kvm.sh --allcpus --duration 10h --configs 18*SRCU-P"
> without any failure.
Nice catch!!! And I do appreciate the testing.
But if we are going to do this, let's please also place lockdep assertions
in srcu_funnel_exp_start() and srcu_funnel_gp_start() to verify that these
functions are in fact invoked within an SRCU read-side critical section.
Also a WARN_ON_ONCE(rcu_seq_done(&ssp->srcu_gp_seq, s)), please.
Things changed to make the rcu_seq_done() unnecessary, so they could just
as easily change again.
Thanx, Paul
> 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 | 13 ++++++-------
> 1 file changed, 6 insertions(+), 7 deletions(-)
>
> diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
> index 1c304fec89c0..7df59fc8073e 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)
> @@ -898,8 +900,6 @@ 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. */
> 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 +935,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 [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] srcu: Remove needless updating of srcu_have_cbs in srcu_gp_end()
2022-11-20 3:40 ` [PATCH 2/2] srcu: Remove needless updating of srcu_have_cbs in srcu_gp_end() Pingfan Liu
@ 2022-11-22 1:19 ` Paul E. McKenney
2022-11-22 9:59 ` Pingfan Liu
0 siblings, 1 reply; 15+ messages in thread
From: Paul E. McKenney @ 2022-11-22 1:19 UTC (permalink / raw)
To: Pingfan Liu
Cc: rcu, Lai Jiangshan, Frederic Weisbecker, Josh Triplett,
Steven Rostedt, Mathieu Desnoyers
On Sun, Nov 20, 2022 at 11:40:14AM +0800, Pingfan Liu wrote:
> At present, snp->srcu_have_cbs[idx] is updated by either
> srcu_funnel_gp_start() or srcu_gp_end().
>
> But as the code changes, now, srcu_funnel_gp_start() is called with srcu
> read lock held. And its input parameter s, s = rcu_seq_snap(&ssp->srcu_gp_seq),
> whose counter field always proceeds that of the srcu_gp_seq by one or two.
>
> If the seq snap only proceeds by one, the state machine should be in
> state SRCU_STATE_IDLE, the held srcu read lock will prevent the state
> machine from moving ahead. So when srcu_gp_end() updates
> snp->srcu_have_cbs[idx], the idx must be the past idx for
> srcu_funnel_gp_start() that is cared by nobody.
>
> So removing the unnecessary updating in srcu_gp_end().
This looks plausible, good eyes!
But is there a debug check that could verify that this is unnecessary?
Logical reasoning is all well and good, but the actual system always
wins arguments. ;-)
Thanx, Paul
> Test info:
> Running "tools/testing/selftests/rcutorture/bin/kvm.sh --allcpus --duration 10h --configs 18*SRCU-P"
> without any failure.
>
> 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 | 2 --
> 1 file changed, 2 deletions(-)
>
> diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
> index 7df59fc8073e..c54d6c04751f 100644
> --- a/kernel/rcu/srcutree.c
> +++ b/kernel/rcu/srcutree.c
> @@ -783,8 +783,6 @@ static void srcu_gp_end(struct srcu_struct *ssp)
> last_lvl = snp >= ssp->level[rcu_num_lvls - 1];
> if (last_lvl)
> cbs = ss_state < SRCU_SIZE_BIG || snp->srcu_have_cbs[idx] == gpseq;
> - snp->srcu_have_cbs[idx] = gpseq;
> - rcu_seq_set_state(&snp->srcu_have_cbs[idx], 1);
> sgsne = snp->srcu_gp_seq_needed_exp;
> if (srcu_invl_snp_seq(sgsne) || ULONG_CMP_LT(sgsne, gpseq))
> WRITE_ONCE(snp->srcu_gp_seq_needed_exp, gpseq);
> --
> 2.31.1
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] srcu: Eliminate the case that snp_seq bigger than snap in srcu_funnel_gp_start()
2022-11-20 15:23 ` Pingfan Liu
@ 2022-11-22 1:20 ` Paul E. McKenney
0 siblings, 0 replies; 15+ messages in thread
From: Paul E. McKenney @ 2022-11-22 1:20 UTC (permalink / raw)
To: Pingfan Liu
Cc: rcu, Lai Jiangshan, Frederic Weisbecker, Josh Triplett,
Steven Rostedt, Mathieu Desnoyers
On Sun, Nov 20, 2022 at 11:23:46PM +0800, Pingfan Liu wrote:
> Sorry for the fragment, but I am just aware of this and think it is
> better to fold into the same series.
Very well, I look forward to seeing the new version of this series.
Thanx, Paul
> Thanks,
>
> Pingfan
>
> On Sun, Nov 20, 2022 at 11:20 PM Pingfan Liu <kernelfans@gmail.com> 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 | 10 +++++-----
> > 2 files changed, 6 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 c54d6c04751f..057752db1125 100644
> > --- a/kernel/rcu/srcutree.c
> > +++ b/kernel/rcu/srcutree.c
> > @@ -900,14 +900,14 @@ 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 [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] srcu: Remove needless rcu_seq_done() check while holding read lock
2022-11-22 1:13 ` Paul E. McKenney
@ 2022-11-22 9:50 ` Pingfan Liu
0 siblings, 0 replies; 15+ messages in thread
From: Pingfan Liu @ 2022-11-22 9:50 UTC (permalink / raw)
To: Paul E. McKenney
Cc: rcu, Lai Jiangshan, Frederic Weisbecker, Josh Triplett,
Steven Rostedt, Mathieu Desnoyers
On Mon, Nov 21, 2022 at 05:13:45PM -0800, Paul E. McKenney wrote:
> On Sun, Nov 20, 2022 at 11:40:13AM +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().
> >
> > Test info:
> > Running "tools/testing/selftests/rcutorture/bin/kvm.sh --allcpus --duration 10h --configs 18*SRCU-P"
> > without any failure.
>
> Nice catch!!! And I do appreciate the testing.
>
> But if we are going to do this, let's please also place lockdep assertions
> in srcu_funnel_exp_start() and srcu_funnel_gp_start() to verify that these
> functions are in fact invoked within an SRCU read-side critical section.
> Also a WARN_ON_ONCE(rcu_seq_done(&ssp->srcu_gp_seq, s)), please.
>
OK, I will do both.
> Things changed to make the rcu_seq_done() unnecessary, so they could just
> as easily change again.
>
Yes. It is wise to prevent from the future variation.
Thanks,
Pingfan
> Thanx, Paul
>
> > 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 | 13 ++++++-------
> > 1 file changed, 6 insertions(+), 7 deletions(-)
> >
> > diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
> > index 1c304fec89c0..7df59fc8073e 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)
> > @@ -898,8 +900,6 @@ 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. */
> > 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 +935,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 [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] srcu: Remove needless updating of srcu_have_cbs in srcu_gp_end()
2022-11-22 1:19 ` Paul E. McKenney
@ 2022-11-22 9:59 ` Pingfan Liu
2022-11-22 14:45 ` Paul E. McKenney
0 siblings, 1 reply; 15+ messages in thread
From: Pingfan Liu @ 2022-11-22 9:59 UTC (permalink / raw)
To: Paul E. McKenney
Cc: rcu, Lai Jiangshan, Frederic Weisbecker, Josh Triplett,
Steven Rostedt, Mathieu Desnoyers
On Mon, Nov 21, 2022 at 05:19:16PM -0800, Paul E. McKenney wrote:
> On Sun, Nov 20, 2022 at 11:40:14AM +0800, Pingfan Liu wrote:
> > At present, snp->srcu_have_cbs[idx] is updated by either
> > srcu_funnel_gp_start() or srcu_gp_end().
> >
> > But as the code changes, now, srcu_funnel_gp_start() is called with srcu
> > read lock held. And its input parameter s, s = rcu_seq_snap(&ssp->srcu_gp_seq),
> > whose counter field always proceeds that of the srcu_gp_seq by one or two.
> >
> > If the seq snap only proceeds by one, the state machine should be in
> > state SRCU_STATE_IDLE, the held srcu read lock will prevent the state
> > machine from moving ahead. So when srcu_gp_end() updates
> > snp->srcu_have_cbs[idx], the idx must be the past idx for
> > srcu_funnel_gp_start() that is cared by nobody.
> >
> > So removing the unnecessary updating in srcu_gp_end().
>
> This looks plausible, good eyes!
>
> But is there a debug check that could verify that this is unnecessary?
> Logical reasoning is all well and good, but the actual system always
> wins arguments. ;-)
>
Agree. Reasoning may miss some ground and render a wrong result.
But it is a little hard to demonstrate the past idx is not accessed. I
will go on to figure a way out.
Thanks,
Pingfan
> Thanx, Paul
>
> > Test info:
> > Running "tools/testing/selftests/rcutorture/bin/kvm.sh --allcpus --duration 10h --configs 18*SRCU-P"
> > without any failure.
> >
> > 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 | 2 --
> > 1 file changed, 2 deletions(-)
> >
> > diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
> > index 7df59fc8073e..c54d6c04751f 100644
> > --- a/kernel/rcu/srcutree.c
> > +++ b/kernel/rcu/srcutree.c
> > @@ -783,8 +783,6 @@ static void srcu_gp_end(struct srcu_struct *ssp)
> > last_lvl = snp >= ssp->level[rcu_num_lvls - 1];
> > if (last_lvl)
> > cbs = ss_state < SRCU_SIZE_BIG || snp->srcu_have_cbs[idx] == gpseq;
> > - snp->srcu_have_cbs[idx] = gpseq;
> > - rcu_seq_set_state(&snp->srcu_have_cbs[idx], 1);
> > sgsne = snp->srcu_gp_seq_needed_exp;
> > if (srcu_invl_snp_seq(sgsne) || ULONG_CMP_LT(sgsne, gpseq))
> > WRITE_ONCE(snp->srcu_gp_seq_needed_exp, gpseq);
> > --
> > 2.31.1
> >
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] srcu: Remove needless updating of srcu_have_cbs in srcu_gp_end()
2022-11-22 9:59 ` Pingfan Liu
@ 2022-11-22 14:45 ` Paul E. McKenney
2022-11-23 13:29 ` Pingfan Liu
0 siblings, 1 reply; 15+ messages in thread
From: Paul E. McKenney @ 2022-11-22 14:45 UTC (permalink / raw)
To: Pingfan Liu
Cc: rcu, Lai Jiangshan, Frederic Weisbecker, Josh Triplett,
Steven Rostedt, Mathieu Desnoyers
On Tue, Nov 22, 2022 at 05:59:12PM +0800, Pingfan Liu wrote:
> On Mon, Nov 21, 2022 at 05:19:16PM -0800, Paul E. McKenney wrote:
> > On Sun, Nov 20, 2022 at 11:40:14AM +0800, Pingfan Liu wrote:
> > > At present, snp->srcu_have_cbs[idx] is updated by either
> > > srcu_funnel_gp_start() or srcu_gp_end().
> > >
> > > But as the code changes, now, srcu_funnel_gp_start() is called with srcu
> > > read lock held. And its input parameter s, s = rcu_seq_snap(&ssp->srcu_gp_seq),
> > > whose counter field always proceeds that of the srcu_gp_seq by one or two.
> > >
> > > If the seq snap only proceeds by one, the state machine should be in
> > > state SRCU_STATE_IDLE, the held srcu read lock will prevent the state
> > > machine from moving ahead. So when srcu_gp_end() updates
> > > snp->srcu_have_cbs[idx], the idx must be the past idx for
> > > srcu_funnel_gp_start() that is cared by nobody.
> > >
> > > So removing the unnecessary updating in srcu_gp_end().
> >
> > This looks plausible, good eyes!
> >
> > But is there a debug check that could verify that this is unnecessary?
> > Logical reasoning is all well and good, but the actual system always
> > wins arguments. ;-)
> >
>
> Agree. Reasoning may miss some ground and render a wrong result.
>
> But it is a little hard to demonstrate the past idx is not accessed. I
> will go on to figure a way out.
On a 64-bit system especially, it should be easy to generate a pattern
that never actually occurs. Even on a 32-bit system, aren't there bit
patterns in the low-order bits that never occur?
Wouldn't it be possible to check for those?
Thanx, Paul
> Thanks,
>
> Pingfan
>
> > Thanx, Paul
> >
> > > Test info:
> > > Running "tools/testing/selftests/rcutorture/bin/kvm.sh --allcpus --duration 10h --configs 18*SRCU-P"
> > > without any failure.
> > >
> > > 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 | 2 --
> > > 1 file changed, 2 deletions(-)
> > >
> > > diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
> > > index 7df59fc8073e..c54d6c04751f 100644
> > > --- a/kernel/rcu/srcutree.c
> > > +++ b/kernel/rcu/srcutree.c
> > > @@ -783,8 +783,6 @@ static void srcu_gp_end(struct srcu_struct *ssp)
> > > last_lvl = snp >= ssp->level[rcu_num_lvls - 1];
> > > if (last_lvl)
> > > cbs = ss_state < SRCU_SIZE_BIG || snp->srcu_have_cbs[idx] == gpseq;
> > > - snp->srcu_have_cbs[idx] = gpseq;
> > > - rcu_seq_set_state(&snp->srcu_have_cbs[idx], 1);
> > > sgsne = snp->srcu_gp_seq_needed_exp;
> > > if (srcu_invl_snp_seq(sgsne) || ULONG_CMP_LT(sgsne, gpseq))
> > > WRITE_ONCE(snp->srcu_gp_seq_needed_exp, gpseq);
> > > --
> > > 2.31.1
> > >
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] srcu: Remove needless updating of srcu_have_cbs in srcu_gp_end()
2022-11-22 14:45 ` Paul E. McKenney
@ 2022-11-23 13:29 ` Pingfan Liu
2022-11-23 18:40 ` Paul E. McKenney
0 siblings, 1 reply; 15+ messages in thread
From: Pingfan Liu @ 2022-11-23 13:29 UTC (permalink / raw)
To: Paul E. McKenney
Cc: rcu, Lai Jiangshan, Frederic Weisbecker, Josh Triplett,
Steven Rostedt, Mathieu Desnoyers
On Tue, Nov 22, 2022 at 06:45:49AM -0800, Paul E. McKenney wrote:
> On Tue, Nov 22, 2022 at 05:59:12PM +0800, Pingfan Liu wrote:
> > On Mon, Nov 21, 2022 at 05:19:16PM -0800, Paul E. McKenney wrote:
> > > On Sun, Nov 20, 2022 at 11:40:14AM +0800, Pingfan Liu wrote:
> > > > At present, snp->srcu_have_cbs[idx] is updated by either
> > > > srcu_funnel_gp_start() or srcu_gp_end().
> > > >
> > > > But as the code changes, now, srcu_funnel_gp_start() is called with srcu
> > > > read lock held. And its input parameter s, s = rcu_seq_snap(&ssp->srcu_gp_seq),
> > > > whose counter field always proceeds that of the srcu_gp_seq by one or two.
> > > >
> > > > If the seq snap only proceeds by one, the state machine should be in
> > > > state SRCU_STATE_IDLE, the held srcu read lock will prevent the state
> > > > machine from moving ahead. So when srcu_gp_end() updates
> > > > snp->srcu_have_cbs[idx], the idx must be the past idx for
> > > > srcu_funnel_gp_start() that is cared by nobody.
> > > >
> > > > So removing the unnecessary updating in srcu_gp_end().
> > >
> > > This looks plausible, good eyes!
> > >
> > > But is there a debug check that could verify that this is unnecessary?
> > > Logical reasoning is all well and good, but the actual system always
> > > wins arguments. ;-)
> > >
> >
> > Agree. Reasoning may miss some ground and render a wrong result.
> >
> > But it is a little hard to demonstrate the past idx is not accessed. I
> > will go on to figure a way out.
>
> On a 64-bit system especially, it should be easy to generate a pattern
> that never actually occurs. Even on a 32-bit system, aren't there bit
> patterns in the low-order bits that never occur?
>
> Wouldn't it be possible to check for those?
>
I had thought about that way, but that means a write and cache invalid.
While on the other way, I still rely on the logic to add some check
code, which should be avoided.
Finally, I turn back to the way which you suggested. But I have a new
plan for this patch. So I will send out V2 for the other two patches.
As for this one, it will come in the next series.
Thanks,
Pingfan
> Thanx, Paul
>
> > Thanks,
> >
> > Pingfan
> >
> > > Thanx, Paul
> > >
> > > > Test info:
> > > > Running "tools/testing/selftests/rcutorture/bin/kvm.sh --allcpus --duration 10h --configs 18*SRCU-P"
> > > > without any failure.
> > > >
> > > > 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 | 2 --
> > > > 1 file changed, 2 deletions(-)
> > > >
> > > > diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
> > > > index 7df59fc8073e..c54d6c04751f 100644
> > > > --- a/kernel/rcu/srcutree.c
> > > > +++ b/kernel/rcu/srcutree.c
> > > > @@ -783,8 +783,6 @@ static void srcu_gp_end(struct srcu_struct *ssp)
> > > > last_lvl = snp >= ssp->level[rcu_num_lvls - 1];
> > > > if (last_lvl)
> > > > cbs = ss_state < SRCU_SIZE_BIG || snp->srcu_have_cbs[idx] == gpseq;
> > > > - snp->srcu_have_cbs[idx] = gpseq;
> > > > - rcu_seq_set_state(&snp->srcu_have_cbs[idx], 1);
> > > > sgsne = snp->srcu_gp_seq_needed_exp;
> > > > if (srcu_invl_snp_seq(sgsne) || ULONG_CMP_LT(sgsne, gpseq))
> > > > WRITE_ONCE(snp->srcu_gp_seq_needed_exp, gpseq);
> > > > --
> > > > 2.31.1
> > > >
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] srcu: Remove needless updating of srcu_have_cbs in srcu_gp_end()
2022-11-23 13:29 ` Pingfan Liu
@ 2022-11-23 18:40 ` Paul E. McKenney
0 siblings, 0 replies; 15+ messages in thread
From: Paul E. McKenney @ 2022-11-23 18:40 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:29:17PM +0800, Pingfan Liu wrote:
> On Tue, Nov 22, 2022 at 06:45:49AM -0800, Paul E. McKenney wrote:
> > On Tue, Nov 22, 2022 at 05:59:12PM +0800, Pingfan Liu wrote:
> > > On Mon, Nov 21, 2022 at 05:19:16PM -0800, Paul E. McKenney wrote:
> > > > On Sun, Nov 20, 2022 at 11:40:14AM +0800, Pingfan Liu wrote:
> > > > > At present, snp->srcu_have_cbs[idx] is updated by either
> > > > > srcu_funnel_gp_start() or srcu_gp_end().
> > > > >
> > > > > But as the code changes, now, srcu_funnel_gp_start() is called with srcu
> > > > > read lock held. And its input parameter s, s = rcu_seq_snap(&ssp->srcu_gp_seq),
> > > > > whose counter field always proceeds that of the srcu_gp_seq by one or two.
> > > > >
> > > > > If the seq snap only proceeds by one, the state machine should be in
> > > > > state SRCU_STATE_IDLE, the held srcu read lock will prevent the state
> > > > > machine from moving ahead. So when srcu_gp_end() updates
> > > > > snp->srcu_have_cbs[idx], the idx must be the past idx for
> > > > > srcu_funnel_gp_start() that is cared by nobody.
> > > > >
> > > > > So removing the unnecessary updating in srcu_gp_end().
> > > >
> > > > This looks plausible, good eyes!
> > > >
> > > > But is there a debug check that could verify that this is unnecessary?
> > > > Logical reasoning is all well and good, but the actual system always
> > > > wins arguments. ;-)
> > > >
> > >
> > > Agree. Reasoning may miss some ground and render a wrong result.
> > >
> > > But it is a little hard to demonstrate the past idx is not accessed. I
> > > will go on to figure a way out.
> >
> > On a 64-bit system especially, it should be easy to generate a pattern
> > that never actually occurs. Even on a 32-bit system, aren't there bit
> > patterns in the low-order bits that never occur?
> >
> > Wouldn't it be possible to check for those?
>
> I had thought about that way, but that means a write and cache invalid.
Which would be one reason to do it only within kernels built with
CONFIG_PROVE_RCU=y. In addition, this write occurs only rarely, so its
effect on overall performance should be extremely small.
> While on the other way, I still rely on the logic to add some check
> code, which should be avoided.
The eternal vulnerabilty of logic is that it is always based on
assumptions, which cannot be proven correct, only invalidated. ;-)
> Finally, I turn back to the way which you suggested. But I have a new
> plan for this patch. So I will send out V2 for the other two patches.
>
> As for this one, it will come in the next series.
Looking forward to seeing it, then.
Thanx, Paul
> Thanks,
>
> Pingfan
>
>
> > Thanx, Paul
> >
> > > Thanks,
> > >
> > > Pingfan
> > >
> > > > Thanx, Paul
> > > >
> > > > > Test info:
> > > > > Running "tools/testing/selftests/rcutorture/bin/kvm.sh --allcpus --duration 10h --configs 18*SRCU-P"
> > > > > without any failure.
> > > > >
> > > > > 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 | 2 --
> > > > > 1 file changed, 2 deletions(-)
> > > > >
> > > > > diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
> > > > > index 7df59fc8073e..c54d6c04751f 100644
> > > > > --- a/kernel/rcu/srcutree.c
> > > > > +++ b/kernel/rcu/srcutree.c
> > > > > @@ -783,8 +783,6 @@ static void srcu_gp_end(struct srcu_struct *ssp)
> > > > > last_lvl = snp >= ssp->level[rcu_num_lvls - 1];
> > > > > if (last_lvl)
> > > > > cbs = ss_state < SRCU_SIZE_BIG || snp->srcu_have_cbs[idx] == gpseq;
> > > > > - snp->srcu_have_cbs[idx] = gpseq;
> > > > > - rcu_seq_set_state(&snp->srcu_have_cbs[idx], 1);
> > > > > sgsne = snp->srcu_gp_seq_needed_exp;
> > > > > if (srcu_invl_snp_seq(sgsne) || ULONG_CMP_LT(sgsne, gpseq))
> > > > > WRITE_ONCE(snp->srcu_gp_seq_needed_exp, gpseq);
> > > > > --
> > > > > 2.31.1
> > > > >
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2022-11-23 18:40 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-11-20 3:40 [PATCH 0/2] srcu: Optimize when srcu_gp_start_if_needed() holds Pingfan Liu
2022-11-20 3:40 ` [PATCH 1/2] srcu: Remove needless rcu_seq_done() check while holding read lock Pingfan Liu
2022-11-20 5:00 ` Zhang, Qiang1
2022-11-20 15:26 ` Pingfan Liu
2022-11-22 1:13 ` Paul E. McKenney
2022-11-22 9:50 ` Pingfan Liu
2022-11-20 3:40 ` [PATCH 2/2] srcu: Remove needless updating of srcu_have_cbs in srcu_gp_end() Pingfan Liu
2022-11-22 1:19 ` Paul E. McKenney
2022-11-22 9:59 ` Pingfan Liu
2022-11-22 14:45 ` Paul E. McKenney
2022-11-23 13:29 ` Pingfan Liu
2022-11-23 18:40 ` Paul E. McKenney
2022-11-20 15:20 ` [PATCH] srcu: Eliminate the case that snp_seq bigger than snap in srcu_funnel_gp_start() Pingfan Liu
2022-11-20 15:23 ` Pingfan Liu
2022-11-22 1:20 ` Paul E. McKenney
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.