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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 16 May 2018 16:13:05 -0700
From:   Joel Fernandes <joel@...lfernandes.org>
To:     "Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>
Cc:     linux-kernel@...r.kernel.org,
        Josh Triplett <josh@...htriplett.org>,
        Steven Rostedt <rostedt@...dmis.org>,
        Mathieu Desnoyers <mathieu.desnoyers@...icios.com>,
        Lai Jiangshan <jiangshanlai@...il.com>, byungchul.park@....com,
        kernel-team@...roid.com
Subject: Re: [PATCH RFC 6/8] rcu: Add back the Startedleaf tracepoint

On Wed, May 16, 2018 at 08:48:29AM -0700, Paul E. McKenney wrote:
> 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@linux.vnet.ibm.com
> > > > > > 
> > > > > > Signed-off-by: Joel Fernandes (Google) <joel@...lfernandes.org>
> > > > > 
> > > > > 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.

Oh yes, ofcourse!! I understand it now. thanks,

 - Joel