diff mbox series

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

Message ID 20200515221732.44078-7-irogers@google.com
State Not Applicable
Delegated to: BPF Maintainers
Headers show
Series Copy hashmap to tools/perf/util, use in perf expr | expand

Commit Message

Ian Rogers May 15, 2020, 10:17 p.m. UTC
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@huawei.com>,
Jiri Olsa <jolsa@redhat.com> and Arnaldo Carvalho de Melo
<acme@kernel.org>.
v2. changes the commit message as event parsing errors no longer cause
the test to fail.

Signed-off-by: Ian Rogers <irogers@google.com>
Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Andi Kleen <ak@linux.intel.com>
Cc: Jin Yao <yao.jin@linux.intel.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: John Garry <john.garry@huawei.com>
Cc: Kajol Jain <kjain@linux.ibm.com>
Cc: Kan Liang <kan.liang@linux.intel.com>
Cc: Leo Yan <leo.yan@linaro.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Paul Clarke <pc@us.ibm.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Stephane Eranian <eranian@google.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@redhat.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(-)

Comments

Arnaldo Carvalho de Melo May 19, 2020, 7:06 p.m. UTC | #1
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@huawei.com>,
> Jiri Olsa <jolsa@redhat.com> and Arnaldo Carvalho de Melo
> <acme@kernel.org>.
> v2. changes the commit message as event parsing errors no longer cause
> the test to fail.
> 
> Signed-off-by: Ian Rogers <irogers@google.com>
> Cc: Adrian Hunter <adrian.hunter@intel.com>
> Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> Cc: Andi Kleen <ak@linux.intel.com>
> Cc: Jin Yao <yao.jin@linux.intel.com>
> Cc: Jiri Olsa <jolsa@redhat.com>
> Cc: John Garry <john.garry@huawei.com>
> Cc: Kajol Jain <kjain@linux.ibm.com>
> Cc: Kan Liang <kan.liang@linux.intel.com>
> Cc: Leo Yan <leo.yan@linaro.org>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Namhyung Kim <namhyung@kernel.org>
> Cc: Paul Clarke <pc@us.ibm.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Stephane Eranian <eranian@google.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@redhat.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:

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))
+		return false;
 	return end_ptr != str;
 }
Ian Rogers May 19, 2020, 8:15 p.m. UTC | #2
On Tue, May 19, 2020 at 12:06 PM Arnaldo Carvalho de Melo
<arnaldo.melo@gmail.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@huawei.com>,
> > Jiri Olsa <jolsa@redhat.com> and Arnaldo Carvalho de Melo
> > <acme@kernel.org>.
> > v2. changes the commit message as event parsing errors no longer cause
> > the test to fail.
> >
> > Signed-off-by: Ian Rogers <irogers@google.com>
> > Cc: Adrian Hunter <adrian.hunter@intel.com>
> > Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> > Cc: Andi Kleen <ak@linux.intel.com>
> > Cc: Jin Yao <yao.jin@linux.intel.com>
> > Cc: Jiri Olsa <jolsa@redhat.com>
> > Cc: John Garry <john.garry@huawei.com>
> > Cc: Kajol Jain <kjain@linux.ibm.com>
> > Cc: Kan Liang <kan.liang@linux.intel.com>
> > Cc: Leo Yan <leo.yan@linaro.org>
> > Cc: Mark Rutland <mark.rutland@arm.com>
> > Cc: Namhyung Kim <namhyung@kernel.org>
> > Cc: Paul Clarke <pc@us.ibm.com>
> > Cc: Peter Zijlstra <peterz@infradead.org>
> > Cc: Stephane Eranian <eranian@google.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@redhat.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;

Thanks,
Ian

> +               return false;
>         return end_ptr != str;
>  }
>
Arnaldo Carvalho de Melo May 20, 2020, 1:15 a.m. UTC | #3
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@gmail.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@huawei.com>,
> > > Jiri Olsa <jolsa@redhat.com> and Arnaldo Carvalho de Melo
> > > <acme@kernel.org>.
> > > v2. changes the commit message as event parsing errors no longer cause
> > > the test to fail.
> > >
> > > Signed-off-by: Ian Rogers <irogers@google.com>
> > > Cc: Adrian Hunter <adrian.hunter@intel.com>
> > > Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> > > Cc: Andi Kleen <ak@linux.intel.com>
> > > Cc: Jin Yao <yao.jin@linux.intel.com>
> > > Cc: Jiri Olsa <jolsa@redhat.com>
> > > Cc: John Garry <john.garry@huawei.com>
> > > Cc: Kajol Jain <kjain@linux.ibm.com>
> > > Cc: Kan Liang <kan.liang@linux.intel.com>
> > > Cc: Leo Yan <leo.yan@linaro.org>
> > > Cc: Mark Rutland <mark.rutland@arm.com>
> > > Cc: Namhyung Kim <namhyung@kernel.org>
> > > Cc: Paul Clarke <pc@us.ibm.com>
> > > Cc: Peter Zijlstra <peterz@infradead.org>
> > > Cc: Stephane Eranian <eranian@google.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@redhat.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 Carvalho de Melo May 20, 2020, 2:34 a.m. UTC | #4
Em Tue, May 19, 2020 at 10:15:48PM -0300, Arnaldo Carvalho de Melo escreveu:
> 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
> > 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.

What I have is in my tmp.perf/core branch, this and the hashmap.h
__WORDSIZE fixups, with those all the alpine Linux containers, that use
Musl libc passed, waiting for the remaining 80 other containers to
finish, now lots of these containers build with and without NO_LIBBPF=1
to make sure we test both variants,

- Arnaldo
diff mbox series

