| Message ID | 20210223001011.127063-11-frederic@kernel.org |
|---|---|
| State | Accepted |
| Commit | 3b2348e2fdf403b25a317b394db605257f321966 |
| Headers | show |
| Series | rcu/nocb updates v2 | expand |
On Tue, Feb 23, 2021 at 01:10:08AM +0100, Frederic Weisbecker wrote: > A NOCB-gp wake up can safely delete the nocb_bypass_timer. nocb_gp_wait() > is going to check again the bypass state and rearm the bypass timer if > necessary. > > Signed-off-by: Frederic Weisbecker <frederic@kernel.org> > Cc: Josh Triplett <josh@joshtriplett.org> > Cc: Lai Jiangshan <jiangshanlai@gmail.com> > Cc: Joel Fernandes <joel@joelfernandes.org> > Cc: Neeraj Upadhyay <neeraju@codeaurora.org> > Cc: Boqun Feng <boqun.feng@gmail.com> Give that you delete this code a couple of patches later in this series, why not just leave it out entirely? ;-) Thanx, Paul > --- > kernel/rcu/tree_plugin.h | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/kernel/rcu/tree_plugin.h b/kernel/rcu/tree_plugin.h > index b62ad79bbda5..9da67b0d3997 100644 > --- a/kernel/rcu/tree_plugin.h > +++ b/kernel/rcu/tree_plugin.h > @@ -1711,6 +1711,8 @@ static bool __wake_nocb_gp(struct rcu_data *rdp_gp, > del_timer(&rdp_gp->nocb_timer); > } > > + del_timer(&rdp_gp->nocb_bypass_timer); > + > if (force || READ_ONCE(rdp_gp->nocb_gp_sleep)) { > WRITE_ONCE(rdp_gp->nocb_gp_sleep, false); > needwake = true; > -- > 2.25.1 >
On Tue, Mar 02, 2021 at 05:24:56PM -0800, Paul E. McKenney wrote: > On Tue, Feb 23, 2021 at 01:10:08AM +0100, Frederic Weisbecker wrote: > > A NOCB-gp wake up can safely delete the nocb_bypass_timer. nocb_gp_wait() > > is going to check again the bypass state and rearm the bypass timer if > > necessary. > > > > Signed-off-by: Frederic Weisbecker <frederic@kernel.org> > > Cc: Josh Triplett <josh@joshtriplett.org> > > Cc: Lai Jiangshan <jiangshanlai@gmail.com> > > Cc: Joel Fernandes <joel@joelfernandes.org> > > Cc: Neeraj Upadhyay <neeraju@codeaurora.org> > > Cc: Boqun Feng <boqun.feng@gmail.com> > > Give that you delete this code a couple of patches later in this series, > why not just leave it out entirely? ;-) It's not exactly deleted later, it's rather merged within the "del_timer(&rdp_gp->nocb_timer)". The purpose of that patch is to make it clear that we explicitly cancel the nocb_bypass_timer here before we do it implicitly later with the merge of nocb_bypass_timer into nocb_timer. We could drop that patch, the resulting code in the end of the patchset will be the same of course but the behaviour detail described here might slip out of the reviewers attention :-) > > Thanx, Paul > > > --- > > kernel/rcu/tree_plugin.h | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/kernel/rcu/tree_plugin.h b/kernel/rcu/tree_plugin.h > > index b62ad79bbda5..9da67b0d3997 100644 > > --- a/kernel/rcu/tree_plugin.h > > +++ b/kernel/rcu/tree_plugin.h > > @@ -1711,6 +1711,8 @@ static bool __wake_nocb_gp(struct rcu_data *rdp_gp, > > del_timer(&rdp_gp->nocb_timer); > > } > > > > + del_timer(&rdp_gp->nocb_bypass_timer); > > + > > if (force || READ_ONCE(rdp_gp->nocb_gp_sleep)) { > > WRITE_ONCE(rdp_gp->nocb_gp_sleep, false); > > needwake = true; Thanks.
On Wed, Mar 10, 2021 at 11:17:02PM +0100, Frederic Weisbecker wrote: > On Tue, Mar 02, 2021 at 05:24:56PM -0800, Paul E. McKenney wrote: > > On Tue, Feb 23, 2021 at 01:10:08AM +0100, Frederic Weisbecker wrote: > > > A NOCB-gp wake up can safely delete the nocb_bypass_timer. nocb_gp_wait() > > > is going to check again the bypass state and rearm the bypass timer if > > > necessary. > > > > > > Signed-off-by: Frederic Weisbecker <frederic@kernel.org> > > > Cc: Josh Triplett <josh@joshtriplett.org> > > > Cc: Lai Jiangshan <jiangshanlai@gmail.com> > > > Cc: Joel Fernandes <joel@joelfernandes.org> > > > Cc: Neeraj Upadhyay <neeraju@codeaurora.org> > > > Cc: Boqun Feng <boqun.feng@gmail.com> > > > > Give that you delete this code a couple of patches later in this series, > > why not just leave it out entirely? ;-) > > It's not exactly deleted later, it's rather merged within the > "del_timer(&rdp_gp->nocb_timer)". > > The purpose of that patch is to make it clear that we explicitly cancel > the nocb_bypass_timer here before we do it implicitly later with the > merge of nocb_bypass_timer into nocb_timer. > > We could drop that patch, the resulting code in the end of the patchset > will be the same of course but the behaviour detail described here might > slip out of the reviewers attention :-) > How about merging the timers first and adding those small improvements later? i.e. move patch #12 #13 right after #7 (IIUC, #7 is the last requirement you need for merging timers), and then patch #8~#11 just follow, because IIUC, those patches are not about correctness but more about avoiding necessary timer fire-ups, right? Just my 2 cents. The overall patchset looks good to me ;-) Feel free to add Reviewed-by: Boqun Feng <boqun.feng@gmail.com> Regards, Boqun > > > > Thanx, Paul > > > > > --- > > > kernel/rcu/tree_plugin.h | 2 ++ > > > 1 file changed, 2 insertions(+) > > > > > > diff --git a/kernel/rcu/tree_plugin.h b/kernel/rcu/tree_plugin.h > > > index b62ad79bbda5..9da67b0d3997 100644 > > > --- a/kernel/rcu/tree_plugin.h > > > +++ b/kernel/rcu/tree_plugin.h > > > @@ -1711,6 +1711,8 @@ static bool __wake_nocb_gp(struct rcu_data *rdp_gp, > > > del_timer(&rdp_gp->nocb_timer); > > > } > > > > > > + del_timer(&rdp_gp->nocb_bypass_timer); > > > + > > > if (force || READ_ONCE(rdp_gp->nocb_gp_sleep)) { > > > WRITE_ONCE(rdp_gp->nocb_gp_sleep, false); > > > needwake = true; > > Thanks.
On Mon, Mar 15, 2021 at 10:53:36PM +0800, Boqun Feng wrote: > On Wed, Mar 10, 2021 at 11:17:02PM +0100, Frederic Weisbecker wrote: > > On Tue, Mar 02, 2021 at 05:24:56PM -0800, Paul E. McKenney wrote: > > > On Tue, Feb 23, 2021 at 01:10:08AM +0100, Frederic Weisbecker wrote: > > > > A NOCB-gp wake up can safely delete the nocb_bypass_timer. nocb_gp_wait() > > > > is going to check again the bypass state and rearm the bypass timer if > > > > necessary. > > > > > > > > Signed-off-by: Frederic Weisbecker <frederic@kernel.org> > > > > Cc: Josh Triplett <josh@joshtriplett.org> > > > > Cc: Lai Jiangshan <jiangshanlai@gmail.com> > > > > Cc: Joel Fernandes <joel@joelfernandes.org> > > > > Cc: Neeraj Upadhyay <neeraju@codeaurora.org> > > > > Cc: Boqun Feng <boqun.feng@gmail.com> > > > > > > Give that you delete this code a couple of patches later in this series, > > > why not just leave it out entirely? ;-) > > > > It's not exactly deleted later, it's rather merged within the > > "del_timer(&rdp_gp->nocb_timer)". > > > > The purpose of that patch is to make it clear that we explicitly cancel > > the nocb_bypass_timer here before we do it implicitly later with the > > merge of nocb_bypass_timer into nocb_timer. > > > > We could drop that patch, the resulting code in the end of the patchset > > will be the same of course but the behaviour detail described here might > > slip out of the reviewers attention :-) > > > > How about merging the timers first and adding those small improvements > later? i.e. move patch #12 #13 right after #7 (IIUC, #7 is the last > requirement you need for merging timers) Hmm, nope, patches 9 and 10 are actually preparation work for timers merge. In fact they could even be skipped and timers could be merged directly but I wanted the unified behaviour to be fully explicit for reviewers through those incremental changes before merging the timers together. >, and then patch #8~#11 just follow Patch 8 really need to stay where it is because it is an important limitation on nocb de-offloading that can be removed right after patch 7 (which itself removes the sole reason for rdp leader to remain nocb) and it doesn't depend on the timers unification that comes after. > > Just my 2 cents. The overall patchset looks good to me ;-) > > Feel free to add > > Reviewed-by: Boqun Feng <boqun.feng@gmail.com> Thanks a lot for checking that!
On Mon, Mar 15, 2021 at 11:56:33PM +0100, Frederic Weisbecker wrote: > On Mon, Mar 15, 2021 at 10:53:36PM +0800, Boqun Feng wrote: > > On Wed, Mar 10, 2021 at 11:17:02PM +0100, Frederic Weisbecker wrote: > > > On Tue, Mar 02, 2021 at 05:24:56PM -0800, Paul E. McKenney wrote: > > > > On Tue, Feb 23, 2021 at 01:10:08AM +0100, Frederic Weisbecker wrote: > > > > > A NOCB-gp wake up can safely delete the nocb_bypass_timer. nocb_gp_wait() > > > > > is going to check again the bypass state and rearm the bypass timer if > > > > > necessary. > > > > > > > > > > Signed-off-by: Frederic Weisbecker <frederic@kernel.org> > > > > > Cc: Josh Triplett <josh@joshtriplett.org> > > > > > Cc: Lai Jiangshan <jiangshanlai@gmail.com> > > > > > Cc: Joel Fernandes <joel@joelfernandes.org> > > > > > Cc: Neeraj Upadhyay <neeraju@codeaurora.org> > > > > > Cc: Boqun Feng <boqun.feng@gmail.com> > > > > > > > > Give that you delete this code a couple of patches later in this series, > > > > why not just leave it out entirely? ;-) > > > > > > It's not exactly deleted later, it's rather merged within the > > > "del_timer(&rdp_gp->nocb_timer)". > > > > > > The purpose of that patch is to make it clear that we explicitly cancel > > > the nocb_bypass_timer here before we do it implicitly later with the > > > merge of nocb_bypass_timer into nocb_timer. > > > > > > We could drop that patch, the resulting code in the end of the patchset > > > will be the same of course but the behaviour detail described here might > > > slip out of the reviewers attention :-) > > > > > > > How about merging the timers first and adding those small improvements > > later? i.e. move patch #12 #13 right after #7 (IIUC, #7 is the last > > requirement you need for merging timers) > > Hmm, nope, patches 9 and 10 are actually preparation work for timers merge. > In fact they could even be skipped and timers could be merged directly but I > wanted the unified behaviour to be fully explicit for reviewers through those > incremental changes before merging the timers together. > > >, and then patch #8~#11 just follow > > Patch 8 really need to stay where it is because it is an important limitation > on nocb de-offloading that can be removed right after patch 7 (which itself > removes the sole reason for rdp leader to remain nocb) and it doesn't depend > on the timers unification that comes after. > > > > > Just my 2 cents. The overall patchset looks good to me ;-) > > > > Feel free to add > > > > Reviewed-by: Boqun Feng <boqun.feng@gmail.com> > > Thanks a lot for checking that! Applied to 10/13, thank you both! Thanx, Paul
diff --git a/kernel/rcu/tree_plugin.h b/kernel/rcu/tree_plugin.h index b62ad79bbda5..9da67b0d3997 100644 --- a/kernel/rcu/tree_plugin.h +++ b/kernel/rcu/tree_plugin.h @@ -1711,6 +1711,8 @@ static bool __wake_nocb_gp(struct rcu_data *rdp_gp, del_timer(&rdp_gp->nocb_timer); } + del_timer(&rdp_gp->nocb_bypass_timer); + if (force || READ_ONCE(rdp_gp->nocb_gp_sleep)) { WRITE_ONCE(rdp_gp->nocb_gp_sleep, false); needwake = true;
A NOCB-gp wake up can safely delete the nocb_bypass_timer. nocb_gp_wait() is going to check again the bypass state and rearm the bypass timer if necessary. Signed-off-by: Frederic Weisbecker <frederic@kernel.org> Cc: Josh Triplett <josh@joshtriplett.org> Cc: Lai Jiangshan <jiangshanlai@gmail.com> Cc: Joel Fernandes <joel@joelfernandes.org> Cc: Neeraj Upadhyay <neeraju@codeaurora.org> Cc: Boqun Feng <boqun.feng@gmail.com> --- kernel/rcu/tree_plugin.h | 2 ++ 1 file changed, 2 insertions(+)