linux-kernel - Re: [PATCH 3/6] perf script: move printing of 'common' data from print

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]

Date:	Thu, 10 Mar 2011 10:20:53 -0500
From:	Steven Rostedt <rostedt@...dmis.org>
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,
	fweisbec@...il.com, paulus@...ba.org, tglx@...utronix.de
Subject: Re: [PATCH 3/6] perf script: move printing of 'common' data from
 print_event and rename

On Wed, Mar 09, 2011 at 10:23:25PM -0700, David Ahern wrote:
> This change does impact output: latency data is trace specific and is now
> printed after the common data - comm, tid, cpu, time and event name.
> 
> Signed-off-by: David Ahern <daahern@...co.com>
> ---
>  tools/perf/builtin-script.c         |   38 ++++++++++++++++++++++-----
>  tools/perf/util/trace-event-parse.c |   49 +++++++---------------------------

I was hoping after the next merge window to start making a common library
for parsing events. This way things like powertop and timechart or
anything that uses the perf interface does not need to write its own
parsing of events, or expect the event formats to be hardcoded.

The trace-event-parse.c was taking from trace-cmd's parse-events.c code
and hopefully the two can merge again. The parse-events.c code in
trace-cmd has gone through several iterations that has made it much more
robust and flexible. I purposely kept it as a separate libarary not
dependent on trace-cmd so that it could be used by other utilities like
perf.

>  tools/perf/util/trace-event.h       |    3 +-
>  3 files changed, 42 insertions(+), 48 deletions(-)
> 
> diff --git a/tools/perf/builtin-script.c b/tools/perf/builtin-script.c
> index b2bdd55..0a79da2 100644
> --- a/tools/perf/builtin-script.c
> +++ b/tools/perf/builtin-script.c
> @@ -20,18 +20,42 @@ static u64			last_timestamp;
>  static u64			nr_unordered;
>  extern const struct option	record_options[];
>  
> +static void print_sample_start(struct perf_sample *sample,
> +			       struct thread *thread)
> +{
> +	int type;
> +	struct event *event;
> +	const char *evname = NULL;
> +	unsigned long secs;
> +	unsigned long usecs;
> +	unsigned long long nsecs = sample->time;
> +
> +	if (latency_format)
> +		printf("%8.8s-%-5d %3d", thread->comm, sample->tid, sample->cpu);
> +	else
> +		printf("%16s-%-5d [%03d]", thread->comm, sample->tid, sample->cpu);
> +
> +	secs = nsecs / NSECS_PER_SEC;
> +	nsecs -= secs * NSECS_PER_SEC;
> +	usecs = nsecs / NSECS_PER_USEC;
> +	printf(" %5lu.%06lu: ", secs, usecs);
> +
> +	type = trace_parse_common_type(sample->raw_data);
> +	event = trace_find_event(type);
> +	if (event)
> +		evname = event->name;
> +
> +	printf("%s: ", evname ? evname : "(unknown)");
> +}
> +
>  static void process_event(union perf_event *event __unused,
>  			  struct perf_sample *sample,
>  			  struct perf_session *session __unused,
>  			  struct thread *thread)
>  {
> -	/*
> -	 * FIXME: better resolve from pid from the struct trace_entry
> -	 * 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);
> +	print_sample_start(sample, thread);
> +	print_trace_event(sample->cpu, sample->raw_data, sample->raw_size);
> +	printf("\n");
>  }
>  
>  static int default_start_script(const char *script __unused,
> diff --git a/tools/perf/util/trace-event-parse.c b/tools/perf/util/trace-event-parse.c
> index dd5f058..0a7ed5b 100644
> --- a/tools/perf/util/trace-event-parse.c
> +++ b/tools/perf/util/trace-event-parse.c
> @@ -2643,9 +2643,9 @@ static void print_lat_fmt(void *data, int size __unused)
>  		printf(".");
>  
>  	if (lock_depth < 0)
> -		printf(".");
> +		printf(". ");
>  	else
> -		printf("%d", lock_depth);
> +		printf("%d ", lock_depth);

For example, I'm removing the field lock_depth, as that was added by
Frederic as a temporary field to help with the removal of the BKL.
Currently this code is unused by perf, but if it is in the future, it
must be able to handle not having this field. In fact, I'm thinking of
modifying other "common" fields to help improve tracing performance.

If we could have a common library for all tools that need to parse
events, then this code can be updated in a single place. Which also
brings another point. Do we want to move the events out of debugfs. And
if we do, I would love to change the way the event format is. I consider
the format in debugfs as it stands a true ABI, but the content that the
format shows is not.

-- Steve
--
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/