Teach perf_trace_##call() to check hlist_empty(perf_events)

PATCH RESEND In review Oleg Nesterov 4 participants 9 messages Jul 18, 2013 – Jul 26, 2013

Subsystems (2)

Tracing ftrace

lore

Share & tools

lore

Oleg Nesterov

author

2013-07-18 18:35 UTC #0 lore raw

cover Teach perf_trace_##call() to check hlist_empty(perf_events)

Hello.

The patches are the same, I only tried to update the changelogs a bit. I am also quoting my old email below, to explain what this hack tries to do.

Say, "perf record -e sched:sched_switch -p1".

Every task except /sbin/init will do perf_trace_sched_switch() and perf_trace_buf_prepare() + perf_trace_buf_submit for no reason(), it doesn't have a counter.

So it makes sense to add the fast-path check at the start of perf_trace_##call(),

if (hlist_empty(event_call->perf_events))
return;

The problem is, we should not do this if __task != NULL (iow, if DECLARE_EVENT_CLASS() uses __perf_task()), perf_tp_event() has the additional code for this case.

So we should do

if (!__task && hlist_empty(event_call->perf_events))
return;

But __task is changed by "{ assign; }" block right before perf_trace_buf_submit(). Too late for the fast-path check, we already called perf_trace_buf_prepare/fetch_regs.

So. After 2/3 __perf_task() (and __perf_count/addr) is called when ftrace_get_offsets_##call(args) evaluates the arguments, and we can check !__task && hlist_empty() right after that.

Oleg.

Oleg Nesterov

author

2013-07-18 18:36 UTC #1 lore raw

PATCH 1/3 tracing/perf: expand TRACE_EVENT(sched_stat_runtime)

To simplify the review of the next patches:

1. We are going to reimplent __perf_task/counter and embedd them into TP_ARGS(). expand TRACE_EVENT(sched_stat_runtime) into DECLARE_EVENT_CLASS() + DEFINE_EVENT(), this way they can use different TP_ARGS's.

2. Change perf_trace_##call() macro to do perf_fetch_caller_regs() right before perf_trace_buf_prepare().

This way it evaluates TP_ARGS() asap, the next patch explores this fact.

Note: after 87f44bbc perf_trace_buf_prepare() doesn't need "struct pt_regs *regs", perhaps it makes sense to remove this argument. And perhaps we can teach perf_trace_buf_submit() to accept regs == NULL and do fetch_caller_regs(CALLER_ADDR1) in this case.

3. Cosmetic, but the typecast from "void*" buys nothing. It just adds the noise, remove it.

Signed-off-by Oleg Nesterov <[email protected]>

---
 include/trace/events/sched.h |    6 +++++-
 include/trace/ftrace.h       |    7 +++----
 2 files changed, 8 insertions(+), 5 deletions(-)

include/trace/events/sched.h +5 -1

