Re: [PATCH v11 17/28] tracing: Add hist trigger 'execname' modifier

[Tom Zanussi]

On Mon, 2015-11-02 at 23:10 +0900, Namhyung Kim wrote:
> On Thu, Oct 22, 2015 at 01:14:21PM -0500, Tom Zanussi wrote:
> > Allow users to have pid fields displayed as program names in the output
> > by appending '.execname' to field names:
> > 
> >    # echo hist:keys=aaa.execname ... \
> >               [ if filter] > event/trigger
> 
> So in this case, 'aaa' should be 'common_pid', right?
> 

Right.

> Also, it's just for display thus tasks which have different pid but
> same comm name will be saved in different entries, right?  If so, it'd
> be better to note that somewhere as it might confuse users..
> 

Right, good point, will document that.

> 
> > 
> > Signed-off-by: Tom Zanussi <tom.zanussi@xxxxxxxxxxxxxxx>
> > Tested-by: Masami Hiramatsu <masami.hiramatsu.pt@xxxxxxxxxxx>
> > ---
> >  kernel/trace/trace.c             |  3 +-
> >  kernel/trace/trace_events_hist.c | 86 +++++++++++++++++++++++++++++++++++++++-
> >  2 files changed, 87 insertions(+), 2 deletions(-)
> > 
> > diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> > index 4f0968e..a143be0 100644
> > --- a/kernel/trace/trace.c
> > +++ b/kernel/trace/trace.c
> > @@ -3822,7 +3822,8 @@ static const char readme_msg[] =
> >  	"\t    following modifiers to the field name, as applicable:\n\n"
> >  	"\t            .hex        display a number as a hex value\n"
> >  	"\t            .sym        display an address as a symbol\n"
> > -	"\t            .sym-offset display an address as a symbol and offset\n\n"
> > +	"\t            .sym-offset display an address as a symbol and offset\n"
> > +	"\t            .execname   display a common_pid as a program name\n\n"
> >  	"\t    The 'pause' parameter can be used to pause an existing hist\n"
> >  	"\t    trigger or to start a hist trigger but not log any events\n"
> >  	"\t    until told to do so.  'continue' can be used to start or\n"
> > diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
> > index e6e5c6e..656973a 100644
> > --- a/kernel/trace/trace_events_hist.c
> > +++ b/kernel/trace/trace_events_hist.c
> > @@ -75,6 +75,7 @@ enum hist_field_flags {
> >  	HIST_FIELD_HEX		= 8,
> >  	HIST_FIELD_SYM		= 16,
> >  	HIST_FIELD_SYM_OFFSET	= 32,
> > +	HIST_FIELD_EXECNAME	= 64,
> >  };
> >  
> >  struct hist_trigger_attrs {
> > @@ -218,6 +219,78 @@ static struct hist_trigger_attrs *parse_hist_trigger_attrs(char *trigger_str)
> >  	return ERR_PTR(ret);
> >  }
> >  
> > +static inline void save_comm(char *comm, struct task_struct *task)
> > +{
> > +	if (!task->pid) {
> > +		strcpy(comm, "<idle>");
> > +		return;
> > +	}
> > +
> > +	if (WARN_ON_ONCE(task->pid < 0)) {
> > +		strcpy(comm, "<XXX>");
> > +		return;
> > +	}
> > +
> > +	if (task->pid > PID_MAX_DEFAULT) {
> > +		strcpy(comm, "<...>");
> > +		return;
> > +	}
> 
> I guess this comes from the saved_cmdlines code which uses a fixed
> array of PID_MAX_DEFAULT size.  But as it doesn't use the array, we
> can simply copy the task->comm IMHO.
> 
> 
> > +
> > +	memcpy(comm, task->comm, TASK_COMM_LEN);
> > +}
> > +
> > +static void hist_trigger_elt_free(struct tracing_map_elt *elt)
> > +{
> > +	kfree((char *)elt->private_data);
> > +}
> > +
> > +static int hist_trigger_elt_alloc(struct tracing_map_elt *elt)
> > +{
> > +	struct hist_trigger_data *hist_data = elt->map->private_data;
> > +	struct hist_field *key_field;
> > +	unsigned int i;
> > +
> > +	for (i = hist_data->n_vals; i < hist_data->n_fields; i++) {
> 
> I think it'd be better to add a list iterator macro like
> for_each_key_field() or something since it's easy to miss that the key
> fields always follows the value fields IMHO.
> 

Yeah, that's a good idea (sometimes I even forget that myself ;-)

> 
> > +		key_field = hist_data->fields[i];
> > +
> > +		if (key_field->flags & HIST_FIELD_EXECNAME) {
> > +			unsigned int size = TASK_COMM_LEN + 1;
> > +
> > +			elt->private_data = kzalloc(size, GFP_KERNEL);
> > +			if (!elt->private_data)
> > +				return -ENOMEM;
> > +			break;
> > +		}
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static void hist_trigger_elt_copy(struct tracing_map_elt *to,
> > +				  struct tracing_map_elt *from)
> > +{
> > +	char *comm_from = from->private_data;
> > +	char *comm_to = to->private_data;
> > +
> > +	if (comm_from)
> > +		memcpy(comm_to, comm_from, TASK_COMM_LEN + 1);
> > +}
> > +
> > +static void hist_trigger_elt_init(struct tracing_map_elt *elt)
> > +{
> > +	char *comm = elt->private_data;
> > +
> > +	if (comm)
> > +		save_comm(comm, current);
> > +}
> > +
> > +static struct tracing_map_ops hist_trigger_ops = {
> > +	.elt_alloc	= hist_trigger_elt_alloc,
> > +	.elt_copy	= hist_trigger_elt_copy,
> > +	.elt_free	= hist_trigger_elt_free,
> > +	.elt_init	= hist_trigger_elt_init,
> > +};
> 
> How about making it const?
> 

Sure.

> 
> > +
> >  static void destroy_hist_field(struct hist_field *hist_field)
> >  {
> >  	kfree(hist_field);
> > @@ -376,6 +449,9 @@ static int create_key_field(struct hist_trigger_data *hist_data,
> >  			flags |= HIST_FIELD_SYM;
> >  		else if (!strcmp(field_str, "sym-offset"))
> >  			flags |= HIST_FIELD_SYM_OFFSET;
> > +		else if (!strcmp(field_str, "execname") &&
> > +			 !strcmp(field_name, "common_pid"))
> > +			flags |= HIST_FIELD_EXECNAME;
> >  		else {
> >  			ret = -EINVAL;
> >  			goto out;
> > @@ -612,7 +688,7 @@ create_hist_data(unsigned int map_bits,
> >  		goto free;
> >  
> >  	hist_data->map = tracing_map_create(map_bits, hist_data->key_size,
> > -					    NULL, hist_data);
> > +					    &hist_trigger_ops, hist_data);
> 
> Why not using the hist_trigger_ops only if HIST_FIELD_EXECNAME is set
> to one of the key fields actually?
> 

Right, they're only currently needed in that case, so should only be set
in that case, will change that.

Thanks,

Tom

> Thanks,
> Namhyung
> 
> 
> >  	if (IS_ERR(hist_data->map)) {
> >  		ret = PTR_ERR(hist_data->map);
> >  		hist_data->map = NULL;
> > @@ -732,6 +808,12 @@ hist_trigger_entry_print(struct seq_file *m,
> >  			sprint_symbol(str, uval);
> >  			seq_printf(m, "%s: [%llx] %-55s",
> >  				   key_field->field->name, uval, str);
> > +		} else if (key_field->flags & HIST_FIELD_EXECNAME) {
> > +			char *comm = elt->private_data;
> > +
> > +			uval = *(u64 *)(key + key_field->offset);
> > +			seq_printf(m, "%s: %-16s[%10llu]",
> > +				   key_field->field->name, comm, uval);
> >  		} else if (key_field->flags & HIST_FIELD_STRING) {
> >  			seq_printf(m, "%s: %-50s", key_field->field->name,
> >  				   (char *)(key + key_field->offset));
> > @@ -851,6 +933,8 @@ static const char *get_hist_field_flags(struct hist_field *hist_field)
> >  		flags_str = "sym";
> >  	else if (hist_field->flags & HIST_FIELD_SYM_OFFSET)
> >  		flags_str = "sym-offset";
> > +	else if (hist_field->flags & HIST_FIELD_EXECNAME)
> > +		flags_str = "execname";
> >  
> >  	return flags_str;
> >  }
> > -- 
> > 1.9.3
> > 


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