[PATCH 1/2] perf expr: Test parsing of floating point numbers

[PATCH 1/2] perf expr: Test parsing of floating point numbers

[Ian Rogers]

Add test for fix in:
commit 5741da3dee4c ("perf expr: Parse numbers as doubles")

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/tests/expr.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tools/perf/tests/expr.c b/tools/perf/tests/expr.c
index f9e8e5628836..3f742612776a 100644
--- a/tools/perf/tests/expr.c
+++ b/tools/perf/tests/expr.c
@@ -39,6 +39,7 @@ int test__expr(struct test *t __maybe_unused, int subtest __maybe_unused)
 	ret |= test(&ctx, "min(1,2) + 1", 2);
 	ret |= test(&ctx, "max(1,2) + 1", 3);
 	ret |= test(&ctx, "1+1 if 3*4 else 0", 2);
+	ret |= test(&ctx, "1.1 + 2.1", 3.2);
 
 	if (ret)
 		return ret;
-- 
2.26.2.645.ge9eca65c58-goog

[PATCH 2/2] perf test: Improve pmu event metric testing

[Ian Rogers]

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, failing if
metrics for the current architecture fail to parse.

Tested on power9, skylakex, haswell, broadwell, westmere, sandybridge and
ivybridge with the patch set in place.
May fail on other architectures if metrics are invalid. In particular s390
is untested, but its expressions are trivial. The event encodings could
be wrong. The other untested architectures with expressions are power8,
cascadelakex, tremontx, skylake, jaketown, ivytown and variants of
haswell and broadwell. For these the expression encoding is valid but
the event encodings may not be.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/tests/builtin-test.c |   5 +
 tools/perf/tests/pmu-events.c   | 158 ++++++++++++++++++++++++++++++--
 tools/perf/tests/tests.h        |   2 +
 3 files changed, 159 insertions(+), 6 deletions(-)

diff --git a/tools/perf/tests/builtin-test.c b/tools/perf/tests/builtin-test.c
index 3471ec52ea11..8147c17c71ab 100644
--- a/tools/perf/tests/builtin-test.c
+++ b/tools/perf/tests/builtin-test.c
@@ -75,6 +75,11 @@ static struct test generic_tests[] = {
 	{
 		.desc = "PMU events",
 		.func = test__pmu_events,
+		.subtest = {
+			.get_nr		= test__pmu_events_subtest_get_nr,
+			.get_desc	= test__pmu_events_subtest_get_desc,
+		},
+
 	},
 	{
 		.desc = "DSO data read",
diff --git a/tools/perf/tests/pmu-events.c b/tools/perf/tests/pmu-events.c
index d64261da8bf7..c18b9ce8cace 100644
--- a/tools/perf/tests/pmu-events.c
+++ b/tools/perf/tests/pmu-events.c
@@ -8,6 +8,10 @@
 #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"
+#include <ctype.h>
 
 struct perf_pmu_test_event {
 	struct pmu_event event;
@@ -144,7 +148,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 +351,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 +378,148 @@ int test__pmu_events(struct test *test __maybe_unused,
 
 	return 0;
 }
+
+static bool is_number(const char *str)
+{
+	size_t i;
+
+	for (i = 0; i < strlen(str); i++) {
+		if (!isdigit(str[i]) && str[i] != '.')
+			return false;
+	}
+	return true;
+}
+
+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) {
+		fprintf(stderr,
+			"\nWARNING: Parse event failed metric '%s' id '%s' expr '%s'\n",
+			pe->metric_name, id, pe->metric_expr);
+		fprintf(stderr, "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);
+	}
+	evlist__delete(evlist);
+	free(error.str);
+	free(error.help);
+	free(error.first_str);
+	free(error.first_help);
+	/* TODO: too many metrics are broken to fail on this test currently. */
+	return 0;
+}
+
+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) {
+			map = NULL;
+			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) {
+				pr_debug("Parse other failed for map %s %s %s\n",
+					map->cpuid, map->version, map->type);
+				pr_debug("On metric %s\n", pe->metric_name);
+				pr_debug("On expression %s\n", pe->metric_expr);
+				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)) {
+				pr_debug("Parse failed for map %s %s %s\n",
+					map->cpuid, map->version, map->type);
+				pr_debug("On metric %s\n", pe->metric_name);
+				pr_debug("On expression %s\n", pe->metric_expr);
+				ret++;
+			}
+			for (k = 0; k < idnum; k++)
+				zfree(&ids[k]);
+			free(ids);
+		}
+	}
+	return ret;
+}
+
+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 i)
+{
+	if (i < 0 || i >= (int)ARRAY_SIZE(pmu_events_testcase_table))
+		return NULL;
+	return pmu_events_testcase_table[i].desc;
+}
+
+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 i)
+{
+	if (i < 0 || i >= (int)ARRAY_SIZE(pmu_events_testcase_table))
+		return TEST_FAIL;
+	return pmu_events_testcase_table[i].func();
+}
diff --git a/tools/perf/tests/tests.h b/tools/perf/tests/tests.h
index d6d4ac34eeb7..8e316c30ed3c 100644
--- a/tools/perf/tests/tests.h
+++ b/tools/perf/tests/tests.h
@@ -50,6 +50,8 @@ 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);
+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);
-- 
2.26.2.645.ge9eca65c58-goog

