| 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
| ||
|
Date: Tue, 19 May 2020 22:15:48 -0300
From: Arnaldo Carvalho de Melo <arnaldo.melo@...il.com>
To: Ian Rogers <irogers@...gle.com>
Cc: Arnaldo Carvalho de Melo <arnaldo.melo@...il.com>,
Peter Zijlstra <peterz@...radead.org>,
Ingo Molnar <mingo@...hat.com>,
Mark Rutland <mark.rutland@....com>,
Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
Jiri Olsa <jolsa@...hat.com>,
Namhyung Kim <namhyung@...nel.org>,
Alexei Starovoitov <ast@...nel.org>,
Daniel Borkmann <daniel@...earbox.net>,
Martin KaFai Lau <kafai@...com>,
Song Liu <songliubraving@...com>, Yonghong Song <yhs@...com>,
Andrii Nakryiko <andriin@...com>,
John Fastabend <john.fastabend@...il.com>,
KP Singh <kpsingh@...omium.org>,
Kajol Jain <kjain@...ux.ibm.com>,
Andi Kleen <ak@...ux.intel.com>,
John Garry <john.garry@...wei.com>,
Jin Yao <yao.jin@...ux.intel.com>,
Kan Liang <kan.liang@...ux.intel.com>,
Cong Wang <xiyou.wangcong@...il.com>,
Kim Phillips <kim.phillips@....com>,
Adrian Hunter <adrian.hunter@...el.com>,
Leo Yan <leo.yan@...aro.org>,
LKML <linux-kernel@...r.kernel.org>,
Networking <netdev@...r.kernel.org>, bpf <bpf@...r.kernel.org>,
Stephane Eranian <eranian@...gle.com>,
Paul Clarke <pc@...ibm.com>,
Arnaldo Carvalho de Melo <acme@...hat.com>
Subject: Re: [PATCH v3 6/7] perf test: Improve pmu event metric testing
Em Tue, May 19, 2020 at 01:15:41PM -0700, Ian Rogers escreveu:
> On Tue, May 19, 2020 at 12:06 PM Arnaldo Carvalho de Melo
> <arnaldo.melo@...il.com> wrote:
> >
> > Em Fri, May 15, 2020 at 03:17:31PM -0700, Ian Rogers escreveu:
> > > Break pmu-events test into 2 and add a test to verify that all pmu
> > > metric expressions simply parse. Try to parse all metric ids/events,
> > > skip/warn if metrics for the current architecture fail to parse. To
> > > support warning for a skip, and an ability for a subtest to describe why
> > > it skips.
> > >
> > > Tested on power9, skylakex, haswell, broadwell, westmere, sandybridge and
> > > ivybridge.
> > >
> > > May skip/warn on other architectures if metrics are invalid. In
> > > particular s390 is untested, but its expressions are trivial. The
> > > untested architectures with expressions are power8, cascadelakex,
> > > tremontx, skylake, jaketown, ivytown and variants of haswell and
> > > broadwell.
> > >
> > > v3. addresses review comments from John Garry <john.garry@...wei.com>,
> > > Jiri Olsa <jolsa@...hat.com> and Arnaldo Carvalho de Melo
> > > <acme@...nel.org>.
> > > v2. changes the commit message as event parsing errors no longer cause
> > > the test to fail.
> > >
> > > Signed-off-by: Ian Rogers <irogers@...gle.com>
> > > Cc: Adrian Hunter <adrian.hunter@...el.com>
> > > Cc: Alexander Shishkin <alexander.shishkin@...ux.intel.com>
> > > Cc: Andi Kleen <ak@...ux.intel.com>
> > > Cc: Jin Yao <yao.jin@...ux.intel.com>
> > > Cc: Jiri Olsa <jolsa@...hat.com>
> > > Cc: John Garry <john.garry@...wei.com>
> > > Cc: Kajol Jain <kjain@...ux.ibm.com>
> > > Cc: Kan Liang <kan.liang@...ux.intel.com>
> > > Cc: Leo Yan <leo.yan@...aro.org>
> > > Cc: Mark Rutland <mark.rutland@....com>
> > > Cc: Namhyung Kim <namhyung@...nel.org>
> > > Cc: Paul Clarke <pc@...ibm.com>
> > > Cc: Peter Zijlstra <peterz@...radead.org>
> > > Cc: Stephane Eranian <eranian@...gle.com>
> > > Link: http://lore.kernel.org/lkml/20200513212933.41273-1-irogers@google.com
> > > [ split from a larger patch ]
> > > Signed-off-by: Arnaldo Carvalho de Melo <acme@...hat.com>
> > > ---
> > > tools/perf/tests/builtin-test.c | 7 ++
> > > tools/perf/tests/pmu-events.c | 168 ++++++++++++++++++++++++++++++--
> > > tools/perf/tests/tests.h | 3 +
> > > 3 files changed, 172 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/tools/perf/tests/builtin-test.c b/tools/perf/tests/builtin-test.c
> > > index baee735e6aa5..9553f8061772 100644
> > > --- a/tools/perf/tests/builtin-test.c
> > > +++ b/tools/perf/tests/builtin-test.c
> > > @@ -75,6 +75,13 @@ static struct test generic_tests[] = {
> > > {
> > > .desc = "PMU events",
> > > .func = test__pmu_events,
> > > + .subtest = {
> > > + .skip_if_fail = false,
> > > + .get_nr = test__pmu_events_subtest_get_nr,
> > > + .get_desc = test__pmu_events_subtest_get_desc,
> > > + .skip_reason = test__pmu_events_subtest_skip_reason,
> > > + },
> > > +
> > > },
> > > {
> > > .desc = "DSO data read",
> > > diff --git a/tools/perf/tests/pmu-events.c b/tools/perf/tests/pmu-events.c
> > > index d64261da8bf7..e21f0addcfbb 100644
> > > --- a/tools/perf/tests/pmu-events.c
> > > +++ b/tools/perf/tests/pmu-events.c
> > > @@ -8,6 +8,9 @@
> > > #include <linux/zalloc.h>
> > > #include "debug.h"
> > > #include "../pmu-events/pmu-events.h"
> > > +#include "util/evlist.h"
> > > +#include "util/expr.h"
> > > +#include "util/parse-events.h"
> > >
> > > struct perf_pmu_test_event {
> > > struct pmu_event event;
> > > @@ -144,7 +147,7 @@ static struct pmu_events_map *__test_pmu_get_events_map(void)
> > > }
> > >
> > > /* Verify generated events from pmu-events.c is as expected */
> > > -static int __test_pmu_event_table(void)
> > > +static int test_pmu_event_table(void)
> > > {
> > > struct pmu_events_map *map = __test_pmu_get_events_map();
> > > struct pmu_event *table;
> > > @@ -347,14 +350,11 @@ static int __test__pmu_event_aliases(char *pmu_name, int *count)
> > > return res;
> > > }
> > >
> > > -int test__pmu_events(struct test *test __maybe_unused,
> > > - int subtest __maybe_unused)
> > > +
> > > +static int test_aliases(void)
> > > {
> > > struct perf_pmu *pmu = NULL;
> > >
> > > - if (__test_pmu_event_table())
> > > - return -1;
> > > -
> > > while ((pmu = perf_pmu__scan(pmu)) != NULL) {
> > > int count = 0;
> > >
> > > @@ -377,3 +377,159 @@ int test__pmu_events(struct test *test __maybe_unused,
> > >
> > > return 0;
> > > }
> > > +
> > > +static bool is_number(const char *str)
> > > +{
> > > + char *end_ptr;
> > > +
> > > + strtod(str, &end_ptr);
> > > + return end_ptr != str;
> > > +}
> >
> > So, this breaks in some systems:
> >
> > cc1: warnings being treated as errors
> > tests/pmu-events.c: In function 'is_number':
> > tests/pmu-events.c:385: error: ignoring return value of 'strtod', declared with attribute warn_unused_result
> > mv: cannot stat `/tmp/build/perf/tests/.pmu-events.o.tmp': No such file or director
> >
> > So I'm changing it to verify the result of strtod() which is, humm,
> > interesting, please check:
>
> Thanks Arnaldo and sorry for the difficulty. This looks like a good fix.
>
> > diff --git a/tools/perf/tests/pmu-events.c b/tools/perf/tests/pmu-events.c
> > index 3de59564deb0..6c58c3a89e6b 100644
> > --- a/tools/perf/tests/pmu-events.c
> > +++ b/tools/perf/tests/pmu-events.c
> > @@ -1,4 +1,5 @@
> > // SPDX-License-Identifier: GPL-2.0
> > +#include "math.h"
> > #include "parse-events.h"
> > #include "pmu.h"
> > #include "tests.h"
> > @@ -381,8 +382,12 @@ static int test_aliases(void)
> > static bool is_number(const char *str)
> > {
> > char *end_ptr;
> > + double v;
> >
> > - strtod(str, &end_ptr);
> > + errno = 0;
> > + v = strtod(str, &end_ptr);
> > + if ((errno == ERANGE && (v == HUGE_VAL || v == -HUGE_VAL)) || (errno != 0 && v == 0.0))
>
> errno can either be 0 or ERANGE here, but we test both. Perhaps use
> errno != 0 for both cases as the man page notes suggest doing this.
> The tests using v are necessary to avoid the unused result, but
> presumably any errno case should return false here? I guess testing
> that is redundant as the return below will catch it. Perhaps this
> should be:
>
> errno = 0;
> v = strtod(str, &end_ptr);
> (void)v; /* We don't care for the value of the double, just that it
> converts. Avoid unused result warnings. */
> return errno == 0 && end_ptr != str;
Ok, I'll try that one.
- Arnaldo
> Thanks,
> Ian
>
> > + return false;
> > return end_ptr != str;
> > }
> >
--
- Arnaldo
Powered by blists - more mailing lists