linux-kernel - Re: [PATCH 05/10] perf, tools, stat: Basic support for TopDown in perf stat

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]

Message-ID: <20151216212104.GZ15533@two.firstfloor.org>
Date:	Wed, 16 Dec 2015 22:21:04 +0100
From:	Andi Kleen <andi@...stfloor.org>
To:	Stephane Eranian <eranian@...gle.com>
Cc:	Andi Kleen <andi@...stfloor.org>,
	Arnaldo Carvalho de Melo <acme@...nel.org>,
	Peter Zijlstra <peterz@...radead.org>,
	Jiri Olsa <jolsa@...nel.org>, Ingo Molnar <mingo@...nel.org>,
	LKML <linux-kernel@...r.kernel.org>,
	Namhyung Kim <namhyung@...nel.org>,
	Andi Kleen <ak@...ux.intel.com>
Subject: Re: [PATCH 05/10] perf, tools, stat: Basic support for TopDown in
 perf stat

> > +/*
> > + * 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.

-Andi
-- 
ak@...ux.intel.com -- Speaking for myself only.
--
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/