Re: [PATCH 2/2] perf test: Improve pmu event metric testing

[Ian Rogers]

On Tue, May 12, 2020 at 11:22 PM Ian Rogers <irogers@google.com> wrote:
>
> 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, failing if
> metrics for the current architecture fail to parse.
>
> Tested on power9, skylakex, haswell, broadwell, westmere, sandybridge and
> ivybridge with the patch set in place.
> May fail on other architectures if metrics are invalid. In particular s390
> is untested, but its expressions are trivial. The event encodings could
> be wrong. The other untested architectures with expressions are power8,
> cascadelakex, tremontx, skylake, jaketown, ivytown and variants of
> haswell and broadwell. For these the expression encoding is valid but
> the event encodings may not be.

Sorry, this commit message needed updating to reflect that event
parsing errors are no longer fatal. Fixed in v2:
https://lore.kernel.org/lkml/20200513062752.3681-1-irogers@google.com/T/#t

Thanks,
Ian

> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  tools/perf/tests/builtin-test.c |   5 +
>  tools/perf/tests/pmu-events.c   | 158 ++++++++++++++++++++++++++++++--
>  tools/perf/tests/tests.h        |   2 +
>  3 files changed, 159 insertions(+), 6 deletions(-)
>
> diff --git a/tools/perf/tests/builtin-test.c b/tools/perf/tests/builtin-test.c
> index 3471ec52ea11..8147c17c71ab 100644
> --- a/tools/perf/tests/builtin-test.c
> +++ b/tools/perf/tests/builtin-test.c
> @@ -75,6 +75,11 @@ static struct test generic_tests[] = {
>         {
>                 .desc = "PMU events",
>                 .func = test__pmu_events,
> +               .subtest = {
> +                       .get_nr         = test__pmu_events_subtest_get_nr,
> +                       .get_desc       = test__pmu_events_subtest_get_desc,
> +               },
> +
>         },
>         {
>                 .desc = "DSO data read",
> diff --git a/tools/perf/tests/pmu-events.c b/tools/perf/tests/pmu-events.c
> index d64261da8bf7..c18b9ce8cace 100644
> --- a/tools/perf/tests/pmu-events.c
> +++ b/tools/perf/tests/pmu-events.c
> @@ -8,6 +8,10 @@
>  #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"
> +#include <ctype.h>
>
>  struct perf_pmu_test_event {
>         struct pmu_event event;
> @@ -144,7 +148,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 +351,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 +378,148 @@ int test__pmu_events(struct test *test __maybe_unused,
>
>         return 0;
>  }
> +
> +static bool is_number(const char *str)
> +{
> +       size_t i;
> +
> +       for (i = 0; i < strlen(str); i++) {
> +               if (!isdigit(str[i]) && str[i] != '.')
> +                       return false;
> +       }
> +       return true;
> +}
> +
> +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) {
> +               fprintf(stderr,
> +                       "\nWARNING: Parse event failed metric '%s' id '%s' expr '%s'\n",
> +                       pe->metric_name, id, pe->metric_expr);
> +               fprintf(stderr, "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);
> +       }
> +       evlist__delete(evlist);
> +       free(error.str);
> +       free(error.help);
> +       free(error.first_str);
> +       free(error.first_help);
> +       /* TODO: too many metrics are broken to fail on this test currently. */
> +       return 0;
> +}
> +
> +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) {
> +                       map = NULL;
> +                       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) {
> +                               pr_debug("Parse other failed for map %s %s %s\n",
> +                                       map->cpuid, map->version, map->type);
> +                               pr_debug("On metric %s\n", pe->metric_name);
> +                               pr_debug("On expression %s\n", pe->metric_expr);
> +                               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)) {
> +                               pr_debug("Parse failed for map %s %s %s\n",
> +                                       map->cpuid, map->version, map->type);
> +                               pr_debug("On metric %s\n", pe->metric_name);
> +                               pr_debug("On expression %s\n", pe->metric_expr);
> +                               ret++;
> +                       }
> +                       for (k = 0; k < idnum; k++)
> +                               zfree(&ids[k]);
> +                       free(ids);
> +               }
> +       }
> +       return ret;
> +}
> +
> +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 i)
> +{
> +       if (i < 0 || i >= (int)ARRAY_SIZE(pmu_events_testcase_table))
> +               return NULL;
> +       return pmu_events_testcase_table[i].desc;
> +}
> +
> +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 i)
> +{
> +       if (i < 0 || i >= (int)ARRAY_SIZE(pmu_events_testcase_table))
> +               return TEST_FAIL;
> +       return pmu_events_testcase_table[i].func();
> +}
> diff --git a/tools/perf/tests/tests.h b/tools/perf/tests/tests.h
> index d6d4ac34eeb7..8e316c30ed3c 100644
> --- a/tools/perf/tests/tests.h
> +++ b/tools/perf/tests/tests.h
> @@ -50,6 +50,8 @@ 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);
> +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);
> --
> 2.26.2.645.ge9eca65c58-goog
>

