Re: [PATCH] perf tools: Fix reading of perf.data file header

Ingo Molnar
2009-08-07T06:32:33Z

* Brice Goglin <Brice.Goglin@inria.fr> wrote:

> Ingo Molnar wrote:
> > It would be nice to add this as some "perf report -s/--stats" flag,
> > to not have to go via -D (which is a 'print debug output' kind of
> > ad-hoc thing and subject to format changes in the future).
> >
> > Would you be interested in sending a patch that adds that flag
> > to 'perf report', to print out these statistics entries (if
> > any), in a tabular form suitable for your purposes? Below is a
> > past patch to builtin-report.c that shows how to add new
> > options.
>
> Here's a quick'n'dirty first try. Read events are copied in the
> show_stat_event array during process_read_event. And __cmd_report
> sorts the array by tid before displaying it.
>
> perf report -S now shows the following after the existing output: (-s is
> already used for something else).
> It shows things like
> # Per-thread statistics:
> # PID TID Event Count
> 16709 16709 cache-misses 82727
> 16709 16709 cache-references 41238768
> 16709 16710 cache-misses 6462
> 16709 16710 cache-references 76119375
> or
> # Per-thread statistics:
> # PID TID Event Count
> 6268 6268 raw 0x1000001e0 494628
> 6268 6268 raw 0x1000002e0 209113
> 6268 6268 raw 0x1000004e0 307215
> 6268 6268 raw 0x1000008e0 9203221
> 6268 6269 raw 0x1000001e0 9210788
> 6268 6269 raw 0x1000002e0 302344
> 6268 6269 raw 0x1000004e0 198705
> 6268 6269 raw 0x1000008e0 473471
>
> Obviously, there's some a lot of nice pretty printing to do, but
> you'll be able to tell whether the general idea is ok or not.

Yeah, the general idea looks OK to me.

I think it would be nice to share the pretty-printing code with
'perf stat' (builtin-stat.c).

Plus, to make it easier for your scripting needs, we could add a
--pretty=raw type of flag to both perf stat and perf report, which
would emit the data in a raw way - to be used in gnuplot almost
straight away, etc.

But for the typical interactive use it would be nice to do the 2D
tabular form that 'perf stat' does. (btw: feel free to enhance that
output as well, where it seems appropriate)

here's a few mostly stylistic comments:

> static int full_paths;
> static int show_nr_samples;
> +static int show_stat;
> +static int show_stat_events;
> +static int show_stat_event_max;
> +static struct read_event *show_stat_event;

> @@ -126,6 +132,8 @@
> struct read_event read;
> } event_t;
>
> +static struct perf_counter_attr *perf_header__find_attr(u64 id);

Small cleanliness detail: could this function be moved to this spot?
That way we could avoid this prototype declaration.

> +static int compar_read_event_by_tid(const void *e1, const void *e2)
> +{
> + const struct read_event *event1 = e1;
> + const struct read_event *event2 = e2;
> + return event1->tid - event2->tid;
> +}

another small detail: i'd suggest s/compar/compare, and please put a
newline after local variables, i.e. something like:

static int compare_read_event_by_tid(const void *e1, const void *e2)
{
const struct read_event *event1 = e1;
const struct read_event *event2 = e2;

return event1->tid - event2->tid;
}

> @@ -1430,6 +1445,21 @@
> }
> fprintf(fp, "\n");
>
> + if (show_stat && show_stat_events) {
> + int i;

[please add a newline here too]

> + qsort(&show_stat_event[0], show_stat_events, sizeof(struct read_event), compar_read_event_by_tid);
> + fprintf(fp, "# Per-thread statistics:\n");
> + fprintf(fp, "# PID TID Event Count\n");
> + for(i=0; i<show_stat_events; i++) {

We write loops a tiny bit differently in the kernel - there's an
easy way to check such details: run your patch through
scripts/checkpatch.pl.

> + struct read_event *event = &show_stat_event[i];
> + struct perf_counter_attr *attr = perf_header__find_attr(event->id);

[please add a newline here too]

> + printf(" %d %d %s %Lu\n",
> + event->pid, event->tid,
> + attr ? __event_name(attr->type, attr->config) : "unknown",
> + event->value);
> + }
> + }

i'd also suggest to put this function into a helper inline function,
which can start with:

if (!show_stat || !show_stat_events)
return;

That way it looks a (tiny) bit more structured and we win an
indentation level.

> + if (show_stat) {
> + if (!show_stat_event) {
> + show_stat_events = 0;
> + show_stat_event_max = 16;
> + show_stat_event = malloc(show_stat_event_max * sizeof(*show_stat_event));
> + if (!show_stat_event)
> + die("cannot allocate show_stat_event array");
> + }
> + if (show_stat_events == show_stat_event_max) {
> + show_stat_event_max *= 2;
> + show_stat_event = realloc(show_stat_event, show_stat_event_max * sizeof(*show_stat_event));
> + if (!show_stat_event)
> + die("cannot enlarge show_stat_event array");
> + }
> + memcpy(&show_stat_event[show_stat_events], &event->read, sizeof(struct read_event));
> + show_stat_events++;
> + }

this too could move into a helper inline function.

> @@ -1998,6 +2046,8 @@
> "Show a column with the number of samples"),
> OPT_STRING('s', "sort", &sort_order, "key[,key2...]",
> "sort by key(s): pid, comm, dso, symbol, parent"),
> + OPT_BOOLEAN('S', "stat", &show_stat,
> + "show per-thread event counters"),

Ok, there's indeed a flag clash with -s/--sort as you noticed.

-S looks good to me, how about going one step further and changing
perf record to use -S/--stat as well, to make the flag consistent
across all tools?

In any case, this patch moves into the right direction - this kind
of functionality is exactly what we need.

Ingo