Re: [PATCH 07/10] perf script: Move printing of 'common' data fromprint_event and rename

From: Frederic Weisbecker
Date: Wed Mar 09 2011 - 18:50:24 EST


On Wed, Mar 09, 2011 at 03:31:28PM -0300, Arnaldo Carvalho de Melo wrote:
> From: David Ahern <daahern@xxxxxxxxx>
>
> 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.
>
> Cc: Frederic Weisbecker <fweisbec@xxxxxxxxx>
> Cc: Ingo Molnar <mingo@xxxxxxx>
> Cc: Paul Mackerras <paulus@xxxxxxxxx>
> Cc: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
> Cc: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> LPU-Reference: <1299606342-26745-3-git-send-email-daahern@xxxxxxxxx>
> Signed-off-by: David Ahern <daahern@xxxxxxxxx>
> [ committer note: Added space after print_lat_fmt() ]
> Signed-off-by: Arnaldo Carvalho de Melo <acme@xxxxxxxxxx>
> ---
> tools/perf/builtin-script.c | 38 ++++++++++--
> tools/perf/util/trace-event-parse.c | 106 +++--------------------------------
> tools/perf/util/trace-event.h | 3 +-
> 3 files changed, 41 insertions(+), 106 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)

I believe we don't yet make use of the latency format with perf.
Like the function graph output, it was included in that code coming
from trace-cmd.

We might use it in the future though, so it looks fine to keep it
and handle it like you did.

> + 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)");
> +}
> +
[...]
> +++ b/tools/perf/util/trace-event-parse.c
> @@ -2648,63 +2648,8 @@ static void print_lat_fmt(void *data, int size __unused)
> printf("%d", lock_depth);
> }
>
> -/* taken from Linux, written by Frederic Weisbecker */
> -static void print_graph_cpu(int cpu)
> -{
> - int i;
> - int log10_this = log10_cpu(cpu);
> - int log10_all = log10_cpu(cpus);
> -
> -
> - /*
> - * Start with a space character - to make it stand out
> - * to the right a bit when trace output is pasted into
> - * email:
> - */
> - printf(" ");
> -
> - /*
> - * Tricky - we space the CPU field according to the max
> - * number of online CPUs. On a 2-cpu system it would take
> - * a maximum of 1 digit - on a 128 cpu system it would
> - * take up to 3 digits:
> - */
> - for (i = 0; i < log10_all - log10_this; i++)
> - printf(" ");
> -
> - printf("%d) ", cpu);
> -}

So, we indeed don't use the function graph tracer with perf yet.
But there are fair chances we will in the future.

So if we remove such code, I would prefer this to be made as
a seperate commit. Something we can easily retrieve and revert
in the future.

Other than that and the callchain bug, the whole series looks
pretty good now.

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