Re: [PATCH v3 19/33] tracing: Add variable reference handling to hist triggers

[Tom Zanussi]

Hi Namhyung,

On Mon, 2017-10-09 at 21:46 +0900, Namhyung Kim wrote:
> On Fri, Sep 22, 2017 at 02:59:59PM -0500, Tom Zanussi wrote:
> > Add the necessary infrastructure to allow the variables defined on one
> > event to be referenced in another.  This allows variables set by a
> > previous event to be referenced and used in expressions combining the
> > variable values saved by that previous event and the event fields of
> > the current event.  For example, here's how a latency can be
> > calculated and saved into yet another variable named 'wakeup_lat':
> > 
> >     # echo 'hist:keys=pid,prio:ts0=common_timestamp ...
> >     # echo 'hist:keys=next_pid:wakeup_lat=common_timestamp-$ts0 ...
> > 
> > In the first event, the event's timetamp is saved into the variable
> > ts0.  In the next line, ts0 is subtracted from the second event's
> > timestamp to produce the latency.
> > 
> > Further users of variable references will be described in subsequent
> > patches, such as for instance how the 'wakeup_lat' variable above can
> > be displayed in a latency histogram.
> > 
> > Signed-off-by: Tom Zanussi <tom.zanussi@xxxxxxxxxxxxxxx>
> > ---
> >  kernel/trace/trace.c                |   2 +
> >  kernel/trace/trace.h                |   3 +
> >  kernel/trace/trace_events_hist.c    | 613 ++++++++++++++++++++++++++++++++----
> >  kernel/trace/trace_events_trigger.c |   6 +
> >  4 files changed, 568 insertions(+), 56 deletions(-)
> > 
> > diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> > index 19ffbe1..5eb313a 100644
> > --- a/kernel/trace/trace.c
> > +++ b/kernel/trace/trace.c
> > @@ -7753,6 +7753,7 @@ static int instance_mkdir(const char *name)
> >  
> >  	INIT_LIST_HEAD(&tr->systems);
> >  	INIT_LIST_HEAD(&tr->events);
> > +	INIT_LIST_HEAD(&tr->hist_vars);
> >  
> >  	if (allocate_trace_buffers(tr, trace_buf_size) < 0)
> >  		goto out_free_tr;
> > @@ -8500,6 +8501,7 @@ __init static int tracer_alloc_buffers(void)
> >  
> >  	INIT_LIST_HEAD(&global_trace.systems);
> >  	INIT_LIST_HEAD(&global_trace.events);
> > +	INIT_LIST_HEAD(&global_trace.hist_vars);
> >  	list_add(&global_trace.list, &ftrace_trace_arrays);
> >  
> >  	apply_trace_boot_options();
> > diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
> > index 02bfd5c..7b78762 100644
> > --- a/kernel/trace/trace.h
> > +++ b/kernel/trace/trace.h
> > @@ -273,6 +273,7 @@ struct trace_array {
> >  	int			function_enabled;
> >  #endif
> >  	int			time_stamp_abs_ref;
> > +	struct list_head	hist_vars;
> >  };
> >  
> >  enum {
> > @@ -1547,6 +1548,8 @@ extern int save_named_trigger(const char *name,
> >  extern void unpause_named_trigger(struct event_trigger_data *data);
> >  extern void set_named_trigger_data(struct event_trigger_data *data,
> >  				   struct event_trigger_data *named_data);
> > +extern struct event_trigger_data *
> > +get_named_trigger_data(struct event_trigger_data *data);
> >  extern int register_event_command(struct event_command *cmd);
> >  extern int unregister_event_command(struct event_command *cmd);
> >  extern int register_trigger_hist_enable_disable_cmds(void);
> > diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
> > index 02170df..6dcef22 100644
> > --- a/kernel/trace/trace_events_hist.c
> > +++ b/kernel/trace/trace_events_hist.c
> > @@ -60,6 +60,9 @@ struct hist_field {
> >  	struct hist_var			var;
> >  	enum field_op_id		operator;
> >  	char				*name;
> > +	unsigned int			var_idx;
> > +	unsigned int			var_ref_idx;
> > +	bool                            read_once;
> >  };
> >  
> >  static u64 hist_field_none(struct hist_field *field,
> > @@ -215,6 +218,7 @@ enum hist_field_flags {
> >  	HIST_FIELD_FL_VAR		= 1 << 12,
> >  	HIST_FIELD_FL_VAR_ONLY		= 1 << 13,
> >  	HIST_FIELD_FL_EXPR		= 1 << 14,
> > +	HIST_FIELD_FL_VAR_REF		= 1 << 15,
> >  };
> >  
> >  struct var_defs {
> > @@ -255,6 +259,8 @@ struct hist_trigger_data {
> >  	struct tracing_map		*map;
> >  	bool				enable_timestamps;
> >  	bool				remove;
> > +	struct hist_field               *var_refs[TRACING_MAP_VARS_MAX];
> > +	unsigned int			n_var_refs;
> >  };
> >  
> >  static u64 hist_field_timestamp(struct hist_field *hist_field,
> > @@ -273,10 +279,344 @@ static u64 hist_field_timestamp(struct hist_field *hist_field,
> >  	return ts;
> >  }
> >  
> > +struct hist_var_data {
> > +	struct list_head list;
> > +	struct hist_trigger_data *hist_data;
> > +};
> > +
> > +static struct hist_field *check_var_ref(struct hist_field *hist_field,
> > +					struct hist_trigger_data *var_data,
> > +					unsigned int var_idx)
> > +{
> > +	struct hist_field *found = NULL;
> > +
> > +	if (hist_field && hist_field->flags & HIST_FIELD_FL_VAR_REF) {
> > +		if (hist_field->var.idx == var_idx &&
> > +		    hist_field->var.hist_data == var_data) {
> > +			found = hist_field;
> > +		}
> > +	}
> > +
> > +	return found;
> > +}
> > +
> > +static struct hist_field *find_var_ref(struct hist_trigger_data *hist_data,
> > +				       struct hist_trigger_data *var_data,
> > +				       unsigned int var_idx)
> > +{
> > +	struct hist_field *hist_field, *found = NULL;
> > +	unsigned int i, j;
> > +
> > +	for_each_hist_field(i, hist_data) {
> > +		hist_field = hist_data->fields[i];
> > +		found = check_var_ref(hist_field, var_data, var_idx);
> > +		if (found)
> > +			return found;
> > +
> > +		for (j = 0; j < HIST_FIELD_OPERANDS_MAX; j++) {
> > +			struct hist_field *operand;
> > +
> > +			operand = hist_field->operands[j];
> > +			found = check_var_ref(operand, var_data, var_idx);
> > +			if (found)
> > +				return found;
> > +		}
> 
> Hmm.. IIUC expression can be nested but it seems to miss var-ref at
> level 2 - something like "a + b + $c", no?
> 
> 

Yes, thanks for pointing these out, will fix, thanks.

Tom