From: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
To: Steven Rostedt <rostedt@goodmis.org>
Cc: linux-kernel@vger.kernel.org, Ingo Molnar <mingo@kernel.org>,
Andrew Morton <akpm@linux-foundation.org>
Subject: Re: [PATCH 5/5 v2] rcu: Fix dyntick-idle tracing
Date: Fri, 7 Apr 2017 08:09:22 -0700 [thread overview]
Message-ID: <20170407150922.GV1600@linux.vnet.ibm.com> (raw)
In-Reply-To: <20170407105316.0cda3282@gandalf.local.home>
On Fri, Apr 07, 2017 at 10:53:16AM -0400, Steven Rostedt wrote:
> On Fri, 7 Apr 2017 07:40:11 -0700
> "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> wrote:
>
> > On Fri, Apr 07, 2017 at 10:01:11AM -0400, Steven Rostedt wrote:
> > > From: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
> > >
> > > The tracing subsystem started using rcu_irq_entry() and rcu_irq_exit()
> > > (with my blessing) to allow the current _rcuidle alternative tracepoint
> > > name to be dispensed with while still maintaining good performance.
> > > Unfortunately, this causes RCU's dyntick-idle entry code's tracing to
> > > appear to RCU like an interrupt that occurs where RCU is not designed
> > > to handle interrupts.
> > >
> > > This commit fixes this problem by moving the zeroing of ->dynticks_nesting
> > > after the offending trace_rcu_dyntick() statement, which narrows the
> > > window of vulnerability to a pair of adjacent statements that are now
> > > marked with comments to that effect.
> > >
> > > Link: http://lkml.kernel.org/r/20170405193928.GM1600@linux.vnet.ibm.com
> > >
> > > Reported-by: Steven Rostedt <rostedt@goodmis.org>
> > > Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> > > Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> > > ---
> > > kernel/rcu/tree.c | 48 +++++++++++++++++++++++-------------------------
> > > 1 file changed, 23 insertions(+), 25 deletions(-)
> > >
> > > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> > > index 50fee7689e71..8b4d273331e4 100644
> > > --- a/kernel/rcu/tree.c
> > > +++ b/kernel/rcu/tree.c
> > > @@ -57,6 +57,7 @@
> > > #include <linux/random.h>
> > > #include <linux/trace_events.h>
> > > #include <linux/suspend.h>
> > > +#include <linux/ftrace.h>
> > >
> > > #include "tree.h"
> > > #include "rcu.h"
> > > @@ -771,25 +772,24 @@ cpu_needs_another_gp(struct rcu_state *rsp, struct rcu_data *rdp)
> > > }
> > >
> > > /*
> > > - * rcu_eqs_enter_common - current CPU is moving towards extended quiescent state
> > > + * rcu_eqs_enter_common - current CPU is entering an extended quiescent state
> > > *
> > > - * If the new value of the ->dynticks_nesting counter now is zero,
> > > - * we really have entered idle, and must do the appropriate accounting.
> > > - * The caller must have disabled interrupts.
> > > + * Enter idle, doing appropriate accounting. The caller must have
> > > + * disabled interrupts.
> > > */
> > > -static void rcu_eqs_enter_common(long long oldval, bool user)
> > > +static void rcu_eqs_enter_common(bool user)
> > > {
> > > struct rcu_state *rsp;
> > > struct rcu_data *rdp;
> > > - RCU_TRACE(struct rcu_dynticks *rdtp = this_cpu_ptr(&rcu_dynticks);)
> > > + struct rcu_dynticks *rdtp = this_cpu_ptr(&rcu_dynticks);
> > >
> > > - trace_rcu_dyntick(TPS("Start"), oldval, rdtp->dynticks_nesting);
> > > + trace_rcu_dyntick(TPS("Start"), rdtp->dynticks_nesting, 0);
> > > if (IS_ENABLED(CONFIG_RCU_EQS_DEBUG) &&
> > > !user && !is_idle_task(current)) {
> > > struct task_struct *idle __maybe_unused =
> > > idle_task(smp_processor_id());
> > >
> > > - trace_rcu_dyntick(TPS("Error on entry: not idle task"), oldval, 0);
> > > + trace_rcu_dyntick(TPS("Error on entry: not idle task"), rdtp->dynticks_nesting, 0);
> > > rcu_ftrace_dump(DUMP_ORIG);
> > > WARN_ONCE(1, "Current pid: %d comm: %s / Idle pid: %d comm: %s",
> > > current->pid, current->comm,
> > > @@ -800,7 +800,10 @@ static void rcu_eqs_enter_common(long long oldval, bool user)
> > > do_nocb_deferred_wakeup(rdp);
> > > }
> > > rcu_prepare_for_idle();
> > > - rcu_dynticks_eqs_enter();
> > > + stack_tracer_disable();
> > > + rdtp->dynticks_nesting = 0; /* Breaks tracing momentarily. */
> > > + rcu_dynticks_eqs_enter(); /* After this, tracing works again. */
> > > + stack_tracer_enable();
> >
> > Hmmm... There is not supposed to be any tracing in this interval,
>
> Why not? function tracing happens without an issue. But then again,
> function tracing doesn't depend on RCU.
>
> > and interrupts are disabled. Wouldn't it be better to have something
> > that made tracing illegal during this interval?
>
> I don't see an issue here. Function tracing is fine. There should be no
> trace_events() as those are static events and shouldn't dynamically
> appear in this interval.
>
> The problem I hit is that stack tracing uses function tracing to check
> the stack of all functions. It doesn't need RCU either, unless it hits
> a new "max stack", which it then calls save_stack_trace(), which does a
> lot, and it does perform an rcu_read_lock(), which is what broke.
>
> I'm fine with tracing, as that works. What doesn't work is tracing a
> new max stack.
>
> >
> > Yeah, I am a bit concerned about idle-entry latency...
> >
>
> Which should now be fine because of the inlined this_cpu_inc/dec()
> which is very efficient and made for fast paths like this.
OK, I am willing to let people complain if they can measure the
difference. If they can measure the difference, there are some things
we can do.
Thanx, Paul
next prev parent reply other threads:[~2017-04-07 15:09 UTC|newest]
Thread overview: 39+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-04-07 14:01 [PATCH 0/5 v2] tracing: Add usecase of synchronize_rcu_tasks() and stack_tracer_disable() Steven Rostedt
2017-04-07 14:01 ` [PATCH 1/5 v2] ftrace: Add use of synchronize_rcu_tasks() with dynamic trampolines Steven Rostedt
2017-04-07 14:01 ` [PATCH 2/5 v2] tracing: Replace the per_cpu() with this_cpu() in trace_stack.c Steven Rostedt
2017-04-07 14:36 ` Paul E. McKenney
2017-04-07 14:48 ` Steven Rostedt
2017-04-07 15:08 ` Paul E. McKenney
2017-04-07 14:01 ` [PATCH 3/5 v2] tracing: Add stack_tracer_disable/enable() functions Steven Rostedt
2017-04-07 14:22 ` Steven Rostedt
2017-04-07 14:25 ` [PATCH 3/5 v2.1] " Steven Rostedt
2017-04-07 14:01 ` [PATCH 4/5 v2] tracing: Rename trace_active to disable_stack_tracer and inline its modification Steven Rostedt
2017-04-07 14:01 ` [PATCH 5/5 v2] rcu: Fix dyntick-idle tracing Steven Rostedt
2017-04-07 14:40 ` Paul E. McKenney
2017-04-07 14:53 ` Steven Rostedt
2017-04-07 15:09 ` Paul E. McKenney [this message]
2017-04-07 15:29 ` Steven Rostedt
2017-04-07 14:43 ` [PATCH 0/5 v2] tracing: Add usecase of synchronize_rcu_tasks() and stack_tracer_disable() Paul E. McKenney
2017-04-07 14:58 ` Steven Rostedt
2017-04-07 15:11 ` Paul E. McKenney
2017-04-07 15:28 ` Steven Rostedt
2017-04-07 16:35 ` [PATCH 6/5]rcu/tracing: Add rcu_disabled to denote when rcu_irq_enter() will not work Steven Rostedt
2017-04-07 16:42 ` Paul E. McKenney
2017-04-07 16:44 ` Steven Rostedt
2017-04-07 16:53 ` Paul E. McKenney
2017-04-07 17:03 ` [PATCH 6/5 v2] rcu/tracing: " Steven Rostedt
2017-04-07 17:15 ` Paul E. McKenney
2017-04-07 17:06 ` [PATCH 7/5] tracing: Make sure rcu_irq_enter() can work for trace_*_rcuidle() trace events Steven Rostedt
2017-04-07 17:15 ` Paul E. McKenney
2017-04-07 17:19 ` Mathieu Desnoyers
2017-04-07 17:26 ` Steven Rostedt
2017-04-07 17:32 ` Steven Rostedt
2017-04-07 17:49 ` Mathieu Desnoyers
2017-04-07 17:55 ` Steven Rostedt
2017-04-07 18:10 ` [PATCH 7/5 v3] " Steven Rostedt
2017-04-07 18:17 ` Mathieu Desnoyers
2017-04-07 19:41 ` [PATCH 7/5 v4] " Steven Rostedt
2017-04-07 19:43 ` Steven Rostedt
2017-04-10 17:11 ` Mathieu Desnoyers
2017-04-07 17:28 ` [PATCH 7/5] " Steven Rostedt
2017-04-07 17:48 ` [PATCH 7/5 v2] " Steven Rostedt
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20170407150922.GV1600@linux.vnet.ibm.com \
--to=paulmck@linux.vnet.ibm.com \
--cc=akpm@linux-foundation.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mingo@kernel.org \
--cc=rostedt@goodmis.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).