Re: [PATCH 2/2] perf test: Improve pmu event metric testing

[John Garry]

On 13/05/2020 07:22, Ian Rogers wrote:
> 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, failing if
> metrics for the current architecture fail to parse.
> 
> Tested on power9, skylakex, haswell, broadwell, westmere, sandybridge and
> ivybridge with the patch set in place.
> May fail on other architectures if metrics are invalid. In particular s390
> is untested, but its expressions are trivial. The event encodings could
> be wrong. The other untested architectures with expressions are power8,
> cascadelakex, tremontx, skylake, jaketown, ivytown and variants of
> haswell and broadwell. For these the expression encoding is valid but
> the event encodings may not be.
> 

Out of interest, if we could move the validation of metrics to jevents, 
how much functionality would we still have here?

> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>   tools/perf/tests/builtin-test.c |   5 +
>   tools/perf/tests/pmu-events.c   | 158 ++++++++++++++++++++++++++++++--
>   tools/perf/tests/tests.h        |   2 +
>   3 files changed, 159 insertions(+), 6 deletions(-)
> 
> diff --git a/tools/perf/tests/builtin-test.c b/tools/perf/tests/builtin-test.c
> index 3471ec52ea11..8147c17c71ab 100644
> --- a/tools/perf/tests/builtin-test.c
> +++ b/tools/perf/tests/builtin-test.c
> @@ -75,6 +75,11 @@ static struct test generic_tests[] = {
>   	{
>   		.desc = "PMU events",
>   		.func = test__pmu_events,
> +		.subtest = {
> +			.get_nr		= test__pmu_events_subtest_get_nr,
> +			.get_desc	= test__pmu_events_subtest_get_desc,
> +		},
> +
>   	},
>   	{
>   		.desc = "DSO data read",
> diff --git a/tools/perf/tests/pmu-events.c b/tools/perf/tests/pmu-events.c
> index d64261da8bf7..c18b9ce8cace 100644
> --- a/tools/perf/tests/pmu-events.c
> +++ b/tools/perf/tests/pmu-events.c
> @@ -8,6 +8,10 @@
>   #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"
> +#include <ctype.h>
>   
>   struct perf_pmu_test_event {
>   	struct pmu_event event;
> @@ -144,7 +148,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 +351,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 +378,148 @@ int test__pmu_events(struct test *test __maybe_unused,
>   
>   	return 0;
>   }
> +
> +static bool is_number(const char *str)
> +{
> +	size_t i;
> +
> +	for (i = 0; i < strlen(str); i++) {
> +		if (!isdigit(str[i]) && str[i] != '.')
> +			return false;
> +	}
> +	return true;
> +}

if there is no helper already for this, is this really the best place to 
put it? Maybe others will need it in future.

> +
> +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) {
> +		fprintf(stderr,
> +			"\nWARNING: Parse event failed metric '%s' id '%s' expr '%s'\n",
> +			pe->metric_name, id, pe->metric_expr);
> +		fprintf(stderr, "Error string '%s' help '%s'\n",
> +			error.str, error.help);

do you really need to spill lines like this?

> +	} 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);
> +	}
> +	evlist__delete(evlist);
> +	free(error.str);
> +	free(error.help);
> +	free(error.first_str);
> +	free(error.first_help);
> +	/* TODO: too many metrics are broken to fail on this test currently. */
> +	return 0;
> +}
> +
> +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) {
> +			map = NULL;
> +			break;
> +		}
> +		j = 0;
> +		for (;;) {
> +			pe = &map->table[j++];
> +			if (!pe->name && !pe->metric_group && !pe->metric_name)
> +				break;
> +			if (!pe->metric_expr)
> +				continue;

we could probably make a for_each_metric_helper(), as this code seems 
duplicated a few times (like metricgroup__add_netric())

> +			if (expr__find_other(pe->metric_expr, NULL,
> +						&ids, &idnum, 0) < 0) {
> +				pr_debug("Parse other failed for map %s %s %s\n",
> +					map->cpuid, map->version, map->type);
> +				pr_debug("On metric %s\n", pe->metric_name);
> +				pr_debug("On expression %s\n", pe->metric_expr);
> +				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)) {
> +				pr_debug("Parse failed for map %s %s %s\n",
> +					map->cpuid, map->version, map->type);
> +				pr_debug("On metric %s\n", pe->metric_name);
> +				pr_debug("On expression %s\n", pe->metric_expr);

this code could be factored out a lot, see about 20 lines above

> +				ret++;
> +			}
> +			for (k = 0; k < idnum; k++)
> +				zfree(&ids[k]);
> +			free(ids);
> +		}
> +	}
> +	return ret;
> +}
> +
> +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",
> +	},
> +};

