[PATCH -next] tracepoint: ipi: don't trace IPI on offline CPUs

[PATCH -next] tracepoint: ipi: don't trace IPI on offline CPUs

[Sudeep Holla]

Tracepoints use RCU for protection and they must not be called on
offline CPUS. So make this tracepoint conditional. Otherwise it can
produce the following warning:

===============================
[ INFO: suspicious RCU usage. ]
4.5.0-rc7-next-20160308 #44 Not tainted
-------------------------------
include/trace/events/ipi.h:35 suspicious rcu_dereference_check() usage!

other info that might help us debug this:

RCU used illegally from offline CPU!
rcu_scheduler_active = 1, debug_locks = 1
no locks held by swapper/3/0.

stack backtrace:
CPU: 3 PID: 0 Comm: swapper/3 Not tainted 4.5.0-rc7-next-20160308 #44
Hardware name: ARM Juno development board (r2) (DT)
Call trace:
 dump_backtrace+0x0/0x218
 show_stack+0x24/0x30
 dump_stack+0xb4/0xf0
 lockdep_rcu_suspicious+0xd0/0x118
 smp_cross_call+0x1a8/0x1b0
 arch_send_call_function_single_ipi+0x3c/0x48
 generic_exec_single+0xc0/0x178
 smp_call_function_single+0xec/0x1b8
 cpuhp_report_idle_dead+0x70/0x88
 cpu_startup_entry+0x358/0x3c0
 secondary_start_kernel+0x190/0x1e8

Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Ingo Molnar <mingo@redhat.com>
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 include/trace/events/ipi.h | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Hi Steven,

I observed that in "include/linux/tracepoint.h", we have
#define __DO_TRACE(tp, proto, args, cond, prercu, postrcu)
...
                 if (!cpu_online(raw_smp_processor_id()))
                         return;

                 if (!(cond))
                         return;
...

where !cond check seems reduntant if it's cpu_online check.
So, does this patch handle the warning correctly or is there any better
way ? I did see few traces with same condition, just thought of checking
with you.

Regards,
Sudeep

diff --git a/include/trace/events/ipi.h b/include/trace/events/ipi.h
index 834a7362a610..d8ca2f2e24ac 100644
--- a/include/trace/events/ipi.h
+++ b/include/trace/events/ipi.h
@@ -15,12 +15,14 @@
  * It is necessary for @reason to be a static string declared with
  * __tracepoint_string.
  */
-TRACE_EVENT(ipi_raise,
+TRACE_EVENT_CONDITION(ipi_raise,

 	TP_PROTO(const struct cpumask *mask, const char *reason),

 	TP_ARGS(mask, reason),

+	TP_CONDITION(cpu_online(raw_smp_processor_id())),
+
 	TP_STRUCT__entry(
 		__bitmask(target_cpus, nr_cpumask_bits)
 		__field(const char *, reason)
--
1.9.1

Re: [PATCH -next] tracepoint: ipi: don't trace IPI on offline CPUs

[Steven Rostedt]

On Wed,  9 Mar 2016 12:22:22 +0000
Sudeep Holla <sudeep.holla@arm.com> wrote:


> Hi Steven,
> 
> I observed that in "include/linux/tracepoint.h", we have
> #define __DO_TRACE(tp, proto, args, cond, prercu, postrcu)
> ...
>                  if (!cpu_online(raw_smp_processor_id()))
>                          return;
> 
>                  if (!(cond))
>                          return;
> ...
> 
> where !cond check seems reduntant if it's cpu_online check.
> So, does this patch handle the warning correctly or is there any better
> way ? I did see few traces with same condition, just thought of checking
> with you.
> 

Bah, I forgot that we have lockdep checks for when the event isn't
enabled. Can you try this patch:

-- Steve

diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h
index acfdbf353a0b..be586c632a0c 100644
--- a/include/linux/tracepoint.h
+++ b/include/linux/tracepoint.h
@@ -134,9 +134,6 @@ extern void syscall_unregfunc(void);
 		void *it_func;						\
 		void *__data;						\
 									\
-		if (!cpu_online(raw_smp_processor_id()))		\
-			return;						\
-									\
 		if (!(cond))						\
 			return;						\
 		prercu;							\
@@ -343,15 +340,19 @@ extern void syscall_unregfunc(void);
  * "void *__data, proto" as the callback prototype.
  */
 #define DECLARE_TRACE_NOARGS(name)					\
-		__DECLARE_TRACE(name, void, , 1, void *__data, __data)
+	__DECLARE_TRACE(name, void, ,					\
+			cpu_online(raw_smp_processor_id()),		\
+			void *__data, __data)
 
 #define DECLARE_TRACE(name, proto, args)				\
-		__DECLARE_TRACE(name, PARAMS(proto), PARAMS(args), 1,	\
-				PARAMS(void *__data, proto),		\
-				PARAMS(__data, args))
+	__DECLARE_TRACE(name, PARAMS(proto), PARAMS(args),		\
+			cpu_online(raw_smp_processor_id()),		\
+			PARAMS(void *__data, proto),			\
+			PARAMS(__data, args))
 
 #define DECLARE_TRACE_CONDITION(name, proto, args, cond)		\
-	__DECLARE_TRACE(name, PARAMS(proto), PARAMS(args), PARAMS(cond), \
+	__DECLARE_TRACE(name, PARAMS(proto), PARAMS(args),		\
+			cpu_online(raw_smp_processor_id()) && (PARAMS(cond)), \
 			PARAMS(void *__data, proto),			\
 			PARAMS(__data, args))
 

Re: [PATCH -next] tracepoint: ipi: don't trace IPI on offline CPUs

[Sudeep Holla]

On 09/03/16 16:05, Steven Rostedt wrote:
> On Wed,  9 Mar 2016 12:22:22 +0000
> Sudeep Holla <sudeep.holla@arm.com> wrote:
>
>
>> Hi Steven,
>>
>> I observed that in "include/linux/tracepoint.h", we have
>> #define __DO_TRACE(tp, proto, args, cond, prercu, postrcu)
>> ...
>>                   if (!cpu_online(raw_smp_processor_id()))
>>                           return;
>>
>>                   if (!(cond))
>>                           return;
>> ...
>>
>> where !cond check seems reduntant if it's cpu_online check.
>> So, does this patch handle the warning correctly or is there any better
>> way ? I did see few traces with same condition, just thought of checking
>> with you.
>>
>
> Bah, I forgot that we have lockdep checks for when the event isn't
> enabled.

Yes I was about to ask you the same. I did further digging to check if I
was missing something after seeing your series[1] especially patch 2/12
(tracing: Remove duplicate checks for online CPUs)

> Can you try this patch:

It works. Thanks for the quick fix.

Tested-by: Sudeep Holla <sudeep.holla@arm.com>

--
Regards,
Sudeep

[1] http://www.spinics.net/lists/kernel/msg2208604.html

Re: [PATCH -next] tracepoint: ipi: don't trace IPI on offline CPUs

[Steven Rostedt]

On Wed, 9 Mar 2016 16:40:16 +0000
Sudeep Holla <sudeep.holla@arm.com> wrote:


> > Can you try this patch:  
> 
> It works. Thanks for the quick fix.
> 
> Tested-by: Sudeep Holla <sudeep.holla@arm.com>

Thanks, maybe I can get this into 4.5 quickly. If not, it will have to
go in via the merge window. Either case, it's still getting a stable
tag on it.

-- Steve