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@xxxxxxxxxx>
> > > Cc: Josh Triplett <josh@xxxxxxxxxxxxxxxx>
> > > Cc: Lai Jiangshan <jiangshanlai@xxxxxxxxx>
> > > Cc: Joel Fernandes <joel@xxxxxxxxxxxxxxxxx>
> > > Cc: Neeraj Upadhyay <neeraju@xxxxxxxxxxxxxx>
> > > Cc: Boqun Feng <boqun.feng@xxxxxxxxx>
> >
> > 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@xxxxxxxxx>
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.
