Re: [PATCH v3 6/7] perf test: Improve pmu event metric testing

[Ian Rogers]

On Tue, May 19, 2020 at 12:06 PM Arnaldo Carvalho de Melo
<arnaldo.melo@xxxxxxxxx> 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@xxxxxxxxxx>,
> > Jiri Olsa <jolsa@xxxxxxxxxx> and Arnaldo Carvalho de Melo
> > <acme@xxxxxxxxxx>.
> > v2. changes the commit message as event parsing errors no longer cause
> > the test to fail.
> >
> > Signed-off-by: Ian Rogers <irogers@xxxxxxxxxx>
> > Cc: Adrian Hunter <adrian.hunter@xxxxxxxxx>
> > Cc: Alexander Shishkin <alexander.shishkin@xxxxxxxxxxxxxxx>
> > Cc: Andi Kleen <ak@xxxxxxxxxxxxxxx>
> > Cc: Jin Yao <yao.jin@xxxxxxxxxxxxxxx>
> > Cc: Jiri Olsa <jolsa@xxxxxxxxxx>
> > Cc: John Garry <john.garry@xxxxxxxxxx>
> > Cc: Kajol Jain <kjain@xxxxxxxxxxxxx>
> > Cc: Kan Liang <kan.liang@xxxxxxxxxxxxxxx>
> > Cc: Leo Yan <leo.yan@xxxxxxxxxx>
> > Cc: Mark Rutland <mark.rutland@xxxxxxx>
> > Cc: Namhyung Kim <namhyung@xxxxxxxxxx>
> > Cc: Paul Clarke <pc@xxxxxxxxxx>
> > Cc: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
> > Cc: Stephane Eranian <eranian@xxxxxxxxxx>
> > Link: http://lore.kernel.org/lkml/20200513212933.41273-1-irogers@xxxxxxxxxx
> > [ split from a larger patch ]
> > Signed-off-by: Arnaldo Carvalho de Melo <acme@xxxxxxxxxx>
> > ---
> >  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;

Thanks,
Ian

> +               return false;
>         return end_ptr != str;
>  }
>