[RFC 00/14] perf tests: Check on subtest for user specified test

[RFC 00/14] perf tests: Check on subtest for user specified test

[Jiri Olsa]

hi,
changes for using metric result in another metric seem
to change lot of core metric code, so it's better we
have some more tests before we do that.

Sending as RFC as it's still alive and you guys might
have some other idea of how to do this.

Also available in here:
  git://git.kernel.org/pub/scm/linux/kernel/git/jolsa/perf.git
  perf/fixes

jirka


---
Jiri Olsa (14):
      perf tests: Check on subtest for user specified test
      perf tools: Do not pass avg to generic_metric
      perf tools: Add struct parse_events_state pointer to scanner
      perf tools: Add fake pmu support
      perf tools: Add parse_events_fake interface
      perf tests: Add another pmu-events tests
      perf tools: Factor out parse_groups function
      perf tools: Add metricgroup__parse_groups_test function
      perf tools: Add fake_pmu to parse_events function
      perf tools: Add map to parse_events function
      perf tools: Factor out prepare_metric function
      perf tools: Add test_generic_metric function
      perf tests: Add parse metric test for ipc metric
      perf tests: Add parse metric test for frontend metric

 tools/perf/tests/Build          |   1 +
 tools/perf/tests/builtin-test.c |  38 ++++++++++++++++++++++------
 tools/perf/tests/parse-metric.c | 163 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 tools/perf/tests/pmu-events.c   | 120 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 tools/perf/tests/tests.h        |   1 +
 tools/perf/util/metricgroup.c   |  53 ++++++++++++++++++++++++++++++---------
 tools/perf/util/metricgroup.h   |   9 +++++++
 tools/perf/util/parse-events.c  |  73 ++++++++++++++++++++++++++++++++++++++---------------
 tools/perf/util/parse-events.h  |   6 ++++-
 tools/perf/util/parse-events.l  |  16 +++++++-----
 tools/perf/util/parse-events.y  |  37 +++++++++++++++++++++++++--
 tools/perf/util/stat-shadow.c   |  77 ++++++++++++++++++++++++++++++++++++--------------------
 tools/perf/util/stat.h          |   3 +++
 13 files changed, 521 insertions(+), 76 deletions(-)
 create mode 100644 tools/perf/tests/parse-metric.c

[PATCH 01/14] perf tests: Check on subtest for user specified test

[Jiri Olsa]

It's now possible to put subtest name as a test filter:

  $ perf test 'PMU event table sanity'
  10: PMU events                                            :
  10.1: PMU event table sanity                              : Ok

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/perf/tests/builtin-test.c | 34 +++++++++++++++++++++++++--------
 1 file changed, 26 insertions(+), 8 deletions(-)

diff --git a/tools/perf/tests/builtin-test.c b/tools/perf/tests/builtin-test.c
index 9553f8061772..a9daaeb9fd27 100644
--- a/tools/perf/tests/builtin-test.c
+++ b/tools/perf/tests/builtin-test.c
@@ -334,7 +334,7 @@ static struct test *tests[] = {
 	arch_tests,
 };
 
-static bool perf_test__matches(struct test *test, int curr, int argc, const char *argv[])
+static bool perf_test__matches(const char *desc, int curr, int argc, const char *argv[])
 {
 	int i;
 
@@ -351,7 +351,7 @@ static bool perf_test__matches(struct test *test, int curr, int argc, const char
 			continue;
 		}
 
-		if (strcasestr(test->desc, argv[i]))
+		if (strcasestr(desc, argv[i]))
 			return true;
 	}
 
@@ -580,7 +580,7 @@ static int run_shell_tests(int argc, const char *argv[], int i, int width)
 			.priv = &st,
 		};
 
-		if (!perf_test__matches(&test, curr, argc, argv))
+		if (!perf_test__matches(test.desc, curr, argc, argv))
 			continue;
 
 		st.file = ent->d_name;
