[RFC][PATCH 0/2 v2] tracing: Add conditional to tracepoints

[RFC][PATCH 0/2 v2] tracing: Add conditional to tracepoints

[Steven Rostedt]

This is an RFC that adds the TP_CONDITION() to the TRACE_EVENT()
code.

There are certain cases that a tracepoint only makes sense if
a specific condition is met. But because we do not want to dirty
the fast path (the non tracing case) with if statements that are
there only to avoid tracing, we just pass the condition variables
to the tracepoint, and let the user filter them out if needed.

A perfect example is the tracepoint sched_wakeup. It traces all
calls to try_to_wake_up() even if it fails to wake up. But if we add:

        if (success)
                trace_sched_wakeup(p);

We have that "if (success)" tested for every time we call try_to_wake_up().
Even when tracing is not (or never will be) enabled.

This patch set adds a variant TRACE_EVENT_CONDITIONAL()
(and DEFINE_EVENT_CONDITIONAL()) that has a "cond" argument.
This argument is encapsulated with "TP_CONDITIONAL()" which turns into:

        if (!cond)
                return;

What's new with this version?

This version adds this if statement to __DO_TRACE(). As it is
in a static inline function, the return still applies.

Also, since the shortcut is made before the callbacks, the test
is at the call site. This means that you can have a DECLARE_EVENT_CLASS()
where one of its DEFINE_EVENTS() is normal and the other is a
DEFINE_EVENT_CONDITION().

The following patches are in:

  git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-2.6-trace.git

    branch: rfc/trace-conditional-v2


Steven Rostedt (2):
      tracing: Add TRACE_EVENT_CONDITIONAL()
      tracing: Only trace sched_wakeup if it actually work something up

----
 include/linux/tracepoint.h   |   29 +++++++++++++++++++++++------
 include/trace/define_trace.h |   15 +++++++++++++++
 include/trace/events/sched.h |   16 ++++++++--------
 3 files changed, 46 insertions(+), 14 deletions(-)

[RFC][PATCH 1/2 v2] tracing: Add TRACE_EVENT_CONDITIONAL()

[Steven Rostedt]

