Re: [PATCH v6 29/37] tracing: Add cpu field for hist triggers

[Tom Zanussi]

Hi Namhyung,

On Thu, 2017-11-23 at 14:25 +0900, Namhyung Kim wrote:
> On Fri, Nov 17, 2017 at 02:33:08PM -0600, Tom Zanussi wrote:
> > A common key to use in a histogram is the cpuid - add a new cpu
> > 'synthetic' field for that purpose.  This field is named cpu rather
> > than $cpu or $common_cpu because 'cpu' already exists as a special
> > filter field and it makes more sense to match that rather than add
> > another name for the same thing.
> > 
> > Signed-off-by: Tom Zanussi <tom.zanussi@xxxxxxxxxxxxxxx>
> > ---
> >  Documentation/trace/histogram.txt | 17 +++++++++++++++++
> >  kernel/trace/trace_events_hist.c  | 28 +++++++++++++++++++++++++++-
> >  2 files changed, 44 insertions(+), 1 deletion(-)
> > 
> > diff --git a/Documentation/trace/histogram.txt b/Documentation/trace/histogram.txt
> > index d1d92ed..cd3ec00 100644
> > --- a/Documentation/trace/histogram.txt
> > +++ b/Documentation/trace/histogram.txt
> > @@ -172,6 +172,23 @@
> >    The examples below provide a more concrete illustration of the
> >    concepts and typical usage patterns discussed above.
> >  
> > +  'special' event fields
> > +  ------------------------
> > +
> > +  There are a number of 'special event fields' available for use as
> > +  keys or values in a hist trigger.  These look like and behave as if
> > +  they were actual event fields, but aren't really part of the event's
> > +  field definition or format file.  They are however available for any
> > +  event, and can be used anywhere an actual event field could be.
> > +  'Special' field names are always prefixed with a '$' character to
> > +  indicate that they're not normal fields (with the exception of
> > +  'cpu', for compatibility with existing filter usage):
> 
> But it also could make a confusion to variables.  How about removing
> '$' character at all?
> 

I think those are good points - it would be more consistent with the
existing cpu, cause less confusion with variable references, and there's
only the $common_timestamp so far.

The downside would be that users might get confused not finding these
fields in the event descriptions, and would basically have to know to
look in the documentation to confirm that these are special fields.

So unless there are any objections, I'll remove this $special syntax for
these.  So $common_timestamp will become just common_timestamp and the
expression syntax will look like this now, for example:

    wakeup_lat=common_timestamp.usecs-$ts0:onmax($wakeup_lat).save(...)

instead of:

    wakeup_lat=$common_timestamp.usecs-$ts0:onmax($wakeup_lat).save(...)


Also, I've gone through all your other v6 comments and agree with those
and have made the suggested changes, so won't comment on the rest of the
individual patches.

Thanks for the thorough reviews!

Tom

> 
> > +
> > +    $common_timestamp      u64 - timestamp (from ring buffer) associated
> > +                                 with the event, in nanoseconds.  May be
> > +				 modified by .usecs to have timestamps
> > +				 interpreted as microseconds.
> > +    cpu                    int - the cpu on which the event occurred.
> >  
> >  6.2 'hist' trigger examples
> >  ---------------------------
> > diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
> > index 121f7ef..afbfa9c 100644
> > --- a/kernel/trace/trace_events_hist.c
> > +++ b/kernel/trace/trace_events_hist.c
> > @@ -227,6 +227,7 @@ enum hist_field_flags {
> >  	HIST_FIELD_FL_VAR		= 1 << 12,
> >  	HIST_FIELD_FL_EXPR		= 1 << 13,
> >  	HIST_FIELD_FL_VAR_REF		= 1 << 14,
> > +	HIST_FIELD_FL_CPU		= 1 << 15,
> >  };
> >  
> >  struct var_defs {
> > @@ -1177,6 +1178,16 @@ static u64 hist_field_timestamp(struct hist_field *hist_field,
> >  	return ts;
> >  }
> >  
> > +static u64 hist_field_cpu(struct hist_field *hist_field,
> > +			  struct tracing_map_elt *elt,
> > +			  struct ring_buffer_event *rbe,
> > +			  void *event)
> > +{
> > +	int cpu = smp_processor_id();
> > +
> > +	return cpu;
> > +}
> > +
> >  static struct hist_field *
> >  check_field_for_var_ref(struct hist_field *hist_field,
> >  			struct hist_trigger_data *var_data,
> > @@ -1622,6 +1633,8 @@ static const char *hist_field_name(struct hist_field *field,
> >  		field_name = hist_field_name(field->operands[0], ++level);
> >  	else if (field->flags & HIST_FIELD_FL_TIMESTAMP)
> >  		field_name = "$common_timestamp";
> > +	else if (field->flags & HIST_FIELD_FL_CPU)
> > +		field_name = "cpu";
> >  	else if (field->flags & HIST_FIELD_FL_EXPR ||
> >  		 field->flags & HIST_FIELD_FL_VAR_REF) {
> >  		if (field->system) {
> > @@ -2125,6 +2138,15 @@ static struct hist_field *create_hist_field(struct hist_trigger_data *hist_data,
> >  		goto out;
> >  	}
> >  
> > +	if (flags & HIST_FIELD_FL_CPU) {
> > +		hist_field->fn = hist_field_cpu;
> > +		hist_field->size = sizeof(int);
> > +		hist_field->type = kstrdup("int", GFP_KERNEL);
> 
> Is it unsigned?
> 
> Thanks,
> Namhyung
> 
> 
> > +		if (!hist_field->type)
> > +			goto free;
> > +		goto out;
> > +	}
> > +
> >  	if (WARN_ON_ONCE(!field))
> >  		goto out;
> >  
> > @@ -2343,7 +2365,9 @@ static struct hist_field *parse_var_ref(struct hist_trigger_data *hist_data,
> >  		hist_data->enable_timestamps = true;
> >  		if (*flags & HIST_FIELD_FL_TIMESTAMP_USECS)
> >  			hist_data->attrs->ts_in_usecs = true;
> > -	} else {
> > +	} else if (strcmp(field_name, "cpu") == 0)
> > +		*flags |= HIST_FIELD_FL_CPU;
> > +	else {
> >  		field = trace_find_event_field(file->event_call, field_name);
> >  		if (!field || !field->size) {
> >  			field = ERR_PTR(-EINVAL);
> > @@ -4612,6 +4636,8 @@ static void hist_field_print(struct seq_file *m, struct hist_field *hist_field)
> >  
> >  	if (hist_field->flags & HIST_FIELD_FL_TIMESTAMP)
> >  		seq_puts(m, "$common_timestamp");
> > +	else if (hist_field->flags & HIST_FIELD_FL_CPU)
> > +		seq_puts(m, "cpu");
> >  	else if (field_name) {
> >  		if (hist_field->flags & HIST_FIELD_FL_VAR_REF)
> >  			seq_putc(m, '$');
> > -- 
> > 1.9.3
> >