Re: [PATCH v3 12/33] tracing: Add variable support to hist triggers

[Tom Zanussi]

Hi Namhyung,

On Mon, 2017-10-09 at 12:27 +0900, Namhyung Kim wrote:
> Hi Tom,
> 
> On Fri, Sep 22, 2017 at 02:59:52PM -0500, Tom Zanussi wrote:
> > Add support for saving the value of a current event's event field by
> > assigning it to a variable that can be read by a subsequent event.
> > 
> > The basic syntax for saving a variable is to simply prefix a unique
> > variable name not corresponding to any keyword along with an '=' sign
> > to any event field.
> > 
> > Both keys and values can be saved and retrieved in this way:
> > 
> >     # echo 'hist:keys=next_pid:vals=$ts0:ts0=common_timestamp ...
> 
> The 'common_timestamp' needs a '$' prefix, right?
> 

Yeah, thanks for pointing this out - the variables got fixed, but missed
the common_timestamp bits.

> >     # echo 'hist:timer_pid=common_pid:key=$timer_pid ...'
> > 
> > If a variable isn't a key variable or prefixed with 'vals=', the
> > associated event field will be saved in a variable but won't be summed
> > as a value:
> > 
> >     # echo 'hist:keys=next_pid:ts1=common_timestamp:...
> 
> Ditto.
> 
> > 
> > Multiple variables can be assigned at the same time:
> > 
> >     # echo 'hist:keys=pid:vals=$ts0,$b,field2:ts0=common_timestamp,b=field1 ...
> > 
> > Multiple (or single) variables can also be assigned at the same time
> > using separate assignments:
> > 
> >     # echo 'hist:keys=pid:vals=$ts0:ts0=common_timestamp:b=field1:c=field2 ...
> 
> Same here..
> 
> So you separated the variable definition and it should not be a part
> of key or value field, right?
> 

Right.

> > 
> > Variables set as above can be used by being referenced from another
> > event, as described in a subsequent patch.
> 
> That means, a variable should be unique (in a tracing instance at least).
> 

Not exactly - I thought it would be too restrictive to force users to
come up with names that would be unique across an instance, so made them
unique to events.  So users can use the same variable name for triggers
on multiple events, and in cases where there are multiple events with
the same name, use the fully-qualified subsys.event_name.variable_name
name to disambiguate them.  If there is only one event with the given
name, thus no ambiguity, specifying the bare variable name suffices.
Again the intent is to make the common case as simple as possible, but
allow for more complex cases without requiring naming gymnastics from
the user.

