From: Steven Rostedt <rostedt@goodmis.org>
To: Joel Fernandes <joel@joelfernandes.org>
Cc: linux-kernel@vger.kernel.org, kernel-team@android.com,
Boqun Feng <boqun.feng@gmail.com>,
Byungchul Park <byungchul.park@lge.com>,
Erick Reyes <erickreyes@google.com>,
Ingo Molnar <mingo@redhat.com>, Julia Cartwright <julia@ni.com>,
Masami Hiramatsu <mhiramat@kernel.org>,
Mathieu Desnoyers <mathieu.desnoyers@efficios.com>,
Namhyung Kim <namhyung@kernel.org>,
Paul McKenney <paulmck@linux.vnet.ibm.com>,
Peter Zijlstra <peterz@infradead.org>,
Thomas Glexiner <tglx@linutronix.de>,
Todd Kjos <tkjos@google.com>,
Tom Zanussi <tom.zanussi@linux.intel.com>,
Will Deacon <will.deacon@arm.com>
Subject: Re: [PATCH v11 3/3] tracing: Centralize preemptirq tracepoints and unify their usage
Date: Fri, 27 Jul 2018 12:28:25 -0400 [thread overview]
Message-ID: <20180727122825.281a3a67@gandalf.local.home> (raw)
In-Reply-To: <20180726235044.10195-4-joel@joelfernandes.org>
On Thu, 26 Jul 2018 16:50:44 -0700
Joel Fernandes <joel@joelfernandes.org> wrote:
> From: "Joel Fernandes (Google)" <joel@joelfernandes.org>
>
> This patch detaches the preemptirq tracepoints from the tracers and
> keeps it separate.
>
> Advantages:
> * Lockdep and irqsoff event can now run in parallel since they no longer
> have their own calls.
>
> * This unifies the usecase of adding hooks to an irqsoff and irqson
> event, and a preemptoff and preempton event.
> 3 users of the events exist:
> - Lockdep
> - irqsoff and preemptoff tracers
> - irqs and preempt trace events
>
> The unification cleans up several ifdefs and makes the code in preempt
> tracer and irqsoff tracers simpler. It gets rid of all the horrific
> ifdeferry around PROVE_LOCKING and makes configuration of the different
> users of the tracepoints more easy and understandable. It also gets rid
> of the time_* function calls from the lockdep hooks used to call into
> the preemptirq tracer which is not needed anymore. The negative delta in
> lines of code in this patch is quite large too.
>
> In the patch we introduce a new CONFIG option PREEMPTIRQ_TRACEPOINTS
> as a single point for registering probes onto the tracepoints. With
> this,
> the web of config options for preempt/irq toggle tracepoints and its
> users becomes:
>
> PREEMPT_TRACER PREEMPTIRQ_EVENTS IRQSOFF_TRACER PROVE_LOCKING
> | | \ | |
> \ (selects) / \ \ (selects) /
> TRACE_PREEMPT_TOGGLE ----> TRACE_IRQFLAGS
> \ /
> \ (depends on) /
> PREEMPTIRQ_TRACEPOINTS
>
> One note, I have to check for lockdep recursion in the code that calls
> the trace events API and bail out if we're in lockdep recursion
> protection to prevent something like the following case: a spin_lock is
> taken. Then lockdep_acquired is called. That does a raw_local_irq_save
> and then sets lockdep_recursion, and then calls __lockdep_acquired. In
> this function, a call to get_lock_stats happens which calls
> preempt_disable, which calls trace IRQS off somewhere which enters my
> tracepoint code and sets the tracing_irq_cpu flag to prevent recursion.
> This flag is then never cleared causing lockdep paths to never be
> entered and thus causing splats and other bad things.
>
> Other than the performance tests mentioned in the previous patch, I also
> ran the locking API test suite. I verified that all tests cases are
> passing.
>
> I also injected issues by not registering lockdep probes onto the
> tracepoints and I see failures to confirm that the probes are indeed
> working.
>
> This series + lockdep probes not registered (just to inject errors):
> [ 0.000000] hard-irqs-on + irq-safe-A/21: ok | ok | ok |
> [ 0.000000] soft-irqs-on + irq-safe-A/21: ok | ok | ok |
> [ 0.000000] sirq-safe-A => hirqs-on/12:FAILED|FAILED| ok |
> [ 0.000000] sirq-safe-A => hirqs-on/21:FAILED|FAILED| ok |
> [ 0.000000] hard-safe-A + irqs-on/12:FAILED|FAILED| ok |
> [ 0.000000] soft-safe-A + irqs-on/12:FAILED|FAILED| ok |
> [ 0.000000] hard-safe-A + irqs-on/21:FAILED|FAILED| ok |
> [ 0.000000] soft-safe-A + irqs-on/21:FAILED|FAILED| ok |
> [ 0.000000] hard-safe-A + unsafe-B #1/123: ok | ok | ok |
> [ 0.000000] soft-safe-A + unsafe-B #1/123: ok | ok | ok |
>
> With this series + lockdep probes registered, all locking tests pass:
>
> [ 0.000000] hard-irqs-on + irq-safe-A/21: ok | ok | ok |
> [ 0.000000] soft-irqs-on + irq-safe-A/21: ok | ok | ok |
> [ 0.000000] sirq-safe-A => hirqs-on/12: ok | ok | ok |
> [ 0.000000] sirq-safe-A => hirqs-on/21: ok | ok | ok |
> [ 0.000000] hard-safe-A + irqs-on/12: ok | ok | ok |
> [ 0.000000] soft-safe-A + irqs-on/12: ok | ok | ok |
> [ 0.000000] hard-safe-A + irqs-on/21: ok | ok | ok |
> [ 0.000000] soft-safe-A + irqs-on/21: ok | ok | ok |
> [ 0.000000] hard-safe-A + unsafe-B #1/123: ok | ok | ok |
> [ 0.000000] soft-safe-A + unsafe-B #1/123: ok | ok | ok |
>
> Reviewed-by: Namhyung Kim <namhyung@kernel.org>
> Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> ---
> include/linux/ftrace.h | 11 +-
> include/linux/irqflags.h | 11 +-
> include/linux/lockdep.h | 8 +-
> include/linux/preempt.h | 2 +-
> include/trace/events/preemptirq.h | 23 +--
> init/main.c | 5 +-
> kernel/locking/lockdep.c | 35 ++---
Did Peter ever give an Acked-by for this patch?
-- Steve
> kernel/sched/core.c | 2 +-
> kernel/trace/Kconfig | 22 ++-
> kernel/trace/Makefile | 2 +-
> kernel/trace/trace_irqsoff.c | 231 ++++++++----------------------
> kernel/trace/trace_preemptirq.c | 72 ++++++++++
> 12 files changed, 195 insertions(+), 229 deletions(-)
> create mode 100644 kernel/trace/trace_preemptirq.c
>
>
next prev parent reply other threads:[~2018-07-27 16:28 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-07-26 23:50 [PATCH v11 0/3] tracing: Centralize preemptirq tracepoints and unify their usage Joel Fernandes
2018-07-26 23:50 ` [PATCH v11 1/3] lockdep: use this_cpu_ptr instead of get_cpu_var stats Joel Fernandes
2018-07-26 23:50 ` [PATCH v11 2/3] tracepoint: Make rcuidle tracepoint callers use SRCU Joel Fernandes
2018-07-27 16:26 ` Steven Rostedt
2018-07-27 18:51 ` Joel Fernandes
2018-07-27 18:57 ` Steven Rostedt
2018-07-27 21:47 ` Joel Fernandes
2018-07-26 23:50 ` [PATCH v11 3/3] tracing: Centralize preemptirq tracepoints and unify their usage Joel Fernandes
2018-07-27 16:28 ` Steven Rostedt [this message]
2018-07-27 18:55 ` Joel Fernandes
2018-07-30 8:58 ` Peter Zijlstra
2018-07-30 19:18 ` Joel Fernandes
2018-07-30 8:59 ` [PATCH v11 0/3] " Peter Zijlstra
2018-07-30 19:25 ` Joel Fernandes
2018-07-30 22:00 ` 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=20180727122825.281a3a67@gandalf.local.home \
--to=rostedt@goodmis.org \
--cc=boqun.feng@gmail.com \
--cc=byungchul.park@lge.com \
--cc=erickreyes@google.com \
--cc=joel@joelfernandes.org \
--cc=julia@ni.com \
--cc=kernel-team@android.com \
--cc=linux-kernel@vger.kernel.org \
--cc=mathieu.desnoyers@efficios.com \
--cc=mhiramat@kernel.org \
--cc=mingo@redhat.com \
--cc=namhyung@kernel.org \
--cc=paulmck@linux.vnet.ibm.com \
--cc=peterz@infradead.org \
--cc=tglx@linutronix.de \
--cc=tkjos@google.com \
--cc=tom.zanussi@linux.intel.com \
--cc=will.deacon@arm.com \
/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).