I assume "perf test 10" will run all testcases, right?

> +
> +const char *test__pmu_events_subtest_get_desc(int i)
> +{
> +	if (i < 0 || i >= (int)ARRAY_SIZE(pmu_events_testcase_table))
> +		return NULL;
> +	return pmu_events_testcase_table[i].desc;
> +}
> +
> +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 i)
> +{
> +	if (i < 0 || i >= (int)ARRAY_SIZE(pmu_events_testcase_table))
> +		return TEST_FAIL;
> +	return pmu_events_testcase_table[i].func();
> +}
> diff --git a/tools/perf/tests/tests.h b/tools/perf/tests/tests.h
> index d6d4ac34eeb7..8e316c30ed3c 100644
> --- a/tools/perf/tests/tests.h
> +++ b/tools/perf/tests/tests.h
> @@ -50,6 +50,8 @@ 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);
> +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);
> 

Re: [PATCH 2/2] perf test: Improve pmu event metric testing

[Ian Rogers]

On Wed, May 13, 2020 at 8:26 AM John Garry <john.garry@huawei.com> wrote:
>
> On 13/05/2020 07:22, Ian Rogers wrote:
> > 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, failing if
> > metrics for the current architecture fail to parse.
> >
> > Tested on power9, skylakex, haswell, broadwell, westmere, sandybridge and
> > ivybridge with the patch set in place.
> > May fail on other architectures if metrics are invalid. In particular s390
> > is untested, but its expressions are trivial. The event encodings could
> > be wrong. The other untested architectures with expressions are power8,
> > cascadelakex, tremontx, skylake, jaketown, ivytown and variants of
> > haswell and broadwell. For these the expression encoding is valid but
> > the event encodings may not be.
> >
>
> Out of interest, if we could move the validation of metrics to jevents,
> how much functionality would we still have here?

If we add checking to jevents then the MetricExpr would be known to be
valid, however, the events (aka ids) within the expression could be
invalid. I'm not sure we could realistically check the events at
jevents (build) time as there is no guarantee that the machine we run
on is the same as the one we compile on.