Patch

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;
+}
+
+static int check_parse_id(const char *id, bool same_cpu, struct pmu_event *pe)
+{
+	struct parse_events_error error;
+	struct evlist *evlist;
+	int ret;
+
+	/* Numbers are always valid. */
+	if (is_number(id))
+		return 0;
+
+	evlist = evlist__new();
+	memset(&error, 0, sizeof(error));
+	ret = parse_events(evlist, id, &error);
+	if (ret && same_cpu) {
+		pr_warning("Parse event failed metric '%s' id '%s' expr '%s'\n",
+			pe->metric_name, id, pe->metric_expr);
+		pr_warning("Error string '%s' help '%s'\n", error.str,
+			error.help);
+	} else if (ret) {
+		pr_debug3("Parse event failed, but for an event that may not be supported by this CPU.\nid '%s' metric '%s' expr '%s'\n",
+			  id, pe->metric_name, pe->metric_expr);
+		ret = 0;
+	}
+	evlist__delete(evlist);
+	free(error.str);
+	free(error.help);
+	free(error.first_str);
+	free(error.first_help);
+	return ret;
+}
+
+static void expr_failure(const char *msg,
+			 const struct pmu_events_map *map,
+			 const struct pmu_event *pe)
+{
+	pr_debug("%s for map %s %s %s\n",
+		msg, map->cpuid, map->version, map->type);
+	pr_debug("On metric %s\n", pe->metric_name);
+	pr_debug("On expression %s\n", pe->metric_expr);
+}
+
+static int test_parsing(void)
+{
+	struct pmu_events_map *cpus_map = perf_pmu__find_map(NULL);
+	struct pmu_events_map *map;
+	struct pmu_event *pe;
+	int i, j, k;
+	const char **ids;
+	int idnum;
+	int ret = 0;
+	struct expr_parse_ctx ctx;
+	double result;
+
+	i = 0;
+	for (;;) {
+		map = &pmu_events_map[i++];
+		if (!map->table)
+			break;
+		j = 0;
+		for (;;) {
+			pe = &map->table[j++];
+			if (!pe->name && !pe->metric_group && !pe->metric_name)
+				break;
+			if (!pe->metric_expr)
+				continue;
+			if (expr__find_other(pe->metric_expr, NULL,
+						&ids, &idnum, 0) < 0) {
+				expr_failure("Parse other failed", map, pe);
+				ret++;
+				continue;
+			}
+			expr__ctx_init(&ctx);
+
+			/*
+			 * Add all ids with a made up value. The value may
+			 * trigger divide by zero when subtracted and so try to
+			 * make them unique.
+			 */
+			for (k = 0; k < idnum; k++)
+				expr__add_id(&ctx, ids[k], k + 1);
+
+			for (k = 0; k < idnum; k++) {
+				if (check_parse_id(ids[k], map == cpus_map, pe))
+					ret++;
+			}
+
+			if (expr__parse(&result, &ctx, pe->metric_expr, 0)) {
+				expr_failure("Parse failed", map, pe);
+				ret++;
+			}
+			for (k = 0; k < idnum; k++)
+				zfree(&ids[k]);
+			free(ids);
+		}
+	}
+	/* TODO: fail when not ok */
+	return ret == 0 ? TEST_OK : TEST_SKIP;
+}
+
+static const struct {
+	int (*func)(void);
+	const char *desc;
+} pmu_events_testcase_table[] = {
+	{
+		.func = test_pmu_event_table,
+		.desc = "PMU event table sanity",
+	},
+	{
+		.func = test_aliases,
+		.desc = "PMU event map aliases",
+	},
+	{
+		.func = test_parsing,
+		.desc = "Parsing of PMU event table metrics",
+	},
+};
+
+const char *test__pmu_events_subtest_get_desc(int subtest)
+{
+	if (subtest < 0 ||
+	    subtest >= (int)ARRAY_SIZE(pmu_events_testcase_table))
+		return NULL;
+	return pmu_events_testcase_table[subtest].desc;
+}
+
+const char *test__pmu_events_subtest_skip_reason(int subtest)
+{
+	if (subtest < 0 ||
+	    subtest >= (int)ARRAY_SIZE(pmu_events_testcase_table))
+		return NULL;
+	if (pmu_events_testcase_table[subtest].func != test_parsing)
+		return NULL;
+	return "some metrics failed";
+}
+
+int test__pmu_events_subtest_get_nr(void)
+{
+	return (int)ARRAY_SIZE(pmu_events_testcase_table);
+}
+
+int test__pmu_events(struct test *test __maybe_unused, int subtest)
+{
+	if (subtest < 0 ||
+	    subtest >= (int)ARRAY_SIZE(pmu_events_testcase_table))
+		return TEST_FAIL;
+	return pmu_events_testcase_table[subtest].func();
+}
diff --git a/tools/perf/tests/tests.h b/tools/perf/tests/tests.h
index 88e45aeab94f..6c6c4b6a4796 100644
--- a/tools/perf/tests/tests.h
+++ b/tools/perf/tests/tests.h
@@ -51,6 +51,9 @@  int test__perf_evsel__tp_sched_test(struct test *test, int subtest);
 int test__syscall_openat_tp_fields(struct test *test, int subtest);
 int test__pmu(struct test *test, int subtest);
 int test__pmu_events(struct test *test, int subtest);
+const char *test__pmu_events_subtest_get_desc(int subtest);
+const char *test__pmu_events_subtest_skip_reason(int subtest);
+int test__pmu_events_subtest_get_nr(void);
 int test__attr(struct test *test, int subtest);
 int test__dso_data(struct test *test, int subtest);
 int test__dso_data_cache(struct test *test, int subtest);