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] [thread-next>] [day] [month] [year] [list]

Message-ID: <4D7518DB.9090004@cisco.com>
Date:	Mon, 07 Mar 2011 10:41:47 -0700
From:	David Ahern <daahern@...co.com>
To:	Frederic Weisbecker <fweisbec@...il.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 03/07/11 09:50, Frederic Weisbecker wrote:
>> +-U::
>> +--show-unresolved::
>> +        Display all addresses including unresolved to a symbol.
> 
> We should always do it I think. As long as the sym format is asked, try
> to resolve it, otherwise print the raw address.

Ok. I'll remove.


>> +/* 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. 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

If user does not specify custom option, set output_fields to default
based on event type - which conceptually can change sample to sample
(though perf currently can't handle a S/W and a trace event in the same
file).

As for H/W events, the cycles format should be the same as the S/W
format. After that perf-record does not mix well with H/W events.


>>  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.

David

> 
> Other than that, looks like a right direction.
> 
> 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/