> > Signed-off-by: Ian Rogers <irogers@google.com>
> > ---
> >   tools/perf/tests/builtin-test.c |   5 +
> >   tools/perf/tests/pmu-events.c   | 158 ++++++++++++++++++++++++++++++--
> >   tools/perf/tests/tests.h        |   2 +
> >   3 files changed, 159 insertions(+), 6 deletions(-)
> >
> > diff --git a/tools/perf/tests/builtin-test.c b/tools/perf/tests/builtin-test.c
> > index 3471ec52ea11..8147c17c71ab 100644
> > --- a/tools/perf/tests/builtin-test.c
> > +++ b/tools/perf/tests/builtin-test.c
> > @@ -75,6 +75,11 @@ static struct test generic_tests[] = {
> >       {
> >               .desc = "PMU events",
> >               .func = test__pmu_events,
> > +             .subtest = {
> > +                     .get_nr         = test__pmu_events_subtest_get_nr,
> > +                     .get_desc       = test__pmu_events_subtest_get_desc,
> > +             },
> > +
> >       },
> >       {
> >               .desc = "DSO data read",
> > diff --git a/tools/perf/tests/pmu-events.c b/tools/perf/tests/pmu-events.c
> > index d64261da8bf7..c18b9ce8cace 100644
> > --- a/tools/perf/tests/pmu-events.c
> > +++ b/tools/perf/tests/pmu-events.c
> > @@ -8,6 +8,10 @@
> >   #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"
> > +#include <ctype.h>
> >
> >   struct perf_pmu_test_event {
> >       struct pmu_event event;
> > @@ -144,7 +148,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 +351,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 +378,148 @@ int test__pmu_events(struct test *test __maybe_unused,
> >
> >       return 0;
> >   }
> > +
> > +static bool is_number(const char *str)
> > +{
> > +     size_t i;
> > +
> > +     for (i = 0; i < strlen(str); i++) {
> > +             if (!isdigit(str[i]) && str[i] != '.')
> > +                     return false;
> > +     }
> > +     return true;
> > +}
>
> if there is no helper already for this, is this really the best place to
> put it? Maybe others will need it in future.

Agreed, I don't mind suggestions to put it somewhere else but making
it non-hidden here wouldn't make sense. I did look for similar
functionality that already existed but didn't find it.

> > +
> > +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) {
> > +             fprintf(stderr,
> > +                     "\nWARNING: Parse event failed metric '%s' id '%s' expr '%s'\n",
> > +                     pe->metric_name, id, pe->metric_expr);
> > +             fprintf(stderr, "Error string '%s' help '%s'\n",
> > +                     error.str, error.help);
>
> do you really need to spill lines like this?

Yeah, I've installed the "kernel" c-style into my emacs and I get
this. I do manually fix indentation often, but I suspect I was fight
80 columns and checkpatch.pl here (outside of "kernel" development
clang-format rocks my world as it puts an end to any bike shedding -
if you don't like the layout fix the format file).

; Kernel formatting, see:
; https://www.kernel.org/doc/html/v4.10/process/coding-style.html
(defun c-lineup-arglist-tabs-only (ignored)
  "Line up argument lists by tabs, not spaces"
  (let* ((anchor (c-langelem-pos c-syntactic-element))
         (column (c-langelem-2nd-pos c-syntactic-element))
         (offset (- (1+ column) anchor))
         (steps (floor offset c-basic-offset)))
    (* (max steps 1)
       c-basic-offset)))

(add-hook 'c-mode-common-hook
          (lambda ()
            ;; Add kernel style
            (c-add-style
             "linux-tabs-only"
             '("linux" (c-offsets-alist
                        (arglist-cont-nonempty
                         c-lineup-gcc-asm-reg
                         c-lineup-arglist-tabs-only))))))

(add-hook 'c-mode-hook
          (lambda ()
            (let ((filename (buffer-file-name)))
              ;; Enable kernel mode for the appropriate files
              (when (and filename
                         (string-match (expand-file-name "~/kernel-trees")
                                       filename))
                (setq indent-tabs-mode t)
                (setq show-trailing-whitespace t)
                (c-set-style "linux-tabs-only")))))

> > +     } 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);
> > +     }
> > +     evlist__delete(evlist);
> > +     free(error.str);
> > +     free(error.help);
> > +     free(error.first_str);
> > +     free(error.first_help);
> > +     /* TODO: too many metrics are broken to fail on this test currently. */
> > +     return 0;
> > +}
> > +
> > +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) {
> > +                     map = NULL;
> > +                     break;
> > +             }
> > +             j = 0;
> > +             for (;;) {
> > +                     pe = &map->table[j++];
> > +                     if (!pe->name && !pe->metric_group && !pe->metric_name)
> > +                             break;
> > +                     if (!pe->metric_expr)
> > +                             continue;
>
> we could probably make a for_each_metric_helper(), as this code seems
> duplicated a few times (like metricgroup__add_netric())

Sounds like useful follow-up clean up.

> > +                     if (expr__find_other(pe->metric_expr, NULL,
> > +                                             &ids, &idnum, 0) < 0) {
> > +                             pr_debug("Parse other failed for map %s %s %s\n",
> > +                                     map->cpuid, map->version, map->type);
> > +                             pr_debug("On metric %s\n", pe->metric_name);
> > +                             pr_debug("On expression %s\n", pe->metric_expr);
> > +                             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)) {
> > +                             pr_debug("Parse failed for map %s %s %s\n",
> > +                                     map->cpuid, map->version, map->type);
> > +                             pr_debug("On metric %s\n", pe->metric_name);
> > +                             pr_debug("On expression %s\n", pe->metric_expr);
>
> this code could be factored out a lot, see about 20 lines above

Ack.

> > +                             ret++;
> > +                     }
> > +                     for (k = 0; k < idnum; k++)
> > +                             zfree(&ids[k]);
> > +                     free(ids);
> > +             }
> > +     }
> > +     return ret;
> > +}
> > +
> > +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",
> > +     },
> > +};
>
> I assume "perf test 10" will run all testcases, right?