index e5586ca..249c024 100644
@@ -372,7 +372,7 @@ DEFINE_EVENT(sched_stat_template, sched_stat_blocked,
  * Tracepoint for accounting runtime (time the task is executing
  * on a CPU).
  */
-TRACE_EVENT(sched_stat_runtime,
+DECLARE_EVENT_CLASS(sched_stat_runtime,
 
 	TP_PROTO(struct task_struct *tsk, u64 runtime, u64 vruntime),

@@ -401,6 +401,10 @@ TRACE_EVENT(sched_stat_runtime,
 			(unsigned long long)__entry->vruntime)
 );
 
+DEFINE_EVENT(sched_stat_runtime, sched_stat_runtime,
+	     TP_PROTO(struct task_struct *tsk, u64 runtime, u64 vruntime),
+	     TP_ARGS(tsk, runtime, vruntime));
+
 /*
  * Tracepoint for showing priority inheritance modifying a tasks
  * priority.

include/trace/ftrace.h +3 -4

index c162a57..aed594a 100644
@@ -659,15 +659,14 @@ perf_trace_##call(void *__data, proto)					\
 	int __data_size;						\
 	int rctx;							\
 									\
-	perf_fetch_caller_regs(&__regs);				\
-									\
 	__data_size = ftrace_get_offsets_##call(&__data_offsets, args); \
 	__entry_size = ALIGN(__data_size + sizeof(*entry) + sizeof(u32),\
 			     sizeof(u64));				\
 	__entry_size -= sizeof(u32);					\
 									\
-	entry = (struct ftrace_raw_##call *)perf_trace_buf_prepare(	\
-		__entry_size, event_call->event.type, &__regs, &rctx);	\
+	perf_fetch_caller_regs(&__regs);				\
+	entry = perf_trace_buf_prepare(__entry_size,			\
+			event_call->event.type, &__regs, &rctx);	\
 	if (!entry)							\
 		return;							\
 									\

Signature · 1 line

1.5.5.1

Oleg Nesterov

author

2013-07-18 18:36 UTC #2 lore raw

PATCH 2/3 tracing/perf: reimplement TP_perf_assign() logic

In reply to Oleg Nesterov

The next patch tries to avoid the costly perf_trace_buf_* calls when possible but there is a problem. We can only do this if __task == NULL, perf_tp_event(task != NULL) has the additional code for this case.

Unfortunately, TP_perf_assign/__perf_xxx which changes the default values of __count/__task variables for perf_trace_buf_submit() is called "too late", after we already did perf_trace_buf_prepare(), and the optimization above can't work.

So this patch simply embeds __perf_xxx() into TP_ARGS(), this way DECLARE_EVENT_CLASS() can use the result of assignments hidden in "args" right after ftrace_get_offsets_##call() which is mostly trivial. This allows us to have the fast-path "__task != NULL" check at the start, see the next patch.

Signed-off-by Oleg Nesterov <[email protected]>

---
 include/trace/events/sched.h |   16 +++-------------
 include/trace/ftrace.h       |   19 +++++++++++--------
 2 files changed, 14 insertions(+), 21 deletions(-)

include/trace/events/sched.h +3 ~~-13~~

index 249c024..2e7d994 100644
@@ -57,7 +57,7 @@ DECLARE_EVENT_CLASS(sched_wakeup_template,
 
 	TP_PROTO(struct task_struct *p, int success),
 
-	TP_ARGS(p, success),
+	TP_ARGS(__perf_task(p), success),
 
 	TP_STRUCT__entry(
 		__array(	char,	comm,	TASK_COMM_LEN	)

@@ -73,9 +73,6 @@ DECLARE_EVENT_CLASS(sched_wakeup_template,
 		__entry->prio		= p->prio;
 		__entry->success	= success;
 		__entry->target_cpu	= task_cpu(p);
-	)
-	TP_perf_assign(
-		__perf_task(p);
 	),
 
 	TP_printk("comm=%s pid=%d prio=%d success=%d target_cpu=%03d",

@@ -313,7 +310,7 @@ DECLARE_EVENT_CLASS(sched_stat_template,
 
 	TP_PROTO(struct task_struct *tsk, u64 delay),
 
-	TP_ARGS(tsk, delay),
+	TP_ARGS(__perf_task(tsk), __perf_count(delay)),
 
 	TP_STRUCT__entry(
 		__array( char,	comm,	TASK_COMM_LEN	)

@@ -325,10 +322,6 @@ DECLARE_EVENT_CLASS(sched_stat_template,
 		memcpy(__entry->comm, tsk->comm, TASK_COMM_LEN);
 		__entry->pid	= tsk->pid;
 		__entry->delay	= delay;
-	)
-	TP_perf_assign(
-		__perf_count(delay);
-		__perf_task(tsk);
 	),
 
 	TP_printk("comm=%s pid=%d delay=%Lu [ns]",

@@ -376,7 +369,7 @@ DECLARE_EVENT_CLASS(sched_stat_runtime,
 
 	TP_PROTO(struct task_struct *tsk, u64 runtime, u64 vruntime),
 
-	TP_ARGS(tsk, runtime, vruntime),
+	TP_ARGS(tsk, __perf_count(runtime), vruntime),
 
 	TP_STRUCT__entry(
 		__array( char,	comm,	TASK_COMM_LEN	)

@@ -390,9 +383,6 @@ DECLARE_EVENT_CLASS(sched_stat_runtime,
 		__entry->pid		= tsk->pid;
 		__entry->runtime	= runtime;
 		__entry->vruntime	= vruntime;
-	)
-	TP_perf_assign(
-		__perf_count(runtime);
 	),
 
 	TP_printk("comm=%s pid=%d runtime=%Lu [ns] vruntime=%Lu [ns]",

include/trace/ftrace.h +11 -8

index aed594a..8886877 100644
@@ -503,8 +503,14 @@ static inline notrace int ftrace_get_offsets_##call(			\
 #undef TP_fast_assign
 #define TP_fast_assign(args...) args
 
-#undef TP_perf_assign
-#define TP_perf_assign(args...)
+#undef __perf_addr
+#define __perf_addr(a)	(a)
+
+#undef __perf_count
+#define __perf_count(c)	(c)
+
+#undef __perf_task
+#define __perf_task(t)	(t)
 
 #undef DECLARE_EVENT_CLASS
 #define DECLARE_EVENT_CLASS(call, proto, args, tstruct, assign, print)	\

@@ -632,16 +638,13 @@ __attribute__((section("_ftrace_events"))) *__event_##call = &event_##call
 #define __get_str(field) (char *)__get_dynamic_array(field)
 
 #undef __perf_addr
-#define __perf_addr(a) __addr = (a)
+#define __perf_addr(a)	(__addr = (a))
 
 #undef __perf_count
-#define __perf_count(c) __count = (c)
+#define __perf_count(c)	(__count = (c))
 
 #undef __perf_task
-#define __perf_task(t) __task = (t)
-
-#undef TP_perf_assign
-#define TP_perf_assign(args...) args
+#define __perf_task(t)	(__task = (t))
 
 #undef DECLARE_EVENT_CLASS
 #define DECLARE_EVENT_CLASS(call, proto, args, tstruct, assign, print)	\

Signature · 1 line

1.5.5.1

Oleg Nesterov

author

2013-07-18 18:36 UTC #3 lore raw

PATCH 3/3 tracing/perf: Avoid perf_trace_buf_*() in perf_trace_##call() when possible

In reply to Oleg Nesterov

perf_trace_buf_prepare() + perf_trace_buf_submit(task => NULL) make no sense if hlist_empty(head). Change perf_trace_##call() to check ->perf_events beforehand and do nothing if it is empty.

This removes the overhead for tasks without events associated with them. For example, "perf record -e sched:sched_switch -p1" attaches the counter(s) to the single task, but every task in system will do perf_trace_buf_prepare/submit() just to realize that it was not attached to this event.

However, we can only do this if __task == NULL, so we also add the __builtin_constant_p(__task) check.

With this patch "perf bench sched pipe" shows approximately 4% improvement when "perf record -p1" runs in parallel, many thanks to Steven for the testing.

Signed-off-by Oleg Nesterov <[email protected]>

---
 include/trace/ftrace.h |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

include/trace/ftrace.h +6 -1

index 8886877..04455b8 100644
@@ -663,6 +663,12 @@ perf_trace_##call(void *__data, proto)					\
 	int rctx;							\
 									\
 	__data_size = ftrace_get_offsets_##call(&__data_offsets, args); \
+									\
+	head = this_cpu_ptr(event_call->perf_events);			\
+	if (__builtin_constant_p(!__task) && !__task &&			\
+				hlist_empty(head))			\
+		return;							\
+									\
 	__entry_size = ALIGN(__data_size + sizeof(*entry) + sizeof(u32),\
 			     sizeof(u64));				\
 	__entry_size -= sizeof(u32);					\

@@ -677,7 +683,6 @@ perf_trace_##call(void *__data, proto)					\
 									\
 	{ assign; }							\
 									\
-	head = this_cpu_ptr(event_call->perf_events);			\
 	perf_trace_buf_submit(entry, __entry_size, rctx, __addr,	\
 		__count, &__regs, head, __task);			\
 }

Signature · 1 line

1.5.5.1

Ingo Molnar

2013-07-19 07:43 UTC #4 lore raw

In reply to Oleg Nesterov

Quoted 31 lines · Oleg Nesterov

Hello.
The patches are the same, I only tried to update the changelogs a bit. I am also quoting my old email below, to explain what this hack tries to do.
Say, "perf record -e sched:sched_switch -p1".
Every task except /sbin/init will do perf_trace_sched_switch() and perf_trace_buf_prepare() + perf_trace_buf_submit for no reason(), it doesn't have a counter.
So it makes sense to add the fast-path check at the start of perf_trace_##call(),
if (hlist_empty(event_call->perf_events))
return;
The problem is, we should not do this if __task != NULL (iow, if DECLARE_EVENT_CLASS() uses __perf_task()), perf_tp_event() has the additional code for this case.
So we should do
if (!__task && hlist_empty(event_call->perf_events))
return;
But __task is changed by "{ assign; }" block right before perf_trace_buf_submit(). Too late for the fast-path check, we already called perf_trace_buf_prepare/fetch_regs.

Quoted 5 lines

So. After 2/3 __perf_task() (and __perf_count/addr) is called when ftrace_get_offsets_##call(args) evaluates the arguments, and we can check !__task && hlist_empty() right after that.
Oleg.

Nice improvement.

Peter, Steve, any objections?

Thanks,

Ingo

Steven Rostedt

maintainer

2013-07-19 20:27 UTC #5 lore raw

Quoted 37 lines · Ingo Molnar

Quoted 35 lines · Oleg Nesterov
Hello.
The patches are the same, I only tried to update the changelogs a bit. I am also quoting my old email below, to explain what this hack tries to do.
Say, "perf record -e sched:sched_switch -p1".
Every task except /sbin/init will do perf_trace_sched_switch() and perf_trace_buf_prepare() + perf_trace_buf_submit for no reason(), it doesn't have a counter.
So it makes sense to add the fast-path check at the start of perf_trace_##call(),
if (hlist_empty(event_call->perf_events))
return;
The problem is, we should not do this if __task != NULL (iow, if DECLARE_EVENT_CLASS() uses __perf_task()), perf_tp_event() has the additional code for this case.
So we should do
if (!__task && hlist_empty(event_call->perf_events))
return;
But __task is changed by "{ assign; }" block right before perf_trace_buf_submit(). Too late for the fast-path check, we already called perf_trace_buf_prepare/fetch_regs.
So. After 2/3 __perf_task() (and __perf_count/addr) is called when ftrace_get_offsets_##call(args) evaluates the arguments, and we can check !__task && hlist_empty() right after that.

Quoted 6 lines

Quoted 1 line
Oleg.
Nice improvement.
Peter, Steve, any objections?

Yep, agreed.

The whole series...

Reviewed-and-Acked-by Steven Rostedt <[email protected]>

-- Steve

David Ahern

Tested-by

2013-07-19 21:00 UTC #6 lore raw

Quoted 45 lines · Steven Rostedt

Quoted 37 lines · Ingo Molnar
Quoted 35 lines · Oleg Nesterov
Hello.
The patches are the same, I only tried to update the changelogs a bit. I am also quoting my old email below, to explain what this hack tries to do.
Say, "perf record -e sched:sched_switch -p1".
Every task except /sbin/init will do perf_trace_sched_switch() and perf_trace_buf_prepare() + perf_trace_buf_submit for no reason(), it doesn't have a counter.
So it makes sense to add the fast-path check at the start of perf_trace_##call(),
if (hlist_empty(event_call->perf_events))
return;
The problem is, we should not do this if __task != NULL (iow, if DECLARE_EVENT_CLASS() uses __perf_task()), perf_tp_event() has the additional code for this case.
So we should do
if (!__task && hlist_empty(event_call->perf_events))
return;
But __task is changed by "{ assign; }" block right before perf_trace_buf_submit(). Too late for the fast-path check, we already called perf_trace_buf_prepare/fetch_regs.
So. After 2/3 __perf_task() (and __perf_count/addr) is called when ftrace_get_offsets_##call(args) evaluates the arguments, and we can check !__task && hlist_empty() right after that.
Quoted 6 lines
Quoted 1 line
Oleg.
Nice improvement.
Peter, Steve, any objections?

Quoted 6 lines

Yep, agreed.
The whole series...
Reviewed-and-Acked-by Steven Rostedt <[email protected]>

For what it is worth,

Tested-by David Ahern <[email protected]>

Oleg Nesterov

author

2013-07-20 15:46 UTC #7 lore raw

In reply to Steven Rostedt

Quoted 6 lines · Steven Rostedt

Quoted 3 lines · Ingo Molnar
Peter, Steve, any objections?

Quoted 6 lines

Yep, agreed.
The whole series...
Reviewed-and-Acked-by Steven Rostedt <[email protected]>

Thanks!

But, to avoid the confusion, please do not forget that this series textually depends on cd92bf61 "Move the PERF_MAX_TRACE_SIZE check into perf_trace_buf_prepare()" in your tree. So this should be routed via rostedt/linux-trace as well.

Oleg.

Oleg Nesterov

author

2013-07-26 15:25 UTC #8 lore raw

Quoted 13 lines · Oleg Nesterov

Quoted 11 lines · Steven Rostedt
Quoted 3 lines · Ingo Molnar
Peter, Steve, any objections?
Yep, agreed.
The whole series...

Quoted 8 lines

Quoted 1 line
Reviewed-and-Acked-by Steven Rostedt <[email protected]>
Thanks!
But, to avoid the confusion, please do not forget that this series textually depends on cd92bf61 "Move the PERF_MAX_TRACE_SIZE check into perf_trace_buf_prepare()" in your tree. So this should be routed via rostedt/linux-trace as well.

Update. cd92bf61 is already in Linus's tree.

So, Ingo, if you were going to take these patches - please ;)

It seems that everybody agree with this hack. Please tell me if I should resend this series once again or make a small branch for git-pull.

Oleg.