@@ -608,9 +608,25 @@ static int __cmd_test(int argc, const char *argv[], struct intlist *skiplist)
 
 	for_each_test(j, t) {
 		int curr = i++, err;
+		int subi;
 
-		if (!perf_test__matches(t, curr, argc, argv))
-			continue;
+		if (!perf_test__matches(t->desc, curr, argc, argv)) {
+			bool skip = true;
+			int subn;
+
+			if (!t->subtest.get_nr)
+				continue;
+
+			subn = t->subtest.get_nr();
+
+			for (subi = 0; subi < subn; subi++) {
+				if (perf_test__matches(t->subtest.get_desc(subi), curr, argc, argv))
+					skip = false;
+			}
+
+			if (skip)
+				continue;
+		}
 
 		if (t->is_supported && !t->is_supported()) {
 			pr_debug("%2d: %-*s: Disabled\n", i, width, t->desc);
@@ -638,7 +654,6 @@ static int __cmd_test(int argc, const char *argv[], struct intlist *skiplist)
 			 */
 			int subw = width > 2 ? width - 2 : width;
 			bool skip = false;
-			int subi;
 
 			if (subn <= 0) {
 				color_fprintf(stderr, PERF_COLOR_YELLOW,
@@ -655,6 +670,9 @@ static int __cmd_test(int argc, const char *argv[], struct intlist *skiplist)
 			}
 
 			for (subi = 0; subi < subn; subi++) {
+				if (!perf_test__matches(t->subtest.get_desc(subi), curr, argc, argv))
+					continue;
+
 				pr_info("%2d.%1d: %-*s:", i, subi + 1, subw,
 					t->subtest.get_desc(subi));
 				err = test_and_print(t, skip, subi);
@@ -688,7 +706,7 @@ static int perf_test__list_shell(int argc, const char **argv, int i)
 			.desc = shell_test__description(bf, sizeof(bf), path, ent->d_name),
 		};
 
-		if (!perf_test__matches(&t, curr, argc, argv))
+		if (!perf_test__matches(t.desc, curr, argc, argv))
 			continue;
 
 		pr_info("%2d: %s\n", i, t.desc);
@@ -707,7 +725,7 @@ static int perf_test__list(int argc, const char **argv)
 	for_each_test(j, t) {
 		int curr = i++;
 
-		if (!perf_test__matches(t, curr, argc, argv) ||
+		if (!perf_test__matches(t->desc, curr, argc, argv) ||
 		    (t->is_supported && !t->is_supported()))
 			continue;
 
-- 
2.25.4

[PATCH 02/14] perf tools: Do not pass avg to generic_metric

[Jiri Olsa]

There's no need to pass the given evsel's count to
metric data, because it will be pushed again within
the following metric_events loop.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/perf/util/stat-shadow.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/tools/perf/util/stat-shadow.c b/tools/perf/util/stat-shadow.c
index c44dc814b377..a7c13a88ecb9 100644
--- a/tools/perf/util/stat-shadow.c
+++ b/tools/perf/util/stat-shadow.c
@@ -737,7 +737,6 @@ static void generic_metric(struct perf_stat_config *config,
 			   const char *metric_name,
 			   const char *metric_unit,
 			   int runtime,
-			   double avg,
 			   int cpu,
 			   struct perf_stat_output_ctx *out,
 			   struct runtime_stat *st)
@@ -750,11 +749,6 @@ static void generic_metric(struct perf_stat_config *config,
 	char *n, *pn;
 
 	expr__ctx_init(&pctx);
-	/* Must be first id entry */
-	n = strdup(name);
-	if (!n)
-		return;
-	expr__add_id(&pctx, n, avg);
 	for (i = 0; metric_events[i]; i++) {
 		struct saved_value *v;
 		struct stats *stats;
@@ -1042,7 +1036,7 @@ void perf_stat__print_shadow_stats(struct perf_stat_config *config,
 			print_metric(config, ctxp, NULL, NULL, name, 0);
 	} else if (evsel->metric_expr) {
 		generic_metric(config, evsel->metric_expr, evsel->metric_events, evsel->name,
-				evsel->metric_name, NULL, 1, avg, cpu, out, st);
+				evsel->metric_name, NULL, 1, cpu, out, st);
 	} else if (runtime_stat_n(st, STAT_NSECS, 0, cpu) != 0) {
 		char unit = 'M';
 		char unit_buf[10];
@@ -1071,7 +1065,7 @@ void perf_stat__print_shadow_stats(struct perf_stat_config *config,
 				out->new_line(config, ctxp);
 			generic_metric(config, mexp->metric_expr, mexp->metric_events,
 					evsel->name, mexp->metric_name,
-					mexp->metric_unit, mexp->runtime, avg, cpu, out, st);
+					mexp->metric_unit, mexp->runtime, cpu, out, st);
 		}
 	}
 	if (num == 0)
-- 
2.25.4

[PATCH 03/14] perf tools: Add struct parse_events_state pointer to scanner

[Jiri Olsa]

We need to pass more data to the scanner so let's
start with having it to take pointer to struct
parse_events_state object instead of just start
token.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/perf/util/parse-events.c | 15 +++++++++------
 tools/perf/util/parse-events.h |  1 +
 tools/perf/util/parse-events.l |  8 ++++----
 3 files changed, 14 insertions(+), 10 deletions(-)

diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index e0ccf027d214..27b8e49d690e 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -26,7 +26,7 @@
 #include <api/fs/tracing_path.h>
 #include <perf/cpumap.h>
 #include "parse-events-bison.h"
-#define YY_EXTRA_TYPE int
+#define YY_EXTRA_TYPE void*
 #include "parse-events-flex.h"
 #include "pmu.h"
 #include "thread_map.h"
@@ -2027,13 +2027,14 @@ perf_pmu__parse_check(const char *name)
 	return r ? r->type : PMU_EVENT_SYMBOL_ERR;
 }
 
-static int parse_events__scanner(const char *str, void *parse_state, int start_token)
+static int parse_events__scanner(const char *str,
+				 struct parse_events_state *parse_state)
 {
 	YY_BUFFER_STATE buffer;
 	void *scanner;
 	int ret;
 
-	ret = parse_events_lex_init_extra(start_token, &scanner);
+	ret = parse_events_lex_init_extra(parse_state, &scanner);
 	if (ret)
 		return ret;
 
@@ -2057,11 +2058,12 @@ static int parse_events__scanner(const char *str, void *parse_state, int start_t
 int parse_events_terms(struct list_head *terms, const char *str)
 {
 	struct parse_events_state parse_state = {
-		.terms = NULL,
+		.terms  = NULL,
+		.stoken = PE_START_TERMS,
 	};
 	int ret;
 
-	ret = parse_events__scanner(str, &parse_state, PE_START_TERMS);
+	ret = parse_events__scanner(str, &parse_state);
 	if (!ret) {
 		list_splice(parse_state.terms, terms);
 		zfree(&parse_state.terms);
@@ -2080,10 +2082,11 @@ int parse_events(struct evlist *evlist, const char *str,
 		.idx    = evlist->core.nr_entries,
 		.error  = err,
 		.evlist = evlist,
+		.stoken = PE_START_EVENTS,
 	};
 	int ret;
 
-	ret = parse_events__scanner(str, &parse_state, PE_START_EVENTS);
+	ret = parse_events__scanner(str, &parse_state);
 	perf_pmu__parse_cleanup();
 
 	if (!ret && list_empty(&parse_state.list)) {
diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h
index 6ead9661238c..d60510e0609f 100644
--- a/tools/perf/util/parse-events.h
+++ b/tools/perf/util/parse-events.h
@@ -128,6 +128,7 @@ struct parse_events_state {
 	struct parse_events_error *error;
 	struct evlist	  *evlist;
 	struct list_head	  *terms;
+	int			   stoken;
 };
 
 void parse_events__handle_error(struct parse_events_error *err, int idx,
diff --git a/tools/perf/util/parse-events.l b/tools/perf/util/parse-events.l
index 394132254447..002802e17059 100644
--- a/tools/perf/util/parse-events.l
+++ b/tools/perf/util/parse-events.l
@@ -209,10 +209,10 @@ modifier_bp	[rwx]{1,3}
 %%
 
 %{
-	{
-		int start_token;
+	struct parse_events_state *_parse_state = parse_events_get_extra(yyscanner);
 
-		start_token = parse_events_get_extra(yyscanner);
+	{
+		int start_token = _parse_state->stoken;
 
 		if (start_token == PE_START_TERMS)
 			BEGIN(config);
@@ -220,7 +220,7 @@ modifier_bp	[rwx]{1,3}
 			BEGIN(event);
 
 		if (start_token) {
-			parse_events_set_extra(NULL, yyscanner);
+			_parse_state->stoken = 0;
 			/*
 			 * The flex parser does not init locations variable
 			 * via the scan_string interface, so we need do the
-- 
2.25.4

[PATCH 04/14] perf tools: Add fake pmu support

[Jiri Olsa]

Adding the way to create pmu event without the actual
PMU being in place. This way we can test metrics defined
for any processors.

The interface is to define fake_pmu in struct parse_events_state
data. It will be used only in tests via special interface
function added in following changes.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/perf/util/parse-events.c | 14 ++++++++++---
 tools/perf/util/parse-events.h |  3 ++-
 tools/perf/util/parse-events.l |  8 ++++++--
 tools/perf/util/parse-events.y | 37 ++++++++++++++++++++++++++++++++--
 4 files changed, 54 insertions(+), 8 deletions(-)

diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index 27b8e49d690e..8304f9b6e6be 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -1415,6 +1415,7 @@ int parse_events_add_pmu(struct parse_events_state *parse_state,
 			 bool auto_merge_stats,
 			 bool use_alias)
 {
+	bool is_fake = parse_state->fake_pmu;
 	struct perf_event_attr attr;
 	struct perf_pmu_info info;
 	struct perf_pmu *pmu;
@@ -1436,7 +1437,14 @@ int parse_events_add_pmu(struct parse_events_state *parse_state,
 		fprintf(stderr, "' that may result in non-fatal errors\n");
 	}
 
-	pmu = perf_pmu__find(name);
+	if (is_fake) {
+		static struct perf_pmu fake_pmu = { };
+
+		pmu = &fake_pmu;
+	} else {
+		pmu = perf_pmu__find(name);
+	}
+
 	if (!pmu) {
 		char *err_str;
 
@@ -1469,7 +1477,7 @@ int parse_events_add_pmu(struct parse_events_state *parse_state,
 		}
 	}
 
-	if (perf_pmu__check_alias(pmu, head_config, &info))
+	if (!is_fake && perf_pmu__check_alias(pmu, head_config, &info))
 		return -EINVAL;
 
 	if (verbose > 1) {
@@ -1502,7 +1510,7 @@ int parse_events_add_pmu(struct parse_events_state *parse_state,
 	if (pmu->default_config && get_config_chgs(pmu, head_config, &config_terms))
 		return -ENOMEM;
 
-	if (perf_pmu__config(pmu, &attr, head_config, parse_state->error)) {
+	if (!is_fake && perf_pmu__config(pmu, &attr, head_config, parse_state->error)) {
 		struct evsel_config_term *pos, *tmp;
 
 		list_for_each_entry_safe(pos, tmp, &config_terms, list) {
diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h
index d60510e0609f..963b0ea6c448 100644
--- a/tools/perf/util/parse-events.h
+++ b/tools/perf/util/parse-events.h
@@ -126,9 +126,10 @@ struct parse_events_state {
 	int			   idx;
 	int			   nr_groups;
 	struct parse_events_error *error;
-	struct evlist	  *evlist;
+	struct evlist		  *evlist;
 	struct list_head	  *terms;
 	int			   stoken;
+	bool			   fake_pmu;
 };
 
 void parse_events__handle_error(struct parse_events_error *err, int idx,
diff --git a/tools/perf/util/parse-events.l b/tools/perf/util/parse-events.l
index 002802e17059..56912c9641f5 100644
--- a/tools/perf/util/parse-events.l
+++ b/tools/perf/util/parse-events.l
@@ -129,12 +129,16 @@ do {								\
 	yyless(0);						\
 } while (0)
 
-static int pmu_str_check(yyscan_t scanner)
+static int pmu_str_check(yyscan_t scanner, struct parse_events_state *parse_state)
 {
 	YYSTYPE *yylval = parse_events_get_lval(scanner);
 	char *text = parse_events_get_text(scanner);
 
 	yylval->str = strdup(text);
+
+	if (parse_state->fake_pmu)
+		return PE_PMU_EVENT_FAKE;
+
 	switch (perf_pmu__parse_check(text)) {
 		case PMU_EVENT_SYMBOL_PREFIX:
 			return PE_PMU_EVENT_PRE;
@@ -376,7 +380,7 @@ r{num_raw_hex}		{ return raw(yyscanner); }
 {modifier_event}	{ return str(yyscanner, PE_MODIFIER_EVENT); }
 {bpf_object}		{ if (!isbpf(yyscanner)) { USER_REJECT }; return str(yyscanner, PE_BPF_OBJECT); }
 {bpf_source}		{ if (!isbpf(yyscanner)) { USER_REJECT }; return str(yyscanner, PE_BPF_SOURCE); }
-{name}			{ return pmu_str_check(yyscanner); }
+{name}			{ return pmu_str_check(yyscanner, _parse_state); }
 {name_tag}		{ return str(yyscanner, PE_NAME); }
 "/"			{ BEGIN(config); return '/'; }
 -			{ return '-'; }
diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y
index c4ca932d092d..d1b04b8d81ea 100644
--- a/tools/perf/util/parse-events.y
+++ b/tools/perf/util/parse-events.y
@@ -69,7 +69,7 @@ static void inc_group_count(struct list_head *list,
 %token PE_NAME_CACHE_TYPE PE_NAME_CACHE_OP_RESULT
 %token PE_PREFIX_MEM PE_PREFIX_RAW PE_PREFIX_GROUP
 %token PE_ERROR
-%token PE_PMU_EVENT_PRE PE_PMU_EVENT_SUF PE_KERNEL_PMU_EVENT
+%token PE_PMU_EVENT_PRE PE_PMU_EVENT_SUF PE_KERNEL_PMU_EVENT PE_PMU_EVENT_FAKE
 %token PE_ARRAY_ALL PE_ARRAY_RANGE
 %token PE_DRV_CFG_TERM
 %type <num> PE_VALUE
@@ -87,7 +87,7 @@ static void inc_group_count(struct list_head *list,
 %type <str> PE_MODIFIER_EVENT
 %type <str> PE_MODIFIER_BP
 %type <str> PE_EVENT_NAME
-%type <str> PE_PMU_EVENT_PRE PE_PMU_EVENT_SUF PE_KERNEL_PMU_EVENT
+%type <str> PE_PMU_EVENT_PRE PE_PMU_EVENT_SUF PE_KERNEL_PMU_EVENT PE_PMU_EVENT_FAKE
 %type <str> PE_DRV_CFG_TERM
 %destructor { free ($$); } <str>
 %type <term> event_term
@@ -356,6 +356,39 @@ PE_PMU_EVENT_PRE '-' PE_PMU_EVENT_SUF sep_dc
 		YYABORT;
 	$$ = list;
 }
+|
+PE_PMU_EVENT_FAKE sep_dc
+{
+	struct list_head *list;
+	int err;
+
+	list = alloc_list();
+	if (!list)
+		YYABORT;
+
+	err = parse_events_add_pmu(_parse_state, list, $1, NULL, false, false);
+	free($1);
+	if (err < 0)
+		YYABORT;
+	$$ = list;
+}
+|
+PE_PMU_EVENT_FAKE opt_pmu_config
+{
+	struct list_head *list;
+	int err;
+
+	list = alloc_list();
+	if (!list)
+		YYABORT;
+
+	err = parse_events_add_pmu(_parse_state, list, $1, $2, false, false);
+	free($1);
+	parse_events_terms__delete($2);
+	if (err < 0)
+		YYABORT;
+	$$ = list;
+}
 
 value_sym:
 PE_VALUE_SYM_HW
-- 
2.25.4

[PATCH 05/14] perf tools: Add parse_events_fake interface

[Jiri Olsa]

Adding parse_events_fake interface to parse events
and use fake pmu event in case pmu event is parsed.

This way it's possible to parse events from PMUs
which are not present in the system. It's available
only for testing purposes coming in following
changes.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/perf/util/parse-events.c | 48 +++++++++++++++++++++++++---------
 tools/perf/util/parse-events.h |  2 ++
 2 files changed, 37 insertions(+), 13 deletions(-)

diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index 8304f9b6e6be..89239695a728 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -2082,22 +2082,15 @@ int parse_events_terms(struct list_head *terms, const char *str)
 	return ret;
 }
 
-int parse_events(struct evlist *evlist, const char *str,
-		 struct parse_events_error *err)
+static int parse_events_state(struct parse_events_state *parse_state,
+			      struct evlist *evlist, const char *str)
 {
-	struct parse_events_state parse_state = {
-		.list   = LIST_HEAD_INIT(parse_state.list),
-		.idx    = evlist->core.nr_entries,
-		.error  = err,
-		.evlist = evlist,
-		.stoken = PE_START_EVENTS,
-	};
 	int ret;
 
-	ret = parse_events__scanner(str, &parse_state);
+	ret = parse_events__scanner(str, parse_state);
 	perf_pmu__parse_cleanup();
 
-	if (!ret && list_empty(&parse_state.list)) {
+	if (!ret && list_empty(&parse_state->list)) {
 		WARN_ONCE(true, "WARNING: event parser found nothing\n");
 		return -1;
 	}
@@ -2105,12 +2098,12 @@ int parse_events(struct evlist *evlist, const char *str,
 	/*
 	 * Add list to the evlist even with errors to allow callers to clean up.
 	 */
-	perf_evlist__splice_list_tail(evlist, &parse_state.list);
+	perf_evlist__splice_list_tail(evlist, &parse_state->list);
 
 	if (!ret) {
 		struct evsel *last;
 
-		evlist->nr_groups += parse_state.nr_groups;
+		evlist->nr_groups += parse_state->nr_groups;
 		last = evlist__last(evlist);
 		last->cmdline_group_boundary = true;
 
@@ -2125,6 +2118,35 @@ int parse_events(struct evlist *evlist, const char *str,
 	return ret;
 }
 
+int parse_events(struct evlist *evlist, const char *str,
+		 struct parse_events_error *err)
+{
+	struct parse_events_state parse_state = {
+		.list   = LIST_HEAD_INIT(parse_state.list),
+		.idx    = evlist->core.nr_entries,
+		.error  = err,
+		.evlist = evlist,
+		.stoken = PE_START_EVENTS,
+	};
+
+	return parse_events_state(&parse_state, evlist, str);
+}
+
+int parse_events_fake(struct evlist *evlist, const char *str,
+		      struct parse_events_error *err)
+{
+	struct parse_events_state parse_state = {
+		.list     = LIST_HEAD_INIT(parse_state.list),
+		.idx      = evlist->core.nr_entries,
+		.error    = err,
+		.evlist   = evlist,
+		.stoken   = PE_START_EVENTS,
+		.fake_pmu = true,
+	};
+
+	return parse_events_state(&parse_state, evlist, str);
+}
+
 #define MAX_WIDTH 1000
 static int get_term_width(void)
 {
diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h
index 963b0ea6c448..4a23b6cd9924 100644
--- a/tools/perf/util/parse-events.h
+++ b/tools/perf/util/parse-events.h
@@ -34,6 +34,8 @@ int parse_events_option(const struct option *opt, const char *str, int unset);
 int parse_events_option_new_evlist(const struct option *opt, const char *str, int unset);
 int parse_events(struct evlist *evlist, const char *str,
 		 struct parse_events_error *error);
+int parse_events_fake(struct evlist *evlist, const char *str,
+		      struct parse_events_error *error);
 int parse_events_terms(struct list_head *terms, const char *str);
 int parse_filter(const struct option *opt, const char *str, int unset);
 int exclude_perf(const struct option *opt, const char *arg, int unset);
-- 
2.25.4

[PATCH 06/14] perf tests: Add another pmu-events tests

[Jiri Olsa]

The test goes through all metrics compiled for arch
within pmu events and try to parse them.

The test also defines its own list of metrics and
tries to parse them. It's handy for developing.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/perf/tests/pmu-events.c | 120 ++++++++++++++++++++++++++++++++++
 1 file changed, 120 insertions(+)

diff --git a/tools/perf/tests/pmu-events.c b/tools/perf/tests/pmu-events.c
index ab64b4a4e284..f100df025440 100644
--- a/tools/perf/tests/pmu-events.c
+++ b/tools/perf/tests/pmu-events.c
@@ -490,6 +490,122 @@ static int test_parsing(void)
 	return ret == 0 ? TEST_OK : TEST_SKIP;
 }
 
+struct test_metric {
+	const char *str;
+};
+
+static struct test_metric metrics[] = {
+	{ "(unc_p_power_state_occupancy.cores_c0 / unc_p_clockticks) * 100." },
+	{ "imx8_ddr0@read\\-cycles@ * 4 * 4", },
+	{ "imx8_ddr0@axid\\-read\\,axi_mask\\=0xffff\\,axi_id\\=0x0000@ * 4", },
+	{ "(cstate_pkg@c2\\-residency@ / msr@tsc@) * 100", },
+	{ "(imx8_ddr0@read\\-cycles@ + imx8_ddr0@write\\-cycles@)", },
+};
+
+static int check_id(const char *id)
+{
+	struct parse_events_error error;
+	struct evlist *evlist;
+	int ret;
+
+	/* Numbers are always valid. */
+	if (is_number(id))
+		return 0;
+
+	evlist = evlist__new();
+	if (!evlist)
+		return -1;
+
+	memset(&error, 0, sizeof(error));
+	ret = parse_events_fake(evlist, id, &error);
+	if (ret) {
+		pr_debug("str        : %s\n", error.str);
+		pr_debug("help       : %s\n", error.help);
+		pr_debug("first_str  : %s\n", error.first_str);
+		pr_debug("first_help : %s\n", error.first_help);
+	}
+
+	evlist__delete(evlist);
+	free(error.str);
+	free(error.help);
+	free(error.first_str);
+	free(error.first_help);
+	return ret;
+}
+
+static int metric_parse_fake(const char *str)
+{
+	struct expr_parse_ctx ctx;
+	struct hashmap_entry *cur;
+	double result;
+	int ret = -1;
+	size_t bkt;
+	int i;
+
+	pr_debug("parsing '%s'\n", str);
+
+	expr__ctx_init(&ctx);
+	if (expr__find_other(str, NULL, &ctx, 0) < 0) {
+		pr_err("expr__find_other failed\n");
+		return -1;
+	}
+
+	i = 1;
+	hashmap__for_each_entry((&ctx.ids), cur, bkt)
+		expr__add_id(&ctx, strdup(cur->key), i++);
+
+	hashmap__for_each_entry((&ctx.ids), cur, bkt) {
+		if (check_id(cur->key)) {
+			pr_err("check_id failed\n");
+			goto out;
+		}
+	}
+
+	if (!expr__parse(&result, &ctx, str, 1))
+		ret = 0;
+	else
+		pr_err("expr__parse failed\n");
+
+out:
+	expr__ctx_clear(&ctx);
+	return ret;
+
+}
+
+static int test_parsing_fake(void)
+{
+	struct pmu_events_map *map;
+	struct pmu_event *pe;
+	unsigned int i, j;
+	int err = 0;
+
+	for (i = 0; !err && i < ARRAY_SIZE(metrics); i++)
+		err = metric_parse_fake(metrics[i].str);
+
+	if (err)
+		return err;
+
+	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;
+			err = metric_parse_fake(pe->metric_expr);
+			if (err)
+				return err;
+		}
+	}
+
+	return 0;
+}
+
 static const struct {
 	int (*func)(void);
 	const char *desc;
@@ -506,6 +622,10 @@ static const struct {
 		.func = test_parsing,
 		.desc = "Parsing of PMU event table metrics",
 	},
+	{
+		.func = test_parsing_fake,
+		.desc = "Parsing of PMU event table metrics with fake PMUs",
+	},
 };
 
 const char *test__pmu_events_subtest_get_desc(int subtest)
-- 
2.25.4

[PATCH 07/14] perf tools: Factor out parse_groups function

[Jiri Olsa]

Factor out the parse_groups function, it will be used
for new test interface coming in following changes.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/perf/util/metricgroup.c | 22 ++++++++++++++++------
 1 file changed, 16 insertions(+), 6 deletions(-)

diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
index 9e21aa767e41..d72a565ec483 100644
--- a/tools/perf/util/metricgroup.c
+++ b/tools/perf/util/metricgroup.c
@@ -709,14 +709,12 @@ static void metricgroup__free_egroups(struct list_head *group_list)
 	}
 }
 
-int metricgroup__parse_groups(const struct option *opt,
-			      const char *str,
-			      bool metric_no_group,
-			      bool metric_no_merge,
-			      struct rblist *metric_events)
+static int parse_groups(struct evlist *perf_evlist, const char *str,
+			bool metric_no_group,
+			bool metric_no_merge,
+			struct rblist *metric_events)
 {
 	struct parse_events_error parse_error;
-	struct evlist *perf_evlist = *(struct evlist **)opt->value;
 	struct strbuf extra_events;
 	LIST_HEAD(group_list);
 	int ret;
@@ -742,6 +740,18 @@ int metricgroup__parse_groups(const struct option *opt,
 	return ret;
 }
 
+int metricgroup__parse_groups(const struct option *opt,
+			      const char *str,
+			      bool metric_no_group,
+			      bool metric_no_merge,
+			      struct rblist *metric_events)
+{
+	struct evlist *perf_evlist = *(struct evlist **)opt->value;
+
+	return parse_groups(perf_evlist, str, metric_no_group,
+			    metric_no_merge, metric_events);
+}
+
 bool metricgroup__has_metric(const char *metric)
 {
 	struct pmu_events_map *map = perf_pmu__find_map(NULL);
-- 
2.25.4

[PATCH 08/14] perf tools: Add metricgroup__parse_groups_test function

[Jiri Olsa]

Adding metricgroup__parse_groups_test function. It will
be used as test's interface to metric parsing in following
changes.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/perf/util/metricgroup.c | 11 +++++++++++
 tools/perf/util/metricgroup.h |  9 +++++++++
 2 files changed, 20 insertions(+)

diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
index d72a565ec483..e2b4c621700c 100644
--- a/tools/perf/util/metricgroup.c
+++ b/tools/perf/util/metricgroup.c
@@ -752,6 +752,17 @@ int metricgroup__parse_groups(const struct option *opt,
 			    metric_no_merge, metric_events);
 }
 
+int metricgroup__parse_groups_test(struct evlist *evlist,
+				   struct pmu_events_map *map,
+				   const char *str,
+				   bool metric_no_group,
+				   bool metric_no_merge,
+				   struct rblist *metric_events)
+{
+	return parse_groups(evlist, str, metric_no_group,
+			    metric_no_merge, true, metric_events, map);
+}
+
 bool metricgroup__has_metric(const char *metric)
 {
 	struct pmu_events_map *map = perf_pmu__find_map(NULL);
diff --git a/tools/perf/util/metricgroup.h b/tools/perf/util/metricgroup.h
index 287850bcdeca..426c824e20bf 100644
--- a/tools/perf/util/metricgroup.h
+++ b/tools/perf/util/metricgroup.h
@@ -7,8 +7,10 @@
 #include <stdbool.h>
 
 struct evsel;
+struct evlist;
 struct option;
 struct rblist;
+struct pmu_events_map;
 
 struct metric_event {
 	struct rb_node nd;
@@ -34,6 +36,13 @@ int metricgroup__parse_groups(const struct option *opt,
 			      bool metric_no_merge,
 			      struct rblist *metric_events);
 
+int metricgroup__parse_groups_test(struct evlist *evlist,
+				   struct pmu_events_map *map,
+				   const char *str,
+				   bool metric_no_group,
+				   bool metric_no_merge,
+				   struct rblist *metric_events);
+
 void metricgroup__print(bool metrics, bool groups, char *filter,
 			bool raw, bool details);
 bool metricgroup__has_metric(const char *metric);
-- 
2.25.4

[PATCH 09/14] perf tools: Add fake_pmu to parse_events function

[Jiri Olsa]

Allow to set fake_pmu new parse_groups function.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/perf/util/metricgroup.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
index e2b4c621700c..547f83ee5c68 100644
--- a/tools/perf/util/metricgroup.c
+++ b/tools/perf/util/metricgroup.c
@@ -712,6 +712,7 @@ static void metricgroup__free_egroups(struct list_head *group_list)
 static int parse_groups(struct evlist *perf_evlist, const char *str,
 			bool metric_no_group,
 			bool metric_no_merge,
+			bool fake_pmu,
 			struct rblist *metric_events)
 {
 	struct parse_events_error parse_error;
@@ -727,7 +728,11 @@ static int parse_groups(struct evlist *perf_evlist, const char *str,
 		return ret;
 	pr_debug("adding %s\n", extra_events.buf);
 	bzero(&parse_error, sizeof(parse_error));
-	ret = parse_events(perf_evlist, extra_events.buf, &parse_error);
+
+	if (fake_pmu)
+		ret = parse_events_fake(perf_evlist, extra_events.buf, &parse_error);
+	else
+		ret = parse_events(perf_evlist, extra_events.buf, &parse_error);
 	if (ret) {
 		parse_events_print_error(&parse_error, extra_events.buf);
 		goto out;
@@ -749,7 +754,7 @@ int metricgroup__parse_groups(const struct option *opt,
 	struct evlist *perf_evlist = *(struct evlist **)opt->value;
 
 	return parse_groups(perf_evlist, str, metric_no_group,
-			    metric_no_merge, metric_events);
+			    metric_no_merge, false, metric_events);
 }
 
 int metricgroup__parse_groups_test(struct evlist *evlist,
-- 
2.25.4

[PATCH 10/14] perf tools: Add map to parse_events function

[Jiri Olsa]

For testing purposes we need to pass our own map of events
from parse_groups through metricgroup__add_metric.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/perf/util/metricgroup.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
index 547f83ee5c68..b8a0bd7e9297 100644
--- a/tools/perf/util/metricgroup.c
+++ b/tools/perf/util/metricgroup.c
@@ -597,9 +597,9 @@ static int __metricgroup__add_metric(struct list_head *group_list,
 
 static int metricgroup__add_metric(const char *metric, bool metric_no_group,
 				   struct strbuf *events,
-				   struct list_head *group_list)
+				   struct list_head *group_list,
+				   struct pmu_events_map *map)
 {
-	struct pmu_events_map *map = perf_pmu__find_map(NULL);
 	struct pmu_event *pe;
 	struct egroup *eg;
 	int i, ret;
@@ -668,7 +668,8 @@ static int metricgroup__add_metric(const char *metric, bool metric_no_group,
 
 static int metricgroup__add_metric_list(const char *list, bool metric_no_group,
 					struct strbuf *events,
-				        struct list_head *group_list)
+					struct list_head *group_list,
+					struct pmu_events_map *map)
 {
 	char *llist, *nlist, *p;
 	int ret = -EINVAL;
@@ -683,7 +684,7 @@ static int metricgroup__add_metric_list(const char *list, bool metric_no_group,
 
 	while ((p = strsep(&llist, ",")) != NULL) {
 		ret = metricgroup__add_metric(p, metric_no_group, events,
-					      group_list);
+					      group_list, map);
 		if (ret == -EINVAL) {
 			fprintf(stderr, "Cannot find metric or group `%s'\n",
 					p);
@@ -713,7 +714,8 @@ static int parse_groups(struct evlist *perf_evlist, const char *str,
 			bool metric_no_group,
 			bool metric_no_merge,
 			bool fake_pmu,
-			struct rblist *metric_events)
+			struct rblist *metric_events,
+			struct pmu_events_map *map)
 {
 	struct parse_events_error parse_error;
 	struct strbuf extra_events;
@@ -723,7 +725,7 @@ static int parse_groups(struct evlist *perf_evlist, const char *str,
 	if (metric_events->nr_entries == 0)
 		metricgroup__rblist_init(metric_events);
 	ret = metricgroup__add_metric_list(str, metric_no_group,
-					   &extra_events, &group_list);
+					   &extra_events, &group_list, map);
 	if (ret)
 		return ret;
 	pr_debug("adding %s\n", extra_events.buf);
@@ -752,9 +754,10 @@ int metricgroup__parse_groups(const struct option *opt,
 			      struct rblist *metric_events)
 {
 	struct evlist *perf_evlist = *(struct evlist **)opt->value;
+	struct pmu_events_map *map = perf_pmu__find_map(NULL);
 
 	return parse_groups(perf_evlist, str, metric_no_group,
-			    metric_no_merge, false, metric_events);
+			    metric_no_merge, false, metric_events, map);
 }
 
 int metricgroup__parse_groups_test(struct evlist *evlist,
-- 
2.25.4

[PATCH 11/14] perf tools: Factor out prepare_metric function

[Jiri Olsa]

Factoring out prepare_metric functio so it can
be used in test interface coming in following
changes.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/perf/util/stat-shadow.c | 53 ++++++++++++++++++++++-------------
 1 file changed, 34 insertions(+), 19 deletions(-)

diff --git a/tools/perf/util/stat-shadow.c b/tools/perf/util/stat-shadow.c
index a7c13a88ecb9..27be7ce2fff4 100644
--- a/tools/perf/util/stat-shadow.c
+++ b/tools/perf/util/stat-shadow.c
@@ -730,25 +730,16 @@ static void print_smi_cost(struct perf_stat_config *config,
 	out->print_metric(config, out->ctx, NULL, "%4.0f", "SMI#", smi_num);
 }
 
-static void generic_metric(struct perf_stat_config *config,
-			   const char *metric_expr,
-			   struct evsel **metric_events,
-			   char *name,
-			   const char *metric_name,
-			   const char *metric_unit,
-			   int runtime,
-			   int cpu,
-			   struct perf_stat_output_ctx *out,
-			   struct runtime_stat *st)
+static int prepare_metric(struct evsel **metric_events,
+			  struct expr_parse_ctx *pctx,
+			  int cpu,
+			  struct runtime_stat *st)
 {
-	print_metric_t print_metric = out->print_metric;
-	struct expr_parse_ctx pctx;
-	double ratio, scale;
-	int i;
-	void *ctxp = out->ctx;
+	double scale;
 	char *n, *pn;
+	int i;
 
-	expr__ctx_init(&pctx);
+	expr__ctx_init(pctx);
 	for (i = 0; metric_events[i]; i++) {
 		struct saved_value *v;
 		struct stats *stats;
@@ -771,7 +762,7 @@ static void generic_metric(struct perf_stat_config *config,
 
 		n = strdup(metric_events[i]->name);
 		if (!n)
-			return;
+			return -ENOMEM;
 		/*
 		 * This display code with --no-merge adds [cpu] postfixes.
 		 * These are not supported by the parser. Remove everything
@@ -782,11 +773,35 @@ static void generic_metric(struct perf_stat_config *config,
 			*pn = 0;
 
 		if (metric_total)
-			expr__add_id(&pctx, n, metric_total);
+			expr__add_id(pctx, n, metric_total);
 		else
-			expr__add_id(&pctx, n, avg_stats(stats)*scale);
+			expr__add_id(pctx, n, avg_stats(stats)*scale);
 	}
 
+	return i;
+}
+
+static void generic_metric(struct perf_stat_config *config,
+			   const char *metric_expr,
+			   struct evsel **metric_events,
+			   char *name,
+			   const char *metric_name,
+			   const char *metric_unit,
+			   int runtime,
+			   int cpu,
+			   struct perf_stat_output_ctx *out,
+			   struct runtime_stat *st)
+{
+	print_metric_t print_metric = out->print_metric;
+	struct expr_parse_ctx pctx;
+	double ratio, scale;
+	int i;
+	void *ctxp = out->ctx;
+
+	i = prepare_metric(metric_events, &pctx, cpu, st);
+	if (i < 0)
+		return;
+
 	if (!metric_events[i]) {
 		if (expr__parse(&ratio, &pctx, metric_expr, runtime) == 0) {
 			char *unit;
-- 
2.25.4

[PATCH 12/14] perf tools: Add test_generic_metric function

[Jiri Olsa]

Adding test_generic_metric that prepares and runs given metric
over the data from psswd struct runtime_stat object.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/perf/util/stat-shadow.c | 14 ++++++++++++++
 tools/perf/util/stat.h        |  3 +++
 2 files changed, 17 insertions(+)

diff --git a/tools/perf/util/stat-shadow.c b/tools/perf/util/stat-shadow.c
index 27be7ce2fff4..8fdef47005e6 100644
--- a/tools/perf/util/stat-shadow.c
+++ b/tools/perf/util/stat-shadow.c
@@ -842,6 +842,20 @@ static void generic_metric(struct perf_stat_config *config,
 	expr__ctx_clear(&pctx);
 }
 
+double test_generic_metric(struct metric_expr *mexp, int cpu, struct runtime_stat *st)
+{
+	struct expr_parse_ctx pctx;
+	double ratio;
+
+	if (prepare_metric(mexp->metric_events, &pctx, cpu, st) < 0)
+		return 0.;
+
+	if (expr__parse(&ratio, &pctx, mexp->metric_expr, 1))
+		return 0.;
+
+	return ratio;
+}
+
 void perf_stat__print_shadow_stats(struct perf_stat_config *config,
 				   struct evsel *evsel,
 				   double avg, int cpu,
diff --git a/tools/perf/util/stat.h b/tools/perf/util/stat.h
index f75ae679eb28..6911c7249199 100644
--- a/tools/perf/util/stat.h
+++ b/tools/perf/util/stat.h
@@ -230,4 +230,7 @@ perf_evlist__print_counters(struct evlist *evlist,
 			    struct target *_target,
 			    struct timespec *ts,
 			    int argc, const char **argv);
+
+struct metric_expr;
+double test_generic_metric(struct metric_expr *mexp, int cpu, struct runtime_stat *st);
 #endif
-- 
2.25.4

[PATCH 13/14] perf tests: Add parse metric test for ipc metric

[Jiri Olsa]

Adding new test that process metrics code and checks
the expected results. Starting with easy ipc metric.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/perf/tests/Build          |   1 +
 tools/perf/tests/builtin-test.c |   4 ++
 tools/perf/tests/parse-metric.c | 117 ++++++++++++++++++++++++++++++++
 tools/perf/tests/tests.h        |   1 +
 4 files changed, 123 insertions(+)
 create mode 100644 tools/perf/tests/parse-metric.c

diff --git a/tools/perf/tests/Build b/tools/perf/tests/Build
index c75557aeef0e..bb7c2d8364d1 100644
--- a/tools/perf/tests/Build
+++ b/tools/perf/tests/Build
@@ -57,6 +57,7 @@ perf-y += maps.o
 perf-y += time-utils-test.o
 perf-y += genelf.o
 perf-y += api-io.o
+perf-y += parse-metric.o
 
 $(OUTPUT)tests/llvm-src-base.c: tests/bpf-script-example.c tests/Build
 	$(call rule_mkdir)
diff --git a/tools/perf/tests/builtin-test.c b/tools/perf/tests/builtin-test.c
index a9daaeb9fd27..bf20abdca0b0 100644
--- a/tools/perf/tests/builtin-test.c
+++ b/tools/perf/tests/builtin-test.c
@@ -324,6 +324,10 @@ static struct test generic_tests[] = {
 		.desc = "maps__merge_in",
 		.func = test__maps__merge_in,
 	},
+	{
+		.desc = "Parse and process metrics",
+		.func = test__parse_metric,
+	},
 	{
 		.func = NULL,
 	},
diff --git a/tools/perf/tests/parse-metric.c b/tools/perf/tests/parse-metric.c
new file mode 100644
index 000000000000..3005d27c5c48
--- /dev/null
+++ b/tools/perf/tests/parse-metric.c
@@ -0,0 +1,117 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <linux/compiler.h>
+#include <string.h>
+#include "metricgroup.h"
+#include "tests.h"
+#include "pmu-events/pmu-events.h"
+#include "evlist.h"
+#include "rblist.h"
+#include "debug.h"
+#include "expr.h"
+#include "stat.h"
+
+static struct pmu_event pme_test[] = {
+{
+	.metric_expr	= "inst_retired.any / cpu_clk_unhalted.thread",
+	.metric_name	= "IPC",
+},
+};
+
+static struct pmu_events_map map = {
+	.cpuid		= "test",
+	.version	= "1",
+	.type		= "core",
+	.table		= pme_test,
+};
+
+static double compute_single(struct rblist *metric_events, struct evsel *evsel,
+			     struct runtime_stat *st)
+{
+	struct metric_event *me;
+
+	me = metricgroup__lookup(metric_events, evsel, false);
+	if (me != NULL) {
+		struct metric_expr *mexp;
+
+		mexp = list_first_entry(&me->head, struct metric_expr, nd);
+		return test_generic_metric(mexp, 0, st);
+	}
+
+	return 0.;
+}
+
+struct value {
+	const char	*event;
+	u64		 val;
+};
+
+static u64 find_value(const char *name, struct value *values)
+{
+	struct value *v = values;
+
+	while (v->event) {
+		if (!strcmp(name, v->event))
+			return v->val;
+		v++;
+	};
+
+	return 0;
+}
+
+static void load_runtime_stat(struct runtime_stat *st, struct evlist *evlist,
+			      struct value *values)
+{
+	struct evsel *evsel;
+	u64 count;
+
+	evlist__for_each_entry(evlist, evsel) {
+		count = find_value(evsel->name, values);
+		perf_stat__update_shadow_stats(evsel, count, 0, st);
+	}
+}
+
+static int test_ipc(void)
+{
+	double ratio;
+	struct rblist metric_events = {
+		.nr_entries = 0,
+	};
+	struct evlist *evlist;
+	struct evsel *evsel;
+	struct value vals[] = {
+		{ .event = "inst_retired.any",        .val = 300 },
+		{ .event = "cpu_clk_unhalted.thread", .val = 200 },
+		{ 0 },
+	};
+	struct runtime_stat st;
+	int err;
+
+	evlist = evlist__new();
+	if (!evlist)
+		return -1;
+
+	err = metricgroup__parse_groups_test(evlist, &map,
+					     "IPC",
+					     false, false,
+					     &metric_events);
+
+	TEST_ASSERT_VAL("failed to parse metrics", err == 0);
+
+	runtime_stat__init(&st);
+	load_runtime_stat(&st, evlist, vals);
+
+	evsel = evlist__first(evlist);
+	ratio = compute_single(&metric_events, evsel, &st);
+
+	TEST_ASSERT_VAL("IPC failed, wrong ratio", ratio == 1.5);
+
+	runtime_stat__exit(&st);
+	evlist__delete(evlist);
+	return 0;
+}
+
+int test__parse_metric(struct test *test __maybe_unused, int subtest __maybe_unused)
+{
+	TEST_ASSERT_VAL("IPC failed", test_ipc() == 0);
+	return 0;
+}
diff --git a/tools/perf/tests/tests.h b/tools/perf/tests/tests.h
index 6c6c4b6a4796..0a7853b72240 100644
--- a/tools/perf/tests/tests.h
+++ b/tools/perf/tests/tests.h
@@ -117,6 +117,7 @@ int test__maps__merge_in(struct test *t, int subtest);
 int test__time_utils(struct test *t, int subtest);
 int test__jit_write_elf(struct test *test, int subtest);
 int test__api_io(struct test *test, int subtest);
+int test__parse_metric(struct test *test, int subtest);
 
 bool test__bp_signal_is_supported(void);
 bool test__bp_account_is_supported(void);
-- 
2.25.4

[PATCH 14/14] perf tests: Add parse metric test for frontend metric

[Jiri Olsa]

Adding new metri test for frontend metric. It's stolen
from x86 pmu events.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/perf/tests/parse-metric.c | 46 +++++++++++++++++++++++++++++++++
 1 file changed, 46 insertions(+)

diff --git a/tools/perf/tests/parse-metric.c b/tools/perf/tests/parse-metric.c
index 3005d27c5c48..38f20850bba3 100644
--- a/tools/perf/tests/parse-metric.c
+++ b/tools/perf/tests/parse-metric.c
@@ -15,6 +15,11 @@ static struct pmu_event pme_test[] = {
 	.metric_expr	= "inst_retired.any / cpu_clk_unhalted.thread",
 	.metric_name	= "IPC",
 },
+{
+	.metric_expr	= "idq_uops_not_delivered.core / (4 * (( ( cpu_clk_unhalted.thread / 2 ) * "
+			  "( 1 + cpu_clk_unhalted.one_thread_active / cpu_clk_unhalted.ref_xclk ) )))",
+	.metric_name	= "Frontend_Bound_SMT",
+},
 };
 
 static struct pmu_events_map map = {
@@ -110,8 +115,49 @@ static int test_ipc(void)
 	return 0;
 }
 
+static int test_frontend(void)
+{
+	double ratio;
+	struct rblist metric_events = { 0 };
+	struct evlist *evlist;
+	struct evsel *evsel;
+	struct value vals[] = {
+		{ .event = "idq_uops_not_delivered.core",        .val = 300 },
+		{ .event = "cpu_clk_unhalted.thread",            .val = 200 },
+		{ .event = "cpu_clk_unhalted.one_thread_active", .val = 400 },
+		{ .event = "cpu_clk_unhalted.ref_xclk",          .val = 600 },
+		{ 0 },
+	};
+	struct runtime_stat st;
+	int err;
+
+	evlist = evlist__new();
+	if (!evlist)
+		return -1;
+
+	err = metricgroup__parse_groups_test(evlist, &map,
+					     "Frontend_Bound_SMT",
+					     false, false,
+					     &metric_events);
+
+	TEST_ASSERT_VAL("failed to parse metrics", err == 0);
+
+	runtime_stat__init(&st);
+	load_runtime_stat(&st, evlist, vals);
+
+	evsel = evlist__first(evlist);
+	ratio = compute_single(&metric_events, evsel, &st);
+
+	TEST_ASSERT_VAL("Frontend_Bound_SMT failed, wrong ratio", ratio == 0.45);
+
+	runtime_stat__exit(&st);
+	evlist__delete(evlist);
+	return 0;
+}
+
 int test__parse_metric(struct test *test __maybe_unused, int subtest __maybe_unused)
 {
 	TEST_ASSERT_VAL("IPC failed", test_ipc() == 0);
+	TEST_ASSERT_VAL("frontend failed", test_frontend() == 0);
 	return 0;
 }
-- 
2.25.4

Re: [RFC 00/14] perf tests: Check on subtest for user specified test

[Michael Petlan]

On Mon, 25 May 2020, Jiri Olsa wrote:
> hi,
> changes for using metric result in another metric seem
> to change lot of core metric code, so it's better we
> have some more tests before we do that.
> 
> Sending as RFC as it's still alive and you guys might
> have some other idea of how to do this.
> 
> Also available in here:
>   git://git.kernel.org/pub/scm/linux/kernel/git/jolsa/perf.git
>   perf/fixes
> 
> jirka
> 
Hi!
Last commit from perf/fixes branch I see there is:

commit 0445062df28fef1002302aa419af65fa80513dd4 (HEAD -> perf/fixes, origin/perf/fixes)
Author: Jiri Olsa <jolsa@kernel.org>
Date:   Fri Dec 6 00:10:13 2019 +0100

Different branch?

> 
> ---
> Jiri Olsa (14):
>       perf tests: Check on subtest for user specified test
>       perf tools: Do not pass avg to generic_metric
>       perf tools: Add struct parse_events_state pointer to scanner
>       perf tools: Add fake pmu support
>       perf tools: Add parse_events_fake interface
>       perf tests: Add another pmu-events tests
>       perf tools: Factor out parse_groups function
>       perf tools: Add metricgroup__parse_groups_test function
>       perf tools: Add fake_pmu to parse_events function
>       perf tools: Add map to parse_events function
>       perf tools: Factor out prepare_metric function
>       perf tools: Add test_generic_metric function
>       perf tests: Add parse metric test for ipc metric
>       perf tests: Add parse metric test for frontend metric
> 
>  tools/perf/tests/Build          |   1 +
>  tools/perf/tests/builtin-test.c |  38 ++++++++++++++++++++++------
>  tools/perf/tests/parse-metric.c | 163 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  tools/perf/tests/pmu-events.c   | 120 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  tools/perf/tests/tests.h        |   1 +
>  tools/perf/util/metricgroup.c   |  53 ++++++++++++++++++++++++++++++---------
>  tools/perf/util/metricgroup.h   |   9 +++++++
>  tools/perf/util/parse-events.c  |  73 ++++++++++++++++++++++++++++++++++++++---------------
>  tools/perf/util/parse-events.h  |   6 ++++-
>  tools/perf/util/parse-events.l  |  16 +++++++-----
>  tools/perf/util/parse-events.y  |  37 +++++++++++++++++++++++++--
>  tools/perf/util/stat-shadow.c   |  77 ++++++++++++++++++++++++++++++++++++--------------------
>  tools/perf/util/stat.h          |   3 +++
>  13 files changed, 521 insertions(+), 76 deletions(-)
>  create mode 100644 tools/perf/tests/parse-metric.c
> 

Re: [RFC 00/14] perf tests: Check on subtest for user specified test

[Jiri Olsa]

On Mon, May 25, 2020 at 02:06:07PM +0200, Michael Petlan wrote:
> On Mon, 25 May 2020, Jiri Olsa wrote:
> > hi,
> > changes for using metric result in another metric seem
> > to change lot of core metric code, so it's better we
> > have some more tests before we do that.
> > 
> > Sending as RFC as it's still alive and you guys might
> > have some other idea of how to do this.
> > 
> > Also available in here:
> >   git://git.kernel.org/pub/scm/linux/kernel/git/jolsa/perf.git
> >   perf/fixes
> > 
> > jirka
> > 
> Hi!
> Last commit from perf/fixes branch I see there is:
> 
> commit 0445062df28fef1002302aa419af65fa80513dd4 (HEAD -> perf/fixes, origin/perf/fixes)
> Author: Jiri Olsa <jolsa@kernel.org>
> Date:   Fri Dec 6 00:10:13 2019 +0100
> 
> Different branch?

ugh.. sorry it's perf/metric_test

thanks,
jirka

Re: [RFC 00/14] perf tests: Check on subtest for user specified test

[Arnaldo Carvalho de Melo]

Em Mon, May 25, 2020 at 12:42:05AM +0200, Jiri Olsa escreveu:
> hi,
> changes for using metric result in another metric seem
> to change lot of core metric code, so it's better we
> have some more tests before we do that.
> 
> Sending as RFC as it's still alive and you guys might
> have some other idea of how to do this.
> 
> Also available in here:
>   git://git.kernel.org/pub/scm/linux/kernel/git/jolsa/perf.git
>   perf/fixes

I applied the first three patches, will wait a bit for Ian and others to
have some time to look at it, but one thing I thought was that instead
of having parse_state->fake_pmu as a bool, you could have it as a
pointer to the fake pmu, this way we would do away with that static
thing in the middle of the parsing code.

+static int check_id(const char *id)
+{
+	struct parse_events_error error;
+	struct evlist *evlist;
+	int ret;
+
+	/* Numbers are always valid. */
+	if (is_number(id))
+               return 0;
+
+	evlist = evlist__new();
+	if (!evlist)
+               return -1;
+
+	memset(&error, 0, sizeof(error));
+       ret = parse_events_fake(evlist, id, &error);
+       if (ret) {
+               pr_debug("str        : %s\n", error.str);
+               pr_debug("help       : %s\n", error.help);
+               pr_debug("first_str  : %s\n", error.first_str);
+               pr_debug("first_help : %s\n", error.first_help);
+       }
+
+       evlist__delete(evlist);
+       free(error.str);
+	free(error.help);
+       free(error.first_str);
+       free(error.first_help);
+	return ret;
+}


Would read:

	struct perf_pmu fake = { 0, };
	.
	.
	.
	ret = parse_events_fake_pmu(evlist, id, &fake, &error);
	.
	.
	.


That also renames parse_events_fake() to parse_events_fake_pmu().
 
> jirka
> 
> 
> ---
> Jiri Olsa (14):
>       perf tests: Check on subtest for user specified test
>       perf tools: Do not pass avg to generic_metric
>       perf tools: Add struct parse_events_state pointer to scanner
>       perf tools: Add fake pmu support
>       perf tools: Add parse_events_fake interface
>       perf tests: Add another pmu-events tests
>       perf tools: Factor out parse_groups function
>       perf tools: Add metricgroup__parse_groups_test function
>       perf tools: Add fake_pmu to parse_events function
>       perf tools: Add map to parse_events function
>       perf tools: Factor out prepare_metric function
>       perf tools: Add test_generic_metric function
>       perf tests: Add parse metric test for ipc metric
>       perf tests: Add parse metric test for frontend metric
> 
>  tools/perf/tests/Build          |   1 +
>  tools/perf/tests/builtin-test.c |  38 ++++++++++++++++++++++------
>  tools/perf/tests/parse-metric.c | 163 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  tools/perf/tests/pmu-events.c   | 120 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  tools/perf/tests/tests.h        |   1 +
>  tools/perf/util/metricgroup.c   |  53 ++++++++++++++++++++++++++++++---------
>  tools/perf/util/metricgroup.h   |   9 +++++++
>  tools/perf/util/parse-events.c  |  73 ++++++++++++++++++++++++++++++++++++++---------------
>  tools/perf/util/parse-events.h  |   6 ++++-
>  tools/perf/util/parse-events.l  |  16 +++++++-----
>  tools/perf/util/parse-events.y  |  37 +++++++++++++++++++++++++--
>  tools/perf/util/stat-shadow.c   |  77 ++++++++++++++++++++++++++++++++++++--------------------
>  tools/perf/util/stat.h          |   3 +++
>  13 files changed, 521 insertions(+), 76 deletions(-)
>  create mode 100644 tools/perf/tests/parse-metric.c
> 

-- 

- Arnaldo

Re: [RFC 00/14] perf tests: Check on subtest for user specified test

[Jiri Olsa]

On Mon, May 25, 2020 at 11:23:00AM -0300, Arnaldo Carvalho de Melo wrote:
> Em Mon, May 25, 2020 at 12:42:05AM +0200, Jiri Olsa escreveu:
> > hi,
> > changes for using metric result in another metric seem
> > to change lot of core metric code, so it's better we
> > have some more tests before we do that.
> > 
> > Sending as RFC as it's still alive and you guys might
> > have some other idea of how to do this.
> > 
> > Also available in here:
> >   git://git.kernel.org/pub/scm/linux/kernel/git/jolsa/perf.git
> >   perf/fixes
> 
> I applied the first three patches, will wait a bit for Ian and others to
> have some time to look at it, but one thing I thought was that instead
> of having parse_state->fake_pmu as a bool, you could have it as a
> pointer to the fake pmu, this way we would do away with that static
> thing in the middle of the parsing code.
> 
> +static int check_id(const char *id)
> +{
> +	struct parse_events_error error;
> +	struct evlist *evlist;
> +	int ret;
> +
> +	/* Numbers are always valid. */
> +	if (is_number(id))
> +               return 0;
> +
> +	evlist = evlist__new();
> +	if (!evlist)
> +               return -1;
> +
> +	memset(&error, 0, sizeof(error));
> +       ret = parse_events_fake(evlist, id, &error);
> +       if (ret) {
> +               pr_debug("str        : %s\n", error.str);
> +               pr_debug("help       : %s\n", error.help);
> +               pr_debug("first_str  : %s\n", error.first_str);
> +               pr_debug("first_help : %s\n", error.first_help);
> +       }
> +
> +       evlist__delete(evlist);
> +       free(error.str);
> +	free(error.help);
> +       free(error.first_str);
> +       free(error.first_help);
> +	return ret;
> +}
> 
> 
> Would read:
> 
> 	struct perf_pmu fake = { 0, };
> 	.
> 	.
> 	.
> 	ret = parse_events_fake_pmu(evlist, id, &fake, &error);

hi,

ok I'll check, but what I'd like to keep is to have the fake pmu
defined in just one place, I was initialy thinking to put it on
the list of pmus, but then it'd appear in other places we dont want,
like perf list ;-)

> 	.
> 	.
> 	.
> 
> 
> That also renames parse_events_fake() to parse_events_fake_pmu().

ok, thanks,
jirka

Re: [PATCH 04/14] perf tools: Add fake pmu support

[Ian Rogers]

On Sun, May 24, 2020 at 3:42 PM Jiri Olsa <jolsa@kernel.org> wrote:
>
> Adding the way to create pmu event without the actual
> PMU being in place. This way we can test metrics defined
> for any processors.
>
> The interface is to define fake_pmu in struct parse_events_state
> data. It will be used only in tests via special interface
> function added in following changes.
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  tools/perf/util/parse-events.c | 14 ++++++++++---
>  tools/perf/util/parse-events.h |  3 ++-
>  tools/perf/util/parse-events.l |  8 ++++++--
>  tools/perf/util/parse-events.y | 37 ++++++++++++++++++++++++++++++++--
>  4 files changed, 54 insertions(+), 8 deletions(-)
>
> diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
> index 27b8e49d690e..8304f9b6e6be 100644
> --- a/tools/perf/util/parse-events.c
> +++ b/tools/perf/util/parse-events.c
> @@ -1415,6 +1415,7 @@ int parse_events_add_pmu(struct parse_events_state *parse_state,
>                          bool auto_merge_stats,
>                          bool use_alias)
>  {
> +       bool is_fake = parse_state->fake_pmu;
>         struct perf_event_attr attr;
>         struct perf_pmu_info info;
>         struct perf_pmu *pmu;
> @@ -1436,7 +1437,14 @@ int parse_events_add_pmu(struct parse_events_state *parse_state,
>                 fprintf(stderr, "' that may result in non-fatal errors\n");
>         }
>
> -       pmu = perf_pmu__find(name);
> +       if (is_fake) {
> +               static struct perf_pmu fake_pmu = { };
> +
> +               pmu = &fake_pmu;
> +       } else {
> +               pmu = perf_pmu__find(name);
> +       }
> +
>         if (!pmu) {
>                 char *err_str;
>
> @@ -1469,7 +1477,7 @@ int parse_events_add_pmu(struct parse_events_state *parse_state,
>                 }
>         }
>
> -       if (perf_pmu__check_alias(pmu, head_config, &info))
> +       if (!is_fake && perf_pmu__check_alias(pmu, head_config, &info))
>                 return -EINVAL;
>
>         if (verbose > 1) {
> @@ -1502,7 +1510,7 @@ int parse_events_add_pmu(struct parse_events_state *parse_state,
>         if (pmu->default_config && get_config_chgs(pmu, head_config, &config_terms))
>                 return -ENOMEM;
>
> -       if (perf_pmu__config(pmu, &attr, head_config, parse_state->error)) {
> +       if (!is_fake && perf_pmu__config(pmu, &attr, head_config, parse_state->error)) {
>                 struct evsel_config_term *pos, *tmp;
>
>                 list_for_each_entry_safe(pos, tmp, &config_terms, list) {
> diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h
> index d60510e0609f..963b0ea6c448 100644
> --- a/tools/perf/util/parse-events.h
> +++ b/tools/perf/util/parse-events.h
> @@ -126,9 +126,10 @@ struct parse_events_state {
>         int                        idx;
>         int                        nr_groups;
>         struct parse_events_error *error;
> -       struct evlist     *evlist;
> +       struct evlist             *evlist;
>         struct list_head          *terms;
>         int                        stoken;
> +       bool                       fake_pmu;
>  };
>
>  void parse_events__handle_error(struct parse_events_error *err, int idx,
> diff --git a/tools/perf/util/parse-events.l b/tools/perf/util/parse-events.l
> index 002802e17059..56912c9641f5 100644
> --- a/tools/perf/util/parse-events.l
> +++ b/tools/perf/util/parse-events.l
> @@ -129,12 +129,16 @@ do {                                                              \
>         yyless(0);                                              \
>  } while (0)
>
> -static int pmu_str_check(yyscan_t scanner)
> +static int pmu_str_check(yyscan_t scanner, struct parse_events_state *parse_state)
>  {
>         YYSTYPE *yylval = parse_events_get_lval(scanner);
>         char *text = parse_events_get_text(scanner);
>
>         yylval->str = strdup(text);
> +
> +       if (parse_state->fake_pmu)
> +               return PE_PMU_EVENT_FAKE;
> +
>         switch (perf_pmu__parse_check(text)) {
>                 case PMU_EVENT_SYMBOL_PREFIX:
>                         return PE_PMU_EVENT_PRE;
> @@ -376,7 +380,7 @@ r{num_raw_hex}              { return raw(yyscanner); }
>  {modifier_event}       { return str(yyscanner, PE_MODIFIER_EVENT); }
>  {bpf_object}           { if (!isbpf(yyscanner)) { USER_REJECT }; return str(yyscanner, PE_BPF_OBJECT); }
>  {bpf_source}           { if (!isbpf(yyscanner)) { USER_REJECT }; return str(yyscanner, PE_BPF_SOURCE); }
> -{name}                 { return pmu_str_check(yyscanner); }
> +{name}                 { return pmu_str_check(yyscanner, _parse_state); }
>  {name_tag}             { return str(yyscanner, PE_NAME); }
>  "/"                    { BEGIN(config); return '/'; }
>  -                      { return '-'; }
> diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y
> index c4ca932d092d..d1b04b8d81ea 100644
> --- a/tools/perf/util/parse-events.y
> +++ b/tools/perf/util/parse-events.y
> @@ -69,7 +69,7 @@ static void inc_group_count(struct list_head *list,
>  %token PE_NAME_CACHE_TYPE PE_NAME_CACHE_OP_RESULT
>  %token PE_PREFIX_MEM PE_PREFIX_RAW PE_PREFIX_GROUP
>  %token PE_ERROR
> -%token PE_PMU_EVENT_PRE PE_PMU_EVENT_SUF PE_KERNEL_PMU_EVENT
> +%token PE_PMU_EVENT_PRE PE_PMU_EVENT_SUF PE_KERNEL_PMU_EVENT PE_PMU_EVENT_FAKE
>  %token PE_ARRAY_ALL PE_ARRAY_RANGE
>  %token PE_DRV_CFG_TERM
>  %type <num> PE_VALUE
> @@ -87,7 +87,7 @@ static void inc_group_count(struct list_head *list,
>  %type <str> PE_MODIFIER_EVENT
>  %type <str> PE_MODIFIER_BP
>  %type <str> PE_EVENT_NAME
> -%type <str> PE_PMU_EVENT_PRE PE_PMU_EVENT_SUF PE_KERNEL_PMU_EVENT
> +%type <str> PE_PMU_EVENT_PRE PE_PMU_EVENT_SUF PE_KERNEL_PMU_EVENT PE_PMU_EVENT_FAKE
>  %type <str> PE_DRV_CFG_TERM
>  %destructor { free ($$); } <str>
>  %type <term> event_term
> @@ -356,6 +356,39 @@ PE_PMU_EVENT_PRE '-' PE_PMU_EVENT_SUF sep_dc
>                 YYABORT;
>         $$ = list;
>  }
> +|
> +PE_PMU_EVENT_FAKE sep_dc
> +{
> +       struct list_head *list;
> +       int err;
> +
> +       list = alloc_list();
> +       if (!list)
> +               YYABORT;
> +
> +       err = parse_events_add_pmu(_parse_state, list, $1, NULL, false, false);
> +       free($1);
> +       if (err < 0)

nit: on error this needs:
free(list);
otherwise something like fuzz testing could go down the error path and
complain about memory leaks.

Thanks,
Ian

> +               YYABORT;
> +       $$ = list;
> +}
> +|
> +PE_PMU_EVENT_FAKE opt_pmu_config
> +{
> +       struct list_head *list;
> +       int err;
> +
> +       list = alloc_list();
> +       if (!list)
> +               YYABORT;
> +
> +       err = parse_events_add_pmu(_parse_state, list, $1, $2, false, false);
> +       free($1);
> +       parse_events_terms__delete($2);
> +       if (err < 0)
> +               YYABORT;
> +       $$ = list;
> +}
>
>  value_sym:
>  PE_VALUE_SYM_HW
> --
> 2.25.4
>

Re: [PATCH 05/14] perf tools: Add parse_events_fake interface

[Ian Rogers]

On Sun, May 24, 2020 at 3:42 PM Jiri Olsa <jolsa@kernel.org> wrote:
>
> Adding parse_events_fake interface to parse events
> and use fake pmu event in case pmu event is parsed.
>
> This way it's possible to parse events from PMUs
> which are not present in the system. It's available
> only for testing purposes coming in following
> changes.
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>

Acked-by: Ian Rogers <irogers@google.com>

Alternatively fake_pmu could be an argument to parse_events.

Thanks,
Ian

> ---
>  tools/perf/util/parse-events.c | 48 +++++++++++++++++++++++++---------
>  tools/perf/util/parse-events.h |  2 ++
>  2 files changed, 37 insertions(+), 13 deletions(-)
>
> diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
> index 8304f9b6e6be..89239695a728 100644
> --- a/tools/perf/util/parse-events.c
> +++ b/tools/perf/util/parse-events.c
> @@ -2082,22 +2082,15 @@ int parse_events_terms(struct list_head *terms, const char *str)
>         return ret;
>  }
>
> -int parse_events(struct evlist *evlist, const char *str,
> -                struct parse_events_error *err)
> +static int parse_events_state(struct parse_events_state *parse_state,
> +                             struct evlist *evlist, const char *str)
>  {
> -       struct parse_events_state parse_state = {
> -               .list   = LIST_HEAD_INIT(parse_state.list),
> -               .idx    = evlist->core.nr_entries,
> -               .error  = err,
> -               .evlist = evlist,
> -               .stoken = PE_START_EVENTS,
> -       };
>         int ret;
>
> -       ret = parse_events__scanner(str, &parse_state);
> +       ret = parse_events__scanner(str, parse_state);
>         perf_pmu__parse_cleanup();
>
> -       if (!ret && list_empty(&parse_state.list)) {
> +       if (!ret && list_empty(&parse_state->list)) {
>                 WARN_ONCE(true, "WARNING: event parser found nothing\n");
>                 return -1;
>         }
> @@ -2105,12 +2098,12 @@ int parse_events(struct evlist *evlist, const char *str,
>         /*
>          * Add list to the evlist even with errors to allow callers to clean up.
>          */
> -       perf_evlist__splice_list_tail(evlist, &parse_state.list);
> +       perf_evlist__splice_list_tail(evlist, &parse_state->list);
>
>         if (!ret) {
>                 struct evsel *last;
>
> -               evlist->nr_groups += parse_state.nr_groups;
> +               evlist->nr_groups += parse_state->nr_groups;
>                 last = evlist__last(evlist);
>                 last->cmdline_group_boundary = true;
>
> @@ -2125,6 +2118,35 @@ int parse_events(struct evlist *evlist, const char *str,
>         return ret;
>  }
>
> +int parse_events(struct evlist *evlist, const char *str,
> +                struct parse_events_error *err)
> +{
> +       struct parse_events_state parse_state = {
> +               .list   = LIST_HEAD_INIT(parse_state.list),
> +               .idx    = evlist->core.nr_entries,
> +               .error  = err,
> +               .evlist = evlist,
> +               .stoken = PE_START_EVENTS,
> +       };
> +
> +       return parse_events_state(&parse_state, evlist, str);
> +}
> +
> +int parse_events_fake(struct evlist *evlist, const char *str,
> +                     struct parse_events_error *err)
> +{
> +       struct parse_events_state parse_state = {
> +               .list     = LIST_HEAD_INIT(parse_state.list),
> +               .idx      = evlist->core.nr_entries,
> +               .error    = err,
> +               .evlist   = evlist,
> +               .stoken   = PE_START_EVENTS,
> +               .fake_pmu = true,
> +       };
> +
> +       return parse_events_state(&parse_state, evlist, str);
> +}
> +
>  #define MAX_WIDTH 1000
>  static int get_term_width(void)
>  {
> diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h
> index 963b0ea6c448..4a23b6cd9924 100644
> --- a/tools/perf/util/parse-events.h
> +++ b/tools/perf/util/parse-events.h
> @@ -34,6 +34,8 @@ int parse_events_option(const struct option *opt, const char *str, int unset);
>  int parse_events_option_new_evlist(const struct option *opt, const char *str, int unset);
>  int parse_events(struct evlist *evlist, const char *str,
>                  struct parse_events_error *error);
> +int parse_events_fake(struct evlist *evlist, const char *str,
> +                     struct parse_events_error *error);
>  int parse_events_terms(struct list_head *terms, const char *str);
>  int parse_filter(const struct option *opt, const char *str, int unset);
>  int exclude_perf(const struct option *opt, const char *arg, int unset);
> --
> 2.25.4
>

Re: [PATCH 06/14] perf tests: Add another pmu-events tests

[Ian Rogers]

On Sun, May 24, 2020 at 3:42 PM Jiri Olsa <jolsa@kernel.org> wrote:
>
> The test goes through all metrics compiled for arch
> within pmu events and try to parse them.
>
> The test also defines its own list of metrics and
> tries to parse them. It's handy for developing.
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  tools/perf/tests/pmu-events.c | 120 ++++++++++++++++++++++++++++++++++
>  1 file changed, 120 insertions(+)
>
> diff --git a/tools/perf/tests/pmu-events.c b/tools/perf/tests/pmu-events.c
> index ab64b4a4e284..f100df025440 100644
> --- a/tools/perf/tests/pmu-events.c
> +++ b/tools/perf/tests/pmu-events.c
> @@ -490,6 +490,122 @@ static int test_parsing(void)
>         return ret == 0 ? TEST_OK : TEST_SKIP;
>  }
>
> +struct test_metric {
> +       const char *str;
> +};
> +
> +static struct test_metric metrics[] = {
> +       { "(unc_p_power_state_occupancy.cores_c0 / unc_p_clockticks) * 100." },
> +       { "imx8_ddr0@read\\-cycles@ * 4 * 4", },
> +       { "imx8_ddr0@axid\\-read\\,axi_mask\\=0xffff\\,axi_id\\=0x0000@ * 4", },
> +       { "(cstate_pkg@c2\\-residency@ / msr@tsc@) * 100", },
> +       { "(imx8_ddr0@read\\-cycles@ + imx8_ddr0@write\\-cycles@)", },
> +};
> +
> +static int check_id(const char *id)
> +{
> +       struct parse_events_error error;
> +       struct evlist *evlist;
> +       int ret;
> +
> +       /* Numbers are always valid. */
> +       if (is_number(id))
> +               return 0;
> +
> +       evlist = evlist__new();
> +       if (!evlist)
> +               return -1;
> +
> +       memset(&error, 0, sizeof(error));
> +       ret = parse_events_fake(evlist, id, &error);
> +       if (ret) {
> +               pr_debug("str        : %s\n", error.str);
> +               pr_debug("help       : %s\n", error.help);
> +               pr_debug("first_str  : %s\n", error.first_str);
> +               pr_debug("first_help : %s\n", error.first_help);
> +       }
> +
> +       evlist__delete(evlist);
> +       free(error.str);
> +       free(error.help);
> +       free(error.first_str);
> +       free(error.first_help);
> +       return ret;
> +}

This is quite similar to check_parse_id, fold them together?

> +
> +static int metric_parse_fake(const char *str)
> +{
> +       struct expr_parse_ctx ctx;
> +       struct hashmap_entry *cur;
> +       double result;
> +       int ret = -1;
> +       size_t bkt;
> +       int i;
> +
> +       pr_debug("parsing '%s'\n", str);
> +
> +       expr__ctx_init(&ctx);
> +       if (expr__find_other(str, NULL, &ctx, 0) < 0) {
> +               pr_err("expr__find_other failed\n");
> +               return -1;
> +       }
> +
> +       i = 1;
> +       hashmap__for_each_entry((&ctx.ids), cur, bkt)
> +               expr__add_id(&ctx, strdup(cur->key), i++);

It might make sense to share the code here with that in test_parsing.
This initialization of ids is commented there and it is a bit special.

Thanks,
Ian

> +
> +       hashmap__for_each_entry((&ctx.ids), cur, bkt) {
> +               if (check_id(cur->key)) {
> +                       pr_err("check_id failed\n");
> +                       goto out;
> +               }
> +       }
> +
> +       if (!expr__parse(&result, &ctx, str, 1))
> +               ret = 0;
> +       else
> +               pr_err("expr__parse failed\n");
> +
> +out:
> +       expr__ctx_clear(&ctx);
> +       return ret;
> +
> +}
> +
> +static int test_parsing_fake(void)
> +{
> +       struct pmu_events_map *map;
> +       struct pmu_event *pe;
> +       unsigned int i, j;
> +       int err = 0;
> +
> +       for (i = 0; !err && i < ARRAY_SIZE(metrics); i++)
> +               err = metric_parse_fake(metrics[i].str);
> +
> +       if (err)
> +               return err;
> +
> +       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;
> +                       err = metric_parse_fake(pe->metric_expr);
> +                       if (err)
> +                               return err;
> +               }
> +       }
> +
> +       return 0;
> +}
> +
>  static const struct {
>         int (*func)(void);
>         const char *desc;
> @@ -506,6 +622,10 @@ static const struct {
>                 .func = test_parsing,
>                 .desc = "Parsing of PMU event table metrics",
>         },
> +       {
> +               .func = test_parsing_fake,
> +               .desc = "Parsing of PMU event table metrics with fake PMUs",
> +       },
>  };
>
>  const char *test__pmu_events_subtest_get_desc(int subtest)
> --
> 2.25.4
>

Re: [PATCH 13/14] perf tests: Add parse metric test for ipc metric

[Ian Rogers]

On Sun, May 24, 2020 at 3:43 PM Jiri Olsa <jolsa@kernel.org> wrote:
>
> Adding new test that process metrics code and checks
> the expected results. Starting with easy ipc metric.
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>

Acked-by: Ian Rogers <irogers@google.com>

I wonder if there's a better organization with testing in
pmu-events.c, expr.c and now parse-metric.c.

Thanks,
Ian

> ---
>  tools/perf/tests/Build          |   1 +
>  tools/perf/tests/builtin-test.c |   4 ++
>  tools/perf/tests/parse-metric.c | 117 ++++++++++++++++++++++++++++++++
>  tools/perf/tests/tests.h        |   1 +
>  4 files changed, 123 insertions(+)
>  create mode 100644 tools/perf/tests/parse-metric.c
>
> diff --git a/tools/perf/tests/Build b/tools/perf/tests/Build
> index c75557aeef0e..bb7c2d8364d1 100644
> --- a/tools/perf/tests/Build
> +++ b/tools/perf/tests/Build
> @@ -57,6 +57,7 @@ perf-y += maps.o
>  perf-y += time-utils-test.o
>  perf-y += genelf.o
>  perf-y += api-io.o
> +perf-y += parse-metric.o
>
>  $(OUTPUT)tests/llvm-src-base.c: tests/bpf-script-example.c tests/Build
>         $(call rule_mkdir)
> diff --git a/tools/perf/tests/builtin-test.c b/tools/perf/tests/builtin-test.c
> index a9daaeb9fd27..bf20abdca0b0 100644
> --- a/tools/perf/tests/builtin-test.c
> +++ b/tools/perf/tests/builtin-test.c
> @@ -324,6 +324,10 @@ static struct test generic_tests[] = {
>                 .desc = "maps__merge_in",
>                 .func = test__maps__merge_in,
>         },
> +       {
> +               .desc = "Parse and process metrics",
> +               .func = test__parse_metric,
> +       },
>         {
>                 .func = NULL,
>         },
> diff --git a/tools/perf/tests/parse-metric.c b/tools/perf/tests/parse-metric.c
> new file mode 100644
> index 000000000000..3005d27c5c48
> --- /dev/null
> +++ b/tools/perf/tests/parse-metric.c
> @@ -0,0 +1,117 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#include <linux/compiler.h>
> +#include <string.h>
> +#include "metricgroup.h"
> +#include "tests.h"
> +#include "pmu-events/pmu-events.h"
> +#include "evlist.h"
> +#include "rblist.h"
> +#include "debug.h"
> +#include "expr.h"
> +#include "stat.h"
> +
> +static struct pmu_event pme_test[] = {
> +{
> +       .metric_expr    = "inst_retired.any / cpu_clk_unhalted.thread",
> +       .metric_name    = "IPC",
> +},
> +};
> +
> +static struct pmu_events_map map = {
> +       .cpuid          = "test",
> +       .version        = "1",
> +       .type           = "core",
> +       .table          = pme_test,
> +};
> +
> +static double compute_single(struct rblist *metric_events, struct evsel *evsel,
> +                            struct runtime_stat *st)
> +{
> +       struct metric_event *me;
> +
> +       me = metricgroup__lookup(metric_events, evsel, false);
> +       if (me != NULL) {
> +               struct metric_expr *mexp;
> +
> +               mexp = list_first_entry(&me->head, struct metric_expr, nd);
> +               return test_generic_metric(mexp, 0, st);
> +       }
> +
> +       return 0.;
> +}
> +
> +struct value {
> +       const char      *event;
> +       u64              val;
> +};
> +
> +static u64 find_value(const char *name, struct value *values)
> +{
> +       struct value *v = values;
> +
> +       while (v->event) {
> +               if (!strcmp(name, v->event))
> +                       return v->val;
> +               v++;
> +       };
> +
> +       return 0;
> +}
> +
> +static void load_runtime_stat(struct runtime_stat *st, struct evlist *evlist,
> +                             struct value *values)
> +{
> +       struct evsel *evsel;
> +       u64 count;
> +
> +       evlist__for_each_entry(evlist, evsel) {
> +               count = find_value(evsel->name, values);
> +               perf_stat__update_shadow_stats(evsel, count, 0, st);
> +       }
> +}
> +
> +static int test_ipc(void)
> +{
> +       double ratio;
> +       struct rblist metric_events = {
> +               .nr_entries = 0,
> +       };
> +       struct evlist *evlist;
> +       struct evsel *evsel;
> +       struct value vals[] = {
> +               { .event = "inst_retired.any",        .val = 300 },
> +               { .event = "cpu_clk_unhalted.thread", .val = 200 },
> +               { 0 },
> +       };
> +       struct runtime_stat st;
> +       int err;
> +
> +       evlist = evlist__new();
> +       if (!evlist)
> +               return -1;
> +
> +       err = metricgroup__parse_groups_test(evlist, &map,
> +                                            "IPC",
> +                                            false, false,
> +                                            &metric_events);
> +
> +       TEST_ASSERT_VAL("failed to parse metrics", err == 0);
> +
> +       runtime_stat__init(&st);
> +       load_runtime_stat(&st, evlist, vals);
> +
> +       evsel = evlist__first(evlist);
> +       ratio = compute_single(&metric_events, evsel, &st);
> +
> +       TEST_ASSERT_VAL("IPC failed, wrong ratio", ratio == 1.5);
> +
> +       runtime_stat__exit(&st);
> +       evlist__delete(evlist);
> +       return 0;
> +}
> +
> +int test__parse_metric(struct test *test __maybe_unused, int subtest __maybe_unused)
> +{
> +       TEST_ASSERT_VAL("IPC failed", test_ipc() == 0);
> +       return 0;
> +}
> diff --git a/tools/perf/tests/tests.h b/tools/perf/tests/tests.h
> index 6c6c4b6a4796..0a7853b72240 100644
> --- a/tools/perf/tests/tests.h
> +++ b/tools/perf/tests/tests.h
> @@ -117,6 +117,7 @@ int test__maps__merge_in(struct test *t, int subtest);
>  int test__time_utils(struct test *t, int subtest);
>  int test__jit_write_elf(struct test *test, int subtest);
>  int test__api_io(struct test *test, int subtest);
> +int test__parse_metric(struct test *test, int subtest);
>
>  bool test__bp_signal_is_supported(void);
>  bool test__bp_account_is_supported(void);
> --
> 2.25.4
>

Re: [PATCH 14/14] perf tests: Add parse metric test for frontend metric

[Ian Rogers]

On Sun, May 24, 2020 at 3:43 PM Jiri Olsa <jolsa@kernel.org> wrote:
>
> Adding new metri test for frontend metric. It's stolen

s/metri/metric/

> from x86 pmu events.
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>

Acked-by: Ian Rogers <irogers@google.com>

Thanks,
Ian

> ---
>  tools/perf/tests/parse-metric.c | 46 +++++++++++++++++++++++++++++++++
>  1 file changed, 46 insertions(+)
>
> diff --git a/tools/perf/tests/parse-metric.c b/tools/perf/tests/parse-metric.c
> index 3005d27c5c48..38f20850bba3 100644
> --- a/tools/perf/tests/parse-metric.c
> +++ b/tools/perf/tests/parse-metric.c
> @@ -15,6 +15,11 @@ static struct pmu_event pme_test[] = {
>         .metric_expr    = "inst_retired.any / cpu_clk_unhalted.thread",
>         .metric_name    = "IPC",
>  },
> +{
> +       .metric_expr    = "idq_uops_not_delivered.core / (4 * (( ( cpu_clk_unhalted.thread / 2 ) * "
> +                         "( 1 + cpu_clk_unhalted.one_thread_active / cpu_clk_unhalted.ref_xclk ) )))",
> +       .metric_name    = "Frontend_Bound_SMT",
> +},
>  };
>
>  static struct pmu_events_map map = {
> @@ -110,8 +115,49 @@ static int test_ipc(void)
>         return 0;
>  }
>
> +static int test_frontend(void)
> +{
> +       double ratio;
> +       struct rblist metric_events = { 0 };
> +       struct evlist *evlist;
> +       struct evsel *evsel;
> +       struct value vals[] = {
> +               { .event = "idq_uops_not_delivered.core",        .val = 300 },
> +               { .event = "cpu_clk_unhalted.thread",            .val = 200 },
> +               { .event = "cpu_clk_unhalted.one_thread_active", .val = 400 },
> +               { .event = "cpu_clk_unhalted.ref_xclk",          .val = 600 },
> +               { 0 },
> +       };
> +       struct runtime_stat st;
> +       int err;
> +
> +       evlist = evlist__new();
> +       if (!evlist)
> +               return -1;
> +
> +       err = metricgroup__parse_groups_test(evlist, &map,
> +                                            "Frontend_Bound_SMT",
> +                                            false, false,
> +                                            &metric_events);
> +
> +       TEST_ASSERT_VAL("failed to parse metrics", err == 0);
> +
> +       runtime_stat__init(&st);
> +       load_runtime_stat(&st, evlist, vals);
> +
> +       evsel = evlist__first(evlist);
> +       ratio = compute_single(&metric_events, evsel, &st);
> +
> +       TEST_ASSERT_VAL("Frontend_Bound_SMT failed, wrong ratio", ratio == 0.45);
> +
> +       runtime_stat__exit(&st);
> +       evlist__delete(evlist);
> +       return 0;
> +}
> +
>  int test__parse_metric(struct test *test __maybe_unused, int subtest __maybe_unused)
>  {
>         TEST_ASSERT_VAL("IPC failed", test_ipc() == 0);
> +       TEST_ASSERT_VAL("frontend failed", test_frontend() == 0);
>         return 0;
>  }
> --
> 2.25.4
>

Re: [PATCH 04/14] perf tools: Add fake pmu support

[Jiri Olsa]

On Mon, Jun 01, 2020 at 12:22:30AM -0700, Ian Rogers wrote:

SNIP

> > +
> > +       list = alloc_list();
> > +       if (!list)
> > +               YYABORT;
> > +
> > +       err = parse_events_add_pmu(_parse_state, list, $1, NULL, false, false);
> > +       free($1);
> > +       if (err < 0)
> 
> nit: on error this needs:
> free(list);
> otherwise something like fuzz testing could go down the error path and
> complain about memory leaks.

right, I'll add that

thanks,
jirka

> 
> Thanks,
> Ian
> 
> > +               YYABORT;
> > +       $$ = list;
> > +}
> > +|
> > +PE_PMU_EVENT_FAKE opt_pmu_config
> > +{
> > +       struct list_head *list;
> > +       int err;
> > +
> > +       list = alloc_list();
> > +       if (!list)
> > +               YYABORT;
> > +
> > +       err = parse_events_add_pmu(_parse_state, list, $1, $2, false, false);
> > +       free($1);
> > +       parse_events_terms__delete($2);
> > +       if (err < 0)
> > +               YYABORT;
> > +       $$ = list;
> > +}
> >
> >  value_sym:
> >  PE_VALUE_SYM_HW
> > --
> > 2.25.4
> >
> 

Re: [PATCH 05/14] perf tools: Add parse_events_fake interface

[Jiri Olsa]

On Mon, Jun 01, 2020 at 12:28:31AM -0700, Ian Rogers wrote:
> On Sun, May 24, 2020 at 3:42 PM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > Adding parse_events_fake interface to parse events
> > and use fake pmu event in case pmu event is parsed.
> >
> > This way it's possible to parse events from PMUs
> > which are not present in the system. It's available
> > only for testing purposes coming in following
> > changes.
> >
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> 
> Acked-by: Ian Rogers <irogers@google.com>
> 
> Alternatively fake_pmu could be an argument to parse_events.

yea I checked that and I was surprised how many parse_events calls
we have in perf, so I went this way.. but I haven't really tried it,
so it might look actually etter despite the many places we need to change,
I'll try

thanks,
jirka

> 
> Thanks,
> Ian
> 
> > ---
> >  tools/perf/util/parse-events.c | 48 +++++++++++++++++++++++++---------
> >  tools/perf/util/parse-events.h |  2 ++
> >  2 files changed, 37 insertions(+), 13 deletions(-)
> >
> > diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
> > index 8304f9b6e6be..89239695a728 100644
> > --- a/tools/perf/util/parse-events.c
> > +++ b/tools/perf/util/parse-events.c
> > @@ -2082,22 +2082,15 @@ int parse_events_terms(struct list_head *terms, const char *str)
> >         return ret;
> >  }
> >
> > -int parse_events(struct evlist *evlist, const char *str,
> > -                struct parse_events_error *err)
> > +static int parse_events_state(struct parse_events_state *parse_state,
> > +                             struct evlist *evlist, const char *str)
> >  {
> > -       struct parse_events_state parse_state = {
> > -               .list   = LIST_HEAD_INIT(parse_state.list),
> > -               .idx    = evlist->core.nr_entries,
> > -               .error  = err,
> > -               .evlist = evlist,
> > -               .stoken = PE_START_EVENTS,
> > -       };
> >         int ret;
> >
> > -       ret = parse_events__scanner(str, &parse_state);
> > +       ret = parse_events__scanner(str, parse_state);
> >         perf_pmu__parse_cleanup();
> >
> > -       if (!ret && list_empty(&parse_state.list)) {
> > +       if (!ret && list_empty(&parse_state->list)) {
> >                 WARN_ONCE(true, "WARNING: event parser found nothing\n");
> >                 return -1;
> >         }
> > @@ -2105,12 +2098,12 @@ int parse_events(struct evlist *evlist, const char *str,
> >         /*
> >          * Add list to the evlist even with errors to allow callers to clean up.
> >          */
> > -       perf_evlist__splice_list_tail(evlist, &parse_state.list);
> > +       perf_evlist__splice_list_tail(evlist, &parse_state->list);
> >
> >         if (!ret) {
> >                 struct evsel *last;
> >
> > -               evlist->nr_groups += parse_state.nr_groups;
> > +               evlist->nr_groups += parse_state->nr_groups;
> >                 last = evlist__last(evlist);
> >                 last->cmdline_group_boundary = true;
> >
> > @@ -2125,6 +2118,35 @@ int parse_events(struct evlist *evlist, const char *str,
> >         return ret;
> >  }
> >
> > +int parse_events(struct evlist *evlist, const char *str,
> > +                struct parse_events_error *err)
> > +{
> > +       struct parse_events_state parse_state = {
> > +               .list   = LIST_HEAD_INIT(parse_state.list),
> > +               .idx    = evlist->core.nr_entries,
> > +               .error  = err,
> > +               .evlist = evlist,
> > +               .stoken = PE_START_EVENTS,
> > +       };
> > +
> > +       return parse_events_state(&parse_state, evlist, str);
> > +}
> > +
> > +int parse_events_fake(struct evlist *evlist, const char *str,
> > +                     struct parse_events_error *err)
> > +{
> > +       struct parse_events_state parse_state = {
> > +               .list     = LIST_HEAD_INIT(parse_state.list),
> > +               .idx      = evlist->core.nr_entries,
> > +               .error    = err,
> > +               .evlist   = evlist,
> > +               .stoken   = PE_START_EVENTS,
> > +               .fake_pmu = true,
> > +       };
> > +
> > +       return parse_events_state(&parse_state, evlist, str);
> > +}
> > +
> >  #define MAX_WIDTH 1000
> >  static int get_term_width(void)
> >  {
> > diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h
> > index 963b0ea6c448..4a23b6cd9924 100644
> > --- a/tools/perf/util/parse-events.h
> > +++ b/tools/perf/util/parse-events.h
> > @@ -34,6 +34,8 @@ int parse_events_option(const struct option *opt, const char *str, int unset);
> >  int parse_events_option_new_evlist(const struct option *opt, const char *str, int unset);
> >  int parse_events(struct evlist *evlist, const char *str,
> >                  struct parse_events_error *error);
> > +int parse_events_fake(struct evlist *evlist, const char *str,
> > +                     struct parse_events_error *error);
> >  int parse_events_terms(struct list_head *terms, const char *str);
> >  int parse_filter(const struct option *opt, const char *str, int unset);
> >  int exclude_perf(const struct option *opt, const char *arg, int unset);
> > --
> > 2.25.4
> >
> 

Re: [PATCH 13/14] perf tests: Add parse metric test for ipc metric

[Jiri Olsa]

On Mon, Jun 01, 2020 at 12:55:44AM -0700, Ian Rogers wrote:
> On Sun, May 24, 2020 at 3:43 PM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > Adding new test that process metrics code and checks
> > the expected results. Starting with easy ipc metric.
> >
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> 
> Acked-by: Ian Rogers <irogers@google.com>
> 
> I wonder if there's a better organization with testing in
> pmu-events.c, expr.c and now parse-metric.c.

hum, so 
 - expr.c is testing core interface,
 - parse-metric is testing specific metric processing from
   parsing to final ratio
 - pmu-events.c is testing pmu events aliases and parsing of
   all the metrics

pmu-events.c is testing both pmu events and metrics,
but I think it fits in the way it's done together

jirka

Re: [PATCH 06/14] perf tests: Add another pmu-events tests

[Jiri Olsa]

On Mon, Jun 01, 2020 at 12:44:15AM -0700, Ian Rogers wrote:

SNIP

> > +       memset(&error, 0, sizeof(error));
> > +       ret = parse_events_fake(evlist, id, &error);
> > +       if (ret) {
> > +               pr_debug("str        : %s\n", error.str);
> > +               pr_debug("help       : %s\n", error.help);
> > +               pr_debug("first_str  : %s\n", error.first_str);
> > +               pr_debug("first_help : %s\n", error.first_help);
> > +       }
> > +
> > +       evlist__delete(evlist);
> > +       free(error.str);
> > +       free(error.help);
> > +       free(error.first_str);
> > +       free(error.first_help);
> > +       return ret;
> > +}
> 
> This is quite similar to check_parse_id, fold them together?

there is the 'same_cpu' logic in check_parse_id,
so I did not want to mess with that

> 
> > +
> > +static int metric_parse_fake(const char *str)
> > +{
> > +       struct expr_parse_ctx ctx;
> > +       struct hashmap_entry *cur;
> > +       double result;
> > +       int ret = -1;
> > +       size_t bkt;
> > +       int i;
> > +
> > +       pr_debug("parsing '%s'\n", str);
> > +
> > +       expr__ctx_init(&ctx);
> > +       if (expr__find_other(str, NULL, &ctx, 0) < 0) {
> > +               pr_err("expr__find_other failed\n");
> > +               return -1;
> > +       }
> > +
> > +       i = 1;
> > +       hashmap__for_each_entry((&ctx.ids), cur, bkt)
> > +               expr__add_id(&ctx, strdup(cur->key), i++);
> 
> It might make sense to share the code here with that in test_parsing.
> This initialization of ids is commented there and it is a bit special.

hum, not sure it's worth to add this complexity to test, I'd like
to keep it simple, it's already not straightforward ;-) I added the
comment you mentioned

jirka

Re: [PATCH 05/14] perf tools: Add parse_events_fake interface

[Arnaldo Carvalho de Melo]

Em Mon, Jun 01, 2020 at 11:08:50AM +0200, Jiri Olsa escreveu:
> On Mon, Jun 01, 2020 at 12:28:31AM -0700, Ian Rogers wrote:
> > On Sun, May 24, 2020 at 3:42 PM Jiri Olsa <jolsa@kernel.org> wrote:
> > >
> > > Adding parse_events_fake interface to parse events
> > > and use fake pmu event in case pmu event is parsed.
> > >
> > > This way it's possible to parse events from PMUs
> > > which are not present in the system. It's available
> > > only for testing purposes coming in following
> > > changes.
> > >
> > > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > 
> > Acked-by: Ian Rogers <irogers@google.com>
> > 
> > Alternatively fake_pmu could be an argument to parse_events.
> 
> yea I checked that and I was surprised how many parse_events calls
> we have in perf, so I went this way.. but I haven't really tried it,
> so it might look actually etter despite the many places we need to change,
> I'll try

Thanks! My admitedly unchecked thinking is that most places will just
pass NULL, only the test case will pass it, right?

- Arnaldo
 
> thanks,
> jirka
> 
> > 
> > Thanks,
> > Ian
> > 
> > > ---
> > >  tools/perf/util/parse-events.c | 48 +++++++++++++++++++++++++---------
> > >  tools/perf/util/parse-events.h |  2 ++
> > >  2 files changed, 37 insertions(+), 13 deletions(-)
> > >
> > > diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
> > > index 8304f9b6e6be..89239695a728 100644
> > > --- a/tools/perf/util/parse-events.c
> > > +++ b/tools/perf/util/parse-events.c
> > > @@ -2082,22 +2082,15 @@ int parse_events_terms(struct list_head *terms, const char *str)
> > >         return ret;
> > >  }
> > >
> > > -int parse_events(struct evlist *evlist, const char *str,
> > > -                struct parse_events_error *err)
> > > +static int parse_events_state(struct parse_events_state *parse_state,
> > > +                             struct evlist *evlist, const char *str)
> > >  {
> > > -       struct parse_events_state parse_state = {
> > > -               .list   = LIST_HEAD_INIT(parse_state.list),
> > > -               .idx    = evlist->core.nr_entries,
> > > -               .error  = err,
> > > -               .evlist = evlist,
> > > -               .stoken = PE_START_EVENTS,
> > > -       };
> > >         int ret;
> > >
> > > -       ret = parse_events__scanner(str, &parse_state);
> > > +       ret = parse_events__scanner(str, parse_state);
> > >         perf_pmu__parse_cleanup();
> > >
> > > -       if (!ret && list_empty(&parse_state.list)) {
> > > +       if (!ret && list_empty(&parse_state->list)) {
> > >                 WARN_ONCE(true, "WARNING: event parser found nothing\n");
> > >                 return -1;
> > >         }
> > > @@ -2105,12 +2098,12 @@ int parse_events(struct evlist *evlist, const char *str,
> > >         /*
> > >          * Add list to the evlist even with errors to allow callers to clean up.
> > >          */
> > > -       perf_evlist__splice_list_tail(evlist, &parse_state.list);
> > > +       perf_evlist__splice_list_tail(evlist, &parse_state->list);
> > >
> > >         if (!ret) {
> > >                 struct evsel *last;
> > >
> > > -               evlist->nr_groups += parse_state.nr_groups;
> > > +               evlist->nr_groups += parse_state->nr_groups;
> > >                 last = evlist__last(evlist);
> > >                 last->cmdline_group_boundary = true;
> > >
> > > @@ -2125,6 +2118,35 @@ int parse_events(struct evlist *evlist, const char *str,
> > >         return ret;
> > >  }
> > >
> > > +int parse_events(struct evlist *evlist, const char *str,
> > > +                struct parse_events_error *err)
> > > +{
> > > +       struct parse_events_state parse_state = {
> > > +               .list   = LIST_HEAD_INIT(parse_state.list),
> > > +               .idx    = evlist->core.nr_entries,
> > > +               .error  = err,
> > > +               .evlist = evlist,
> > > +               .stoken = PE_START_EVENTS,
> > > +       };
> > > +
> > > +       return parse_events_state(&parse_state, evlist, str);
> > > +}
> > > +
> > > +int parse_events_fake(struct evlist *evlist, const char *str,
> > > +                     struct parse_events_error *err)
> > > +{
> > > +       struct parse_events_state parse_state = {
> > > +               .list     = LIST_HEAD_INIT(parse_state.list),
> > > +               .idx      = evlist->core.nr_entries,
> > > +               .error    = err,
> > > +               .evlist   = evlist,
> > > +               .stoken   = PE_START_EVENTS,
> > > +               .fake_pmu = true,
> > > +       };
> > > +
> > > +       return parse_events_state(&parse_state, evlist, str);
> > > +}
> > > +
> > >  #define MAX_WIDTH 1000
> > >  static int get_term_width(void)
> > >  {
> > > diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h
> > > index 963b0ea6c448..4a23b6cd9924 100644
> > > --- a/tools/perf/util/parse-events.h
> > > +++ b/tools/perf/util/parse-events.h
> > > @@ -34,6 +34,8 @@ int parse_events_option(const struct option *opt, const char *str, int unset);
> > >  int parse_events_option_new_evlist(const struct option *opt, const char *str, int unset);
> > >  int parse_events(struct evlist *evlist, const char *str,
> > >                  struct parse_events_error *error);
> > > +int parse_events_fake(struct evlist *evlist, const char *str,
> > > +                     struct parse_events_error *error);
> > >  int parse_events_terms(struct list_head *terms, const char *str);
> > >  int parse_filter(const struct option *opt, const char *str, int unset);
> > >  int exclude_perf(const struct option *opt, const char *arg, int unset);
> > > --
> > > 2.25.4
> > >
> > 
> 

-- 

- Arnaldo

Re: [PATCH 13/14] perf tests: Add parse metric test for ipc metric

[Arnaldo Carvalho de Melo]

Em Mon, Jun 01, 2020 at 12:55:44AM -0700, Ian Rogers escreveu:
> On Sun, May 24, 2020 at 3:43 PM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > Adding new test that process metrics code and checks
> > the expected results. Starting with easy ipc metric.
> >
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> 
> Acked-by: Ian Rogers <irogers@google.com>
> 
> I wonder if there's a better organization with testing in
> pmu-events.c, expr.c and now parse-metric.c.

This is on a patchkit that has some other stuff to be reworked, so
please collect the Acked-by when you submit v2, ok Jiri?

- Arnaldo
 
> Thanks,
> Ian
> 
> > ---
> >  tools/perf/tests/Build          |   1 +
> >  tools/perf/tests/builtin-test.c |   4 ++
> >  tools/perf/tests/parse-metric.c | 117 ++++++++++++++++++++++++++++++++
> >  tools/perf/tests/tests.h        |   1 +
> >  4 files changed, 123 insertions(+)
> >  create mode 100644 tools/perf/tests/parse-metric.c
> >
> > diff --git a/tools/perf/tests/Build b/tools/perf/tests/Build
> > index c75557aeef0e..bb7c2d8364d1 100644
> > --- a/tools/perf/tests/Build
> > +++ b/tools/perf/tests/Build
> > @@ -57,6 +57,7 @@ perf-y += maps.o
> >  perf-y += time-utils-test.o
> >  perf-y += genelf.o
> >  perf-y += api-io.o
> > +perf-y += parse-metric.o
> >
> >  $(OUTPUT)tests/llvm-src-base.c: tests/bpf-script-example.c tests/Build
> >         $(call rule_mkdir)
> > diff --git a/tools/perf/tests/builtin-test.c b/tools/perf/tests/builtin-test.c
> > index a9daaeb9fd27..bf20abdca0b0 100644
> > --- a/tools/perf/tests/builtin-test.c
> > +++ b/tools/perf/tests/builtin-test.c
> > @@ -324,6 +324,10 @@ static struct test generic_tests[] = {
> >                 .desc = "maps__merge_in",
> >                 .func = test__maps__merge_in,
> >         },
> > +       {
> > +               .desc = "Parse and process metrics",
> > +               .func = test__parse_metric,
> > +       },
> >         {
> >                 .func = NULL,
> >         },
> > diff --git a/tools/perf/tests/parse-metric.c b/tools/perf/tests/parse-metric.c
> > new file mode 100644
> > index 000000000000..3005d27c5c48
> > --- /dev/null
> > +++ b/tools/perf/tests/parse-metric.c
> > @@ -0,0 +1,117 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +#include <linux/compiler.h>
> > +#include <string.h>
> > +#include "metricgroup.h"
> > +#include "tests.h"
> > +#include "pmu-events/pmu-events.h"
> > +#include "evlist.h"
> > +#include "rblist.h"
> > +#include "debug.h"
> > +#include "expr.h"
> > +#include "stat.h"
> > +
> > +static struct pmu_event pme_test[] = {
> > +{
> > +       .metric_expr    = "inst_retired.any / cpu_clk_unhalted.thread",
> > +       .metric_name    = "IPC",
> > +},
> > +};
> > +
> > +static struct pmu_events_map map = {
> > +       .cpuid          = "test",
> > +       .version        = "1",
> > +       .type           = "core",
> > +       .table          = pme_test,
> > +};
> > +
> > +static double compute_single(struct rblist *metric_events, struct evsel *evsel,
> > +                            struct runtime_stat *st)
> > +{
> > +       struct metric_event *me;
> > +
> > +       me = metricgroup__lookup(metric_events, evsel, false);
> > +       if (me != NULL) {
> > +               struct metric_expr *mexp;
> > +
> > +               mexp = list_first_entry(&me->head, struct metric_expr, nd);
> > +               return test_generic_metric(mexp, 0, st);
> > +       }
> > +
> > +       return 0.;
> > +}
> > +
> > +struct value {
> > +       const char      *event;
> > +       u64              val;
> > +};
> > +
> > +static u64 find_value(const char *name, struct value *values)
> > +{
> > +       struct value *v = values;
> > +
> > +       while (v->event) {
> > +               if (!strcmp(name, v->event))
> > +                       return v->val;
> > +               v++;
> > +       };
> > +
> > +       return 0;
> > +}
> > +
> > +static void load_runtime_stat(struct runtime_stat *st, struct evlist *evlist,
> > +                             struct value *values)
> > +{
> > +       struct evsel *evsel;
> > +       u64 count;
> > +
> > +       evlist__for_each_entry(evlist, evsel) {
> > +               count = find_value(evsel->name, values);
> > +               perf_stat__update_shadow_stats(evsel, count, 0, st);
> > +       }
> > +}
> > +
> > +static int test_ipc(void)
> > +{
> > +       double ratio;
> > +       struct rblist metric_events = {
> > +               .nr_entries = 0,
> > +       };
> > +       struct evlist *evlist;
> > +       struct evsel *evsel;
> > +       struct value vals[] = {
> > +               { .event = "inst_retired.any",        .val = 300 },
> > +               { .event = "cpu_clk_unhalted.thread", .val = 200 },
> > +               { 0 },
> > +       };
> > +       struct runtime_stat st;
> > +       int err;
> > +
> > +       evlist = evlist__new();
> > +       if (!evlist)
> > +               return -1;
> > +
> > +       err = metricgroup__parse_groups_test(evlist, &map,
> > +                                            "IPC",
> > +                                            false, false,
> > +                                            &metric_events);
> > +
> > +       TEST_ASSERT_VAL("failed to parse metrics", err == 0);
> > +
> > +       runtime_stat__init(&st);
> > +       load_runtime_stat(&st, evlist, vals);
> > +
> > +       evsel = evlist__first(evlist);
> > +       ratio = compute_single(&metric_events, evsel, &st);
> > +
> > +       TEST_ASSERT_VAL("IPC failed, wrong ratio", ratio == 1.5);
> > +
> > +       runtime_stat__exit(&st);
> > +       evlist__delete(evlist);
> > +       return 0;
> > +}
> > +
> > +int test__parse_metric(struct test *test __maybe_unused, int subtest __maybe_unused)
> > +{
> > +       TEST_ASSERT_VAL("IPC failed", test_ipc() == 0);
> > +       return 0;
> > +}
> > diff --git a/tools/perf/tests/tests.h b/tools/perf/tests/tests.h
> > index 6c6c4b6a4796..0a7853b72240 100644
> > --- a/tools/perf/tests/tests.h
> > +++ b/tools/perf/tests/tests.h
> > @@ -117,6 +117,7 @@ int test__maps__merge_in(struct test *t, int subtest);
> >  int test__time_utils(struct test *t, int subtest);
> >  int test__jit_write_elf(struct test *test, int subtest);
> >  int test__api_io(struct test *test, int subtest);
> > +int test__parse_metric(struct test *test, int subtest);
> >
> >  bool test__bp_signal_is_supported(void);
> >  bool test__bp_account_is_supported(void);
> > --
> > 2.25.4
> >

-- 

- Arnaldo

Re: [PATCH 14/14] perf tests: Add parse metric test for frontend metric

[Arnaldo Carvalho de Melo]

Em Mon, Jun 01, 2020 at 01:06:01AM -0700, Ian Rogers escreveu:
> On Sun, May 24, 2020 at 3:43 PM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > Adding new metri test for frontend metric. It's stolen
> 
> s/metri/metric/
> 
> > from x86 pmu events.
> >
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> 
> Acked-by: Ian Rogers <irogers@google.com>

Ditto.

- Arnaldo
 
> Thanks,
> Ian
> 
> > ---
> >  tools/perf/tests/parse-metric.c | 46 +++++++++++++++++++++++++++++++++
> >  1 file changed, 46 insertions(+)
> >
> > diff --git a/tools/perf/tests/parse-metric.c b/tools/perf/tests/parse-metric.c
> > index 3005d27c5c48..38f20850bba3 100644
> > --- a/tools/perf/tests/parse-metric.c
> > +++ b/tools/perf/tests/parse-metric.c
> > @@ -15,6 +15,11 @@ static struct pmu_event pme_test[] = {
> >         .metric_expr    = "inst_retired.any / cpu_clk_unhalted.thread",
> >         .metric_name    = "IPC",
> >  },
> > +{
> > +       .metric_expr    = "idq_uops_not_delivered.core / (4 * (( ( cpu_clk_unhalted.thread / 2 ) * "
> > +                         "( 1 + cpu_clk_unhalted.one_thread_active / cpu_clk_unhalted.ref_xclk ) )))",
> > +       .metric_name    = "Frontend_Bound_SMT",
> > +},
> >  };
> >
> >  static struct pmu_events_map map = {
> > @@ -110,8 +115,49 @@ static int test_ipc(void)
> >         return 0;
> >  }
> >
> > +static int test_frontend(void)
> > +{
> > +       double ratio;
> > +       struct rblist metric_events = { 0 };
> > +       struct evlist *evlist;
> > +       struct evsel *evsel;
> > +       struct value vals[] = {
> > +               { .event = "idq_uops_not_delivered.core",        .val = 300 },
> > +               { .event = "cpu_clk_unhalted.thread",            .val = 200 },
> > +               { .event = "cpu_clk_unhalted.one_thread_active", .val = 400 },
> > +               { .event = "cpu_clk_unhalted.ref_xclk",          .val = 600 },
> > +               { 0 },
> > +       };
> > +       struct runtime_stat st;
> > +       int err;
> > +
> > +       evlist = evlist__new();
> > +       if (!evlist)
> > +               return -1;
> > +
> > +       err = metricgroup__parse_groups_test(evlist, &map,
> > +                                            "Frontend_Bound_SMT",
> > +                                            false, false,
> > +                                            &metric_events);
> > +
> > +       TEST_ASSERT_VAL("failed to parse metrics", err == 0);
> > +
> > +       runtime_stat__init(&st);
> > +       load_runtime_stat(&st, evlist, vals);
> > +
> > +       evsel = evlist__first(evlist);
> > +       ratio = compute_single(&metric_events, evsel, &st);
> > +
> > +       TEST_ASSERT_VAL("Frontend_Bound_SMT failed, wrong ratio", ratio == 0.45);
> > +
> > +       runtime_stat__exit(&st);
> > +       evlist__delete(evlist);
> > +       return 0;
> > +}
> > +
> >  int test__parse_metric(struct test *test __maybe_unused, int subtest __maybe_unused)
> >  {
> >         TEST_ASSERT_VAL("IPC failed", test_ipc() == 0);
> > +       TEST_ASSERT_VAL("frontend failed", test_frontend() == 0);
> >         return 0;
> >  }
> > --
> > 2.25.4
> >

-- 

- Arnaldo

Re: [PATCH 05/14] perf tools: Add parse_events_fake interface

[Jiri Olsa]

On Mon, Jun 01, 2020 at 12:04:28PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Mon, Jun 01, 2020 at 11:08:50AM +0200, Jiri Olsa escreveu:
> > On Mon, Jun 01, 2020 at 12:28:31AM -0700, Ian Rogers wrote:
> > > On Sun, May 24, 2020 at 3:42 PM Jiri Olsa <jolsa@kernel.org> wrote:
> > > >
> > > > Adding parse_events_fake interface to parse events
> > > > and use fake pmu event in case pmu event is parsed.
> > > >
> > > > This way it's possible to parse events from PMUs
> > > > which are not present in the system. It's available
> > > > only for testing purposes coming in following
> > > > changes.
> > > >
> > > > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > > 
> > > Acked-by: Ian Rogers <irogers@google.com>
> > > 
> > > Alternatively fake_pmu could be an argument to parse_events.
> > 
> > yea I checked that and I was surprised how many parse_events calls
> > we have in perf, so I went this way.. but I haven't really tried it,
> > so it might look actually etter despite the many places we need to change,
> > I'll try
> 
> Thanks! My admitedly unchecked thinking is that most places will just
> pass NULL, only the test case will pass it, right?

I changed that per Ian's suggestion and it looks better,
so I'll post it in v2

jirka

Re: [PATCH 13/14] perf tests: Add parse metric test for ipc metric

[Jiri Olsa]

On Mon, Jun 01, 2020 at 12:08:43PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Mon, Jun 01, 2020 at 12:55:44AM -0700, Ian Rogers escreveu:
> > On Sun, May 24, 2020 at 3:43 PM Jiri Olsa <jolsa@kernel.org> wrote:
> > >
> > > Adding new test that process metrics code and checks
> > > the expected results. Starting with easy ipc metric.
> > >
> > > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > 
> > Acked-by: Ian Rogers <irogers@google.com>
> > 
> > I wonder if there's a better organization with testing in
> > pmu-events.c, expr.c and now parse-metric.c.
> 
> This is on a patchkit that has some other stuff to be reworked, so
> please collect the Acked-by when you submit v2, ok Jiri?

yep, will do

jirka

Re: [PATCH 13/14] perf tests: Add parse metric test for ipc metric

[Ian Rogers]

On Mon, Jun 1, 2020 at 6:09 AM Jiri Olsa <jolsa@redhat.com> wrote:
>
> On Mon, Jun 01, 2020 at 12:55:44AM -0700, Ian Rogers wrote:
> > On Sun, May 24, 2020 at 3:43 PM Jiri Olsa <jolsa@kernel.org> wrote:
> > >
> > > Adding new test that process metrics code and checks
> > > the expected results. Starting with easy ipc metric.
> > >
> > > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> >
> > Acked-by: Ian Rogers <irogers@google.com>
> >
> > I wonder if there's a better organization with testing in
> > pmu-events.c, expr.c and now parse-metric.c.
>
> hum, so
>  - expr.c is testing core interface,
>  - parse-metric is testing specific metric processing from
>    parsing to final ratio
>  - pmu-events.c is testing pmu events aliases and parsing of
>    all the metrics
>
> pmu-events.c is testing both pmu events and metrics,
> but I think it fits in the way it's done together

Agreed, it makes following this a little bit of a challenge. When I
did the parsing in pmu-events I'd originally done it in expr.c for
example. Perhaps if there were a parse-metric in tools/perf/util then
things would align better as well. Just thinking out loud :-)

Thanks,
Ian

> jirka
>

Re: [PATCH 06/14] perf tests: Add another pmu-events tests

[Ian Rogers]

On Mon, Jun 1, 2020 at 6:21 AM Jiri Olsa <jolsa@redhat.com> wrote:
>
> On Mon, Jun 01, 2020 at 12:44:15AM -0700, Ian Rogers wrote:
>
> SNIP
>
> > > +       memset(&error, 0, sizeof(error));
> > > +       ret = parse_events_fake(evlist, id, &error);
> > > +       if (ret) {
> > > +               pr_debug("str        : %s\n", error.str);
> > > +               pr_debug("help       : %s\n", error.help);
> > > +               pr_debug("first_str  : %s\n", error.first_str);
> > > +               pr_debug("first_help : %s\n", error.first_help);
> > > +       }
> > > +
> > > +       evlist__delete(evlist);
> > > +       free(error.str);
> > > +       free(error.help);
> > > +       free(error.first_str);
> > > +       free(error.first_help);
> > > +       return ret;
> > > +}
> >
> > This is quite similar to check_parse_id, fold them together?
>
> there is the 'same_cpu' logic in check_parse_id,
> so I did not want to mess with that

Agreed. We could handle ret and same_cpu in the caller.

> >
> > > +
> > > +static int metric_parse_fake(const char *str)
> > > +{
> > > +       struct expr_parse_ctx ctx;
> > > +       struct hashmap_entry *cur;
> > > +       double result;
> > > +       int ret = -1;
> > > +       size_t bkt;
> > > +       int i;
> > > +
> > > +       pr_debug("parsing '%s'\n", str);
> > > +
> > > +       expr__ctx_init(&ctx);
> > > +       if (expr__find_other(str, NULL, &ctx, 0) < 0) {
> > > +               pr_err("expr__find_other failed\n");
> > > +               return -1;
> > > +       }
> > > +
> > > +       i = 1;
> > > +       hashmap__for_each_entry((&ctx.ids), cur, bkt)
> > > +               expr__add_id(&ctx, strdup(cur->key), i++);
> >
> > It might make sense to share the code here with that in test_parsing.
> > This initialization of ids is commented there and it is a bit special.
>
> hum, not sure it's worth to add this complexity to test, I'd like
> to keep it simple, it's already not straightforward ;-) I added the
> comment you mentioned

Ok, sounds good to me. More testing is the priority :-)

Thanks,
Ian

> jirka
>