Yes. Doing it this way breaks out which test fails.

> > +
> > +const char *test__pmu_events_subtest_get_desc(int i)
> > +{
> > +     if (i < 0 || i >= (int)ARRAY_SIZE(pmu_events_testcase_table))
> > +             return NULL;
> > +     return pmu_events_testcase_table[i].desc;
> > +}
> > +
> > +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 i)
> > +{
> > +     if (i < 0 || i >= (int)ARRAY_SIZE(pmu_events_testcase_table))
> > +             return TEST_FAIL;
> > +     return pmu_events_testcase_table[i].func();
> > +}
> > diff --git a/tools/perf/tests/tests.h b/tools/perf/tests/tests.h
> > index d6d4ac34eeb7..8e316c30ed3c 100644
> > --- a/tools/perf/tests/tests.h
> > +++ b/tools/perf/tests/tests.h
> > @@ -50,6 +50,8 @@ 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);
> > +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);
> >
>

Re: [PATCH 2/2] perf test: Improve pmu event metric testing

[Jiri Olsa]

On Tue, May 12, 2020 at 11:22:36PM -0700, Ian Rogers wrote:

SNIP

> +
> +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) {
> +		fprintf(stderr,
> +			"\nWARNING: Parse event failed metric '%s' id '%s' expr '%s'\n",
> +			pe->metric_name, id, pe->metric_expr);
> +		fprintf(stderr, "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);
> +	}

I wonder if we could add 'fake pmu' that would be used for tests
and use it parse_events_add_pmu to add 'fake' evsel but the name
would be correct.. we could add parse_events_state::fake_pmu bool
for that

rest of the event types (other than pmu syntax) might be ok


> +	evlist__delete(evlist);
> +	free(error.str);
> +	free(error.help);
> +	free(error.first_str);
> +	free(error.first_help);
> +	/* TODO: too many metrics are broken to fail on this test currently. */
> +	return 0;
> +}
> +
> +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) {
> +			map = NULL;

hum, what's the map = NULL for in here?

thanks,
jirka

> +			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) {
> +				pr_debug("Parse other failed for map %s %s %s\n",
> +					map->cpuid, map->version, map->type);
> +				pr_debug("On metric %s\n", pe->metric_name);
> +				pr_debug("On expression %s\n", pe->metric_expr);
> +				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)) {
> +				pr_debug("Parse failed for map %s %s %s\n",
> +					map->cpuid, map->version, map->type);
> +				pr_debug("On metric %s\n", pe->metric_name);
> +				pr_debug("On expression %s\n", pe->metric_expr);
> +				ret++;
> +			}
> +			for (k = 0; k < idnum; k++)
> +				zfree(&ids[k]);
> +			free(ids);
> +		}
> +	}
> +	return ret;

SNIP

Re: [PATCH 2/2] perf test: Improve pmu event metric testing

[John Garry]

On 13/05/2020 17:10, Ian Rogers wrote:
>> Out of interest, if we could move the validation of metrics to jevents,
>> how much functionality would we still have here?
> If we add checking to jevents then the MetricExpr would be known to be
> valid, however, the events (aka ids) within the expression could be
> invalid.

So I think that has some value. I mean, just to detect syntax errors, 
like those remedied in "perf metrics: fix parse errors in power8 metrics".

> I'm not sure we could realistically check the events at
> jevents (build) time as there is no guarantee that the machine we run
> on is the same as the one we compile on.

But we could at least check that there are event aliases for that CPU, 
right? (by examining the JSONs for that cpu). If the event alias does 
not actually match on the target CPU, then that can't be helped.

Cheers,
John

Re: [PATCH 2/2] perf test: Improve pmu event metric testing

[Ian Rogers]

