Re: [PATCH RFC 6/8] rcu: Add back the Startedleaf tracepoint

[Paul E. McKenney]

On Tue, May 15, 2018 at 04:04:30PM -0700, Joel Fernandes wrote:
> On Mon, May 14, 2018 at 08:46:03PM -0700, Paul E. McKenney wrote:
> > On Mon, May 14, 2018 at 05:57:09PM -0700, Joel Fernandes wrote:
> > > On Mon, May 14, 2018 at 11:38:23AM -0700, Paul E. McKenney wrote:
> > > > On Sun, May 13, 2018 at 08:15:39PM -0700, Joel Fernandes (Google) wrote:
> > > > > In recent discussion [1], the check for whether a leaf believes RCU is
> > > > > not idle, is being added back to funnel locking code, to avoid more
> > > > > locking. In this we are marking the leaf node for a future grace-period
> > > > > and bailing out since a GP is currently in progress. However the
> > > > > tracepoint is missing. Lets add it back.
> > > > > 
> > > > > Also add a small comment about why we do this check (basically the point
> > > > > is to avoid locking intermediate nodes unnecessarily) and clarify the
> > > > > comments in the trace event header now that we are doing traversal of
> > > > > one or more intermediate nodes.
> > > > > 
> > > > > [1] http://lkml.kernel.org/r/20180513190906.GL26088@xxxxxxxxxxxxxxxxxx
> > > > > 
> > > > > Signed-off-by: Joel Fernandes (Google) <joel@xxxxxxxxxxxxxxxxx>
> > > > 
> > > > Looks like a good idea, but it does not apply -- which is not a surprise,
> > > > given the change rate in this code.  I hand-applied as a modification
> > > > to c1b3f9fce26f ("rcu: Don't funnel-lock above leaf node if GP in progress")
> > > > with attribution, but with the changes below.  Please let me know if I
> > > > am missing something.
> > > > 
> > > > Ah, I see -- this commit depends on your earlier name-change commit.
> > > > I therefore made this patch use the old names.
> > > 
> > > Ok, I'll check your new tree and rebase.
> > 
> > Sounds good!
> > 
> > > > > ---
> > > > >  include/trace/events/rcu.h |  4 ++--
> > > > >  kernel/rcu/tree.c          | 11 ++++++++++-
> > > > >  2 files changed, 12 insertions(+), 3 deletions(-)
> > > > > 
> > > > > diff --git a/include/trace/events/rcu.h b/include/trace/events/rcu.h
> > > > > index 539900a9f8c7..dc0bd11739c7 100644
> > > > > --- a/include/trace/events/rcu.h
> > > > > +++ b/include/trace/events/rcu.h
> > > > > @@ -91,8 +91,8 @@ TRACE_EVENT(rcu_grace_period,
> > > > >   *
> > > > >   * "Startleaf": Request a grace period based on leaf-node data.
> > > > >   * "Prestarted": Someone beat us to the request
> > > > > - * "Startedleaf": Leaf-node start proved sufficient.
> > > > > - * "Startedleafroot": Leaf-node start proved sufficient after checking root.
> > > > > + * "Startedleaf": Leaf and one or more non-root nodes marked for future start.
> > > > 
> > > > Actually, we only get to that trace if all we did was mark the leaf
> > > > node, right?
> > > 
> > > I didn't think so. In the code we are doing the check for rnp every time we
> > > walk up the tree. So even when we are on an intermediate node, we do the
> > > check of the node we started with. I thought that's what you wanted to do. It
> > > makes sense to me to do so too.
> > 
> > If we are not on the initial (usually leaf) node, then the similar check
> > in the previous "if" statement would have sent us to unlock_out, right?
> > 
> > (And yes, I should have said "mark the initial node" above.)
> 
> I may have missed this, sorry. 
> 
> Yes, that would be true unless the check could be true not at the firsti
> iteration, but after the first iteration? (i.e. another path started the
> initially idle GP). That's why I changed it to "one or more non-root nodes
> marked".

After the first iteration, the check after setting ->gp_seq_needed is
dead code.  If that check would have succeeded, the same check in the
big "if" statement would have taken the early exit.

							Thanx, Paul

> What do you think?
> 
> thanks,
> 
> - Joel
>