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]

Message-ID: <20180515005709.GE209519@joelaf.mtv.corp.google.com>
Date:   Mon, 14 May 2018 17:57:09 -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 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.

> > ---
> >  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.

> > + * "Startedleafroot": all non-root nodes from leaf to root marked for future start.
> 
> I got rid of the "non-root" part, given that we had to have marked
> the root to break out of the loop.

Ah yes, sorry. That's absolutely true.

thanks,

- Joel