On Thu, May 14, 2020 at 2:00 AM John Garry <john.garry@huawei.com> wrote:
>
> On 13/05/2020 17:10, Ian Rogers wrote:
> >> Out of interest, if we could move the validation of metrics to jevents,
> >> how much functionality would we still have here?
> > If we add checking to jevents then the MetricExpr would be known to be
> > valid, however, the events (aka ids) within the expression could be
> > invalid.
>
> So I think that has some value. I mean, just to detect syntax errors,
> like those remedied in "perf metrics: fix parse errors in power8 metrics".
>
> > I'm not sure we could realistically check the events at
> > jevents (build) time as there is no guarantee that the machine we run
> > on is the same as the one we compile on.
>
> But we could at least check that there are event aliases for that CPU,
> right? (by examining the JSONs for that cpu). If the event alias does
> not actually match on the target CPU, then that can't be helped.

Agreed, I think there will be some cases where something more can be
done. Jiri has proposed fake pmus as well:
https://www.spinics.net/lists/linux-perf-users/msg11760.html
I don't know how much sense it makes trying to get this in jevents, as
long as 'perf test' is run.

Thanks,
Ian

> Cheers,
> John
>

Re: [PATCH 2/2] perf test: Improve pmu event metric testing

[John Garry]

On 15/05/2020 00:02, Ian Rogers wrote:
> On Thu, May 14, 2020 at 2:00 AM John Garry <john.garry@huawei.com> wrote:
>>
>> On 13/05/2020 17:10, Ian Rogers wrote:
>>>> Out of interest, if we could move the validation of metrics to jevents,
>>>> how much functionality would we still have here?
>>> If we add checking to jevents then the MetricExpr would be known to be
>>> valid, however, the events (aka ids) within the expression could be
>>> invalid.
>>
>> So I think that has some value. I mean, just to detect syntax errors,
>> like those remedied in "perf metrics: fix parse errors in power8 metrics".
>>
>>> I'm not sure we could realistically check the events at
>>> jevents (build) time as there is no guarantee that the machine we run
>>> on is the same as the one we compile on.
>>
>> But we could at least check that there are event aliases for that CPU,
>> right? (by examining the JSONs for that cpu). If the event alias does
>> not actually match on the target CPU, then that can't be helped.
> 
> Agreed, I think there will be some cases where something more can be
> done. Jiri has proposed fake pmus as well:
> https://www.spinics.net/lists/linux-perf-users/msg11760.html
> I don't know how much sense it makes trying to get this in jevents, as
> long as 'perf test' is run.

At a glance, that does not look like something we would want in jevents. 
But rather the metric expr parsing error detection and alias checking.

About jirka's patch:

--- a/tools/perf/tests/pmu-events.c
+++ b/tools/perf/tests/pmu-events.c
@@ -485,6 +485,102 @@ static int test_parsing(void)
  	return ret == 0 ? TEST_OK : TEST_SKIP;
  }

+
+static struct test_metric metrics[] = {
+	{ .metric = "imx8_ddr0@read\\-cycles@ * 4 * 4", },
+	{ .metric = 
"imx8_ddr0@axid\\-read\\,axi_mask\\=0xffff\\,axi_id\\=0x0000@ * 4", },
+	{ .metric = "(cstate_pkg@c2\\-residency@ / msr@tsc@) * 100", },
+	{ .metric = "(imx8_ddr0@read\\-cycles@ + imx8_ddr0@write\\-cycles@)", },
+};

Maybe we could add these to pmu-events/arch/test/test_cpu/metric.json, 
and get at them that way.

Thanks,
John

Re: [PATCH 2/2] perf test: Improve pmu event metric testing

[Jiri Olsa]

