Messages in this thread | | | Date | Mon, 25 May 2009 10:27:45 +0200 | From | Frederic Weisbecker <> | Subject | Re: [PATCH v2 1/2] ftrace: Add task_comm support for trace_event |
| |
On Mon, May 25, 2009 at 11:54:27AM +0800, Zhaolei wrote: > * From: "Frederic Weisbecker" <fweisbec@gmail.com> > > Hi, > > > > > > On Fri, May 22, 2009 at 06:05:37PM +0800, Zhaolei wrote: > >> If we use trace_event alone(without function trace, .etc), > >> it can't output enough task command information. > >> > >> Before patch: > >> # echo 1 > debugfs/tracing/events/sched/sched_switch/enable > >> # cat debugfs/tracing/trace > >> # tracer: nop > >> # > >> # TASK-PID CPU# TIMESTAMP FUNCTION > >> # | | | | | > >> <...>-2289 [000] 526276.724790: sched_switch: task bash:2289 [120] ==> sshd:2287 [120] > >> <...>-2287 [000] 526276.725231: sched_switch: task sshd:2287 [120] ==> bash:2289 [120] > >> <...>-2289 [000] 526276.725452: sched_switch: task bash:2289 [120] ==> sshd:2287 [120] > >> <...>-2287 [000] 526276.727181: sched_switch: task sshd:2287 [120] ==> swapper:0 [140] > >> <idle>-0 [000] 526277.032734: sched_switch: task swapper:0 [140] ==> events/0:5 [115] > >> <...>-5 [000] 526277.032782: sched_switch: task events/0:5 [115] ==> swapper:0 [140] > >> ... > >> > >> After patch: > >> # tracer: nop > >> # > >> # TASK-PID CPU# TIMESTAMP FUNCTION > >> # | | | | | > >> bash-2269 [000] 527347.989229: sched_switch: task bash:2269 [120] ==> sshd:2267 [120] > >> sshd-2267 [000] 527347.990960: sched_switch: task sshd:2267 [120] ==> bash:2269 [120] > >> bash-2269 [000] 527347.991143: sched_switch: task bash:2269 [120] ==> sshd:2267 [120] > >> sshd-2267 [000] 527347.992959: sched_switch: task sshd:2267 [120] ==> swapper:0 [140] > >> <idle>-0 [000] 527348.531989: sched_switch: task swapper:0 [140] ==> events/0:5 [115] > >> events/0-5 [000] 527348.532115: sched_switch: task events/0:5 [115] ==> swapper:0 [140] > >> ... > >> > >> Signed-off-by: Zhao Lei <zhaolei@cn.fujitsu.com> > > > > > > Thanks! > > This is fine but I think it can be factorized. > > > > You could call start_cmdline_record() from > > > > ftrace_raw_reg_event_##call() > > > > and the stop in > > > > ftrace_raw_unreg_event_##call() > > > > No? > > Hello, Frederic > > Thanks for your advice. > > Actually, I considered to put start_cmdline_record() into ftrace_raw_reg_event_##call(), > but finally I selected to put it into tracing_start_cmdline_record(). > > IMHO, we have following reason: > 1: It can make source more readable. > Read function is more easy than read macro. > 2: These two way have same performance. > 3: Put start_cmdline_record() into ftrace_event_enable_disable() will reduce > binary file size than ftrace_raw_reg_event_##call(). > > So I think put start_cmdline_record() into ftrace_event_enable_disable() maybe better. > > What is your opinion? > > Thanks > Zhaolei
Yeah, there are pros and cons. Putting it at the lower level will increase image size but make easier the maintainance...
I don't know which one is better :) I guess both are valuable.
Thanks.
| |