Re: [RFC][PATCH 02/10] tracing: Allow for modules to export their trace enums as well

[Namhyung Kim]

On Fri, Mar 27, 2015 at 05:37:06PM -0400, Steven Rostedt wrote:
> From: "Steven Rostedt (Red Hat)" <rostedt@xxxxxxxxxxx>
> 
> Update the infrastructure such that modules that declare TRACE_DEFINE_ENUM()
> will have those enums shown in the enum_map files in the tracing directory.
> 
> Cc: Rusty Russell <rusty@xxxxxxxxxxxxxxx>
> Signed-off-by: Steven Rostedt <rostedt@xxxxxxxxxxx>
> ---
>  include/linux/module.h |   2 +
>  kernel/module.c        |   3 +
>  kernel/trace/trace.c   | 184 ++++++++++++++++++++++++++++++++++++++++++++-----
>  3 files changed, 173 insertions(+), 16 deletions(-)
> 
> diff --git a/include/linux/module.h b/include/linux/module.h
> index 42999fe2dbd0..53dc41dd5c62 100644
> --- a/include/linux/module.h
> +++ b/include/linux/module.h
> @@ -338,6 +338,8 @@ struct module {
>  #ifdef CONFIG_EVENT_TRACING
>  	struct ftrace_event_call **trace_events;
>  	unsigned int num_trace_events;
> +	struct trace_enum_map **trace_enums;
> +	unsigned int num_trace_enums;
>  #endif
>  #ifdef CONFIG_FTRACE_MCOUNT_RECORD
>  	unsigned int num_ftrace_callsites;
> diff --git a/kernel/module.c b/kernel/module.c
> index b3d634ed06c9..d8f8ab271c2b 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -2753,6 +2753,9 @@ static int find_module_sections(struct module *mod, struct load_info *info)
>  	mod->trace_events = section_objs(info, "_ftrace_events",
>  					 sizeof(*mod->trace_events),
>  					 &mod->num_trace_events);
> +	mod->trace_enums = section_objs(info, "_ftrace_enum_map",
> +					sizeof(*mod->trace_enums),
> +					&mod->num_trace_enums);
>  #endif
>  #ifdef CONFIG_TRACING
>  	mod->trace_bprintk_fmt_start = section_objs(info, "__trace_printk_fmt",
> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index 53b449c522a7..97031878eb3d 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
> @@ -124,7 +124,38 @@ enum ftrace_dump_mode ftrace_dump_on_oops;
>  int __disable_trace_on_warning;
>  
>  /* Map of enums to their values, for "enum_map" file */
> -static struct trace_enum_map *trace_enum_maps;
> +struct trace_enum_map_head {
> +	struct module			*mod;
> +	unsigned long			length;
> +};
> +
> +union trace_enum_map_item;
> +
> +struct trace_enum_map_tail {
> +	/*
> +	 * "end" is first and points to NULL as it must be different
> +	 * than "mod" or "enum_string"
> +	 */
> +	const char			*end;	/* points to NULL */
> +	union trace_enum_map_item	*next;
> +};
> +
> +static DEFINE_MUTEX(trace_enum_mutex);
> +
> +/*
> + * The trace_enum_maps are saved in an array whith two extra elements,

s/whith/with/

> + * one at the beginning, and one at the end. The beginning item contains
> + * the count of the saved maps (head.length), and the module they
> + * belong to if not built in (head.mod). The ending item contains a
> + * pointer to the next array of saved enum_map items.
> + */
> +union trace_enum_map_item {
> +	struct trace_enum_map		map;
> +	struct trace_enum_map_head	head;
> +	struct trace_enum_map_tail	tail;
> +};
> +
> +static union trace_enum_map_item *trace_enum_maps;
>  
>  static int tracing_set_tracer(struct trace_array *tr, const char *buf);
>  
> @@ -3911,29 +3942,47 @@ static const struct file_operations tracing_saved_cmdlines_size_fops = {
>  	.write		= tracing_saved_cmdlines_size_write,
>  };
>  
> +static union trace_enum_map_item *
> +update_enum_map(union trace_enum_map_item *ptr)
> +{
> +	if (!ptr->map.enum_string) {
> +		if (ptr->tail.next) {
> +			ptr = ptr->tail.next;
> +			ptr++;

I guess this additional increment is to skip head item.  Maybe it
deserves a comment. :)


> +		} else
> +			return NULL;
> +	}
> +	return ptr;
> +}
> +
>  static void *enum_map_next(struct seq_file *m, void *v, loff_t *pos)
>  {
> -	struct trace_enum_map *ptr = v;
> +	union trace_enum_map_item *ptr = v;
>  
> -	if (!ptr->enum_string)
> +	ptr = update_enum_map(ptr);

Again, is this necessary?


> +	if (!ptr)
>  		return NULL;
>  
>  	ptr++;
>  
>  	(*pos)++;
>  
> -	if (!ptr->enum_string)
> -		return NULL;
> +	ptr = update_enum_map(ptr);
>  
>  	return ptr;
>  }
>  
>  static void *enum_map_start(struct seq_file *m, loff_t *pos)
>  {
> -	struct trace_enum_map *v;
> +	union trace_enum_map_item *v;
>  	loff_t l = 0;
>  
> +	mutex_lock(&trace_enum_mutex);
> +
>  	v = trace_enum_maps;
> +	if (v)
> +		v++;
> +
>  	while (v && l < *pos) {
>  		v = enum_map_next(m, v, &l);
>  	}
> @@ -3943,13 +3992,15 @@ static void *enum_map_start(struct seq_file *m, loff_t *pos)
>  
>  static void enum_map_stop(struct seq_file *m, void *v)
>  {
> +	mutex_unlock(&trace_enum_mutex);
>  }
>  
>  static int enum_map_show(struct seq_file *m, void *v)
>  {
> -	struct trace_enum_map *ptr = v;
> +	union trace_enum_map_item *ptr = v;
> +
> +	seq_printf(m, "%s %ld\n", ptr->map.enum_string, ptr->map.enum_value);
>  
> -	seq_printf(m, "%s %ld\n", ptr->enum_string, ptr->enum_value);
>  	return 0;
>  }
>  
> @@ -3975,29 +4026,56 @@ static const struct file_operations tracing_enum_map_fops = {
>  	.release	= seq_release,
>  };
>  
> -static void
> -trace_insert_enum_map(struct trace_enum_map **start, struct trace_enum_map **stop)
> +static void trace_insert_enum_map(struct module *mod,
> +				  struct trace_enum_map **start, int len)
>  {
> +	struct trace_enum_map **stop;
>  	struct trace_enum_map **map;
> -	struct trace_enum_map *map_array;
> -	int len = stop - start;
> +	union trace_enum_map_item *map_array;
> +	union trace_enum_map_item *ptr;
>  
>  	if (len <= 0)
>  		return;
>  
> -	map_array = kmalloc(sizeof(*map_array) * (len + 1), GFP_KERNEL);
> +	stop = start + len;
> +
> +	/*
> +	 * The trace_enum_maps contains the map plus a head and tail item,
> +	 * where the head holds the module and length of array, and the
> +	 * tail holds a pointer to the next list.
> +	 */
> +	map_array = kmalloc(sizeof(*map_array) * (len + 2), GFP_KERNEL);
>  	if (!map_array) {
>  		pr_warning("Unable to allocate trace enum mapping\n");
>  		return;
>  	}
>  
> -	trace_enum_maps = map_array;
> +	mutex_lock(&trace_enum_mutex);
> +
> +	if (!trace_enum_maps)
> +		trace_enum_maps = map_array;
> +	else {
> +		ptr = trace_enum_maps;
> +		for (;;) {
> +			ptr = ptr + ptr->head.length + 1;

Looks like access to tail item is used frequently.  How about adding a
helper function like enum_maps_tail_item()?

Thanks,
Namhyung

> +			if (!ptr->tail.next)
> +				break;
> +			ptr = ptr->tail.next;
> +
> +		}
> +		ptr->tail.next = map_array;
> +	}
> +	map_array->head.mod = mod;
> +	map_array->head.length = len;
> +	map_array++;
>  
>  	for (map = start; (unsigned long)map < (unsigned long)stop; map++) {
> -		*map_array = **map;
> +		map_array->map = **map;
>  		map_array++;
>  	}
>  	memset(map_array, 0, sizeof(*map_array));
> +
> +	mutex_unlock(&trace_enum_mutex);
>  }
>  
>  
> @@ -6640,9 +6718,79 @@ 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);
> +	int len;
> +
> +	len = __stop_ftrace_enum_maps - __start_ftrace_enum_maps;
> +	trace_insert_enum_map(NULL, __start_ftrace_enum_maps, len);
>  }
>  
> +#ifdef CONFIG_MODULES
> +static void trace_module_add_enums(struct module *mod)
> +{
> +	if (!mod->num_trace_enums)
> +		return;
> +
> +	/*
> +	 * Modules with bad taint do not have events created, do
> +	 * not bother with enums either.
> +	 */
> +	if (trace_module_has_bad_taint(mod))
> +		return;
> +
> +	trace_insert_enum_map(mod, mod->trace_enums, mod->num_trace_enums);
> +}
> +
> +static void trace_module_remove_enums(struct module *mod)
> +{
> +	union trace_enum_map_item *map;
> +	union trace_enum_map_item **last = &trace_enum_maps;
> +
> +	if (!mod->num_trace_enums)
> +		return;
> +
> +	mutex_lock(&trace_enum_mutex);
> +
> +	map = trace_enum_maps;
> +
> +	while (map) {
> +		if (map->head.mod == mod)
> +			break;
> +		map += map->head.length + 1;
> +		last = &map->tail.next;
> +		map = map->tail.next;
> +	}
> +	if (!map)
> +		goto out;
> +
> +	*last = map[map->head.length + 1].tail.next;
> +	kfree(map);
> + out:
> +	mutex_unlock(&trace_enum_mutex);
> +}
> +
> +static int trace_module_notify(struct notifier_block *self,
> +			       unsigned long val, void *data)
> +{
> +	struct module *mod = data;
> +
> +	switch (val) {
> +	case MODULE_STATE_COMING:
> +		trace_module_add_enums(mod);
> +		break;
> +	case MODULE_STATE_GOING:
> +		trace_module_remove_enums(mod);
> +		break;
> +	}
> +
> +	return 0;
> +}
> +
> +static struct notifier_block trace_module_nb = {
> +	.notifier_call = trace_module_notify,
> +	.priority = 0,
> +};
> +#endif
> +
>  static __init int tracer_init_debugfs(void)
>  {
>  	struct dentry *d_tracer;
> @@ -6672,6 +6820,10 @@ static __init int tracer_init_debugfs(void)
>  	trace_create_file("enum_map", 0444, d_tracer,
>  			  NULL, &tracing_enum_map_fops);
>  
> +#ifdef CONFIG_MODULES
> +	register_module_notifier(&trace_module_nb);
> +#endif
> +
>  #ifdef CONFIG_DYNAMIC_FTRACE
>  	trace_create_file("dyn_ftrace_total_info", 0444, d_tracer,
>  			&ftrace_update_tot_cnt, &tracing_dyn_info_fops);
> -- 
> 2.1.4
> 
> 
> --
> 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/
--
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/