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

[Steven Rostedt <rostedt@goodmis.org>]

On Mon, 30 Mar 2015 11:41:00 +0900
Namhyung Kim <namhyung@kernel.org> wrote:


> > + * The trace_enum_maps are saved in an array whith two extra elements,
> 
> s/whith/with/

Heh, thanks for catching that.

> 
> > + * 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. :)

Yep it does. I'll add one.

> 
> 
> > +		} 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?

Not really, but I'm doing it to be paranoid. If for some reason 'ptr'
is at the tail end (which it should never happen), then the increment
below will cause an invalid pointer dereference.

I should at least add a comment here saying I'm being paranoid. It's
far from a fast path so I don't really care about slowing things down
here to be extra safe.

> 
> 
> > +	if (!ptr)
> >  		return NULL;
> >  
> >  	ptr++;
> >  
> >  	(*pos)++;
> >  
> > -	if (!ptr->enum_string)
> > -		return NULL;
> > +	ptr = update_enum_map(ptr);
> >  
> >  	return ptr;
> >  }
> >  



> > +	/*
> > +	 * 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()?

I'll look into it. Thanks for the review.

-- Steve