| Message ID | 20221120034014.7390-2-kernelfans@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | srcu: Optimize when srcu_gp_start_if_needed() holds | expand |
> >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);
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 >
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 >
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 > >
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);
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(-)