Hi Namhyung,
On Tue, 2017-12-12 at 00:17 +0900, Namhyung Kim wrote:
> Hi Tom,
>
> On Wed, Dec 06, 2017 at 04:38:03PM -0600, 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.zanu...@linux.intel.com>
> > ---
>
> [SNIP]
> > @@ -313,10 +529,150 @@ static struct hist_field *find_var(struct
> > hist_trigger_data *hist_data,
> > return NULL;
> > }
> >
> > +static struct trace_event_file *find_var_file(struct trace_array *tr,
> > + char *system,
> > + char *event_name,
> > + char *var_name)
> > +{
> > + struct hist_trigger_data *var_hist_data;
> > + struct hist_var_data *var_data;
> > + struct trace_event_call *call;
> > + struct trace_event_file *file, *found = NULL;
> > + const char *name;
> > +
> > + list_for_each_entry(var_data, &tr->hist_vars, list) {
> > + var_hist_data = var_data->hist_data;
> > + file = var_hist_data->event_file;
> > + if (file == found)
> > + continue;
> > + call = file->event_call;
> > + name = trace_event_name(call);
> > +
> > + if (!system || !event_name) {
> > + if (find_var(var_hist_data, file, var_name)) {
>
> Is find_var() really needed? I guess find_var_field() could do the
> job with lower overhead..
>
> > + if (found) {
> > + return NULL;
> > + }
> > +
> > + found = file;
> > + }
> > + continue;
> > + }
> > +
> > + if (strcmp(event_name, name) != 0)
> > + continue;
> > + if (strcmp(system, call->class->system) != 0)
> > + continue;
>
> Also it doesn't need to iterate the loop when system and event name is
> given. Please see below
>
>
> > +
> > + found = file;
> > + break;
> > + }
> > +
> > + return found;
> > +}
>
>
> How about this?
>
> find_var_file()
> {
> if (system)
> return find_event_file(tr, system, event);
>
> list_for_each_entry(var_data, &tr->hist_vars, list) {
> var_hist_data = var_data->hist_data;
> file = var_hist_data->event_file;
> if (file == found)
> continue;
>
> if (find_var_field(var_hist_data, var_name)) {
> if (found)
> return NULL;
>
> found = file;
> }
> }
> }
>
>
Nice improvement, have incorporated it, thanks!
Tom