[-- Attachment #1: 0001-tracing-Add-TRACE_EVENT_CONDITIONAL.patch --]
[-- Type: text/plain, Size: 6772 bytes --]

From: Steven Rostedt <srostedt@redhat.com>

There are instances in the kernel that we only want to trace
a tracepoint when a certain condition is set. But we do not
want to test for that condition in the core kernel.
If we test for that condition before calling the tracepoin, then
we will be performing that test even when tracing is not enabled.
This is 99.99% of the time.

We currently can just filter out on that condition, but that happens
after we write to the trace buffer. We just wasted time writing to
the ring buffer for an event we never cared about.

This patch adds:

   TRACE_EVENT_CONDITION() and DEFINE_EVENT_CONDITION()

These have a new TP_CONDITION() argument that comes right after
the TP_ARGS().  This condition can use the parameters of TP_ARGS()
in the TRACE_EVENT() to determine if the tracepoint should be traced
or not. The TP_CONDITION() will be placed in a if (cond) trace;

For example, for the tracepoint sched_wakeup, it is useless to
trace a wakeup event where the caller never actually wakes
anything up (where success == 0). So adding:

	TP_CONDITION(success),

which uses the "success" parameter of the wakeup tracepoint
will have it only trace when we have successfully woken up a
task.

Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Arjan van de Ven <arjan@infradead.org>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 include/linux/tracepoint.h   |   29 +++++++++++++++++++++++------
 include/trace/define_trace.h |   15 +++++++++++++++
 2 files changed, 38 insertions(+), 6 deletions(-)

diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h
index 5a6074f..d3e4f87 100644
--- a/include/linux/tracepoint.h
+++ b/include/linux/tracepoint.h
@@ -106,6 +106,7 @@ static inline void tracepoint_update_probe_range(struct tracepoint *begin,
 
 #define TP_PROTO(args...)	args
 #define TP_ARGS(args...)	args
+#define TP_CONDITION(args...)	args
 
 #ifdef CONFIG_TRACEPOINTS
 
@@ -119,12 +120,14 @@ static inline void tracepoint_update_probe_range(struct tracepoint *begin,
  * as "(void *, void)". The DECLARE_TRACE_NOARGS() will pass in just
  * "void *data", where as the DECLARE_TRACE() will pass in "void *data, proto".
  */
-#define __DO_TRACE(tp, proto, args)					\
+#define __DO_TRACE(tp, proto, args, cond)				\
 	do {								\
 		struct tracepoint_func *it_func_ptr;			\
 		void *it_func;						\
 		void *__data;						\
 									\
+		if (!(cond))						\
+			return;						\
 		rcu_read_lock_sched_notrace();				\
 		it_func_ptr = rcu_dereference_sched((tp)->funcs);	\
 		if (it_func_ptr) {					\
@@ -142,7 +145,7 @@ static inline void tracepoint_update_probe_range(struct tracepoint *begin,
  * not add unwanted padding between the beginning of the section and the
  * structure. Force alignment to the same alignment as the section start.
  */
-#define __DECLARE_TRACE(name, proto, args, data_proto, data_args)	\
+#define __DECLARE_TRACE(name, proto, args, cond, data_proto, data_args)	\
 	extern struct tracepoint __tracepoint_##name;			\
 	static inline void trace_##name(proto)				\
 	{								\
@@ -151,7 +154,8 @@ static inline void tracepoint_update_probe_range(struct tracepoint *begin,
 do_trace:								\
 			__DO_TRACE(&__tracepoint_##name,		\
 				TP_PROTO(data_proto),			\
-				TP_ARGS(data_args));			\
+				TP_ARGS(data_args),			\
+				TP_CONDITION(cond));			\
 	}								\
 	static inline int						\
 	register_trace_##name(void (*probe)(data_proto), void *data)	\
@@ -186,7 +190,7 @@ do_trace:								\
 	EXPORT_SYMBOL(__tracepoint_##name)
 
 #else /* !CONFIG_TRACEPOINTS */
-#define __DECLARE_TRACE(name, proto, args, data_proto, data_args)	\
+#define __DECLARE_TRACE(name, proto, args, cond, data_proto, data_args)	\
 	static inline void trace_##name(proto)				\
 	{ }								\
 	static inline int						\
@@ -227,13 +231,18 @@ do_trace:								\
  * "void *__data, proto" as the callback prototype.
  */
 #define DECLARE_TRACE_NOARGS(name)					\
-		__DECLARE_TRACE(name, void, , void *__data, __data)
+		__DECLARE_TRACE(name, void, , 1, void *__data, __data)
 
 #define DECLARE_TRACE(name, proto, args)				\
-		__DECLARE_TRACE(name, PARAMS(proto), PARAMS(args),	\
+		__DECLARE_TRACE(name, PARAMS(proto), PARAMS(args), 1,	\
 				PARAMS(void *__data, proto),		\
 				PARAMS(__data, args))
 
+#define DECLARE_TRACE_CONDITION(name, proto, args, cond)		\
+	__DECLARE_TRACE(name, PARAMS(proto), PARAMS(args), PARAMS(cond), \
+			PARAMS(void *__data, proto),			\
+			PARAMS(__data, args))
+
 #define TRACE_EVENT_FLAGS(event, flag)
 
 #endif /* DECLARE_TRACE */
@@ -349,12 +358,20 @@ do_trace:								\
 	DECLARE_TRACE(name, PARAMS(proto), PARAMS(args))
 #define DEFINE_EVENT_PRINT(template, name, proto, args, print)	\
 	DECLARE_TRACE(name, PARAMS(proto), PARAMS(args))
+#define DEFINE_EVENT_CONDITION(template, name, proto,		\
+			       args, cond)			\
+	DECLARE_TRACE_CONDITION(name, PARAMS(proto),		\
+				PARAMS(args), PARAMS(cond))
 
 #define TRACE_EVENT(name, proto, args, struct, assign, print)	\
 	DECLARE_TRACE(name, PARAMS(proto), PARAMS(args))
 #define TRACE_EVENT_FN(name, proto, args, struct,		\
 		assign, print, reg, unreg)			\
 	DECLARE_TRACE(name, PARAMS(proto), PARAMS(args))
+#define TRACE_EVENT_CONDITION(name, proto, args, cond,		\
+			      struct, assign, print)		\
+	DECLARE_TRACE_CONDITION(name, PARAMS(proto),		\
+				PARAMS(args), PARAMS(cond))
 
 #define TRACE_EVENT_FLAGS(event, flag)
 
diff --git a/include/trace/define_trace.h b/include/trace/define_trace.h
index 1dfab54..b0b4eb2 100644
--- a/include/trace/define_trace.h
+++ b/include/trace/define_trace.h
@@ -26,6 +26,15 @@
 #define TRACE_EVENT(name, proto, args, tstruct, assign, print)	\
 	DEFINE_TRACE(name)
 
+#undef TRACE_EVENT_CONDITION
+#define TRACE_EVENT_CONDITION(name, proto, args, cond, tstruct, assign, print) \
+	TRACE_EVENT(name,						\
+		PARAMS(proto),						\
+		PARAMS(args),						\
+		PARAMS(tstruct),					\
+		PARAMS(assign),						\
+		PARAMS(print))
+
 #undef TRACE_EVENT_FN
 #define TRACE_EVENT_FN(name, proto, args, tstruct,		\
 		assign, print, reg, unreg)			\
@@ -39,6 +48,10 @@
 #define DEFINE_EVENT_PRINT(template, name, proto, args, print)	\
 	DEFINE_TRACE(name)
 
+#undef DEFINE_EVENT_CONDITION
+#define DEFINE_EVENT_CONDITION(template, name, proto, args, cond) \
+	DEFINE_EVENT(template, name, PARAMS(proto), PARAMS(args))
+
 #undef DECLARE_TRACE
 #define DECLARE_TRACE(name, proto, args)	\
 	DEFINE_TRACE(name)
@@ -75,9 +88,11 @@
 
 #undef TRACE_EVENT
 #undef TRACE_EVENT_FN
+#undef TRACE_EVENT_CONDITION
 #undef DECLARE_EVENT_CLASS
 #undef DEFINE_EVENT
 #undef DEFINE_EVENT_PRINT
+#undef DEFINE_EVENT_CONDITION
 #undef TRACE_HEADER_MULTI_READ
 #undef DECLARE_TRACE
 
-- 
1.7.2.3

[RFC][PATCH 2/2 v2] tracing: Only trace sched_wakeup if it actually work something up

[Steven Rostedt]

[-- Attachment #1: 0002-tracing-Only-trace-sched_wakeup-if-it-actually-work-.patch --]
[-- Type: text/plain, Size: 2327 bytes --]

From: Steven Rostedt <srostedt@redhat.com>

Currently the tracepoint sched_wakeup traces the wakeup event even
if the wakeup failed to wake anything up. This is quite stupid
but it happens because we did not want to add a conditional
to the core kernel code that would just slow down the wakeup events.

This patch changes the wakeup tracepoints to use the

 DEFINE_EVENT_CONDITIONAL()

to test the "success" parameter and will only trace the event if
the wakeup was successfull.

The success field in the tracepoint is removed since it is no
longer needed.

Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 include/trace/events/sched.h |   16 ++++++++--------
 1 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/include/trace/events/sched.h b/include/trace/events/sched.h
index f633478..29e6030 100644
--- a/include/trace/events/sched.h
+++ b/include/trace/events/sched.h
@@ -62,7 +62,6 @@ DECLARE_EVENT_CLASS(sched_wakeup_template,
 		__array(	char,	comm,	TASK_COMM_LEN	)
 		__field(	pid_t,	pid			)
 		__field(	int,	prio			)
-		__field(	int,	success			)
 		__field(	int,	target_cpu		)
 	),
 
@@ -70,25 +69,26 @@ DECLARE_EVENT_CLASS(sched_wakeup_template,
 		memcpy(__entry->comm, p->comm, TASK_COMM_LEN);
 		__entry->pid		= p->pid;
 		__entry->prio		= p->prio;
-		__entry->success	= success;
 		__entry->target_cpu	= task_cpu(p);
 	),
 
-	TP_printk("comm=%s pid=%d prio=%d success=%d target_cpu=%03d",
+	TP_printk("comm=%s pid=%d prio=%d target_cpu=%03d",
 		  __entry->comm, __entry->pid, __entry->prio,
-		  __entry->success, __entry->target_cpu)
+		  __entry->target_cpu)
 );
 
-DEFINE_EVENT(sched_wakeup_template, sched_wakeup,
+DEFINE_EVENT_CONDITION(sched_wakeup_template, sched_wakeup,
 	     TP_PROTO(struct task_struct *p, int success),
-	     TP_ARGS(p, success));
+	     TP_ARGS(p, success),
+	     TP_CONDITION(success));
 
 /*
  * Tracepoint for waking up a new task:
  */
-DEFINE_EVENT(sched_wakeup_template, sched_wakeup_new,
+DEFINE_EVENT_CONDITION(sched_wakeup_template, sched_wakeup_new,
 	     TP_PROTO(struct task_struct *p, int success),
-	     TP_ARGS(p, success));
+	     TP_ARGS(p, success),
+	     TP_CONDITION(success));
 
 #ifdef CREATE_TRACE_POINTS
 static inline long __trace_sched_switch_state(struct task_struct *p)
-- 
1.7.2.3

Re: [RFC][PATCH 1/2 v2] tracing: Add TRACE_EVENT_CONDITIONAL()

[Mathieu Desnoyers]

* Steven Rostedt (rostedt@goodmis.org) wrote:
[...]
> -#define __DO_TRACE(tp, proto, args)					\
> +#define __DO_TRACE(tp, proto, args, cond)				\
>  	do {								\
>  		struct tracepoint_func *it_func_ptr;			\
>  		void *it_func;						\
>  		void *__data;						\
>  									\
> +		if (!(cond))						\

One small documentation-related detail: my guess is that you are leaving
"cond" without likely/unlikely builtin expect purposefully so that we
can write, in TP_CONDITION:

  TP_CONDITION(unlikely(someparam)),

when we expect the condition to be usually false (and likely() for the
reverse). Maybe it could be worth documenting that expressions like the
following are valid :

  TP_CONDITION((likely(param1) && unlikely(param2)) || likely(param3))

It's fair to assume that kernel developers know this already, but given
we plan to re-use TRACE_EVENT() for the user-space tracer soon enough,
documenting this kind of use-case now can save us the trouble in the
future.

Other than that,

Acked-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>

Thanks!

Mathieu

-- 
Mathieu Desnoyers
Operating System Efficiency R&D Consultant
EfficiOS Inc.
http://www.efficios.com

Re: [RFC][PATCH 1/2 v2] tracing: Add TRACE_EVENT_CONDITIONAL()

[Steven Rostedt]

On Thu, 2010-12-02 at 23:54 -0500, Mathieu Desnoyers wrote:
> * Steven Rostedt (rostedt@goodmis.org) wrote:
> [...]
> > -#define __DO_TRACE(tp, proto, args)					\
> > +#define __DO_TRACE(tp, proto, args, cond)				\
> >  	do {								\
> >  		struct tracepoint_func *it_func_ptr;			\
> >  		void *it_func;						\
> >  		void *__data;						\
> >  									\
> > +		if (!(cond))						\
> 
> One small documentation-related detail: my guess is that you are leaving
> "cond" without likely/unlikely builtin expect purposefully so that we
> can write, in TP_CONDITION:
> 
>   TP_CONDITION(unlikely(someparam)),

I actually think this is an abuse of "unlikely".

> 
> when we expect the condition to be usually false (and likely() for the
> reverse). Maybe it could be worth documenting that expressions like the
> following are valid :
> 
>   TP_CONDITION((likely(param1) && unlikely(param2)) || likely(param3))
> 
> It's fair to assume that kernel developers know this already, but given
> we plan to re-use TRACE_EVENT() for the user-space tracer soon enough,
> documenting this kind of use-case now can save us the trouble in the
> future.

I would frown on someone using unlikely here. But a TRACE_EVENT()
belongs to the maintainer, not me.

> 
> Other than that,
> 
> Acked-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>

Thanks!

-- Steve

Re: [RFC][PATCH 0/2 v2] tracing: Add conditional to tracepoints

[Frederic Weisbecker]

On Thu, Dec 02, 2010 at 11:03:58PM -0500, Steven Rostedt wrote:
> This is an RFC that adds the TP_CONDITION() to the TRACE_EVENT()
> code.
> 
> There are certain cases that a tracepoint only makes sense if
> a specific condition is met. But because we do not want to dirty
> the fast path (the non tracing case) with if statements that are
> there only to avoid tracing, we just pass the condition variables
> to the tracepoint, and let the user filter them out if needed.
> 
> A perfect example is the tracepoint sched_wakeup. It traces all
> calls to try_to_wake_up() even if it fails to wake up. But if we add:
> 
>         if (success)
>                 trace_sched_wakeup(p);
> 
> We have that "if (success)" tested for every time we call try_to_wake_up().
> Even when tracing is not (or never will be) enabled.
> 
> This patch set adds a variant TRACE_EVENT_CONDITIONAL()
> (and DEFINE_EVENT_CONDITIONAL()) that has a "cond" argument.
> This argument is encapsulated with "TP_CONDITIONAL()" which turns into:
> 
>         if (!cond)
>                 return;
> 
> What's new with this version?
> 
> This version adds this if statement to __DO_TRACE(). As it is
> in a static inline function, the return still applies.
> 
> Also, since the shortcut is made before the callbacks, the test
> is at the call site. This means that you can have a DECLARE_EVENT_CLASS()
> where one of its DEFINE_EVENTS() is normal and the other is a
> DEFINE_EVENT_CONDITION().
> 
> The following patches are in:
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-2.6-trace.git
> 
>     branch: rfc/trace-conditional-v2
> 
> 
> Steven Rostedt (2):
>       tracing: Add TRACE_EVENT_CONDITIONAL()
>       tracing: Only trace sched_wakeup if it actually work something up
> 
> ----
>  include/linux/tracepoint.h   |   29 +++++++++++++++++++++++------
>  include/trace/define_trace.h |   15 +++++++++++++++
>  include/trace/events/sched.h |   16 ++++++++--------
>  3 files changed, 46 insertions(+), 14 deletions(-)


Looks good

Acked-by: Frederic Weisbecker <fweisbec@gmail.com>

I suspect we'll need to fix some scripts/subperf commands in perf. Those that do:

	if (success)

tests after reading a trace from a wakeup event.

Re: [RFC][PATCH 0/2 v2] tracing: Add conditional to tracepoints

[Steven Rostedt]

On Fri, 2010-12-03 at 15:47 +0100, Frederic Weisbecker wrote:

> Looks good
> 
> Acked-by: Frederic Weisbecker <fweisbec@gmail.com>
> 
> I suspect we'll need to fix some scripts/subperf commands in perf. Those that do:
> 
> 	if (success)
> 
> tests after reading a trace from a wakeup event.

I  guess the question is, is that "success" part of the ABI or not?

-- Steve

Re: [RFC][PATCH 0/2 v2] tracing: Add conditional to tracepoints

[Frederic Weisbecker]

On Fri, Dec 03, 2010 at 09:53:12AM -0500, Steven Rostedt wrote:
> On Fri, 2010-12-03 at 15:47 +0100, Frederic Weisbecker wrote:
> 
> > Looks good
> > 
> > Acked-by: Frederic Weisbecker <fweisbec@gmail.com>
> > 
> > I suspect we'll need to fix some scripts/subperf commands in perf. Those that do:
> > 
> > 	if (success)
> > 
> > tests after reading a trace from a wakeup event.
> 
> I  guess the question is, is that "success" part of the ABI or not?


The fact is: if old tools read new sched wakeup traces, the tools won't work.

Now the debate is always the same.

Re: [RFC][PATCH 1/2 v2] tracing: Add TRACE_EVENT_CONDITIONAL()

[Mathieu Desnoyers]

* Steven Rostedt (rostedt@goodmis.org) wrote:
> On Thu, 2010-12-02 at 23:54 -0500, Mathieu Desnoyers wrote:
[...]
> > 
> > One small documentation-related detail: my guess is that you are leaving
> > "cond" without likely/unlikely builtin expect purposefully so that we
> > can write, in TP_CONDITION:
> > 
> >   TP_CONDITION(unlikely(someparam)),
> 
> I actually think this is an abuse of "unlikely".

Why are you considering this an abuse ?

Mathieu

-- 
Mathieu Desnoyers
Operating System Efficiency R&D Consultant
EfficiOS Inc.
http://www.efficios.com

Re: [RFC][PATCH 1/2 v2] tracing: Add TRACE_EVENT_CONDITIONAL()

[Steven Rostedt]

On Fri, 2010-12-03 at 10:27 -0500, Mathieu Desnoyers wrote:

> > >   TP_CONDITION(unlikely(someparam)),
> > 
> > I actually think this is an abuse of "unlikely".
> 
> Why are you considering this an abuse ?

Because it is overused. I would rather get rid of most unlikely()'s
because they are mostly meaningless. Just run the unlikely profiler, and
you will see a large number of them are just plain incorrect.

Adding them here probably doesn't do any good. The only reason for this
TP_CONDITION() is to ignore those cases that it just does not make sense
to trace. Like a wake up tracepoint that does not wake anything up. No
need for "unlikely" or "likely", by trying to do that, you will most
likely get it wrong.

	unlikely(use_likely_correctly)


-- Steve

Re: [RFC][PATCH 1/2 v2] tracing: Add TRACE_EVENT_CONDITIONAL()

[Mathieu Desnoyers]

* Steven Rostedt (rostedt@goodmis.org) wrote:
> On Fri, 2010-12-03 at 10:27 -0500, Mathieu Desnoyers wrote:
> 
> > > >   TP_CONDITION(unlikely(someparam)),
> > > 
> > > I actually think this is an abuse of "unlikely".
> > 
> > Why are you considering this an abuse ?
> 
> Because it is overused. I would rather get rid of most unlikely()'s
> because they are mostly meaningless. Just run the unlikely profiler, and
> you will see a large number of them are just plain incorrect.
> 
> Adding them here probably doesn't do any good. The only reason for this
> TP_CONDITION() is to ignore those cases that it just does not make sense
> to trace. Like a wake up tracepoint that does not wake anything up. No
> need for "unlikely" or "likely", by trying to do that, you will most
> likely get it wrong.
> 
> 	unlikely(use_likely_correctly)

Ah OK. You are afraid that people will misuse it, not saying that it would be
technically incorrect. Fair enough. It sounds like a good enough reason for not
documenting this use-case.

Thanks,

Mathieu

-- 
Mathieu Desnoyers
Operating System Efficiency R&D Consultant
EfficiOS Inc.
http://www.efficios.com

Re: [RFC][PATCH 2/2 v2] tracing: Only trace sched_wakeup if it actually work something up

[Peter Zijlstra]

On Thu, 2010-12-02 at 23:04 -0500, Steven Rostedt wrote:
> Currently the tracepoint sched_wakeup traces the wakeup event even
> if the wakeup failed to wake anything up. This is quite stupid
> but it happens because we did not want to add a conditional
> to the core kernel code that would just slow down the wakeup events.
> 
Its actually quite useful at times, so no, I don't much like this.

Re: [RFC][PATCH 2/2 v2] tracing: Only trace sched_wakeup if it actually work something up

[Steven Rostedt]

On Wed, 2010-12-08 at 13:12 +0100, Peter Zijlstra wrote:
> On Thu, 2010-12-02 at 23:04 -0500, Steven Rostedt wrote:
> > Currently the tracepoint sched_wakeup traces the wakeup event even
> > if the wakeup failed to wake anything up. This is quite stupid
> > but it happens because we did not want to add a conditional
> > to the core kernel code that would just slow down the wakeup events.
> > 
> Its actually quite useful at times, so no, I don't much like this.

OK, I'll remove this patch. This is why I kept it at the end of the
queue.

But I'm curious. Linus ripped into me about the uselessness of this
event when success was not true. What use do you see of this tracepoint
when a wake up does not happen? I, personally, just filter these out.

-- Steve

Re: [RFC][PATCH 2/2 v2] tracing: Only trace sched_wakeup if it actually work something up

[Peter Zijlstra]

On Wed, 2010-12-08 at 09:06 -0500, Steven Rostedt wrote:
> On Wed, 2010-12-08 at 13:12 +0100, Peter Zijlstra wrote:
> > On Thu, 2010-12-02 at 23:04 -0500, Steven Rostedt wrote:
> > > Currently the tracepoint sched_wakeup traces the wakeup event even
> > > if the wakeup failed to wake anything up. This is quite stupid
> > > but it happens because we did not want to add a conditional
> > > to the core kernel code that would just slow down the wakeup events.
> > > 
> > Its actually quite useful at times, so no, I don't much like this.
> 
> OK, I'll remove this patch. This is why I kept it at the end of the
> queue.
> 
> But I'm curious. Linus ripped into me about the uselessness of this
> event when success was not true. What use do you see of this tracepoint
> when a wake up does not happen? I, personally, just filter these out.

Oh, some wake-up race like scenarios. Either when there's multiple
wakeups or things where the task was preempted and didn't need the full
wakeup.

There's also a sleep race, where the wakeup came before we went to
sleep, etc..

The success=0 wakeup is lots faster than the full wakeup (for obvious
reasons), so when analyzing performance related things, one of the
things I look at is the ratio of wakeup success. If the faster case got
lots of unsuccessful wakeups you know there's probably some (wakeup)
preemption like thing gone wrong with the slower case.

In all those cases its nice to see _that_ a wakeup was attempted and
when it was, even if it was unsuccessful.