Re: [PATCH 4/7] tracing: new format for specialized trace points

[Steven Rostedt]

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
> 
> 
> 
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/