Re: [RFC][PATCH 01/10] tracing: Add TRACE_DEFINE_ENUM() macro to map enums to their values

[Namhyung Kim <namhyung@kernel.org>]

Hi Steve,

On Fri, Mar 27, 2015 at 05:37:05PM -0400, Steven Rostedt wrote:
> From: "Steven Rostedt (Red Hat)" <rostedt@goodmis.org>
> 
> Several tracepoints use the helper functions __print_symbolic() or
> __print_flags() and pass in enums that do the mapping between the
> binary data stored and the value to print. This works well for reading
> the ASCII trace files, but when the data is read via userspace tools
> such as perf and trace-cmd, the conversion of the binary value to a
> human string format is lost if an enum is used, as userspace does not
> have access to what the ENUM is.
> 
> For example, the tracepoint trace_tlb_flush() has:
> 
>  __print_symbolic(REC->reason,
>     { TLB_FLUSH_ON_TASK_SWITCH, "flush on task switch" },
>     { TLB_REMOTE_SHOOTDOWN, "remote shootdown" },
>     { TLB_LOCAL_SHOOTDOWN, "local shootdown" },
>     { TLB_LOCAL_MM_SHOOTDOWN, "local mm shootdown" })
> 
> Which maps the enum values to the strings they represent. But perf and
> trace-cmd do no know what value TLB_LOCAL_MM_SHOOTDOWN is, and would
> not be able to map it.
> 
> With TRACE_DEFINE_ENUM(), developers can place these in the event header
> files and ftrace will export the value of the enum via the file:
> 
>  tracing/enum_map
> 
> By adding:
> 
>  TRACE_DEFINE_ENUM(TLB_FLUSH_ON_TASK_SWITCH);
>  TRACE_DEFINE_ENUM(TLB_REMOTE_SHOOTDOWN);
>  TRACE_DEFINE_ENUM(TLB_LOCAL_SHOOTDOWN);
>  TRACE_DEFINE_ENUM(TLB_LOCAL_MM_SHOOTDOWN);
> 
>  $ cat /sys/kernel/debug/tracing/enum_map
> TLB_FLUSH_ON_TASK_SWITCH 0
> TLB_REMOTE_SHOOTDOWN 1
> TLB_LOCAL_SHOOTDOWN 2
> TLB_LOCAL_MM_SHOOTDOWN 3
> 
> The above can be easily parsed by userspace and it can be able to
> convert the enums to their values and properly parse the enums within
> the __print_symbolic() and __print_flags() helper functions.
> 
> Cc: Guilherme Cox <cox@computer.org>
> Cc: Tony Luck <tony.luck@gmail.com>
> Cc: Xie XiuQi <xiexiuqi@huawei.com>
> Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
> ---