On Fri, May 15, 2020 at 10:09:10AM +0100, John Garry wrote:
> On 15/05/2020 00:02, Ian Rogers wrote:
> > On Thu, May 14, 2020 at 2:00 AM John Garry <john.garry@huawei.com> wrote:
> > > 
> > > On 13/05/2020 17:10, Ian Rogers wrote:
> > > > > Out of interest, if we could move the validation of metrics to jevents,
> > > > > how much functionality would we still have here?
> > > > If we add checking to jevents then the MetricExpr would be known to be
> > > > valid, however, the events (aka ids) within the expression could be
> > > > invalid.
> > > 
> > > So I think that has some value. I mean, just to detect syntax errors,
> > > like those remedied in "perf metrics: fix parse errors in power8 metrics".
> > > 
> > > > I'm not sure we could realistically check the events at
> > > > jevents (build) time as there is no guarantee that the machine we run
> > > > on is the same as the one we compile on.
> > > 
> > > But we could at least check that there are event aliases for that CPU,
> > > right? (by examining the JSONs for that cpu). If the event alias does
> > > not actually match on the target CPU, then that can't be helped.
> > 
> > Agreed, I think there will be some cases where something more can be
> > done. Jiri has proposed fake pmus as well:
> > https://www.spinics.net/lists/linux-perf-users/msg11760.html
> > I don't know how much sense it makes trying to get this in jevents, as
> > long as 'perf test' is run.
> 
> At a glance, that does not look like something we would want in jevents. But
> rather the metric expr parsing error detection and alias checking.
> 
> About jirka's patch:
> 
> --- a/tools/perf/tests/pmu-events.c
> +++ b/tools/perf/tests/pmu-events.c
> @@ -485,6 +485,102 @@ static int test_parsing(void)
>  	return ret == 0 ? TEST_OK : TEST_SKIP;
>  }
> 
> +
> +static struct test_metric metrics[] = {
> +	{ .metric = "imx8_ddr0@read\\-cycles@ * 4 * 4", },
> +	{ .metric = "imx8_ddr0@axid\\-read\\,axi_mask\\=0xffff\\,axi_id\\=0x0000@
> * 4", },
> +	{ .metric = "(cstate_pkg@c2\\-residency@ / msr@tsc@) * 100", },
> +	{ .metric = "(imx8_ddr0@read\\-cycles@ + imx8_ddr0@write\\-cycles@)", },
> +};
> 
> Maybe we could add these to pmu-events/arch/test/test_cpu/metric.json, and
> get at them that way.

that test sets the 'fake pmu' stuff.. could we do that in your test?

jirka

Re: [PATCH 2/2] perf test: Improve pmu event metric testing

[John Garry]

On 15/05/2020 12:48, Jiri Olsa wrote:
> On Fri, May 15, 2020 at 10:09:10AM +0100, John Garry wrote:
>> On 15/05/2020 00:02, Ian Rogers wrote:
>>> On Thu, May 14, 2020 at 2:00 AM John Garry <john.garry@huawei.com> wrote:
>>>>
>>>> On 13/05/2020 17:10, Ian Rogers wrote:
>>>>>> Out of interest, if we could move the validation of metrics to jevents,
>>>>>> how much functionality would we still have here?
>>>>> If we add checking to jevents then the MetricExpr would be known to be
>>>>> valid, however, the events (aka ids) within the expression could be
>>>>> invalid.
>>>>
>>>> So I think that has some value. I mean, just to detect syntax errors,
>>>> like those remedied in "perf metrics: fix parse errors in power8 metrics".
>>>>
>>>>> I'm not sure we could realistically check the events at
>>>>> jevents (build) time as there is no guarantee that the machine we run
>>>>> on is the same as the one we compile on.
>>>>
>>>> But we could at least check that there are event aliases for that CPU,
>>>> right? (by examining the JSONs for that cpu). If the event alias does
>>>> not actually match on the target CPU, then that can't be helped.
>>>
>>> Agreed, I think there will be some cases where something more can be
>>> done. Jiri has proposed fake pmus as well:
>>> https://www.spinics.net/lists/linux-perf-users/msg11760.html
>>> I don't know how much sense it makes trying to get this in jevents, as
>>> long as 'perf test' is run.
>>
>> At a glance, that does not look like something we would want in jevents. But
>> rather the metric expr parsing error detection and alias checking.
>>
>> About jirka's patch:
>>
>> --- a/tools/perf/tests/pmu-events.c
>> +++ b/tools/perf/tests/pmu-events.c
>> @@ -485,6 +485,102 @@ static int test_parsing(void)
>>   	return ret == 0 ? TEST_OK : TEST_SKIP;
>>   }
>>
>> +
>> +static struct test_metric metrics[] = {
>> +	{ .metric = "imx8_ddr0@read\\-cycles@ * 4 * 4", },
>> +	{ .metric = "imx8_ddr0@axid\\-read\\,axi_mask\\=0xffff\\,axi_id\\=0x0000@
>> * 4", },
>> +	{ .metric = "(cstate_pkg@c2\\-residency@ / msr@tsc@) * 100", },
>> +	{ .metric = "(imx8_ddr0@read\\-cycles@ + imx8_ddr0@write\\-cycles@)", },
>> +};
>>
>> Maybe we could add these to pmu-events/arch/test/test_cpu/metric.json, and
>> get at them that way.
> 
> that test sets the 'fake pmu' stuff.. could we do that in your test?

I'm not sure about a test comparable to the alias match testing, but at 
least it should be possible to verify that jevents generates as-expected 
metric events (as done in __test_pmu_event_table() for regular events).

Cheers,
John