Messages in this thread Patch in this message | | | Date | Tue, 10 Mar 2009 09:55:05 -0400 (EDT) | From | Steven Rostedt <> | Subject | Re: [PATCH 4/7] tracing: new format for specialized trace points |
| |
On Tue, 10 Mar 2009, KOSAKI Motohiro wrote:
> Hi > > I have few comment. > > > > diff --git a/kernel/trace/trace_event_types.h b/kernel/trace/trace_event_types.h > > index d94179a..5cca4c9 100644 > > --- a/kernel/trace/trace_event_types.h > > +++ b/kernel/trace/trace_event_types.h > > @@ -106,9 +106,10 @@ TRACE_EVENT_FORMAT(print, TRACE_PRINT, print_entry, ignore, > > TRACE_STRUCT( > > TRACE_FIELD(unsigned long, ip, ip) > > TRACE_FIELD(unsigned int, depth, depth) > > + TRACE_FIELD(char *, fmt, fmt) > > TRACE_FIELD_ZERO_CHAR(buf) > > ), > > - TP_RAW_FMT("%08lx (%d) %s") > > + TP_RAW_FMT("%08lx (%d) fmt:%p %s") > > ); > > I'm not sure about what mean /sys/kernel/debug/tracing/events/ftrace/print/format.. > > FWIW, I think the internal representation of trace_printk() is > > struct print_entry { > struct trace_entry ent; > unsigned long ip; > int depth; > const char *fmt; > u32 buf[]; > }; > > then, buf doesn't gurantee NUL-termination, right?
Right but the record has a size value, and a buf[] means that that record will take up the rest of the record.
The trace_printk is special since it is now a binary format. I'm going to export the pointers and fmts next. That way a binary reader can see what the fmt maps to.
> > > > @@ -665,21 +534,6 @@ event_create_dir(struct ftrace_event_call *call, struct dentry *d_events) > > "'%s/enable' entry\n", call->name); > > } > > > > - /* Only let type be writable, if we can change it */ > > - entry = debugfs_create_file("type", > > - call->raw_init ? 0644 : 0444, > > - call->dir, call, > > - &ftrace_type_fops); > > - if (!entry) > > - pr_warning("Could not create debugfs " > > - "'%s/type' entry\n", call->name); > > - > > - entry = debugfs_create_file("available_types", 0444, call->dir, call, > > - &ftrace_available_types_fops); > > - if (!entry) > > - pr_warning("Could not create debugfs " > > - "'%s/available_types' entry\n", call->name); > > - > > /* A trace may not want to export its format */ > > if (!call->show_format) > > return 0; > > this code mean, the output type of event use debug/tracing/trace_options now, right?
No. The above code is the fact that the new format no longer gives an option for a trace point to be a "pretty print printf" format or a "raw" format. The new TRACE_EVENT macro is powerful enough to not need the two options. You can now have your cake and eat it too.
> > > > @@ -704,13 +558,6 @@ static __init int event_trace_init(void) > > if (!d_tracer) > > return 0; > > > > - entry = debugfs_create_file("available_events", 0444, d_tracer, > > - (void *)&show_event_seq_ops, > > - &ftrace_avail_fops); > > - if (!entry) > > - pr_warning("Could not create debugfs " > > - "'available_events' entry\n"); > > - > > entry = debugfs_create_file("set_event", 0644, d_tracer, > > (void *)&show_set_event_seq_ops, > > &ftrace_set_event_fops); > > Why do you remove available_events file?
Arg! Good catch! That was me getting too happy with the knife. I'll add that back.
> > > > diff --git a/kernel/trace/trace_events_stage_2.h b/kernel/trace/trace_events_stage_2.h > > index 8e2e0f5..d91bf4c 100644 > > --- a/kernel/trace/trace_events_stage_2.h > > +++ b/kernel/trace/trace_events_stage_2.h > > @@ -32,23 +32,14 @@ > > * in binary. > > */ > > > > -#undef TRACE_STRUCT > > -#define TRACE_STRUCT(args...) args > > +#undef __entry > > +#define __entry field > > > > -#undef TRACE_FIELD > > -#define TRACE_FIELD(type, item, assign) \ > > - field->item, > > +#undef TP_printk > > +#define TP_printk(fmt, args...) fmt "\n", args > > > > -#undef TRACE_FIELD_SPECIAL > > -#define TRACE_FIELD_SPECIAL(type_item, item, cmd) \ > > - field->item, > > - > > - > > -#undef TP_RAW_FMT > > -#define TP_RAW_FMT(args...) args > > - > > -#undef TRACE_EVENT_FORMAT > > -#define TRACE_EVENT_FORMAT(call, proto, args, fmt, tstruct, tpfmt) \ > > +#undef TRACE_EVENT > > +#define TRACE_EVENT(call, proto, args, tstruct, print, assign) \ > > enum print_line_t \ > > ftrace_raw_output_##call(struct trace_iterator *iter, int flags) \ > > { \ > > @@ -66,14 +57,76 @@ ftrace_raw_output_##call(struct trace_iterator *iter, int flags) \ > > \ > > field = (typeof(field))entry; \ > > \ > > - ret = trace_seq_printf(s, tpfmt "%s", tstruct "\n"); \ > > + ret = trace_seq_printf(s, print); \ > > if (!ret) \ > > return TRACE_TYPE_PARTIAL_LINE; \ > > \ > > return TRACE_TYPE_HANDLED; \ > > } > > the header comment need the same chnge too.
I'll change it once more, but if I find that I need to maintain two versions, the comment and the header, I will simply cut out the entire comment. I only did the comment because I thought it would be easier than reading the macros. Not sure it is worth the trouble.
Thanks for the review,
-- Steve
> > > > #include <trace/trace_event_types.h> > > diff --git a/kernel/trace/trace_events_stage_3.h b/kernel/trace/trace_events_stage_3.h > > index 41b82b9..8e398d8 100644 > > --- a/kernel/trace/trace_events_stage_3.h > > +++ b/kernel/trace/trace_events_stage_3.h > > @@ -144,27 +144,15 @@ __attribute__((section("_ftrace_events"))) event_##call = { \ > > .unregfunc = ftrace_unreg_event_##call, \ > > } > > > > -#undef TRACE_FIELD > > -#define TRACE_FIELD(type, item, assign)\ > > - entry->item = assign; > > - > > -#undef TRACE_FIELD > > -#define TRACE_FIELD(type, item, assign)\ > > - entry->item = assign; > > - > > -#undef TP_CMD > > -#define TP_CMD(cmd...) cmd > > - > > -#undef TRACE_ENTRY > > -#define TRACE_ENTRY entry > > +#undef TRACE_EVENT_FORMAT > > +#define TRACE_EVENT_FORMAT(call, proto, args, fmt, tstruct, raw) \ > > + TRACE_FORMAT(call, PARAMS(proto), PARAMS(args), PARAMS(fmt)) > > > > -#undef TRACE_FIELD_SPECIAL > > -#define TRACE_FIELD_SPECIAL(type_item, item, cmd) \ > > - cmd; > > +#undef __entry > > +#define __entry entry > > > > -#undef TRACE_EVENT_FORMAT > > -#define TRACE_EVENT_FORMAT(call, proto, args, fmt, tstruct, tpfmt) \ > > -_TRACE_FORMAT(call, PARAMS(proto), PARAMS(args), PARAMS(fmt)) \ > > +#undef TRACE_EVENT > > +#define TRACE_EVENT(call, proto, args, tstruct, print, assign) \ > > \ > > static struct ftrace_event_call event_##call; \ > > \ > > @@ -185,7 +173,7 @@ static void ftrace_raw_event_##call(proto) \ > > return; \ > > entry = ring_buffer_event_data(event); \ > > \ > > - tstruct; \ > > + assign; \ > > \ > > trace_current_buffer_unlock_commit(event, irq_flags, pc); \ > > } \ > > ditto > > > @@ -226,10 +214,8 @@ __attribute__((__aligned__(4))) \ > > __attribute__((section("_ftrace_events"))) event_##call = { \ > > .name = #call, \ > > .system = __stringify(TRACE_SYSTEM), \ > > - .regfunc = ftrace_reg_event_##call, \ > > - .unregfunc = ftrace_unreg_event_##call, \ > > .raw_init = ftrace_raw_init_event_##call, \ > > - .raw_reg = ftrace_raw_reg_event_##call, \ > > - .raw_unreg = ftrace_raw_unreg_event_##call, \ > > + .regfunc = ftrace_raw_reg_event_##call, \ > > + .unregfunc = ftrace_raw_unreg_event_##call, \ > > .show_format = ftrace_format_##call, \ > > } > > ditto > > > >
| |