linux-kernel - Re: [PATCH 2/2] perf script: support custom field selection for output

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] [day] [month] [year] [list]
Message-ID: <20110307182319.GH1873@nowhere>
Date:	Mon, 7 Mar 2011 19:23:22 +0100
From:	Frederic Weisbecker <fweisbec@...il.com>
To:	David Ahern <daahern@...co.com>
Cc:	linux-perf-users@...r.kernel.org, linux-kernel@...r.kernel.org,
	acme@...stprotocols.net, mingo@...e.hu, peterz@...radead.org,
	paulus@...ba.org, tglx@...utronix.de
Subject: Re: [PATCH 2/2] perf script: support custom field selection for
 output

On Mon, Mar 07, 2011 at 10:41:47AM -0700, David Ahern wrote:
> On 03/07/11 09:50, Frederic Weisbecker wrote:
> >> +/* default set to maintain compatibility with current format */
> >> +#define output_fields_default   (PERF_OUTPUT_COMM | PERF_OUTPUT_PID | \
> >> +				PERF_OUTPUT_CPU | PERF_OUTPUT_TIME | \
> >> +				PERF_OUTPUT_EVNAME | PERF_OUTPUT_TRACE)
> >> +
> >> +u64 output_fields = output_fields_default;
> > 
> > Hm, we should have one default for tracepoint events and one
> > for others. For example dso and sym make sense for hardware,
> > breakpoint and software event, but it makes few sense for tracepoints.
> 
> That default was strictly to maintain backwards compatibility with
> existing output.

Well, we don't necessarily need to keep a backward compatibility. If
one day we realize the comm is not needed on the traces output, we can
just remove it right away. We only want to keep the exisiting default
because it (seems to) make sense. Now the default necessary headers
that make sense for tracepoint may not be the same than for hardware
events.

> So you would like to see:
> 
> out_fields_def_trace = <the above default>
> 
> out_fields_def_sw = PERF_OUTPUT_COMM | PERF_OUTPUT_TID | \
>                     PERF_OUTPUT_CPU | PERF_OUTPUT_TIME | \
>                     PERF_OUTPUT_EVNAM | PERF_OUTPUT_SYM

Yep, I guess that would be sensible.

> 
> If user does not specify custom option, set output_fields to default
> based on event type - which conceptually can change sample to sample

Exactly. Which means that a global output_fields wouldn't make
much sense anymore.

> (though perf currently can't handle a S/W and a trace event in the same
> file).

Yeah, but let's prepare the ground for that.

> As for H/W events, the cycles format should be the same as the S/W
> format.

Probably. Unless we find later that H/W event need some special treatment
over software ones.

> After that perf-record does not mix well with H/W events.

Hm?
 
> >>  static void process_event(union perf_event *event __unused,
> >>  			  struct perf_sample *sample,
> >> @@ -31,8 +107,15 @@ static void process_event(union perf_event *event __unused,
> >>  	 * field, although it should be the same than this perf
> >>  	 * event pid
> >>  	 */
> >> -	print_event(sample->cpu, sample->raw_data, sample->raw_size,
> >> -		    sample->time, thread->comm);
> >> +	if (PRINT_FIELD(TRACE))
> >> +		print_event(sample->cpu, sample->raw_data,
> >> +			    sample->raw_size, sample->time,
> >> +			    thread->comm);
> > 
> > The print_event() thing we have in trace-event-parse.c should really handle only
> > the raw trace itself. More on that below when I reach that file.
> 
> Ok
> 
> 
> >> +		callchain_cursor_commit(cursor);
> >> +		if (cursor->nr == 0)
> >> +			return;
> > 
> > I guess you don't need the above check.
> 
> Ok. I'll remove.
> 
> 
> >> diff --git a/tools/perf/util/trace-event-parse.c b/tools/perf/util/trace-event-parse.c
> >> index d8e622d..9ac6524 100644
> >> --- a/tools/perf/util/trace-event-parse.c
> >> +++ b/tools/perf/util/trace-event-parse.c
> >> @@ -32,6 +32,7 @@
> >>  #include "../perf.h"
> >>  #include "util.h"
> >>  #include "trace-event.h"
> >> +#include "output.h"
> >>  
> >>  int header_page_ts_offset;
> >>  int header_page_ts_size;
> >> @@ -2683,13 +2684,18 @@ static void print_graph_proc(int pid, const char *comm)
> >>  	/* sign + log10(MAX_INT) + '\0' */
> >>  	char pid_str[11];
> >>  	int spaces = 0;
> >> -	int len;
> >> +	int len = 0;
> >>  	int i;
> >>  
> >>  	sprintf(pid_str, "%d", pid);
> >>  
> >>  	/* 1 stands for the "-" character */
> >> -	len = strlen(comm) + strlen(pid_str) + 1;
> >> +	if (PRINT_FIELD(COMM) && PRINT_FIELD(PID))
> >> +		len = strlen(comm) + strlen(pid_str) + 1;
> >> +	else if (PRINT_FIELD(COMM))
> >> +		len = strlen(comm);
> >> +	else if (PRINT_FIELD(PID))
> >> +		len = strlen(pid_str);
> > 
> > For now we don't use that function because the function graph tracer
> > is not yet supported by perf.
> > 
> > Just forget about that, we'll take care of that later. Consider
> > pretty_print_func_graph() can't be called.
> 
> That certainly helps make it simpler. I left the printing of those
> fields in that file b/c of that function.
> 
> > 
> > trace-event-parse.c and its print_event() pretty printing should not care
> > about the output format. It only needs to print the raw trace itself.
> > All the comm, time, etc... things should be handled from perf script
> > core.
> > 
> > In fact, print_event() should really be renamed print_trace() or something.
> 
> Ok. I'll pull the comm, pid, time options into builtin-script's version
> and rename the function.

Thanks!
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/