linux-kernel - Re: [PATCH v7 22/37] tracing: Add variable reference handling to hist triggers

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1513014790.4353.7.camel@tzanussi-mobl.amr.corp.intel.com>
Date:   Mon, 11 Dec 2017 11:53:10 -0600
From:   Tom Zanussi <tom.zanussi@...ux.intel.com>
To:     Namhyung Kim <namhyung@...nel.org>
Cc:     rostedt@...dmis.org, tglx@...utronix.de, mhiramat@...nel.org,
        vedang.patel@...el.com, bigeasy@...utronix.de,
        joel.opensrc@...il.com, joelaf@...gle.com,
        mathieu.desnoyers@...icios.com, baohong.liu@...el.com,
        rajvi.jingar@...el.com, julia@...com, fengguang.wu@...el.com,
        linux-kernel@...r.kernel.org, linux-rt-users@...r.kernel.org,
        kernel-team@....com
Subject: Re: [PATCH v7 22/37] tracing: Add variable reference handling to
 hist triggers

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.zanussi@...ux.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