On Mon, 6 Aug 2018 07:14:35 -0700 Joel Fernandes <joe...@google.com> wrote:
> On Mon, Aug 6, 2018 at 7:05 AM, Masami Hiramatsu <mhira...@kernel.org> wrote: > > On Sun, 5 Aug 2018 20:40:49 -0700 > > "Joel Fernandes (Google)" <j...@joelfernandes.org> wrote: > > > >> Recently we tried to make the preemptirqsoff tracer to use irqsoff > >> tracepoint probes. However this causes issues as reported by Masami: > >> > >> [2.271078] Testing tracer preemptirqsoff: .. no entries found ..FAILED! > >> [2.381015] WARNING: CPU: 0 PID: 1 at /home/mhiramat/ksrc/linux/kernel/ > >> trace/trace.c:1512 run_tracer_selftest+0xf3/0x154 > >> > >> This is due to the tracepoint code increasing the preempt nesting count > >> by calling an additional preempt_disable before calling into the > >> preemptoff tracer which messes up the preempt_count() check in > >> tracer_hardirqs_off. > >> > >> To fix this, make the irqsoff tracer probes balance the additional outer > >> preempt_disable with a preempt_enable_notrace. > > > > I've tested it and ensured this fixes the problem. > > > > Tested-by: Masami Hiramatsu <mhira...@kernel.org> > > Thanks! > > >> The other way to fix this is to just use SRCU for all tracepoints. > >> However we can't do that because we can't use NMIs from RCU context. > >> > >> Fixes: c3bc8fd637a9 ("tracing: Centralize preemptirq tracepoints > >> and unify their usage") > >> Fixes: e6753f23d961 ("tracepoint: Make rcuidle tracepoint callers use > >> SRCU") > >> Reported-by: Masami Hiramatsu <mhira...@kernel.org> > >> Signed-off-by: Joel Fernandes (Google) <j...@joelfernandes.org> > >> --- > >> kernel/trace/trace_irqsoff.c | 26 ++++++++++++++++++++++++++ > >> 1 file changed, 26 insertions(+) > >> > >> diff --git a/kernel/trace/trace_irqsoff.c b/kernel/trace/trace_irqsoff.c > >> index 770cd30cda40..ffbf1505d5bc 100644 > >> --- a/kernel/trace/trace_irqsoff.c > >> +++ b/kernel/trace/trace_irqsoff.c > >> @@ -603,14 +603,40 @@ static void irqsoff_tracer_stop(struct trace_array > >> *tr) > >> */ > >> static void tracer_hardirqs_on(void *none, unsigned long a0, unsigned > >> long a1) > >> { > > > > To ensure this function must not be preempted even if we increment preempt > > count, I think you should check irq_disabled() whole this process, put below > > here. > > > > if (unlikely(!irq_disabled())) > > return; > > > > Since irq_disabled() will be checked in irq_trace() anyway, so no problem > > to return here when !irq_disabled(). > > IRQs can never be enabled here. The trace hooks are called only after > disabling interrupts, or before enabling them. Right? > Even though, it should be verified or atleast commented on the function header. Thank you, -- Masami Hiramatsu <mhira...@kernel.org>