Re: [PATCH v12 3/3] tracing: Centralize preemptirq tracepoints and unify their usage

From: Joel Fernandes <joelaf@google.com>
To: Steven Rostedt <rostedt@goodmis.org>
Cc: Joel Fernandes <joel@joelfernandes.org>,
	LKML <linux-kernel@vger.kernel.org>,
	"Cc: Android Kernel" <kernel-team@android.com>,
	Boqun Feng <boqun.feng@gmail.com>,
	Byungchul Park <byungchul.park@lge.com>,
	Ingo Molnar <mingo@redhat.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>,
	Tom Zanussi <tom.zanussi@linux.intel.com>
Subject: Re: [PATCH v12 3/3] tracing: Centralize preemptirq tracepoints and unify their usage
Date: Tue, 07 Aug 2018 19:54:13 -0400	[thread overview]
Message-ID: <6D0A3FD6-2190-4CC0-A3C0-7B3759E73243@google.com> (raw)
In-Reply-To: <20180807194515.4e549c1a@gandalf.local.home>

On August 7, 2018 7:45:15 PM EDT, Steven Rostedt <rostedt@goodmis.org> wrote:
>On Tue, 07 Aug 2018 11:24:13 -0400
>Joel Fernandes <joelaf@google.com> wrote:
>
>> On August 7, 2018 11:09:06 AM EDT, Steven Rostedt
><rostedt@goodmis.org> wrote:
>> >On Tue, 07 Aug 2018 10:48:05 -0400
>> >Joel Fernandes <joelaf@google.com> wrote:
>> >  
>> >> >You mean if someone add a tracepoint callback to the irq disable
>> >> >tracepoint, and did a lockdep assert to make sure interrupts are
>> >> >disabled?    
>> >> 
>> >> Yes that's what I meant.  
>> >
>> >That sounds like a "Doctor, it hurts me when I do this" problem ;-) 
>
>> 
>> Haha, yes true. But just to clarify, I didn't do this to see the
>problem but noticed it with turning on existing things. :-) but I see
>your point...
>> 
>
>OK, I hit this bug, but it's not because of the partial revert. This
>bug seems it needs to be another partial revert. I like you movement of
>the code, but I'm starting to doubt that we can use a trace event as a
>hook for critical areas yet. Well, not until we can use srcu in NMI.
>

I sent a patch to use srcu for all tracepoints including nmi. That patch also removes this warning and fixes the one other issue masami reported (hot plug causing a warning).

If Paul and Mathieu can confirm SRCU works on offline CPUs that would be great.

It's just this one warning or anything else that makes you feel tracepoints for critical paths isn't feasible?

Thanks.

>#define __DO_TRACE(tp, proto, args, cond, rcuidle)			\
>	do {								\
>		struct tracepoint_func *it_func_ptr;			\
>		void *it_func;						\
>		void *__data;						\
>		int __maybe_unused idx = 0;				\
>									\
>		if (!(cond))						\
>			return;						\
>									\
>		/* srcu can't be used from NMI */			\
>
>		WARN_ON_ONCE(rcuidle && in_nmi()); <== WARN_ON_ONCE hit!
>
>WARNING: CPU: 3 PID: 3727 at
>/work/build/trace/nobackup/linux-test.git/include/trace/events/preemptirq.h:38
>trace_irq_disable_rcuidle+0x2a/0x6c
>Modules linked in: ip6t_REJECT nf_reject_ipv6 nf_conntrack_ipv6
>nf_defrag_ipv6 ip6table_filter ip6_tables ipv6 crc_ccitt r8169 ppdev
>parport_pc parport
>CPU: 3 PID: 3727 Comm: trace-cmd Not tainted 4.18.0-rc6-test+ #14
>Hardware name: MSI MS-7823/CSM-H87M-G43 (MS-7823), BIOS V1.6 02/22/2014
>EIP: trace_irq_disable_rcuidle+0x2a/0x6c
>Code: e9 01 00 00 00 c3 64 8b 0d 24 e1 50 c1 0f a3 0d e0 f1 3f c1 73 55
>55 89 e5 57 56 53 51 64 8b 0d cc 37 51 c1 0f ba e1 14 73 02 <0f> 0b 89
>d7 89 c6 b8 e0 d8 2e c1 e8 8e 5b fa ff 8b 1d 9c 27 3d c1 
>EAX: c0401509 EBX: efa43680 ECX: 80110000 EDX: c0dc81f3
>ESI: ed823d44 EDI: efa43680 EBP: ed823cd0 ESP: ed823cc0
>DS: 007b ES: 007b FS: 00d8 GS: 00e0 SS: 0068 EFLAGS: 00010047
>CR0: 80050033 CR2: b7f06000 CR3: 30513b00 CR4: 001406f0
>Call Trace:
> trace_hardirqs_off_caller+0x23/0x2d
> trace_hardirqs_off_thunk+0xc/0x10
>EIP: default_do_nmi+0x1/0x157
>
>
>-- Steve

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.