Teach perf_trace_##call() to check hlist_empty(perf_events)
v2 cover Teach perf_trace_##call() to check hlist_empty(perf_events)
Quoted 4 lines · Steven Rostedt
Quoted 2 lines
Sorry... should I resend once again ?
Sure, why not. It's only 3 patches :-)
OK. Added "v2" to avoid the confusion.
The only change is
- Reviewed-and-Acked-by: Steven Rostedt <[email protected]>
+ Reviewed-by: Steven Rostedt <[email protected]>
+ Acked-by: Steven Rostedt <[email protected]>
Quoted 1 line
I wonder if the Reviewed-by assumes the Acked-by?
I dunno. But since you gave me both, I'd better preserve them all.
Oleg.
include/trace/events/sched.h | 22 ++++++++-------------- include/trace/ftrace.h | 33 ++++++++++++++++++++------------- 2 files changed, 28 insertions(+), 27 deletions(-)
PATCH v2 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.
--- 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
@@ -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
@@ -663,15 +663,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
tip-bot for Oleg Nesterov
merge bot[tip:perf/core] tracing/perf: Expand TRACE_EVENT( sched_stat_runtime)
tracing/perf: Expand TRACE_EVENT(sched_stat_runtime)
Commit-ID: 36009d07b79d2a168d6037947357d96e5d8cebe7 Gitweb: http://git.kernel.org/tip/36009d07b79d2a168d6037947357d96e5d8cebe7 Author: Oleg Nesterov <[email protected]> AuthorDate: Tue, 6 Aug 2013 18:08:41 +0200 Committer: Steven Rostedt <[email protected]> CommitDate: Tue, 13 Aug 2013 21:05:12 -0400
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.
--- 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
@@ -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
@@ -663,15 +663,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; \ \
PATCH v2 2/3 tracing/perf: Reimplement TP_perf_assign() logic
In reply to Oleg NesterovThe 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.
--- 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
@@ -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
@@ -507,8 +507,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) \
@@ -636,16 +642,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
tip-bot for Oleg Nesterov
merge bot[tip:perf/core] tracing/perf: Reimplement TP_perf_assign() logic
tracing/perf: Reimplement TP_perf_assign() logic
Commit-ID: 12473965c38a527a0c6f7a38d23edce60957f873 Gitweb: http://git.kernel.org/tip/12473965c38a527a0c6f7a38d23edce60957f873 Author: Oleg Nesterov <[email protected]> AuthorDate: Tue, 6 Aug 2013 18:08:44 +0200 Committer: Steven Rostedt <[email protected]> CommitDate: Tue, 13 Aug 2013 21:05:57 -0400
tracing/perf: Reimplement TP_perf_assign() logic
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.
--- 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
@@ -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
@@ -507,8 +507,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) \
@@ -636,16 +642,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) \
PATCH v2 3/3 tracing/perf: Avoid perf_trace_buf_*() in perf_trace_##call() when possible
In reply to Oleg Nesterovperf_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.
--- include/trace/ftrace.h | 7 ++++++- 1 files changed, 6 insertions(+), 1 deletions(-)
include/trace/ftrace.h +6 -1
@@ -667,6 +667,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); \
@@ -681,7 +687,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
tip-bot for Oleg Nesterov
merge bot[tip:perf/core] tracing/perf: Avoid perf_trace_buf_*() in perf_trace_##call() when possible
tracing/perf: Avoid perf_trace_buf_*() in perf_trace_##call() when possible
Commit-ID: d027e6a9c83440bf1ca9e5503539d58d8e0914f1 Gitweb: http://git.kernel.org/tip/d027e6a9c83440bf1ca9e5503539d58d8e0914f1 Author: Oleg Nesterov <[email protected]> AuthorDate: Tue, 6 Aug 2013 18:08:47 +0200 Committer: Steven Rostedt <[email protected]> CommitDate: Tue, 13 Aug 2013 21:06:30 -0400
tracing/perf: Avoid perf_trace_buf_*() in perf_trace_##call() when possible
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.
--- include/trace/ftrace.h | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)
include/trace/ftrace.h +6 -1
@@ -667,6 +667,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); \
@@ -681,7 +687,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); \ }
Quoted 10 lines · Oleg Nesterov
Quoted 4 lines · Steven Rostedt
Quoted 2 lines
Sorry... should I resend once again ?
Sure, why not. It's only 3 patches :-)
OK. Added "v2" to avoid the confusion.
The only change is
Quoted 7 lines
- Reviewed-and-Acked-by: Steven Rostedt <[email protected]>
+ Reviewed-by: Steven Rostedt <[email protected]>
+ Acked-by: Steven Rostedt <[email protected]>Quoted 1 line
I wonder if the Reviewed-by assumes the Acked-by?
I dunno. But since you gave me both, I'd better preserve them all.
So I suppose the down-side to putting them in TP_ARGS() is that you cannot use arbitrary expressions for them anymore; like:
TP_ARGS(foo);
TP_perf_assign(
__perf_task(foo->ponies);
__perf_count(foo->horses);
),Not that we actually did something like that, but I imagine it might've been useful.. A well, lets not worry too much about that and go with this. We'll get creative again if we ever need something like that.
Thanks!
Quoted 11 lines · Peter Zijlstra
So I suppose the down-side to putting them in
TP_ARGS()is that you cannot use arbitrary expressions for them anymore; like:TP_ARGS(foo); TP_perf_assign( __perf_task(foo->ponies); __perf_count(foo->horses); ),
Quoted 2 lines
Not that we actually did something like that, but I imagine it might've been useful..
Yes. This is of course less generic. And more confusing, I agree.
Quoted 4 lines
A well, lets not worry too much about that and go with this. We'll get creative again if we ever need something like that.
Acked-by Peter Zijlstra <[email protected]>
Thanks ;)
BTW. Can't we kill __perf_addr() and the corresponding argument in perf_trace_buf_submit/perf_tp_event ?
Or do you think it can have a new user?
Oleg.
Quoted 4 lines · Oleg Nesterov
BTW. Can't we kill
__perf_addr()and the corresponding argument in perf_trace_buf_submit/perf_tp_event ?Or do you think it can have a new user?
Oh, right.. I suppose that was to enable tracepoints for fault (like) events. Clearly those never happened.
Yeah, I think we can kill that, another thing to reconsider once we grow someone wanting it I suppose.