Messages in this thread Patch in this message | | | Date | Fri, 7 Aug 2009 08:32:26 +0200 | From | Ingo Molnar <> | Subject | Re: [PATCH] perf tools: Fix reading of perf.data file header |
| |
* 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
| |