Message-Id: <20180802235527.ddb0d1837154e3f98571b3b0@kernel.org>
Date:   Thu, 2 Aug 2018 23:55:27 +0900
From:   Masami Hiramatsu <mhiramat@...nel.org>
To:     Joel Fernandes <joel@...lfernandes.org>
Cc:     linux-kernel@...r.kernel.org, kernel-team@...roid.com,
        Boqun Feng <boqun.feng@...il.com>,
        Byungchul Park <byungchul.park@....com>,
        Ingo Molnar <mingo@...hat.com>,
        Masami Hiramatsu <mhiramat@...nel.org>,
        Mathieu Desnoyers <mathieu.desnoyers@...icios.com>,
        Namhyung Kim <namhyung@...nel.org>,
        Paul McKenney <paulmck@...ux.vnet.ibm.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Steven Rostedt <rostedt@...dmis.org>,
        Thomas Glexiner <tglx@...utronix.de>,
        Tom Zanussi <tom.zanussi@...ux.intel.com>
Subject: Re: [PATCH v12 0/3] tracing: Centralize preemptirq tracepoints and
 unify their usage

Hi Joel,

I found this caused several issues when testing ftrace.

#1) ftrace boottest (FTRACE_STARTUP_TEST) fails
#2) mmiotrace reports "IRQs not enabled as expected" error
#3) lock subsystem event boottest causes "IRQs not disabled as expected" error (sometimes)
#4) ftracetest test.d/event/toplevel-enable.tc causes "suspicious RCU usage" warning

#1-#3 were resolved if I reverted this [3/3] patch.
#4 is resolved if I revered this [2/3] patch.

See attached logs for details.
I also attached my kernel .config.
I guess the first one comes from PREEMPTIRQ_TRACEPOINTS=n,
so it should also disable preemptirq tracer.
But #2-#4 we should look into it.
Could you help us to solve these issues?

Thank you,


On Mon, 30 Jul 2018 15:24:20 -0700
Joel Fernandes <joel@...lfernandes.org> wrote:

> From: "Joel Fernandes (Google)" <joel@...lfernandes.org>
> 
> This series contains the last 2 patches with minor changes suggested by Peter
> and Steven, and an additional clean up of get_lock_stats as suggested by Peter.
> 
> The preempt/irq tracepoints exist but not everything in the kernel is using it
> whenever they need to be notified that a preempt disable/enable or an irq
> disable/enable has occurred.  This makes things not work simultaneously (for
> example, only either lockdep or irqsoff trace-events can be used at a time).
> 
> This is particularly painful to deal with, since turning on lockdep breaks
> tracers that install probes on IRQ events, such as the BCC atomic critical
> section tracer [1]. This constraint also makes it not possible to use synthetic
> events to trace irqsoff sections with lockdep simulataneously turned on.
> 
> This series solves that, and also results in a nice clean up of relevant parts
> of the kernel. Several ifdefs are simpler, and the design is more unified and
> better. Also as a result of this, we also speeded performance all rcuidle
> tracepoints since their handling is simpler.
> 
> [1] https://github.com/iovisor/bcc/pull/1801/
> 
> v11->v12:
> - minor corrections to changelog 
> - Added PeterZ's Acks
> - Squashed in fix for get_lock_stats
> 
> v10->v11:
> - Dropped extra unneeded else statement since
>   rcu_read_lock_sched_notrace is same as prempt_disable_notrace (PeterZ)
> 
> v9->v10:
> - Dropped first 3 and last 2 patches that were applied previously
> - Folded SPDK license into the main patch introducing trace_preemptirq.c (Steve)
> - Dropped lockdep_recursing & use simplify get_cpu_var instead (PeterZ)
> - Simplify __DO_TRACE and use rcu_dereference_raw for both RCU and SRCU (PeterZ)
> 
> v8->v9:
> - Small style changes to tracepoint code (Mathieu)
> - Minor style fix to use PTR_ERR_OR_ZERO (0-day bot)
> - Minor fix to test_atomic_sections to use unsigned long.
> - Added Namhyung's, Mathieu's Reviewed-by to some patches.
> 
> v7->v8:
> - Refactored irqsoff tracer probe defines (Namhyung)
> 
> v6->v7:
> - Added a module to simulate an atomic section, a kselftest to load and
>   and trigger it which verifies the preempt-tracer and this series.
> 
> - Fixed a new warning after I rebased in early boot, this is because
> early_boot_irqs_disabled was set too early, I moved it after the lockdep
> initialization.
> 
> - added back the softirq fix since it appears it wasn't picked up.
> 
> - Ran Ingo's locking API selftest suite which are passing with this
>   series.
> 
> - Mathieu suggested ifdef'ing the tracepoint_synchronize_unregister
>   function incase tracepoints aren't enabled, did that.
> 
> Joel Fernandes (Google) (3):
>   lockdep: use this_cpu_ptr instead of get_cpu_var stats
>   tracepoint: Make rcuidle tracepoint callers use SRCU
>   tracing: Centralize preemptirq tracepoints and unify their usage
> 
>  include/linux/ftrace.h            |  11 +-
>  include/linux/irqflags.h          |  11 +-
>  include/linux/lockdep.h           |   8 +-
>  include/linux/preempt.h           |   2 +-
>  include/linux/tracepoint.h        |  41 ++++--
>  include/trace/events/preemptirq.h |  23 +--
>  init/main.c                       |   5 +-
>  kernel/locking/lockdep.c          |  45 ++----
>  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 ++++++++++
>  kernel/tracepoint.c               |  16 ++-
>  14 files changed, 244 insertions(+), 247 deletions(-)
>  create mode 100644 kernel/trace/trace_preemptirq.c
> 
> -- 
> 2.18.0.345.g5c9ce644c3-goog


-- 
Masami Hiramatsu <mhiramat@...nel.org>

View attachment "bug_no1_boot_test.txt" of type "text/plain" (2344 bytes)

View attachment "bug_no2_mmiotrace.txt" of type "text/plain" (3191 bytes)

View attachment "bug_no3_lock_event_boottest.txt" of type "text/plain" (14394 bytes)

View attachment "bug_no4_toplevel-kselftest.txt" of type "text/plain" (7155 bytes)

Download attachment ".config" of type "application/octet-stream" (76048 bytes)

Powered by blists - more mailing lists