> 
> > 
> > Signed-off-by: Tom Zanussi <tom.zanussi@xxxxxxxxxxxxxxx>
> > Signed-off-by: Baohong Liu <baohong.liu@xxxxxxxxx>
> > ---
> >  kernel/trace/trace_events_hist.c | 374 ++++++++++++++++++++++++++++++++++-----
> >  1 file changed, 334 insertions(+), 40 deletions(-)
> > 
> > diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
> > index c2abe41..0d99548 100644
> > --- a/kernel/trace/trace_events_hist.c
> > +++ b/kernel/trace/trace_events_hist.c
> > @@ -30,6 +30,13 @@ typedef u64 (*hist_field_fn_t) (struct hist_field *field, void *event,
> >  				struct ring_buffer_event *rbe);
> >  
> >  #define HIST_FIELD_OPERANDS_MAX	2
> > +#define HIST_FIELDS_MAX		(TRACING_MAP_FIELDS_MAX + TRACING_MAP_VARS_MAX)
> > +
> > +struct hist_var {
> > +	char				*name;
> > +	struct hist_trigger_data	*hist_data;
> > +	unsigned int			idx;
> > +};
> >  
> >  struct hist_field {
> >  	struct ftrace_event_field	*field;
> > @@ -40,6 +47,7 @@ struct hist_field {
> >  	unsigned int                    is_signed;
> >  	struct hist_field		*operands[HIST_FIELD_OPERANDS_MAX];
> >  	struct hist_trigger_data	*hist_data;
> > +	struct hist_var			var;
> >  };
> >  
> >  static u64 hist_field_none(struct hist_field *field, void *event,
> > @@ -138,6 +146,14 @@ enum hist_field_flags {
> >  	HIST_FIELD_FL_LOG2		= 1 << 9,
> >  	HIST_FIELD_FL_TIMESTAMP		= 1 << 10,
> >  	HIST_FIELD_FL_TIMESTAMP_USECS	= 1 << 11,
> > +	HIST_FIELD_FL_VAR		= 1 << 12,
> > +	HIST_FIELD_FL_VAR_ONLY		= 1 << 13,
> 
> And then I'm not sure what _VAR_ONLY flag is for.  IIUC it's used to
> identify pure variable definition from a definition in value fields.
> But it's not possible anymore, no?
> 

Right, the recent change to separate them makes the VAR_ONLY redundant,
will remove it.

> 
> > +};
> > +
> > +struct var_defs {
> > +	unsigned int	n_vars;
> > +	char		*name[TRACING_MAP_VARS_MAX];
> > +	char		*expr[TRACING_MAP_VARS_MAX];
> >  };
> >
> 
> [SNIP]
> 
> > +static int create_var_field(struct hist_trigger_data *hist_data,
> > +			    unsigned int val_idx,
> > +			    struct trace_event_file *file,
> > +			    char *var_name, char *expr_str)
> > +{
> > +	unsigned long flags = 0;
> > +
> > +	if (WARN_ON(val_idx >= TRACING_MAP_VALS_MAX + TRACING_MAP_VARS_MAX))
> > +		return -EINVAL;
> > +
> > +	if (find_var(file, var_name) && !hist_data->remove) {
> 
> Is this for the uniqueness check?  But I think it only checks
> variables in the current event (file).
> 

Right, hopefully this make sense now in the context discussed above.

> 
> > +		return -EINVAL;
> > +	}
> > +
> > +	flags |= HIST_FIELD_FL_VAR;
> > +	hist_data->n_vars++;
> > +	if (hist_data->n_vars > TRACING_MAP_VARS_MAX) {
> > +		return -EINVAL;
> > +	}
> > +
> > +	flags |= HIST_FIELD_FL_VAR_ONLY;
> > +
> > +	return __create_val_field(hist_data, val_idx, file, var_name, expr_str, flags);
> > +}
> > +
> 
> [SNIP]
> 
> > +static int parse_var_defs(struct hist_trigger_data *hist_data)
> > +{
> > +	char *s, *str, *var_name, *field_str;
> > +	unsigned int i, j, n_vars = 0;
> > +	int ret = 0;
> > +
> > +	for (i = 0; i < hist_data->attrs->n_assignments; i++) {
> > +		str = hist_data->attrs->assignment_str[i];
> > +		for (j = 0; j < TRACING_MAP_VARS_MAX; j++) {
> > +			field_str = strsep(&str, ",");
> > +			if (!field_str)
> > +				break;
> > +
> > +			var_name = strsep(&field_str, "=");
> > +			if (!var_name || !field_str) {
> > +				ret = -EINVAL;
> > +				goto free;
> > +			}
> > +
> > +			s = kstrdup(var_name, GFP_KERNEL);
> > +			if (!s) {
> > +				ret = -ENOMEM;
> > +				goto free;
> > +			}
> > +			hist_data->attrs->var_defs.name[n_vars] = s;
> > +
> > +			s = kstrdup(field_str, GFP_KERNEL);
> > +			if (!s) {
> > +				ret = -ENOMEM;
> > +				goto free;
> 
> It seems that it might leak the copy of var_name here..
> 

Unless I'm missing something, the free_var_defs() should handle this
case too...

Thanks,

Tom

> Thanks,
> Namhyung
> 
> 
> > +			}
> > +			hist_data->attrs->var_defs.expr[n_vars++] = s;
> > +
> > +			hist_data->attrs->var_defs.n_vars = n_vars;
> > +
> > +			if (n_vars == TRACING_MAP_VARS_MAX)
> > +				goto free;
> > +		}
> > +	}
> > +
> > +	return ret;
> > + free:
> > +	free_var_defs(hist_data);
> > +
> > +	return ret;
> > +}
> > +