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

[KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>]

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?


> @@ -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?


> @@ -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?


> 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.


>  #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