Messages in this thread | | | Date | Thu, 17 Dec 2015 01:26:33 -0800 | Subject | Re: [PATCH 05/10] perf, tools, stat: Basic support for TopDown in perf stat | From | Stephane Eranian <> |
| |
Andi,
On Wed, Dec 16, 2015 at 1:21 PM, Andi Kleen <andi@firstfloor.org> wrote: >> > +/* >> > + * Check whether we can use a group for top down. >> > + * Without a group may get bad results due to multiplexing. >> > + */ >> >> That is not because you have a counter used by the NMI that >> you cannot group. If HT is off you have plenty of counters to >> do this. > > Such a heuristic wouldn't work on Atom where there are no more > counters in this case. > >> > +static bool check_group(bool *warn) >> > +{ >> > + int n; >> > + >> > + if (sysctl__read_int("kernel/nmi_watchdog", &n) < 0) >> > + return false; >> > + if (n > 0) { >> > + *warn = true; >> > + return false; >> > + } >> > + return true; >> > +} >> > + >> >> I do not like this part very much. You are pulling in x86 specific >> knowledge into >> builtin_stat.c. Why not move this into an x86 specific file? > > Done. > >> > err = parse_events(evsel_list, transaction_attrs, NULL); >> > @@ -1511,6 +1579,36 @@ static int add_default_attributes(void) >> > return 0; >> > } >> > >> > + if (topdown_run) { >> > + char *str = NULL; >> > + bool warn = false; >> > + >> > + filter_events(topdown_attrs, &str, check_group(&warn)); >> > + if (topdown_attrs[0] && str) { >> > + if (warn) >> > + fprintf(stderr, >> > + "nmi_watchdog enabled with topdown. May give wrong results.\n" >> > + "Disable with echo 0 > /proc/sys/kernel/nmi_watchdog\n"); >> >> This is x86 specific. Why not just try it out and in case of error >> suggest checking >> if pinned system-wide events exist (such as NMI watchdog on x86). that would >> be more generic. > > That's really complicated, i would have to tear down all state and then > resubmit all the events. I think just checking the NMI watchdog is good > enough. I couldn't give a sensible error message for the generic case > anyways. > What about that fancy error reporting extension that was proposed a few months back? It could help print a sensible error msg. Did it ever make it in?
| |