[SNIP]
> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index 62c6506d663f..53b449c522a7 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
> @@ -123,6 +123,9 @@ enum ftrace_dump_mode ftrace_dump_on_oops;
>  /* When set, tracing will stop when a WARN*() is hit */
>  int __disable_trace_on_warning;
>  
> +/* Map of enums to their values, for "enum_map" file */
> +static struct trace_enum_map *trace_enum_maps;
> +
>  static int tracing_set_tracer(struct trace_array *tr, const char *buf);
>  
>  #define MAX_TRACER_SIZE		100
> @@ -3908,6 +3911,96 @@ static const struct file_operations tracing_saved_cmdlines_size_fops = {
>  	.write		= tracing_saved_cmdlines_size_write,
>  };
>  
> +static void *enum_map_next(struct seq_file *m, void *v, loff_t *pos)
> +{
> +	struct trace_enum_map *ptr = v;
> +
> +	if (!ptr->enum_string)
> +		return NULL;

Do we really need to check NULL string here?  Seems like duplicated..

Thanks,
Namhyung


> +
> +	ptr++;
> +
> +	(*pos)++;
> +
> +	if (!ptr->enum_string)
> +		return NULL;
> +
> +	return ptr;
> +}
> +
> +static void *enum_map_start(struct seq_file *m, loff_t *pos)
> +{
> +	struct trace_enum_map *v;
> +	loff_t l = 0;
> +
> +	v = trace_enum_maps;
> +	while (v && l < *pos) {
> +		v = enum_map_next(m, v, &l);
> +	}
> +
> +	return v;
> +}
> +
> +static void enum_map_stop(struct seq_file *m, void *v)
> +{
> +}
> +
> +static int enum_map_show(struct seq_file *m, void *v)
> +{
> +	struct trace_enum_map *ptr = v;
> +
> +	seq_printf(m, "%s %ld\n", ptr->enum_string, ptr->enum_value);
> +	return 0;
> +}
> +
> +static const struct seq_operations tracing_enum_map_seq_ops = {
> +	.start		= enum_map_start,
> +	.next		= enum_map_next,
> +	.stop		= enum_map_stop,
> +	.show		= enum_map_show,
> +};
> +
> +static int tracing_enum_map_open(struct inode *inode, struct file *filp)
> +{
> +	if (tracing_disabled)
> +		return -ENODEV;
> +
> +	return seq_open(filp, &tracing_enum_map_seq_ops);
> +}
> +
> +static const struct file_operations tracing_enum_map_fops = {
> +	.open		= tracing_enum_map_open,
> +	.read		= seq_read,
> +	.llseek		= seq_lseek,
> +	.release	= seq_release,
> +};
> +
> +static void
> +trace_insert_enum_map(struct trace_enum_map **start, struct trace_enum_map **stop)
> +{
> +	struct trace_enum_map **map;
> +	struct trace_enum_map *map_array;
> +	int len = stop - start;
> +
> +	if (len <= 0)
> +		return;
> +
> +	map_array = kmalloc(sizeof(*map_array) * (len + 1), GFP_KERNEL);
> +	if (!map_array) {
> +		pr_warning("Unable to allocate trace enum mapping\n");
> +		return;
> +	}
> +
> +	trace_enum_maps = map_array;
> +
> +	for (map = start; (unsigned long)map < (unsigned long)stop; map++) {
> +		*map_array = **map;
> +		map_array++;
> +	}
> +	memset(map_array, 0, sizeof(*map_array));
> +}
> +
> +
>  static ssize_t
>  tracing_set_trace_read(struct file *filp, char __user *ubuf,
>  		       size_t cnt, loff_t *ppos)
> @@ -6542,6 +6635,14 @@ struct dentry *tracing_init_dentry(void)
>  	return tr->dir;
>  }
>  
> +extern struct trace_enum_map *__start_ftrace_enum_maps[];
> +extern struct trace_enum_map *__stop_ftrace_enum_maps[];
> +
> +static void __init trace_enum_init(void)
> +{
> +	trace_insert_enum_map(__start_ftrace_enum_maps, __stop_ftrace_enum_maps);
> +}
> +
>  static __init int tracer_init_debugfs(void)
>  {
>  	struct dentry *d_tracer;
> @@ -6566,6 +6667,11 @@ static __init int tracer_init_debugfs(void)
>  	trace_create_file("saved_cmdlines_size", 0644, d_tracer,
>  			  NULL, &tracing_saved_cmdlines_size_fops);
>  
> +	trace_enum_init();
> +
> +	trace_create_file("enum_map", 0444, d_tracer,
> +			  NULL, &tracing_enum_map_fops);
> +
>  #ifdef CONFIG_DYNAMIC_FTRACE
>  	trace_create_file("dyn_ftrace_total_info", 0444, d_tracer,
>  			&ftrace_update_tot_cnt, &tracing_dyn_info_fops);
> @@ -6888,7 +6994,7 @@ void __init trace_init(void)
>  			tracepoint_printk = 0;
>  	}
>  	tracer_alloc_buffers();
> -	trace_event_init();	
> +	trace_event_init();
>  }
>  
>  __init static int clear_boot_tracer(void)
> -- 
> 2.1.4
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/