[PATCH 00/18] perf metric: Add support to reuse metric

[PATCH 00/18] perf metric: Add support to reuse metric

[Jiri Olsa]

hi,
this patchset is adding the support to reused metric in another 
metric. The metric needs to be referenced by 'metric:' prefix.

For example, to define IPC by using CPI with change like:

         "BriefDescription": "Instructions Per Cycle (per Logical Processor)",
 -       "MetricExpr": "INST_RETIRED.ANY / CPU_CLK_UNHALTED.THREAD",
 +       "MetricExpr": "1/metric:CPI",
         "MetricGroup": "TopDownL1",
         "MetricName": "IPC"

I won't be able to find all the possible places we could
use this at, so I wonder you guys (who was asking for this)
would try it and come up with comments if there's something
missing or we could already use it at some places.

It's based on Arnaldo's tmp.perf/core.

v2 changes:
  - collected Ian's acks for few patches [Ian]
  - renamed expr__add_id to expr__add_id_val [Ian]
  - renamed expr_parse_data to expr_id_data [Ian]
  - added recursion check [Ian]
  - added metric test for DCache_L2 metric [Ian]
  - added some renames as discussed in review [Ian]

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

thanks,
jirka


---
Jiri Olsa (18):
      perf metric: Rename expr__add_id to expr__add_val
      perf metric: Add struct expr_id_data to keep expr value
      perf metric: Add expr__add_id function
      perf metric: Change expr__get_id to return struct expr_id_data
      perf metric: Add expr__del_id function
      perf metric: Add find_metric function
      perf metric: Add add_metric function
      perf metric: Rename __metricgroup__add_metric to __add_metric
      perf metric: Collect referenced metrics in struct metric_ref_node
      perf metric: Collect referenced metrics in struct metric_expr
      perf metric: Add referenced metrics to hash data
      perf metric: Compute referenced metrics
      perf metric: Add events for the current group
      perf metric: Add cache_miss_cycles to metric parse test
      perf metric: Add DCache_L2 to metric parse test
      perf metric: Add recursion check when processing nested metrics
      perf metric: Rename struct egroup to metric
      perf metric: Rename group_list to list

 tools/perf/tests/expr.c         |   7 +-
 tools/perf/tests/parse-metric.c | 131 +++++++++++++++++++++++++++++++++++-
 tools/perf/tests/pmu-events.c   |   4 +-
 tools/perf/util/expr.c          | 130 ++++++++++++++++++++++++++++++------
 tools/perf/util/expr.h          |  34 +++++++++-
 tools/perf/util/expr.y          |  16 +++--
 tools/perf/util/metricgroup.c   | 435 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++---------------------------
 tools/perf/util/metricgroup.h   |   6 ++
 tools/perf/util/stat-shadow.c   |  24 ++++---
 9 files changed, 644 insertions(+), 143 deletions(-)

[PATCH 01/18] perf metric: Rename expr__add_id to expr__add_val

[Jiri Olsa]

Renaming expr__add_id to expr__add_val so we can use
expr__add_id to actually add just id without any value
in following changes.

There's no functional change.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/perf/tests/expr.c       | 4 ++--
 tools/perf/tests/pmu-events.c | 4 ++--
 tools/perf/util/expr.c        | 2 +-
 tools/perf/util/expr.h        | 2 +-
 tools/perf/util/expr.y        | 2 +-
 tools/perf/util/stat-shadow.c | 4 ++--
 6 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/tools/perf/tests/expr.c b/tools/perf/tests/expr.c
index b7e5ef3007fc..d13fc1dfd5ef 100644
--- a/tools/perf/tests/expr.c
+++ b/tools/perf/tests/expr.c
@@ -24,8 +24,8 @@ int test__expr(struct test *t __maybe_unused, int subtest __maybe_unused)
 	struct expr_parse_ctx ctx;
 
 	expr__ctx_init(&ctx);
-	expr__add_id(&ctx, strdup("FOO"), 1);
-	expr__add_id(&ctx, strdup("BAR"), 2);
+	expr__add_id_val(&ctx, strdup("FOO"), 1);
+	expr__add_id_val(&ctx, strdup("BAR"), 2);
 
 	ret = test(&ctx, "1+1", 2);
 	ret |= test(&ctx, "FOO+BAR", 3);
diff --git a/tools/perf/tests/pmu-events.c b/tools/perf/tests/pmu-events.c
index b66b021476ec..eb19f9a0bc15 100644
--- a/tools/perf/tests/pmu-events.c
+++ b/tools/perf/tests/pmu-events.c
@@ -492,7 +492,7 @@ static int test_parsing(void)
 			 */
 			k = 1;
 			hashmap__for_each_entry((&ctx.ids), cur, bkt)
-				expr__add_id(&ctx, strdup(cur->key), k++);
+				expr__add_id_val(&ctx, strdup(cur->key), k++);
 
 			hashmap__for_each_entry((&ctx.ids), cur, bkt) {
 				if (check_parse_cpu(cur->key, map == cpus_map,
@@ -547,7 +547,7 @@ static int metric_parse_fake(const char *str)
 	 */
 	i = 1;
 	hashmap__for_each_entry((&ctx.ids), cur, bkt)
-		expr__add_id(&ctx, strdup(cur->key), i++);
+		expr__add_id_val(&ctx, strdup(cur->key), i++);
 
 	hashmap__for_each_entry((&ctx.ids), cur, bkt) {
 		if (check_parse_fake(cur->key)) {
diff --git a/tools/perf/util/expr.c b/tools/perf/util/expr.c
index e8f777830a23..9116a3a01eea 100644
--- a/tools/perf/util/expr.c
+++ b/tools/perf/util/expr.c
@@ -33,7 +33,7 @@ static bool key_equal(const void *key1, const void *key2,
 }
 
 /* Caller must make sure id is allocated */
-int expr__add_id(struct expr_parse_ctx *ctx, const char *name, double val)
+int expr__add_id_val(struct expr_parse_ctx *ctx, const char *name, double val)
 {
 	double *val_ptr = NULL, *old_val = NULL;
 	char *old_key = NULL;
diff --git a/tools/perf/util/expr.h b/tools/perf/util/expr.h
index 8a2c1074f90f..bb6bac836b48 100644
--- a/tools/perf/util/expr.h
+++ b/tools/perf/util/expr.h
@@ -22,7 +22,7 @@ struct expr_scanner_ctx {
 
 void expr__ctx_init(struct expr_parse_ctx *ctx);
 void expr__ctx_clear(struct expr_parse_ctx *ctx);
-int expr__add_id(struct expr_parse_ctx *ctx, const char *id, double val);
+int expr__add_id_val(struct expr_parse_ctx *ctx, const char *id, double val);
 int expr__get_id(struct expr_parse_ctx *ctx, const char *id, double *val_ptr);
 int expr__parse(double *final_val, struct expr_parse_ctx *ctx,
 		const char *expr, int runtime);
diff --git a/tools/perf/util/expr.y b/tools/perf/util/expr.y
index 5fcb98800f9c..b2b3420ea6ec 100644
--- a/tools/perf/util/expr.y
+++ b/tools/perf/util/expr.y
@@ -69,7 +69,7 @@ all_other: all_other other
 
 other: ID
 {
-	expr__add_id(ctx, $1, 0.0);
+	expr__add_id_val(ctx, $1, 0.0);
 }
 |
 MIN | MAX | IF | ELSE | SMT_ON | NUMBER | '|' | '^' | '&' | '-' | '+' | '*' | '/' | '%' | '(' | ')' | ','
diff --git a/tools/perf/util/stat-shadow.c b/tools/perf/util/stat-shadow.c
index 8fdef47005e6..fc9ac4b4218e 100644
--- a/tools/perf/util/stat-shadow.c
+++ b/tools/perf/util/stat-shadow.c
@@ -773,9 +773,9 @@ static int prepare_metric(struct evsel **metric_events,
 			*pn = 0;
 
 		if (metric_total)
-			expr__add_id(pctx, n, metric_total);
+			expr__add_id_val(pctx, n, metric_total);
 		else
-			expr__add_id(pctx, n, avg_stats(stats)*scale);
+			expr__add_id_val(pctx, n, avg_stats(stats)*scale);
 	}
 
 	return i;
-- 
2.25.4

[PATCH 02/18] perf metric: Add struct expr_id_data to keep expr value

[Jiri Olsa]

Adding struct expr_id_data to keep expr value
instead of just simple double pointer, so we can
store more data for ID in following changes.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/perf/tests/expr.c       |  3 ++-
 tools/perf/util/expr.c        | 22 +++++++++++-----------
 tools/perf/util/expr.h        |  4 ++++
 tools/perf/util/metricgroup.c |  2 +-
 4 files changed, 18 insertions(+), 13 deletions(-)

diff --git a/tools/perf/tests/expr.c b/tools/perf/tests/expr.c
index d13fc1dfd5ef..4d01051951cd 100644
--- a/tools/perf/tests/expr.c
+++ b/tools/perf/tests/expr.c
@@ -18,8 +18,9 @@ static int test(struct expr_parse_ctx *ctx, const char *e, double val2)
 
 int test__expr(struct test *t __maybe_unused, int subtest __maybe_unused)
 {
+	struct expr_id_data *val_ptr;
 	const char *p;
-	double val, *val_ptr;
+	double val;
 	int ret;
 	struct expr_parse_ctx ctx;
 
diff --git a/tools/perf/util/expr.c b/tools/perf/util/expr.c
index 9116a3a01eea..5d05f9765ed8 100644
--- a/tools/perf/util/expr.c
+++ b/tools/perf/util/expr.c
@@ -33,32 +33,32 @@ static bool key_equal(const void *key1, const void *key2,
 }
 
 /* Caller must make sure id is allocated */
-int expr__add_id_val(struct expr_parse_ctx *ctx, const char *name, double val)
+int expr__add_id_val(struct expr_parse_ctx *ctx, const char *id, double val)
 {
-	double *val_ptr = NULL, *old_val = NULL;
+	struct expr_id_data *data_ptr = NULL, *old_data = NULL;
 	char *old_key = NULL;
 	int ret;
 
 	if (val != 0.0) {
-		val_ptr = malloc(sizeof(double));
-		if (!val_ptr)
+		data_ptr = malloc(sizeof(*data_ptr));
+		if (!data_ptr)
 			return -ENOMEM;
-		*val_ptr = val;
+		data_ptr->val = val;
 	}
-	ret = hashmap__set(&ctx->ids, name, val_ptr,
-			   (const void **)&old_key, (void **)&old_val);
+	ret = hashmap__set(&ctx->ids, id, data_ptr,
+			   (const void **)&old_key, (void **)&old_data);
 	free(old_key);
-	free(old_val);
+	free(old_data);
 	return ret;
 }
 
 int expr__get_id(struct expr_parse_ctx *ctx, const char *id, double *val_ptr)
 {
-	double *data;
+	struct expr_id_data *data;
 
 	if (!hashmap__find(&ctx->ids, id, (void **)&data))
 		return -1;
-	*val_ptr = (data == NULL) ?  0.0 : *data;
+	*val_ptr = (data == NULL) ?  0.0 : data->val;
 	return 0;
 }
 
@@ -119,7 +119,7 @@ int expr__parse(double *final_val, struct expr_parse_ctx *ctx,
 int expr__find_other(const char *expr, const char *one,
 		     struct expr_parse_ctx *ctx, int runtime)
 {
-	double *old_val = NULL;
+	struct expr_id_data *old_val = NULL;
 	char *old_key = NULL;
 	int ret = __expr__parse(NULL, ctx, expr, EXPR_OTHER, runtime);
 
diff --git a/tools/perf/util/expr.h b/tools/perf/util/expr.h
index bb6bac836b48..21fe5bd85718 100644
--- a/tools/perf/util/expr.h
+++ b/tools/perf/util/expr.h
@@ -15,6 +15,10 @@ struct expr_parse_ctx {
 	struct hashmap ids;
 };
 
+struct expr_id_data {
+	double	val;
+};
+
 struct expr_scanner_ctx {
 	int start_token;
 	int runtime;
diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
index 82fecb5a302d..df0356ec120d 100644
--- a/tools/perf/util/metricgroup.c
+++ b/tools/perf/util/metricgroup.c
@@ -138,7 +138,7 @@ static struct evsel *find_evsel_group(struct evlist *perf_evlist,
 				      unsigned long *evlist_used)
 {
 	struct evsel *ev, *current_leader = NULL;
-	double *val_ptr;
+	struct expr_id_data *val_ptr;
 	int i = 0, matched_events = 0, events_to_match;
 	const int idnum = (int)hashmap__size(&pctx->ids);
 
-- 
2.25.4

[PATCH 03/18] perf metric: Add expr__add_id function

[Jiri Olsa]

Adding expr__add_id function to data for ID
with zero value, which is used when scanning
the expression for IDs.

Acked-by: Ian Rogers <irogers@google.com>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/perf/util/expr.c | 29 +++++++++++++++++++++++------
 tools/perf/util/expr.h |  1 +
 tools/perf/util/expr.y |  2 +-
 3 files changed, 25 insertions(+), 7 deletions(-)

diff --git a/tools/perf/util/expr.c b/tools/perf/util/expr.c
index 5d05f9765ed8..4b8953605aed 100644
--- a/tools/perf/util/expr.c
+++ b/tools/perf/util/expr.c
@@ -32,6 +32,24 @@ static bool key_equal(const void *key1, const void *key2,
 	return !strcmp((const char *)key1, (const char *)key2);
 }
 
+/* Caller must make sure id is allocated */
+int expr__add_id(struct expr_parse_ctx *ctx, const char *id)
+{
+	struct expr_id_data *data_ptr = NULL, *old_data = NULL;
+	char *old_key = NULL;
+	int ret;
+
+	data_ptr = malloc(sizeof(*data_ptr));
+	if (!data_ptr)
+		return -ENOMEM;
+
+	ret = hashmap__set(&ctx->ids, id, data_ptr,
+			   (const void **)&old_key, (void **)&old_data);
+	free(old_key);
+	free(old_data);
+	return ret;
+}
+
 /* Caller must make sure id is allocated */
 int expr__add_id_val(struct expr_parse_ctx *ctx, const char *id, double val)
 {
@@ -39,12 +57,11 @@ int expr__add_id_val(struct expr_parse_ctx *ctx, const char *id, double val)
 	char *old_key = NULL;
 	int ret;
 
-	if (val != 0.0) {
-		data_ptr = malloc(sizeof(*data_ptr));
-		if (!data_ptr)
-			return -ENOMEM;
-		data_ptr->val = val;
-	}
+	data_ptr = malloc(sizeof(*data_ptr));
+	if (!data_ptr)
+		return -ENOMEM;
+	data_ptr->val = val;
+
 	ret = hashmap__set(&ctx->ids, id, data_ptr,
 			   (const void **)&old_key, (void **)&old_data);
 	free(old_key);
diff --git a/tools/perf/util/expr.h b/tools/perf/util/expr.h
index 21fe5bd85718..ac32cda42006 100644
--- a/tools/perf/util/expr.h
+++ b/tools/perf/util/expr.h
@@ -26,6 +26,7 @@ struct expr_scanner_ctx {
 
 void expr__ctx_init(struct expr_parse_ctx *ctx);
 void expr__ctx_clear(struct expr_parse_ctx *ctx);
+int expr__add_id(struct expr_parse_ctx *ctx, const char *id);
 int expr__add_id_val(struct expr_parse_ctx *ctx, const char *id, double val);
 int expr__get_id(struct expr_parse_ctx *ctx, const char *id, double *val_ptr);
 int expr__parse(double *final_val, struct expr_parse_ctx *ctx,
diff --git a/tools/perf/util/expr.y b/tools/perf/util/expr.y
index b2b3420ea6ec..8befe4a46f87 100644
--- a/tools/perf/util/expr.y
+++ b/tools/perf/util/expr.y
@@ -69,7 +69,7 @@ all_other: all_other other
 
 other: ID
 {
-	expr__add_id_val(ctx, $1, 0.0);
+	expr__add_id(ctx, $1);
 }
 |
 MIN | MAX | IF | ELSE | SMT_ON | NUMBER | '|' | '^' | '&' | '-' | '+' | '*' | '/' | '%' | '(' | ')' | ','
-- 
2.25.4

[PATCH 04/18] perf metric: Change expr__get_id to return struct expr_id_data

[Jiri Olsa]

Changing expr__get_id to use and return struct expr_id_data
pointer as value for the ID. This way we can access data other
than value for given ID in following changes.

Acked-by: Ian Rogers <irogers@google.com>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/perf/util/expr.c | 10 +++-------
 tools/perf/util/expr.h |  3 ++-
 tools/perf/util/expr.y | 14 +++++++++-----
 3 files changed, 14 insertions(+), 13 deletions(-)

diff --git a/tools/perf/util/expr.c b/tools/perf/util/expr.c
index 4b8953605aed..7d02d29286bc 100644
--- a/tools/perf/util/expr.c
+++ b/tools/perf/util/expr.c
@@ -69,14 +69,10 @@ int expr__add_id_val(struct expr_parse_ctx *ctx, const char *id, double val)
 	return ret;
 }
 
-int expr__get_id(struct expr_parse_ctx *ctx, const char *id, double *val_ptr)
+int expr__get_id(struct expr_parse_ctx *ctx, const char *id,
+		 struct expr_id_data **data)
 {
-	struct expr_id_data *data;
-
-	if (!hashmap__find(&ctx->ids, id, (void **)&data))
-		return -1;
-	*val_ptr = (data == NULL) ?  0.0 : data->val;
-	return 0;
+	return hashmap__find(&ctx->ids, id, (void **)data) ? 0 : -1;
 }
 
 void expr__ctx_init(struct expr_parse_ctx *ctx)
diff --git a/tools/perf/util/expr.h b/tools/perf/util/expr.h
index ac32cda42006..f38292fdab19 100644
--- a/tools/perf/util/expr.h
+++ b/tools/perf/util/expr.h
@@ -28,7 +28,8 @@ void expr__ctx_init(struct expr_parse_ctx *ctx);
 void expr__ctx_clear(struct expr_parse_ctx *ctx);
 int expr__add_id(struct expr_parse_ctx *ctx, const char *id);
 int expr__add_id_val(struct expr_parse_ctx *ctx, const char *id, double val);
-int expr__get_id(struct expr_parse_ctx *ctx, const char *id, double *val_ptr);
+int expr__get_id(struct expr_parse_ctx *ctx, const char *id,
+		 struct expr_id_data **data);
 int expr__parse(double *final_val, struct expr_parse_ctx *ctx,
 		const char *expr, int runtime);
 int expr__find_other(const char *expr, const char *one,
diff --git a/tools/perf/util/expr.y b/tools/perf/util/expr.y
index 8befe4a46f87..0d4f5d324be7 100644
--- a/tools/perf/util/expr.y
+++ b/tools/perf/util/expr.y
@@ -85,12 +85,16 @@ if_expr:
 	;
 
 expr:	  NUMBER
-	| ID			{ if (expr__get_id(ctx, $1, &$$)) {
-					pr_debug("%s not found\n", $1);
+	| ID			{
+					struct expr_id_data *data;
+
+					if (expr__get_id(ctx, $1, &data) || !data) {
+						pr_debug("%s not found\n", $1);
+						free($1);
+						YYABORT;
+					}
+					$$ = data->val;
 					free($1);
-					YYABORT;
-				  }
-				  free($1);
 				}
 	| expr '|' expr		{ $$ = (long)$1 | (long)$3; }
 	| expr '&' expr		{ $$ = (long)$1 & (long)$3; }
-- 
2.25.4

[PATCH 05/18] perf metric: Add expr__del_id function

[Jiri Olsa]

Adding expr__del_id function to remove ID from hashmap.
It will save us few lines in following changes.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/perf/util/expr.c | 21 +++++++++++++--------
 tools/perf/util/expr.h |  1 +
 2 files changed, 14 insertions(+), 8 deletions(-)

diff --git a/tools/perf/util/expr.c b/tools/perf/util/expr.c
index 7d02d29286bc..91142d4c3419 100644
--- a/tools/perf/util/expr.c
+++ b/tools/perf/util/expr.c
@@ -75,6 +75,17 @@ int expr__get_id(struct expr_parse_ctx *ctx, const char *id,
 	return hashmap__find(&ctx->ids, id, (void **)data) ? 0 : -1;
 }
 
+void expr__del_id(struct expr_parse_ctx *ctx, const char *id)
+{
+	struct expr_id_data *old_val = NULL;
+	char *old_key = NULL;
+
+	hashmap__delete(&ctx->ids, id,
+			(const void **)&old_key, (void **)&old_val);
+	free(old_key);
+	free(old_val);
+}
+
 void expr__ctx_init(struct expr_parse_ctx *ctx)
 {
 	hashmap__init(&ctx->ids, key_hash, key_equal, NULL);
@@ -132,16 +143,10 @@ int expr__parse(double *final_val, struct expr_parse_ctx *ctx,
 int expr__find_other(const char *expr, const char *one,
 		     struct expr_parse_ctx *ctx, int runtime)
 {
-	struct expr_id_data *old_val = NULL;
-	char *old_key = NULL;
 	int ret = __expr__parse(NULL, ctx, expr, EXPR_OTHER, runtime);
 
-	if (one) {
-		hashmap__delete(&ctx->ids, one,
-				(const void **)&old_key, (void **)&old_val);
-		free(old_key);
-		free(old_val);
-	}
+	if (one)
+		expr__del_id(ctx, one);
 
 	return ret;
 }
diff --git a/tools/perf/util/expr.h b/tools/perf/util/expr.h
index f38292fdab19..2462abd0ac65 100644
--- a/tools/perf/util/expr.h
+++ b/tools/perf/util/expr.h
@@ -26,6 +26,7 @@ struct expr_scanner_ctx {
 
 void expr__ctx_init(struct expr_parse_ctx *ctx);
 void expr__ctx_clear(struct expr_parse_ctx *ctx);
+void expr__del_id(struct expr_parse_ctx *ctx, const char *id);
 int expr__add_id(struct expr_parse_ctx *ctx, const char *id);
 int expr__add_id_val(struct expr_parse_ctx *ctx, const char *id, double val);
 int expr__get_id(struct expr_parse_ctx *ctx, const char *id,
-- 
2.25.4

[PATCH 06/18] perf metric: Add find_metric function

[Jiri Olsa]

Decouple lookup metric logic into find_metric function,
so it can be used from other places in following changes.

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

diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
index df0356ec120d..72552608ff7d 100644
--- a/tools/perf/util/metricgroup.c
+++ b/tools/perf/util/metricgroup.c
@@ -614,57 +614,64 @@ static int __metricgroup__add_metric(struct list_head *group_list,
 	return 0;
 }
 
-static int metricgroup__add_metric(const char *metric, bool metric_no_group,
-				   struct strbuf *events,
-				   struct list_head *group_list,
-				   struct pmu_events_map *map)
+static struct pmu_event *find_metric(const char *metric, struct pmu_events_map *map)
 {
 	struct pmu_event *pe;
-	struct egroup *eg;
-	int i, ret;
-	bool has_match = false;
+	int i;
 
 	for (i = 0; ; i++) {
 		pe = &map->table[i];
-
-		if (!pe->name && !pe->metric_group && !pe->metric_name) {
-			/* End of pmu events. */
-			if (!has_match)
-				return -EINVAL;
+		/* End of pmu events. */
+		if (!pe->name && !pe->metric_group && !pe->metric_name)
 			break;
-		}
 		if (!pe->metric_expr)
 			continue;
 		if (match_metric(pe->metric_group, metric) ||
-		    match_metric(pe->metric_name, metric)) {
-			has_match = true;
-			pr_debug("metric expr %s for %s\n", pe->metric_expr, pe->metric_name);
-
-			if (!strstr(pe->metric_expr, "?")) {
-				ret = __metricgroup__add_metric(group_list,
-								pe,
-								metric_no_group,
-								1);
-				if (ret)
-					return ret;
-			} else {
-				int j, count;
-
-				count = arch_get_runtimeparam();
-
-				/* This loop is added to create multiple
-				 * events depend on count value and add
-				 * those events to group_list.
-				 */
+		    match_metric(pe->metric_name, metric))
+			return pe;
+	}
 
-				for (j = 0; j < count; j++) {
-					ret = __metricgroup__add_metric(
-						group_list, pe,
-						metric_no_group, j);
-					if (ret)
-						return ret;
-				}
-			}
+	return NULL;
+}
+
+static int metricgroup__add_metric(const char *metric, bool metric_no_group,
+				   struct strbuf *events,
+				   struct list_head *group_list,
+				   struct pmu_events_map *map)
+{
+	struct pmu_event *pe;
+	struct egroup *eg;
+	int ret;
+
+	pe = find_metric(metric, map);
+	if (!pe)
+		return -EINVAL;
+
+	pr_debug("metric expr %s for %s\n", pe->metric_expr, pe->metric_name);
+
+	if (!strstr(pe->metric_expr, "?")) {
+		ret = __metricgroup__add_metric(group_list,
+						pe,
+						metric_no_group,
+						1);
+		if (ret)
+			return ret;
+	} else {
+		int j, count;
+
+		count = arch_get_runtimeparam();
+
+		/* This loop is added to create multiple
+		 * events depend on count value and add
+		 * those events to group_list.
+		 */
+
+		for (j = 0; j < count; j++) {
+			ret = __metricgroup__add_metric(
+				group_list, pe,
+				metric_no_group, j);
+			if (ret)
+				return ret;
 		}
 	}
 	list_for_each_entry(eg, group_list, nd) {
-- 
2.25.4

[PATCH 07/18] perf metric: Add add_metric function

[Jiri Olsa]

Decouple metric adding login into add_metric function,
so it can be used from other places in following changes.

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

diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
index 72552608ff7d..9a168f3df7a4 100644
--- a/tools/perf/util/metricgroup.c
+++ b/tools/perf/util/metricgroup.c
@@ -634,18 +634,11 @@ static struct pmu_event *find_metric(const char *metric, struct pmu_events_map *
 	return NULL;
 }
 
-static int metricgroup__add_metric(const char *metric, bool metric_no_group,
-				   struct strbuf *events,
-				   struct list_head *group_list,
-				   struct pmu_events_map *map)
+static int add_metric(struct list_head *group_list,
+		      struct pmu_event *pe,
+		      bool metric_no_group)
 {
-	struct pmu_event *pe;
-	struct egroup *eg;
-	int ret;
-
-	pe = find_metric(metric, map);
-	if (!pe)
-		return -EINVAL;
+	int ret = 0;
 
 	pr_debug("metric expr %s for %s\n", pe->metric_expr, pe->metric_name);
 
@@ -654,8 +647,6 @@ static int metricgroup__add_metric(const char *metric, bool metric_no_group,
 						pe,
 						metric_no_group,
 						1);
-		if (ret)
-			return ret;
 	} else {
 		int j, count;
 
@@ -666,14 +657,33 @@ static int metricgroup__add_metric(const char *metric, bool metric_no_group,
 		 * those events to group_list.
 		 */
 
-		for (j = 0; j < count; j++) {
+		for (j = 0; j < count && !ret; j++) {
 			ret = __metricgroup__add_metric(
 				group_list, pe,
 				metric_no_group, j);
-			if (ret)
-				return ret;
 		}
 	}
+
+	return ret;
+}
+
+static int metricgroup__add_metric(const char *metric, bool metric_no_group,
+				   struct strbuf *events,
+				   struct list_head *group_list,
+				   struct pmu_events_map *map)
+{
+	struct pmu_event *pe;
+	struct egroup *eg;
+	int ret;
+
+	pe = find_metric(metric, map);
+	if (!pe)
+		return -EINVAL;
+
+	ret = add_metric(group_list, pe, metric_no_group);
+	if (ret)
+		return ret;
+
 	list_for_each_entry(eg, group_list, nd) {
 		if (events->len > 0)
 			strbuf_addf(events, ",");
-- 
2.25.4

[PATCH 08/18] perf metric: Rename __metricgroup__add_metric to __add_metric

[Jiri Olsa]

Renaming __metricgroup__add_metric to __add_metric
to fit in the current function names.

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

diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
index 9a168f3df7a4..f0b0a053bfd2 100644
--- a/tools/perf/util/metricgroup.c
+++ b/tools/perf/util/metricgroup.c
@@ -571,10 +571,10 @@ int __weak arch_get_runtimeparam(void)
 	return 1;
 }
 
-static int __metricgroup__add_metric(struct list_head *group_list,
-				     struct pmu_event *pe,
-				     bool metric_no_group,
-				     int runtime)
+static int __add_metric(struct list_head *group_list,
+			struct pmu_event *pe,
+			bool metric_no_group,
+			int runtime)
 {
 	struct egroup *eg;
 
@@ -643,10 +643,7 @@ static int add_metric(struct list_head *group_list,
 	pr_debug("metric expr %s for %s\n", pe->metric_expr, pe->metric_name);
 
 	if (!strstr(pe->metric_expr, "?")) {
-		ret = __metricgroup__add_metric(group_list,
-						pe,
-						metric_no_group,
-						1);
+		ret = __add_metric(group_list, pe, metric_no_group, 1);
 	} else {
 		int j, count;
 
@@ -658,9 +655,7 @@ static int add_metric(struct list_head *group_list,
 		 */
 
 		for (j = 0; j < count && !ret; j++) {
-			ret = __metricgroup__add_metric(
-				group_list, pe,
-				metric_no_group, j);
+			ret = __add_metric(group_list, pe, metric_no_group, j);
 		}
 	}
 
-- 
2.25.4

[PATCH 09/18] perf metric: Collect referenced metrics in struct metric_ref_node

[Jiri Olsa]

Collecting referenced metrics in struct metric_ref_node object,
so we can process them later on.

The change will parse nested metric names out of expression and
'resolve' them.

All referenced metrics are dissolved into one context, meaning all
nested metrics events and added to the parent context.

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

diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
index f0b0a053bfd2..9923eef1e2d4 100644
--- a/tools/perf/util/metricgroup.c
+++ b/tools/perf/util/metricgroup.c
@@ -102,12 +102,20 @@ void metricgroup__rblist_exit(struct rblist *metric_events)
 	rblist__exit(metric_events);
 }
 
+struct metric_ref_node {
+	const char *metric_name;
+	const char *metric_expr;
+	struct list_head list;
+};
+
 struct egroup {
 	struct list_head nd;
 	struct expr_parse_ctx pctx;
 	const char *metric_name;
 	const char *metric_expr;
 	const char *metric_unit;
+	struct list_head refs;
+	int refs_cnt;
 	int runtime;
 	bool has_constraint;
 };
@@ -574,27 +582,66 @@ int __weak arch_get_runtimeparam(void)
 static int __add_metric(struct list_head *group_list,
 			struct pmu_event *pe,
 			bool metric_no_group,
-			int runtime)
+			int runtime,
+		        struct egroup **egp)
 {
+	struct metric_ref_node *ref;
 	struct egroup *eg;
 
-	eg = malloc(sizeof(*eg));
-	if (!eg)
-		return -ENOMEM;
+	if (*egp == NULL) {
+		/*
+		 * We got in here for the master group,
+		 * allocate it and put it on the list.
+		 */
+		eg = malloc(sizeof(*eg));
+		if (!eg)
+			return -ENOMEM;
+
+		expr__ctx_init(&eg->pctx);
+		eg->metric_name = pe->metric_name;
+		eg->metric_expr = pe->metric_expr;
+		eg->metric_unit = pe->unit;
+		eg->runtime = runtime;
+		eg->has_constraint = metric_no_group || metricgroup__has_constraint(pe);
+		INIT_LIST_HEAD(&eg->refs);
+		eg->refs_cnt = 0;
+		*egp = eg;
+	} else {
+		/*
+		 * We got here for the referenced metric, via the
+		 * recursive metricgroup__add_metric call, add
+		 * it to the master group.
+		 */
+		eg = *egp;
+
+		ref = malloc(sizeof(*ref));
+		if (!ref)
+			return -ENOMEM;
 
-	expr__ctx_init(&eg->pctx);
-	eg->metric_name = pe->metric_name;
-	eg->metric_expr = pe->metric_expr;
-	eg->metric_unit = pe->unit;
-	eg->runtime = runtime;
-	eg->has_constraint = metric_no_group || metricgroup__has_constraint(pe);
+		ref->metric_name = pe->metric_name;
+		ref->metric_expr = pe->metric_expr;
+		list_add(&ref->list, &eg->refs);
+		eg->refs_cnt++;
+		eg->has_constraint |= metricgroup__has_constraint(pe);
+	}
 
+	/*
+	 * For both the master and referenced metrics, we parse
+	 * all the metric's IDs and add it to the parent context.
+	 */
 	if (expr__find_other(pe->metric_expr, NULL, &eg->pctx, runtime) < 0) {
 		expr__ctx_clear(&eg->pctx);
 		free(eg);
 		return -EINVAL;
 	}
 
+	/*
+	 * We add new group only in the 'master' call,
+	 * so bail out for referenced metric case.
+	 */
+	if (eg->refs_cnt)
+		return 0;
+
 	if (list_empty(group_list))
 		list_add(&eg->nd, group_list);
 	else {
@@ -636,14 +683,63 @@ static struct pmu_event *find_metric(const char *metric, struct pmu_events_map *
 
 static int add_metric(struct list_head *group_list,
 		      struct pmu_event *pe,
-		      bool metric_no_group)
+		      bool metric_no_group,
+		      struct egroup **egp);
+
+static int resolve_metric(struct egroup *eg,
+			  bool metric_no_group,
+			  struct list_head *group_list,
+			  struct pmu_events_map *map)
+{
+	struct hashmap_entry *cur;
+	size_t bkt;
+	bool all;
+	int ret;
+
+	/*
+	 * Iterate all the parsed IDs and if there's metric,
+	 * add it to the context.
+	 */
+	do {
+		all = true;
+		hashmap__for_each_entry((&eg->pctx.ids), cur, bkt) {
+			struct pmu_event *pe;
+
+			pe = find_metric(cur->key, map);
+			if (!pe)
+				continue;
+
+			all = false;
+			/* The metric key itself needs to go out.. */
+			expr__del_id(&eg->pctx, cur->key);
+
+			/* ... and it gets resolved to the parent context. */
+			ret = add_metric(group_list, pe, metric_no_group, &eg);
+			if (ret)
+				return ret;
+
+			/*
+			 * We added new metric to hashmap, so we need
+			 * to break the iteration and start over.
+			 */
+			break;
+		}
+	} while (!all);
+
+	return 0;
+}
+
+static int add_metric(struct list_head *group_list,
+		      struct pmu_event *pe,
+		      bool metric_no_group,
+		      struct egroup **egp)
 {
 	int ret = 0;
 
 	pr_debug("metric expr %s for %s\n", pe->metric_expr, pe->metric_name);
 
 	if (!strstr(pe->metric_expr, "?")) {
-		ret = __add_metric(group_list, pe, metric_no_group, 1);
+		ret = __add_metric(group_list, pe, metric_no_group, 1, egp);
 	} else {
 		int j, count;
 
@@ -655,7 +751,7 @@ static int add_metric(struct list_head *group_list,
 		 */
 
 		for (j = 0; j < count && !ret; j++) {
-			ret = __add_metric(group_list, pe, metric_no_group, j);
+			ret = __add_metric(group_list, pe, metric_no_group, j, egp);
 		}
 	}
 
@@ -666,16 +762,26 @@ static int metricgroup__add_metric(const char *metric, bool metric_no_group,
 				   struct strbuf *events,
 				   struct list_head *group_list,
 				   struct pmu_events_map *map)
+
 {
+	struct egroup *eg = NULL;
 	struct pmu_event *pe;
-	struct egroup *eg;
 	int ret;
 
 	pe = find_metric(metric, map);
 	if (!pe)
 		return -EINVAL;
 
-	ret = add_metric(group_list, pe, metric_no_group);
+	ret = add_metric(group_list, pe, metric_no_group, &eg);
+	if (ret)
+		return ret;
+
+	/*
+	 * Process any possible referenced metrics
+	 * included in the expression.
+	 */
+	ret = resolve_metric(eg, metric_no_group,
+			     group_list, map);
 	if (ret)
 		return ret;
 
@@ -727,11 +833,22 @@ static int metricgroup__add_metric_list(const char *list, bool metric_no_group,
 	return ret;
 }
 
+static void egroup__free_refs(struct egroup *egroup)
+{
+	struct metric_ref_node *ref, *tmp;
+
+	list_for_each_entry_safe(ref, tmp, &egroup->refs, list) {
+		list_del(&ref->list);
+		free(ref);
+	}
+}
+
 static void metricgroup__free_egroups(struct list_head *group_list)
 {
 	struct egroup *eg, *egtmp;
 
 	list_for_each_entry_safe (eg, egtmp, group_list, nd) {
+		egroup__free_refs(eg);
 		expr__ctx_clear(&eg->pctx);
 		list_del_init(&eg->nd);
 		free(eg);
-- 
2.25.4

[PATCH 10/18] perf metric: Collect referenced metrics in struct metric_expr

[Jiri Olsa]

Add referenced metrics into struct metric_expr object,
so they are accessible when computing the metric.

Storing just name and expression itself, so the metric
can be resolved and computed.

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

diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
index 9923eef1e2d4..8cbcc5e05fef 100644
--- a/tools/perf/util/metricgroup.c
+++ b/tools/perf/util/metricgroup.c
@@ -83,6 +83,7 @@ static void metric_event_delete(struct rblist *rblist __maybe_unused,
 	struct metric_expr *expr, *tmp;
 
 	list_for_each_entry_safe(expr, tmp, &me->head, nd) {
+		free(expr->metric_refs);
 		free(expr);
 	}
 
@@ -243,6 +244,7 @@ static int metricgroup__setup_events(struct list_head *groups,
 
 	list_for_each_entry (eg, groups, nd) {
 		struct evsel **metric_events;
+		struct metric_ref *metric_refs = NULL;
 
 		metric_events = calloc(sizeof(void *),
 				hashmap__size(&eg->pctx.ids) + 1);
@@ -274,6 +276,30 @@ static int metricgroup__setup_events(struct list_head *groups,
 			free(metric_events);
 			break;
 		}
+
+		/*
+		 * Collect and store collected nested expressions
+		 * for metric processing.
+		 */
+		if (eg->refs_cnt) {
+			struct metric_ref_node *ref;
+
+			metric_refs = zalloc(sizeof(struct metric_ref) * (eg->refs_cnt + 1));
+			if (!metric_refs) {
+				ret = -ENOMEM;
+				free(metric_events);
+				break;
+			}
+
+			i = 0;
+			list_for_each_entry(ref, &eg->refs, list) {
+				metric_refs[i].metric_name = ref->metric_name;
+				metric_refs[i].metric_expr = ref->metric_expr;
+				i++;
+			}
+		};
+
+		expr->metric_refs = metric_refs;
 		expr->metric_expr = eg->metric_expr;
 		expr->metric_name = eg->metric_name;
 		expr->metric_unit = eg->metric_unit;
diff --git a/tools/perf/util/metricgroup.h b/tools/perf/util/metricgroup.h
index 8315bd1a7da4..62623a39cbec 100644
--- a/tools/perf/util/metricgroup.h
+++ b/tools/perf/util/metricgroup.h
@@ -18,12 +18,18 @@ struct metric_event {
 	struct list_head head; /* list of metric_expr */
 };
 
+struct metric_ref {
+	const char *metric_name;
+	const char *metric_expr;
+};
+
 struct metric_expr {
 	struct list_head nd;
 	const char *metric_expr;
 	const char *metric_name;
 	const char *metric_unit;
 	struct evsel **metric_events;
+	struct metric_ref *metric_refs;
 	int runtime;
 };
 
-- 
2.25.4

[PATCH 11/18] perf metric: Add referenced metrics to hash data

[Jiri Olsa]

Adding referenced metrics to the parsing context so they
can be resolved during the metric processing.

Adding expr__add_ref function to store referenced metrics
into parse context.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/perf/util/expr.c        | 35 +++++++++++++++++++++++++++++++++++
 tools/perf/util/expr.h        | 13 ++++++++++++-
 tools/perf/util/stat-shadow.c | 20 ++++++++++++++------
 3 files changed, 61 insertions(+), 7 deletions(-)

diff --git a/tools/perf/util/expr.c b/tools/perf/util/expr.c
index 91142d4c3419..6bf8a21f5c53 100644
--- a/tools/perf/util/expr.c
+++ b/tools/perf/util/expr.c
@@ -4,6 +4,8 @@
 #include <errno.h>
 #include <stdlib.h>
 #include <string.h>
+#include "metricgroup.h"
+#include "debug.h"
 #include "expr.h"
 #include "expr-bison.h"
 #include "expr-flex.h"
@@ -61,6 +63,7 @@ int expr__add_id_val(struct expr_parse_ctx *ctx, const char *id, double val)
 	if (!data_ptr)
 		return -ENOMEM;
 	data_ptr->val = val;
+	data_ptr->is_ref = false;
 
 	ret = hashmap__set(&ctx->ids, id, data_ptr,
 			   (const void **)&old_key, (void **)&old_data);
@@ -69,6 +72,38 @@ int expr__add_id_val(struct expr_parse_ctx *ctx, const char *id, double val)
 	return ret;
 }
 
+int expr__add_ref(struct expr_parse_ctx *ctx, struct metric_ref *ref)
+{
+	struct expr_id_data *data_ptr = NULL, *old_data = NULL;
+	char *old_key = NULL;
+	char *name;
+	int ret;
+
+	data_ptr = zalloc(sizeof(*data_ptr));
+	if (!data_ptr)
+		return -ENOMEM;
+
+	name = strdup(ref->metric_name);
+	if (!name) {
+		free(data_ptr);
+		return -ENOMEM;
+	}
+
+	data_ptr->ref.metric_name = ref->metric_name;
+	data_ptr->ref.metric_expr = ref->metric_expr;
+	data_ptr->is_ref = true;
+
+	ret = hashmap__set(&ctx->ids, name, data_ptr,
+			   (const void **)&old_key, (void **)&old_data);
+
+	pr_debug2("adding ref metric %s: %s\n",
+		  ref->metric_name, ref->metric_expr);
+
+	free(old_key);
+	free(old_data);
+	return ret;
+}
+
 int expr__get_id(struct expr_parse_ctx *ctx, const char *id,
 		 struct expr_id_data **data)
 {
diff --git a/tools/perf/util/expr.h b/tools/perf/util/expr.h
index 2462abd0ac65..d19e66915228 100644
--- a/tools/perf/util/expr.h
+++ b/tools/perf/util/expr.h
@@ -11,12 +11,22 @@
 #include "util/hashmap.h"
 //#endif
 
+struct metric_ref;
+
 struct expr_parse_ctx {
 	struct hashmap ids;
 };
 
 struct expr_id_data {
-	double	val;
+	bool	is_ref;
+
+	union {
+		double	val;
+		struct {
+			const char *metric_name;
+			const char *metric_expr;
+		} ref;
+	};
 };
 
 struct expr_scanner_ctx {
@@ -29,6 +39,7 @@ void expr__ctx_clear(struct expr_parse_ctx *ctx);
 void expr__del_id(struct expr_parse_ctx *ctx, const char *id);
 int expr__add_id(struct expr_parse_ctx *ctx, const char *id);
 int expr__add_id_val(struct expr_parse_ctx *ctx, const char *id, double val);
+int expr__add_ref(struct expr_parse_ctx *ctx, struct metric_ref *ref);
 int expr__get_id(struct expr_parse_ctx *ctx, const char *id,
 		 struct expr_id_data **data);
 int expr__parse(double *final_val, struct expr_parse_ctx *ctx,
diff --git a/tools/perf/util/stat-shadow.c b/tools/perf/util/stat-shadow.c
index fc9ac4b4218e..e1ba6c1b916a 100644
--- a/tools/perf/util/stat-shadow.c
+++ b/tools/perf/util/stat-shadow.c
@@ -731,13 +731,14 @@ static void print_smi_cost(struct perf_stat_config *config,
 }
 
 static int prepare_metric(struct evsel **metric_events,
+			  struct metric_ref *metric_refs,
 			  struct expr_parse_ctx *pctx,
 			  int cpu,
 			  struct runtime_stat *st)
 {
 	double scale;
 	char *n, *pn;
-	int i;
+	int i, j, ret;
 
 	expr__ctx_init(pctx);
 	for (i = 0; metric_events[i]; i++) {
@@ -778,12 +779,19 @@ static int prepare_metric(struct evsel **metric_events,
 			expr__add_id_val(pctx, n, avg_stats(stats)*scale);
 	}
 
+	for (j = 0; metric_refs && metric_refs[j].metric_name; j++) {
+		ret = expr__add_ref(pctx, &metric_refs[j]);
+		if (ret)
+			return ret;
+	}
+
 	return i;
 }
 
 static void generic_metric(struct perf_stat_config *config,
 			   const char *metric_expr,
 			   struct evsel **metric_events,
+			   struct metric_ref *metric_refs,
 			   char *name,
 			   const char *metric_name,
 			   const char *metric_unit,
@@ -798,7 +806,7 @@ static void generic_metric(struct perf_stat_config *config,
 	int i;
 	void *ctxp = out->ctx;
 
-	i = prepare_metric(metric_events, &pctx, cpu, st);
+	i = prepare_metric(metric_events, metric_refs, &pctx, cpu, st);
 	if (i < 0)
 		return;
 
@@ -847,7 +855,7 @@ double test_generic_metric(struct metric_expr *mexp, int cpu, struct runtime_sta
 	struct expr_parse_ctx pctx;
 	double ratio;
 
-	if (prepare_metric(mexp->metric_events, &pctx, cpu, st) < 0)
+	if (prepare_metric(mexp->metric_events, mexp->metric_refs, &pctx, cpu, st) < 0)
 		return 0.;
 
 	if (expr__parse(&ratio, &pctx, mexp->metric_expr, 1))
@@ -1064,8 +1072,8 @@ void perf_stat__print_shadow_stats(struct perf_stat_config *config,
 		else
 			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, cpu, out, st);
+		generic_metric(config, evsel->metric_expr, evsel->metric_events, NULL,
+				evsel->name, 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];
@@ -1093,7 +1101,7 @@ void perf_stat__print_shadow_stats(struct perf_stat_config *config,
 			if (num++ > 0)
 				out->new_line(config, ctxp);
 			generic_metric(config, mexp->metric_expr, mexp->metric_events,
-					evsel->name, mexp->metric_name,
+					mexp->metric_refs, evsel->name, mexp->metric_name,
 					mexp->metric_unit, mexp->runtime, cpu, out, st);
 		}
 	}
-- 
2.25.4

[PATCH 12/18] perf metric: Compute referenced metrics

[Jiri Olsa]

Adding computation (expr__parse call) of referenced metric at
the point when it needs to be resolved during the 'master'
metric computation.

Once the inner metric is computed, the result is stored and
used if there's another usage of that metric.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/perf/util/expr.c | 31 +++++++++++++++++++++++++++++++
 tools/perf/util/expr.h |  3 +++
 tools/perf/util/expr.y |  4 ++--
 3 files changed, 36 insertions(+), 2 deletions(-)

diff --git a/tools/perf/util/expr.c b/tools/perf/util/expr.c
index 6bf8a21f5c53..2784df38f8c2 100644
--- a/tools/perf/util/expr.c
+++ b/tools/perf/util/expr.c
@@ -91,6 +91,7 @@ int expr__add_ref(struct expr_parse_ctx *ctx, struct metric_ref *ref)
 
 	data_ptr->ref.metric_name = ref->metric_name;
 	data_ptr->ref.metric_expr = ref->metric_expr;
+	data_ptr->ref.counted = false;
 	data_ptr->is_ref = true;
 
 	ret = hashmap__set(&ctx->ids, name, data_ptr,
@@ -110,6 +111,34 @@ int expr__get_id(struct expr_parse_ctx *ctx, const char *id,
 	return hashmap__find(&ctx->ids, id, (void **)data) ? 0 : -1;
 }
 
+int expr__resolve_id(struct expr_parse_ctx *ctx, const char *id,
+		     struct expr_id_data **datap)
+{
+	struct expr_id_data *data;
+
+	if (expr__get_id(ctx, id, datap) || !*datap) {
+		pr_debug("%s not found\n", id);
+		return -1;
+	}
+
+	data = *datap;
+
+	pr_debug2("lookup: is_ref %d, counted %d, val %f: %s\n",
+		  data->is_ref, data->ref.counted, data->val, id);
+
+	if (data->is_ref && !data->ref.counted) {
+		data->ref.counted = true;
+		pr_debug("processing metric: %s ENTRY\n", id);
+		if (expr__parse(&data->val, ctx, data->ref.metric_expr, 1)) {
+			pr_debug("%s failed to count\n", id);
+			return -1;
+		}
+		pr_debug("processing metric: %s EXIT: %f\n", id, data->val);
+	}
+
+	return 0;
+}
+
 void expr__del_id(struct expr_parse_ctx *ctx, const char *id)
 {
 	struct expr_id_data *old_val = NULL;
@@ -150,6 +179,8 @@ __expr__parse(double *val, struct expr_parse_ctx *ctx, const char *expr,
 	void *scanner;
 	int ret;
 
+	pr_debug2("parsing metric: %s\n", expr);
+
 	ret = expr_lex_init_extra(&scanner_ctx, &scanner);
 	if (ret)
 		return ret;
diff --git a/tools/perf/util/expr.h b/tools/perf/util/expr.h
index d19e66915228..b6d1fba64eaa 100644
--- a/tools/perf/util/expr.h
+++ b/tools/perf/util/expr.h
@@ -25,6 +25,7 @@ struct expr_id_data {
 		struct {
 			const char *metric_name;
 			const char *metric_expr;
+			bool counted;
 		} ref;
 	};
 };
@@ -42,6 +43,8 @@ int expr__add_id_val(struct expr_parse_ctx *ctx, const char *id, double val);
 int expr__add_ref(struct expr_parse_ctx *ctx, struct metric_ref *ref);
 int expr__get_id(struct expr_parse_ctx *ctx, const char *id,
 		 struct expr_id_data **data);
+int expr__resolve_id(struct expr_parse_ctx *ctx, const char *id,
+		     struct expr_id_data **datap);
 int expr__parse(double *final_val, struct expr_parse_ctx *ctx,
 		const char *expr, int runtime);
 int expr__find_other(const char *expr, const char *one,
diff --git a/tools/perf/util/expr.y b/tools/perf/util/expr.y
index 0d4f5d324be7..d34b370391c6 100644
--- a/tools/perf/util/expr.y
+++ b/tools/perf/util/expr.y
@@ -88,11 +88,11 @@ expr:	  NUMBER
 	| ID			{
 					struct expr_id_data *data;
 
-					if (expr__get_id(ctx, $1, &data) || !data) {
-						pr_debug("%s not found\n", $1);
+					if (expr__resolve_id(ctx, $1, &data)) {
 						free($1);
 						YYABORT;
 					}
+
 					$$ = data->val;
 					free($1);
 				}
-- 
2.25.4

[PATCH 13/18] perf metric: Add events for the current group

[Jiri Olsa]

There's no need to iterate the whole list of groups,
when adding new events. The currently created group
is the one we want to add.

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

diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
index 8cbcc5e05fef..66f25362702d 100644
--- a/tools/perf/util/metricgroup.c
+++ b/tools/perf/util/metricgroup.c
@@ -811,17 +811,19 @@ static int metricgroup__add_metric(const char *metric, bool metric_no_group,
 	if (ret)
 		return ret;
 
-	list_for_each_entry(eg, group_list, nd) {
-		if (events->len > 0)
-			strbuf_addf(events, ",");
+	if (events->len > 0)
+		strbuf_addf(events, ",");
 
-		if (eg->has_constraint) {
-			metricgroup__add_metric_non_group(events,
-							  &eg->pctx);
-		} else {
-			metricgroup__add_metric_weak_group(events,
-							   &eg->pctx);
-		}
+	/*
+	 * Even if we add multiple groups through the runtime
+	 * param, they share same events.
+	 */
+	if (eg->has_constraint) {
+		metricgroup__add_metric_non_group(events,
+						  &eg->pctx);
+	} else {
+		metricgroup__add_metric_weak_group(events,
+						   &eg->pctx);
 	}
 	return 0;
 }
-- 
2.25.4

[PATCH 14/18] perf metric: Add cache_miss_cycles to metric parse test

[Jiri Olsa]

Adding test that compute metric with other metrics in it.

  cache_miss_cycles = metric:dcache_miss_cpi + metric:icache_miss_cycles

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

diff --git a/tools/perf/tests/parse-metric.c b/tools/perf/tests/parse-metric.c
index 8c48251425e1..28f33893338b 100644
--- a/tools/perf/tests/parse-metric.c
+++ b/tools/perf/tests/parse-metric.c
@@ -11,6 +11,8 @@
 #include "debug.h"
 #include "expr.h"
 #include "stat.h"
+#include <perf/cpumap.h>
+#include <perf/evlist.h>
 
 static struct pmu_event pme_test[] = {
 {
@@ -22,6 +24,18 @@ static struct pmu_event pme_test[] = {
 			  "( 1 + cpu_clk_unhalted.one_thread_active / cpu_clk_unhalted.ref_xclk ) )))",
 	.metric_name	= "Frontend_Bound_SMT",
 },
+{
+	.metric_expr	= "l1d\\-loads\\-misses / inst_retired.any",
+	.metric_name	= "dcache_miss_cpi",
+},
+{
+	.metric_expr	= "l1i\\-loads\\-misses / inst_retired.any",
+	.metric_name	= "icache_miss_cycles",
+},
+{
+	.metric_expr	= "(dcache_miss_cpi + icache_miss_cycles)",
+	.metric_name	= "cache_miss_cycles",
+},
 };
 
 static struct pmu_events_map map = {
@@ -162,9 +176,28 @@ static int test_frontend(void)
 	return 0;
 }
 
+static int test_cache_miss_cycles(void)
+{
+	double ratio;
+	struct value vals[] = {
+		{ .event = "l1d-loads-misses",  .val = 300 },
+		{ .event = "l1i-loads-misses",  .val = 200 },
+		{ .event = "inst_retired.any",  .val = 400 },
+		{ 0 },
+	};
+
+	TEST_ASSERT_VAL("failed to compute metric",
+			compute_metric("cache_miss_cycles", vals, &ratio) == 0);
+
+	TEST_ASSERT_VAL("cache_miss_cycles failed, wrong ratio",
+			ratio == 1.25);
+	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);
+	TEST_ASSERT_VAL("cache_miss_cycles failed", test_cache_miss_cycles() == 0);
 	return 0;
 }
-- 
2.25.4

[PATCH 15/18] perf metric: Add DCache_L2 to metric parse test

[Jiri Olsa]

Adding test that compute DCache_L2 metrics with other related metrics in it.

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

diff --git a/tools/perf/tests/parse-metric.c b/tools/perf/tests/parse-metric.c
index 28f33893338b..b50e2a3f3b73 100644
--- a/tools/perf/tests/parse-metric.c
+++ b/tools/perf/tests/parse-metric.c
@@ -36,6 +36,27 @@ static struct pmu_event pme_test[] = {
 	.metric_expr	= "(dcache_miss_cpi + icache_miss_cycles)",
 	.metric_name	= "cache_miss_cycles",
 },
+{
+	.metric_expr	= "L2_RQSTS.DEMAND_DATA_RD_HIT + L2_RQSTS.PF_HIT + L2_RQSTS.RFO_HIT",
+	.metric_name	= "DCache_L2_All_Hits",
+},
+{
+	.metric_expr	= "max(L2_RQSTS.ALL_DEMAND_DATA_RD - L2_RQSTS.DEMAND_DATA_RD_HIT, 0) + "
+			  "L2_RQSTS.PF_MISS + L2_RQSTS.RFO_MISS",
+	.metric_name	= "DCache_L2_All_Miss",
+},
+{
+	.metric_expr	= "DCache_L2_All_Hits + DCache_L2_All_Miss",
+	.metric_name	= "DCache_L2_All",
+},
+{
+	.metric_expr	= "d_ratio(DCache_L2_All_Hits, DCache_L2_All)",
+	.metric_name	= "DCache_L2_Hits",
+},
+{
+	.metric_expr	= "d_ratio(DCache_L2_All_Miss, DCache_L2_All)",
+	.metric_name	= "DCache_L2_Misses",
+},
 };
 
 static struct pmu_events_map map = {
@@ -194,10 +215,60 @@ static int test_cache_miss_cycles(void)
 	return 0;
 }
 
+
+/*
+ * DCache_L2_All_Hits = L2_RQSTS.DEMAND_DATA_RD_HIT + L2_RQSTS.PF_HIT + L2_RQSTS.RFO_HI
+ * DCache_L2_All_Miss = MAX(L2_RQSTS.ALL_DEMAND_DATA_RD - L2_RQSTS.DEMAND_DATA_RD_HIT, 0) +
+ *                      L2_RQSTS.PF_MISS + L2_RQSTS.RFO_MISS
+ * DCache_L2_All      = DCache_L2_All_Hits + DCache_L2_All_Miss
+ * DCache_L2_Hits     = d_ratio(DCache_L2_All_Hits, DCache_L2_All)
+ * DCache_L2_Misses   = d_ratio(DCache_L2_All_Miss, DCache_L2_All)
+ *
+ * L2_RQSTS.DEMAND_DATA_RD_HIT = 100
+ * L2_RQSTS.PF_HIT             = 200
+ * L2_RQSTS.RFO_HI             = 300
+ * L2_RQSTS.ALL_DEMAND_DATA_RD = 400
+ * L2_RQSTS.PF_MISS            = 500
+ * L2_RQSTS.RFO_MISS           = 600
+ *
+ * DCache_L2_All_Hits = 600
+ * DCache_L2_All_Miss = MAX(400 - 100, 0) + 500 + 600 = 1400
+ * DCache_L2_All      = 600 + 1400  = 2000
+ * DCache_L2_Hits     = 600 / 2000  = 0.3
+ * DCache_L2_Misses   = 1400 / 2000 = 0.7
+ */
+static int test_dcache_l2(void)
+{
+	double ratio;
+	struct value vals[] = {
+		{ .event = "L2_RQSTS.DEMAND_DATA_RD_HIT", .val = 100 },
+		{ .event = "L2_RQSTS.PF_HIT",             .val = 200 },
+		{ .event = "L2_RQSTS.RFO_HIT",            .val = 300 },
+		{ .event = "L2_RQSTS.ALL_DEMAND_DATA_RD", .val = 400 },
+		{ .event = "L2_RQSTS.PF_MISS",            .val = 500 },
+		{ .event = "L2_RQSTS.RFO_MISS",           .val = 600 },
+		{ 0 },
+	};
+
+	TEST_ASSERT_VAL("failed to compute metric",
+			compute_metric("DCache_L2_Hits", vals, &ratio) == 0);
+
+	TEST_ASSERT_VAL("DCache_L2_Hits failed, wrong ratio",
+			ratio == 0.3);
+
+	TEST_ASSERT_VAL("failed to compute metric",
+			compute_metric("DCache_L2_Misses", vals, &ratio) == 0);
+
+	TEST_ASSERT_VAL("DCache_L2_Misses failed, wrong ratio",
+			ratio == 0.7);
+	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);
 	TEST_ASSERT_VAL("cache_miss_cycles failed", test_cache_miss_cycles() == 0);
+	TEST_ASSERT_VAL("DCache_L2 failed", test_dcache_l2() == 0);
 	return 0;
 }
-- 
2.25.4

[PATCH 16/18] perf metric: Add recursion check when processing nested metrics

[Jiri Olsa]

Keeping the stack of nested metrics via 'struct expr_id' objecs
and checking if we are in recursion via already processed metric.

The stack is implemented as static array within the struct egroup
with 100 entries, which should be enough nesting depth for any
metric we have or plan to have at the moment.

Adding test that simulates the recursion and checks we can
detect it.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/perf/tests/parse-metric.c |  27 ++++++++-
 tools/perf/util/expr.c          |   2 +
 tools/perf/util/expr.h          |   9 ++-
 tools/perf/util/metricgroup.c   | 101 +++++++++++++++++++++++++++++---
 4 files changed, 128 insertions(+), 11 deletions(-)

diff --git a/tools/perf/tests/parse-metric.c b/tools/perf/tests/parse-metric.c
index b50e2a3f3b73..238a805edd55 100644
--- a/tools/perf/tests/parse-metric.c
+++ b/tools/perf/tests/parse-metric.c
@@ -57,6 +57,14 @@ static struct pmu_event pme_test[] = {
 	.metric_expr	= "d_ratio(DCache_L2_All_Miss, DCache_L2_All)",
 	.metric_name	= "DCache_L2_Misses",
 },
+{
+	.metric_expr	= "IPC + M2",
+	.metric_name	= "M1",
+},
+{
+	.metric_expr	= "IPC + M1",
+	.metric_name	= "M2",
+},
 };
 
 static struct pmu_events_map map = {
@@ -139,8 +147,8 @@ static int compute_metric(const char *name, struct value *vals, double *ratio)
 	err = metricgroup__parse_groups_test(evlist, &map, name,
 					     false, false,
 					     &metric_events);
-
-	TEST_ASSERT_VAL("failed to parse metric", err == 0);
+	if (err)
+		return err;
 
 	if (perf_evlist__alloc_stats(evlist, false))
 		return -1;
@@ -264,11 +272,26 @@ static int test_dcache_l2(void)
 	return 0;
 }
 
+static int test_recursion_fail(void)
+{
+	double ratio;
+	struct value vals[] = {
+		{ .event = "inst_retired.any",        .val = 300 },
+		{ .event = "cpu_clk_unhalted.thread", .val = 200 },
+		{ 0 },
+	};
+
+	TEST_ASSERT_VAL("failed to find recursion",
+			compute_metric("M1", vals, &ratio) == -1);
+	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);
 	TEST_ASSERT_VAL("cache_miss_cycles failed", test_cache_miss_cycles() == 0);
 	TEST_ASSERT_VAL("DCache_L2 failed", test_dcache_l2() == 0);
+	TEST_ASSERT_VAL("recursion fail failed", test_recursion_fail() == 0);
 	return 0;
 }
diff --git a/tools/perf/util/expr.c b/tools/perf/util/expr.c
index 2784df38f8c2..d23d8897bc61 100644
--- a/tools/perf/util/expr.c
+++ b/tools/perf/util/expr.c
@@ -45,6 +45,8 @@ int expr__add_id(struct expr_parse_ctx *ctx, const char *id)
 	if (!data_ptr)
 		return -ENOMEM;
 
+	data_ptr->parent = ctx->parent;
+
 	ret = hashmap__set(&ctx->ids, id, data_ptr,
 			   (const void **)&old_key, (void **)&old_data);
 	free(old_key);
diff --git a/tools/perf/util/expr.h b/tools/perf/util/expr.h
index b6d1fba64eaa..e5fdeab928c7 100644
--- a/tools/perf/util/expr.h
+++ b/tools/perf/util/expr.h
@@ -13,8 +13,14 @@
 
 struct metric_ref;
 
+struct expr_id {
+	char		*id;
+	struct expr_id	*parent;
+};
+
 struct expr_parse_ctx {
-	struct hashmap ids;
+	struct hashmap	 ids;
+	struct expr_id	*parent;
 };
 
 struct expr_id_data {
@@ -27,6 +33,7 @@ struct expr_id_data {
 			const char *metric_expr;
 			bool counted;
 		} ref;
+		struct expr_id	*parent;
 	};
 };
 
diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
index 66f25362702d..69ec20dd737b 100644
--- a/tools/perf/util/metricgroup.c
+++ b/tools/perf/util/metricgroup.c
@@ -24,6 +24,7 @@
 #include <subcmd/parse-options.h>
 #include <api/fs/fs.h>
 #include "util.h"
+#include <asm/bug.h>
 
 struct metric_event *metricgroup__lookup(struct rblist *metric_events,
 					 struct evsel *evsel,
@@ -109,6 +110,8 @@ struct metric_ref_node {
 	struct list_head list;
 };
 
+#define RECURSION_ID_MAX 100
+
 struct egroup {
 	struct list_head nd;
 	struct expr_parse_ctx pctx;
@@ -119,6 +122,11 @@ struct egroup {
 	int refs_cnt;
 	int runtime;
 	bool has_constraint;
+
+	struct {
+		struct expr_id	id[RECURSION_ID_MAX];
+		int		cnt;
+	} recursion;
 };
 
 /**
@@ -609,7 +617,8 @@ static int __add_metric(struct list_head *group_list,
 			struct pmu_event *pe,
 			bool metric_no_group,
 			int runtime,
-		        struct egroup **egp)
+			struct egroup **egp,
+			struct expr_id *parent)
 {
 	struct metric_ref_node *ref;
 	struct egroup *eg;
@@ -619,7 +628,7 @@ static int __add_metric(struct list_head *group_list,
 		 * We got in here for the master group,
 		 * allocate it and put it on the list.
 		 */
-		eg = malloc(sizeof(*eg));
+		eg = zalloc(sizeof(*eg));
 		if (!eg)
 			return -ENOMEM;
 
@@ -632,6 +641,16 @@ static int __add_metric(struct list_head *group_list,
 		INIT_LIST_HEAD(&eg->refs);
 		eg->refs_cnt = 0;
 		*egp = eg;
+
+		/* Initialize first recursion exr id with base name. */
+		parent = &eg->recursion.id[0];
+		eg->recursion.cnt = 1;
+
+		parent->id = strdup(pe->metric_name);
+		if (!parent->id) {
+			free(eg);
+			return -ENOMEM;
+		}
 	} else {
 		/*
 		 * We got here for the referenced metric, via the
@@ -651,6 +670,10 @@ static int __add_metric(struct list_head *group_list,
 		eg->has_constraint |= metricgroup__has_constraint(pe);
 	}
 
+	/* Force all found IDs in metric to have us as parent ID. */
+	WARN_ON_ONCE(!parent);
+	eg->pctx.parent = parent;
+
 	/*
 	 * For both the master and referenced metrics, we parse
 	 * all the metric's IDs and add it to the parent context.
@@ -707,10 +730,56 @@ static struct pmu_event *find_metric(const char *metric, struct pmu_events_map *
 	return NULL;
 }
 
+static int recursion_check(struct egroup *eg, const char *id, struct expr_id **parent)
+{
+	struct expr_id_data *data;
+	struct expr_id *p;
+	int ret;
+
+	/*
+	 * We get the parent referenced by 'id' argument and
+	 * traverse through all the parent object IDs to check
+	 * if we already processed 'id', if we did, it's recursion
+	 * and we fail.
+	 */
+	ret = expr__get_id(&eg->pctx, id, &data);
+	if (ret)
+		return ret;
+
+	p = data->parent;
+
+	while (p->parent) {
+		if (!strcmp(p->id, id)) {
+			pr_err("failed: recursion detected for %s\n", id);
+			return -1;
+		}
+		p = p->parent;
+	}
+
+	/*
+	 * If we are over the limit of static entris, the metric
+	 * is too difficult/nested to process, fail as well.
+	 */
+	if (eg->recursion.cnt + 1 >= RECURSION_ID_MAX) {
+		pr_err("failed: too many nested metrics\n");
+		return -EINVAL;
+	}
+
+	p = &eg->recursion.id[eg->recursion.cnt];
+	eg->recursion.cnt++;
+
+	p->id     = strdup(id);
+	p->parent = data->parent;
+	*parent   = p;
+
+	return p->id ? 0 : -ENOMEM;
+}
+
 static int add_metric(struct list_head *group_list,
 		      struct pmu_event *pe,
 		      bool metric_no_group,
-		      struct egroup **egp);
+		      struct egroup **egp,
+		      struct expr_id *parent);
 
 static int resolve_metric(struct egroup *eg,
 			  bool metric_no_group,
@@ -729,18 +798,23 @@ static int resolve_metric(struct egroup *eg,
 	do {
 		all = true;
 		hashmap__for_each_entry((&eg->pctx.ids), cur, bkt) {
+			struct expr_id *parent;
 			struct pmu_event *pe;
 
 			pe = find_metric(cur->key, map);
 			if (!pe)
 				continue;
 
+			ret = recursion_check(eg, cur->key, &parent);
+			if (ret)
+				return ret;
+
 			all = false;
 			/* The metric key itself needs to go out.. */
 			expr__del_id(&eg->pctx, cur->key);
 
 			/* ... and it gets resolved to the parent context. */
-			ret = add_metric(group_list, pe, metric_no_group, &eg);
+			ret = add_metric(group_list, pe, metric_no_group, &eg, parent);
 			if (ret)
 				return ret;
 
@@ -758,14 +832,15 @@ static int resolve_metric(struct egroup *eg,
 static int add_metric(struct list_head *group_list,
 		      struct pmu_event *pe,
 		      bool metric_no_group,
-		      struct egroup **egp)
+		      struct egroup **egp,
+		      struct expr_id *parent)
 {
 	int ret = 0;
 
 	pr_debug("metric expr %s for %s\n", pe->metric_expr, pe->metric_name);
 
 	if (!strstr(pe->metric_expr, "?")) {
-		ret = __add_metric(group_list, pe, metric_no_group, 1, egp);
+		ret = __add_metric(group_list, pe, metric_no_group, 1, egp, parent);
 	} else {
 		int j, count;
 
@@ -777,7 +852,7 @@ static int add_metric(struct list_head *group_list,
 		 */
 
 		for (j = 0; j < count && !ret; j++) {
-			ret = __add_metric(group_list, pe, metric_no_group, j, egp);
+			ret = __add_metric(group_list, pe, metric_no_group, j, egp, parent);
 		}
 	}
 
@@ -798,7 +873,7 @@ static int metricgroup__add_metric(const char *metric, bool metric_no_group,
 	if (!pe)
 		return -EINVAL;
 
-	ret = add_metric(group_list, pe, metric_no_group, &eg);
+	ret = add_metric(group_list, pe, metric_no_group, &eg, NULL);
 	if (ret)
 		return ret;
 
@@ -871,11 +946,21 @@ static void egroup__free_refs(struct egroup *egroup)
 	}
 }
 
+static void egroup__free_recursion(struct egroup *egroup)
+{
+	int i;
+
+	for (i = 0; i < egroup->recursion.cnt; i++) {
+		free(egroup->recursion.id[i].id);
+	}
+}
+
 static void metricgroup__free_egroups(struct list_head *group_list)
 {
 	struct egroup *eg, *egtmp;
 
 	list_for_each_entry_safe (eg, egtmp, group_list, nd) {
+		egroup__free_recursion(eg);
 		egroup__free_refs(eg);
 		expr__ctx_clear(&eg->pctx);
 		list_del_init(&eg->nd);
-- 
2.25.4

[PATCH 17/18] perf metric: Rename struct egroup to metric

[Jiri Olsa]

Renaming struct egroup to metric, because it seems
to make more sense. Plus renaming all the variables
that hold egroup to appropriate names.

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

diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
index 69ec20dd737b..9b880a5fb52b 100644
--- a/tools/perf/util/metricgroup.c
+++ b/tools/perf/util/metricgroup.c
@@ -112,7 +112,7 @@ struct metric_ref_node {
 
 #define RECURSION_ID_MAX 100
 
-struct egroup {
+struct metric {
 	struct list_head nd;
 	struct expr_parse_ctx pctx;
 	const char *metric_name;
@@ -242,7 +242,7 @@ static int metricgroup__setup_events(struct list_head *groups,
 	struct metric_expr *expr;
 	int i = 0;
 	int ret = 0;
-	struct egroup *eg;
+	struct metric *m;
 	struct evsel *evsel, *tmp;
 	unsigned long *evlist_used;
 
@@ -250,23 +250,23 @@ static int metricgroup__setup_events(struct list_head *groups,
 	if (!evlist_used)
 		return -ENOMEM;
 
-	list_for_each_entry (eg, groups, nd) {
+	list_for_each_entry (m, groups, nd) {
 		struct evsel **metric_events;
 		struct metric_ref *metric_refs = NULL;
 
 		metric_events = calloc(sizeof(void *),
-				hashmap__size(&eg->pctx.ids) + 1);
+				hashmap__size(&m->pctx.ids) + 1);
 		if (!metric_events) {
 			ret = -ENOMEM;
 			break;
 		}
-		evsel = find_evsel_group(perf_evlist, &eg->pctx,
+		evsel = find_evsel_group(perf_evlist, &m->pctx,
 					 metric_no_merge,
-					 eg->has_constraint, metric_events,
+					 m->has_constraint, metric_events,
 					 evlist_used);
 		if (!evsel) {
 			pr_debug("Cannot resolve %s: %s\n",
-					eg->metric_name, eg->metric_expr);
+					m->metric_name, m->metric_expr);
 			free(metric_events);
 			continue;
 		}
@@ -289,10 +289,10 @@ static int metricgroup__setup_events(struct list_head *groups,
 		 * Collect and store collected nested expressions
 		 * for metric processing.
 		 */
-		if (eg->refs_cnt) {
+		if (m->refs_cnt) {
 			struct metric_ref_node *ref;
 
-			metric_refs = zalloc(sizeof(struct metric_ref) * (eg->refs_cnt + 1));
+			metric_refs = zalloc(sizeof(struct metric_ref) * (m->refs_cnt + 1));
 			if (!metric_refs) {
 				ret = -ENOMEM;
 				free(metric_events);
@@ -300,7 +300,7 @@ static int metricgroup__setup_events(struct list_head *groups,
 			}
 
 			i = 0;
-			list_for_each_entry(ref, &eg->refs, list) {
+			list_for_each_entry(ref, &m->refs, list) {
 				metric_refs[i].metric_name = ref->metric_name;
 				metric_refs[i].metric_expr = ref->metric_expr;
 				i++;
@@ -308,11 +308,11 @@ static int metricgroup__setup_events(struct list_head *groups,
 		};
 
 		expr->metric_refs = metric_refs;
-		expr->metric_expr = eg->metric_expr;
-		expr->metric_name = eg->metric_name;
-		expr->metric_unit = eg->metric_unit;
+		expr->metric_expr = m->metric_expr;
+		expr->metric_name = m->metric_name;
+		expr->metric_unit = m->metric_unit;
 		expr->metric_events = metric_events;
-		expr->runtime = eg->runtime;
+		expr->runtime = m->runtime;
 		list_add(&expr->nd, &me->head);
 	}
 
@@ -617,38 +617,38 @@ static int __add_metric(struct list_head *group_list,
 			struct pmu_event *pe,
 			bool metric_no_group,
 			int runtime,
-			struct egroup **egp,
+		        struct metric **mp,
 			struct expr_id *parent)
 {
 	struct metric_ref_node *ref;
-	struct egroup *eg;
+	struct metric *m;
 
-	if (*egp == NULL) {
+	if (*mp == NULL) {
 		/*
 		 * We got in here for the master group,
 		 * allocate it and put it on the list.
 		 */
-		eg = zalloc(sizeof(*eg));
-		if (!eg)
+		m = zalloc(sizeof(*m));
+		if (!m)
 			return -ENOMEM;
 
-		expr__ctx_init(&eg->pctx);
-		eg->metric_name = pe->metric_name;
-		eg->metric_expr = pe->metric_expr;
-		eg->metric_unit = pe->unit;
-		eg->runtime = runtime;
-		eg->has_constraint = metric_no_group || metricgroup__has_constraint(pe);
-		INIT_LIST_HEAD(&eg->refs);
-		eg->refs_cnt = 0;
-		*egp = eg;
+		expr__ctx_init(&m->pctx);
+		m->metric_name = pe->metric_name;
+		m->metric_expr = pe->metric_expr;
+		m->metric_unit = pe->unit;
+		m->runtime = runtime;
+		m->has_constraint = metric_no_group || metricgroup__has_constraint(pe);
+		INIT_LIST_HEAD(&m->refs);
+		m->refs_cnt = 0;
+		*mp = m;
 
 		/* Initialize first recursion exr id with base name. */
-		parent = &eg->recursion.id[0];
-		eg->recursion.cnt = 1;
+		parent = &m->recursion.id[0];
+		m->recursion.cnt = 1;
 
 		parent->id = strdup(pe->metric_name);
 		if (!parent->id) {
-			free(eg);
+			free(m);
 			return -ENOMEM;
 		}
 	} else {
@@ -657,7 +657,7 @@ static int __add_metric(struct list_head *group_list,
 		 * recursive metricgroup__add_metric call, add
 		 * it to the master group.
 		 */
-		eg = *egp;
+		m = *mp;
 
 		ref = malloc(sizeof(*ref));
 		if (!ref)
@@ -665,22 +665,22 @@ static int __add_metric(struct list_head *group_list,
 
 		ref->metric_name = pe->metric_name;
 		ref->metric_expr = pe->metric_expr;
-		list_add(&ref->list, &eg->refs);
-		eg->refs_cnt++;
-		eg->has_constraint |= metricgroup__has_constraint(pe);
+		list_add(&ref->list, &m->refs);
+		m->refs_cnt++;
+		m->has_constraint |= metricgroup__has_constraint(pe);
 	}
 
 	/* Force all found IDs in metric to have us as parent ID. */
 	WARN_ON_ONCE(!parent);
-	eg->pctx.parent = parent;
+	m->pctx.parent = parent;
 
 	/*
 	 * For both the master and referenced metrics, we parse
 	 * all the metric's IDs and add it to the parent context.
 	 */
-	if (expr__find_other(pe->metric_expr, NULL, &eg->pctx, runtime) < 0) {
-		expr__ctx_clear(&eg->pctx);
-		free(eg);
+	if (expr__find_other(pe->metric_expr, NULL, &m->pctx, runtime) < 0) {
+		expr__ctx_clear(&m->pctx);
+		free(m);
 		return -EINVAL;
 	}
 
@@ -688,23 +688,23 @@ static int __add_metric(struct list_head *group_list,
 	 * We add new group only in the 'master' call,
 	 * so bail out for referenced metric case.
 	 */
-	if (eg->refs_cnt)
+	if (m->refs_cnt)
 		return 0;
 
 	if (list_empty(group_list))
-		list_add(&eg->nd, group_list);
+		list_add(&m->nd, group_list);
 	else {
 		struct list_head *pos;
 
 		/* Place the largest groups at the front. */
 		list_for_each_prev(pos, group_list) {
-			struct egroup *old = list_entry(pos, struct egroup, nd);
+			struct metric *old = list_entry(pos, struct metric, nd);
 
-			if (hashmap__size(&eg->pctx.ids) <=
+			if (hashmap__size(&m->pctx.ids) <=
 			    hashmap__size(&old->pctx.ids))
 				break;
 		}
-		list_add(&eg->nd, pos);
+		list_add(&m->nd, pos);
 	}
 
 	return 0;
@@ -730,7 +730,7 @@ static struct pmu_event *find_metric(const char *metric, struct pmu_events_map *
 	return NULL;
 }
 
-static int recursion_check(struct egroup *eg, const char *id, struct expr_id **parent)
+static int recursion_check(struct metric *m, const char *id, struct expr_id **parent)
 {
 	struct expr_id_data *data;
 	struct expr_id *p;
@@ -742,7 +742,7 @@ static int recursion_check(struct egroup *eg, const char *id, struct expr_id **p
 	 * if we already processed 'id', if we did, it's recursion
 	 * and we fail.
 	 */
-	ret = expr__get_id(&eg->pctx, id, &data);
+	ret = expr__get_id(&m->pctx, id, &data);
 	if (ret)
 		return ret;
 
@@ -760,13 +760,13 @@ static int recursion_check(struct egroup *eg, const char *id, struct expr_id **p
 	 * If we are over the limit of static entris, the metric
 	 * is too difficult/nested to process, fail as well.
 	 */
-	if (eg->recursion.cnt + 1 >= RECURSION_ID_MAX) {
+	if (m->recursion.cnt + 1 >= RECURSION_ID_MAX) {
 		pr_err("failed: too many nested metrics\n");
 		return -EINVAL;
 	}
 
-	p = &eg->recursion.id[eg->recursion.cnt];
-	eg->recursion.cnt++;
+	p = &m->recursion.id[m->recursion.cnt];
+	m->recursion.cnt++;
 
 	p->id     = strdup(id);
 	p->parent = data->parent;
@@ -778,10 +778,10 @@ static int recursion_check(struct egroup *eg, const char *id, struct expr_id **p
 static int add_metric(struct list_head *group_list,
 		      struct pmu_event *pe,
 		      bool metric_no_group,
-		      struct egroup **egp,
+		      struct metric **mp,
 		      struct expr_id *parent);
 
-static int resolve_metric(struct egroup *eg,
+static int resolve_metric(struct metric *m,
 			  bool metric_no_group,
 			  struct list_head *group_list,
 			  struct pmu_events_map *map)
@@ -797,7 +797,7 @@ static int resolve_metric(struct egroup *eg,
 	 */
 	do {
 		all = true;
-		hashmap__for_each_entry((&eg->pctx.ids), cur, bkt) {
+		hashmap__for_each_entry((&m->pctx.ids), cur, bkt) {
 			struct expr_id *parent;
 			struct pmu_event *pe;
 
@@ -805,16 +805,16 @@ static int resolve_metric(struct egroup *eg,
 			if (!pe)
 				continue;
 
-			ret = recursion_check(eg, cur->key, &parent);
+			ret = recursion_check(m, cur->key, &parent);
 			if (ret)
 				return ret;
 
 			all = false;
 			/* The metric key itself needs to go out.. */
-			expr__del_id(&eg->pctx, cur->key);
+			expr__del_id(&m->pctx, cur->key);
 
 			/* ... and it gets resolved to the parent context. */
-			ret = add_metric(group_list, pe, metric_no_group, &eg, parent);
+			ret = add_metric(group_list, pe, metric_no_group, &m, parent);
 			if (ret)
 				return ret;
 
@@ -832,7 +832,7 @@ static int resolve_metric(struct egroup *eg,
 static int add_metric(struct list_head *group_list,
 		      struct pmu_event *pe,
 		      bool metric_no_group,
-		      struct egroup **egp,
+		      struct metric **m,
 		      struct expr_id *parent)
 {
 	int ret = 0;
@@ -840,7 +840,7 @@ static int add_metric(struct list_head *group_list,
 	pr_debug("metric expr %s for %s\n", pe->metric_expr, pe->metric_name);
 
 	if (!strstr(pe->metric_expr, "?")) {
-		ret = __add_metric(group_list, pe, metric_no_group, 1, egp, parent);
+		ret = __add_metric(group_list, pe, metric_no_group, 1, m, parent);
 	} else {
 		int j, count;
 
@@ -852,7 +852,7 @@ static int add_metric(struct list_head *group_list,
 		 */
 
 		for (j = 0; j < count && !ret; j++) {
-			ret = __add_metric(group_list, pe, metric_no_group, j, egp, parent);
+			ret = __add_metric(group_list, pe, metric_no_group, j, m, parent);
 		}
 	}
 
@@ -865,7 +865,7 @@ static int metricgroup__add_metric(const char *metric, bool metric_no_group,
 				   struct pmu_events_map *map)
 
 {
-	struct egroup *eg = NULL;
+	struct metric *m = NULL;
 	struct pmu_event *pe;
 	int ret;
 
@@ -873,7 +873,7 @@ static int metricgroup__add_metric(const char *metric, bool metric_no_group,
 	if (!pe)
 		return -EINVAL;
 
-	ret = add_metric(group_list, pe, metric_no_group, &eg, NULL);
+	ret = add_metric(group_list, pe, metric_no_group, &m, NULL);
 	if (ret)
 		return ret;
 
@@ -881,7 +881,7 @@ static int metricgroup__add_metric(const char *metric, bool metric_no_group,
 	 * Process any possible referenced metrics
 	 * included in the expression.
 	 */
-	ret = resolve_metric(eg, metric_no_group,
+	ret = resolve_metric(m, metric_no_group,
 			     group_list, map);
 	if (ret)
 		return ret;
@@ -893,12 +893,12 @@ static int metricgroup__add_metric(const char *metric, bool metric_no_group,
 	 * Even if we add multiple groups through the runtime
 	 * param, they share same events.
 	 */
-	if (eg->has_constraint) {
+	if (m->has_constraint) {
 		metricgroup__add_metric_non_group(events,
-						  &eg->pctx);
+						  &m->pctx);
 	} else {
 		metricgroup__add_metric_weak_group(events,
-						   &eg->pctx);
+						   &m->pctx);
 	}
 	return 0;
 }
@@ -936,35 +936,35 @@ static int metricgroup__add_metric_list(const char *list, bool metric_no_group,
 	return ret;
 }
 
-static void egroup__free_refs(struct egroup *egroup)
+static void metric__free_refs(struct metric *metric)
 {
 	struct metric_ref_node *ref, *tmp;
 
-	list_for_each_entry_safe(ref, tmp, &egroup->refs, list) {
+	list_for_each_entry_safe(ref, tmp, &metric->refs, list) {
 		list_del(&ref->list);
 		free(ref);
 	}
 }
 
-static void egroup__free_recursion(struct egroup *egroup)
+static void metric__free_recursion(struct metric *metric)
 {
 	int i;
 
-	for (i = 0; i < egroup->recursion.cnt; i++) {
-		free(egroup->recursion.id[i].id);
+	for (i = 0; i < metric->recursion.cnt; i++) {
+		free(metric->recursion.id[i].id);
 	}
 }
 
-static void metricgroup__free_egroups(struct list_head *group_list)
+static void metricgroup__free_metrics(struct list_head *group_list)
 {
-	struct egroup *eg, *egtmp;
-
-	list_for_each_entry_safe (eg, egtmp, group_list, nd) {
-		egroup__free_recursion(eg);
-		egroup__free_refs(eg);
-		expr__ctx_clear(&eg->pctx);
-		list_del_init(&eg->nd);
-		free(eg);
+	struct metric *m, *tmp;
+
+	list_for_each_entry_safe (m, tmp, group_list, nd) {
+		metric__free_recursion(m);
+		metric__free_refs(m);
+		expr__ctx_clear(&m->pctx);
+		list_del_init(&m->nd);
+		free(m);
 	}
 }
 
@@ -997,7 +997,7 @@ static int parse_groups(struct evlist *perf_evlist, const char *str,
 	ret = metricgroup__setup_events(&group_list, metric_no_merge,
 					perf_evlist, metric_events);
 out:
-	metricgroup__free_egroups(&group_list);
+	metricgroup__free_metrics(&group_list);
 	return ret;
 }
 
-- 
2.25.4

[PATCH 18/18] perf metric: Rename group_list to list

[Jiri Olsa]

Following the previous change that rename egroup
to metric, there's no reason to call the list
'group_list' anymore, renaming it to list.

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

diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
index 9b880a5fb52b..0645ac1031fe 100644
--- a/tools/perf/util/metricgroup.c
+++ b/tools/perf/util/metricgroup.c
@@ -613,7 +613,7 @@ int __weak arch_get_runtimeparam(void)
 	return 1;
 }
 
-static int __add_metric(struct list_head *group_list,
+static int __add_metric(struct list_head *list,
 			struct pmu_event *pe,
 			bool metric_no_group,
 			int runtime,
@@ -691,13 +691,13 @@ static int __add_metric(struct list_head *group_list,
 	if (m->refs_cnt)
 		return 0;
 
-	if (list_empty(group_list))
-		list_add(&m->nd, group_list);
+	if (list_empty(list))
+		list_add(&m->nd, list);
 	else {
 		struct list_head *pos;
 
 		/* Place the largest groups at the front. */
-		list_for_each_prev(pos, group_list) {
+		list_for_each_prev(pos, list) {
 			struct metric *old = list_entry(pos, struct metric, nd);
 
 			if (hashmap__size(&m->pctx.ids) <=
@@ -775,7 +775,7 @@ static int recursion_check(struct metric *m, const char *id, struct expr_id **pa
 	return p->id ? 0 : -ENOMEM;
 }
 
-static int add_metric(struct list_head *group_list,
+static int add_metric(struct list_head *list,
 		      struct pmu_event *pe,
 		      bool metric_no_group,
 		      struct metric **mp,
@@ -783,7 +783,7 @@ static int add_metric(struct list_head *group_list,
 
 static int resolve_metric(struct metric *m,
 			  bool metric_no_group,
-			  struct list_head *group_list,
+			  struct list_head *list,
 			  struct pmu_events_map *map)
 {
 	struct hashmap_entry *cur;
@@ -814,7 +814,7 @@ static int resolve_metric(struct metric *m,
 			expr__del_id(&m->pctx, cur->key);
 
 			/* ... and it gets resolved to the parent context. */
-			ret = add_metric(group_list, pe, metric_no_group, &m, parent);
+			ret = add_metric(list, pe, metric_no_group, &m, parent);
 			if (ret)
 				return ret;
 
@@ -829,7 +829,7 @@ static int resolve_metric(struct metric *m,
 	return 0;
 }
 
-static int add_metric(struct list_head *group_list,
+static int add_metric(struct list_head *list,
 		      struct pmu_event *pe,
 		      bool metric_no_group,
 		      struct metric **m,
@@ -840,7 +840,7 @@ static int add_metric(struct list_head *group_list,
 	pr_debug("metric expr %s for %s\n", pe->metric_expr, pe->metric_name);
 
 	if (!strstr(pe->metric_expr, "?")) {
-		ret = __add_metric(group_list, pe, metric_no_group, 1, m, parent);
+		ret = __add_metric(list, pe, metric_no_group, 1, m, parent);
 	} else {
 		int j, count;
 
@@ -848,11 +848,11 @@ static int add_metric(struct list_head *group_list,
 
 		/* This loop is added to create multiple
 		 * events depend on count value and add
-		 * those events to group_list.
+		 * those events to list.
 		 */
 
 		for (j = 0; j < count && !ret; j++) {
-			ret = __add_metric(group_list, pe, metric_no_group, j, m, parent);
+			ret = __add_metric(list, pe, metric_no_group, j, m, parent);
 		}
 	}
 
@@ -861,7 +861,7 @@ static int 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 *list,
 				   struct pmu_events_map *map)
 
 {
@@ -873,7 +873,7 @@ static int metricgroup__add_metric(const char *metric, bool metric_no_group,
 	if (!pe)
 		return -EINVAL;
 
-	ret = add_metric(group_list, pe, metric_no_group, &m, NULL);
+	ret = add_metric(list, pe, metric_no_group, &m, NULL);
 	if (ret)
 		return ret;
 
@@ -881,8 +881,7 @@ static int metricgroup__add_metric(const char *metric, bool metric_no_group,
 	 * Process any possible referenced metrics
 	 * included in the expression.
 	 */
-	ret = resolve_metric(m, metric_no_group,
-			     group_list, map);
+	ret = resolve_metric(m, metric_no_group, list, map);
 	if (ret)
 		return ret;
 
@@ -905,7 +904,7 @@ 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 *mlist,
 					struct pmu_events_map *map)
 {
 	char *llist, *nlist, *p;
@@ -921,7 +920,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, map);
+					      mlist, map);
 		if (ret == -EINVAL) {
 			fprintf(stderr, "Cannot find metric or group `%s'\n",
 					p);
@@ -955,11 +954,11 @@ static void metric__free_recursion(struct metric *metric)
 	}
 }
 
-static void metricgroup__free_metrics(struct list_head *group_list)
+static void metricgroup__free_metrics(struct list_head *list)
 {
 	struct metric *m, *tmp;
 
-	list_for_each_entry_safe (m, tmp, group_list, nd) {
+	list_for_each_entry_safe (m, tmp, list, nd) {
 		metric__free_recursion(m);
 		metric__free_refs(m);
 		expr__ctx_clear(&m->pctx);
@@ -977,13 +976,13 @@ static int parse_groups(struct evlist *perf_evlist, const char *str,
 {
 	struct parse_events_error parse_error;
 	struct strbuf extra_events;
-	LIST_HEAD(group_list);
+	LIST_HEAD(list);
 	int ret;
 
 	if (metric_events->nr_entries == 0)
 		metricgroup__rblist_init(metric_events);
 	ret = metricgroup__add_metric_list(str, metric_no_group,
-					   &extra_events, &group_list, map);
+					   &extra_events, &list, map);
 	if (ret)
 		return ret;
 	pr_debug("adding %s\n", extra_events.buf);
@@ -994,10 +993,10 @@ static int parse_groups(struct evlist *perf_evlist, const char *str,
 		goto out;
 	}
 	strbuf_release(&extra_events);
-	ret = metricgroup__setup_events(&group_list, metric_no_merge,
+	ret = metricgroup__setup_events(&list, metric_no_merge,
 					perf_evlist, metric_events);
 out:
-	metricgroup__free_metrics(&group_list);
+	metricgroup__free_metrics(&list);
 	return ret;
 }
 
-- 
2.25.4

Re: [PATCH 11/18] perf metric: Add referenced metrics to hash data

[John Garry]

On 12/07/2020 14:26, Jiri Olsa wrote:
>   
> +int expr__add_ref(struct expr_parse_ctx *ctx, struct metric_ref *ref)
> +{
> +	struct expr_id_data *data_ptr = NULL, *old_data = NULL;
> +	char *old_key = NULL;
> +	char *name;
> +	int ret;
> +
> +	data_ptr = zalloc(sizeof(*data_ptr));
> +	if (!data_ptr)
> +		return -ENOMEM;
> +
> +	name = strdup(ref->metric_name);
> +	if (!name) {
> +		free(data_ptr);
> +		return -ENOMEM;
> +	}

JFYI, this was not compiling for me:

util/expr.c: In function ‘expr__add_ref’:
util/expr.c:84:13: error: implicit declaration of function ‘zalloc’; did 
you mean ‘valloc’? [-Werror=implicit-function-declaration]
  data_ptr = zalloc(sizeof(*data_ptr));
  ^~~~~~
  valloc
util/expr.c:84:13: error: nested extern declaration of ‘zalloc’ 
[-Werror=nested-externs]
util/expr.c:84:11: error: assignment to ‘struct expr_id_data *’ from 
‘int’ makes pointer from integer without a cast [-Werror=int-conversion]
  data_ptr = zalloc(sizeof(*data_ptr));
^
  LDutil/arm-spe-decoder/perf-in.o
cc1: all warnings being treated as errors

I think the zalloc.h include is missing.

Thanks,
john

Re: [PATCH 11/18] perf metric: Add referenced metrics to hash data

[Jiri Olsa]

On Mon, Jul 13, 2020 at 05:27:53PM +0100, John Garry wrote:
> On 12/07/2020 14:26, Jiri Olsa wrote:
> > +int expr__add_ref(struct expr_parse_ctx *ctx, struct metric_ref *ref)
> > +{
> > +	struct expr_id_data *data_ptr = NULL, *old_data = NULL;
> > +	char *old_key = NULL;
> > +	char *name;
> > +	int ret;
> > +
> > +	data_ptr = zalloc(sizeof(*data_ptr));
> > +	if (!data_ptr)
> > +		return -ENOMEM;
> > +
> > +	name = strdup(ref->metric_name);
> > +	if (!name) {
> > +		free(data_ptr);
> > +		return -ENOMEM;
> > +	}
> 
> JFYI, this was not compiling for me:
> 
> util/expr.c: In function ‘expr__add_ref’:
> util/expr.c:84:13: error: implicit declaration of function ‘zalloc’; did you
> mean ‘valloc’? [-Werror=implicit-function-declaration]
>  data_ptr = zalloc(sizeof(*data_ptr));
>  ^~~~~~
>  valloc
> util/expr.c:84:13: error: nested extern declaration of ‘zalloc’
> [-Werror=nested-externs]
> util/expr.c:84:11: error: assignment to ‘struct expr_id_data *’ from ‘int’
> makes pointer from integer without a cast [-Werror=int-conversion]
>  data_ptr = zalloc(sizeof(*data_ptr));
> ^
>  LDutil/arm-spe-decoder/perf-in.o
> cc1: all warnings being treated as errors
> 
> I think the zalloc.h include is missing.

yea, strange it compiles for me.. I'll add it

thanks,
jirka

Re: [PATCH 01/18] perf metric: Rename expr__add_id to expr__add_val

[Ian Rogers]

On Sun, Jul 12, 2020 at 6:26 AM Jiri Olsa <jolsa@kernel.org> wrote:
>
> Renaming expr__add_id to expr__add_val so we can use
> expr__add_id to actually add just id without any value
> in following changes.
>
> There's no functional change.
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>

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

Thanks,
Ian

> ---
>  tools/perf/tests/expr.c       | 4 ++--
>  tools/perf/tests/pmu-events.c | 4 ++--
>  tools/perf/util/expr.c        | 2 +-
>  tools/perf/util/expr.h        | 2 +-
>  tools/perf/util/expr.y        | 2 +-
>  tools/perf/util/stat-shadow.c | 4 ++--
>  6 files changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/tools/perf/tests/expr.c b/tools/perf/tests/expr.c
> index b7e5ef3007fc..d13fc1dfd5ef 100644
> --- a/tools/perf/tests/expr.c
> +++ b/tools/perf/tests/expr.c
> @@ -24,8 +24,8 @@ int test__expr(struct test *t __maybe_unused, int subtest __maybe_unused)
>         struct expr_parse_ctx ctx;
>
>         expr__ctx_init(&ctx);
> -       expr__add_id(&ctx, strdup("FOO"), 1);
> -       expr__add_id(&ctx, strdup("BAR"), 2);
> +       expr__add_id_val(&ctx, strdup("FOO"), 1);
> +       expr__add_id_val(&ctx, strdup("BAR"), 2);
>
>         ret = test(&ctx, "1+1", 2);
>         ret |= test(&ctx, "FOO+BAR", 3);
> diff --git a/tools/perf/tests/pmu-events.c b/tools/perf/tests/pmu-events.c
> index b66b021476ec..eb19f9a0bc15 100644
> --- a/tools/perf/tests/pmu-events.c
> +++ b/tools/perf/tests/pmu-events.c
> @@ -492,7 +492,7 @@ static int test_parsing(void)
>                          */
>                         k = 1;
>                         hashmap__for_each_entry((&ctx.ids), cur, bkt)
> -                               expr__add_id(&ctx, strdup(cur->key), k++);
> +                               expr__add_id_val(&ctx, strdup(cur->key), k++);
>
>                         hashmap__for_each_entry((&ctx.ids), cur, bkt) {
>                                 if (check_parse_cpu(cur->key, map == cpus_map,
> @@ -547,7 +547,7 @@ static int metric_parse_fake(const char *str)
>          */
>         i = 1;
>         hashmap__for_each_entry((&ctx.ids), cur, bkt)
> -               expr__add_id(&ctx, strdup(cur->key), i++);
> +               expr__add_id_val(&ctx, strdup(cur->key), i++);
>
>         hashmap__for_each_entry((&ctx.ids), cur, bkt) {
>                 if (check_parse_fake(cur->key)) {
> diff --git a/tools/perf/util/expr.c b/tools/perf/util/expr.c
> index e8f777830a23..9116a3a01eea 100644
> --- a/tools/perf/util/expr.c
> +++ b/tools/perf/util/expr.c
> @@ -33,7 +33,7 @@ static bool key_equal(const void *key1, const void *key2,
>  }
>
>  /* Caller must make sure id is allocated */
> -int expr__add_id(struct expr_parse_ctx *ctx, const char *name, double val)
> +int expr__add_id_val(struct expr_parse_ctx *ctx, const char *name, double val)
>  {
>         double *val_ptr = NULL, *old_val = NULL;
>         char *old_key = NULL;
> diff --git a/tools/perf/util/expr.h b/tools/perf/util/expr.h
> index 8a2c1074f90f..bb6bac836b48 100644
> --- a/tools/perf/util/expr.h
> +++ b/tools/perf/util/expr.h
> @@ -22,7 +22,7 @@ struct expr_scanner_ctx {
>
>  void expr__ctx_init(struct expr_parse_ctx *ctx);
>  void expr__ctx_clear(struct expr_parse_ctx *ctx);
> -int expr__add_id(struct expr_parse_ctx *ctx, const char *id, double val);
> +int expr__add_id_val(struct expr_parse_ctx *ctx, const char *id, double val);
>  int expr__get_id(struct expr_parse_ctx *ctx, const char *id, double *val_ptr);
>  int expr__parse(double *final_val, struct expr_parse_ctx *ctx,
>                 const char *expr, int runtime);
> diff --git a/tools/perf/util/expr.y b/tools/perf/util/expr.y
> index 5fcb98800f9c..b2b3420ea6ec 100644
> --- a/tools/perf/util/expr.y
> +++ b/tools/perf/util/expr.y
> @@ -69,7 +69,7 @@ all_other: all_other other
>
>  other: ID
>  {
> -       expr__add_id(ctx, $1, 0.0);
> +       expr__add_id_val(ctx, $1, 0.0);
>  }
>  |
>  MIN | MAX | IF | ELSE | SMT_ON | NUMBER | '|' | '^' | '&' | '-' | '+' | '*' | '/' | '%' | '(' | ')' | ','
> diff --git a/tools/perf/util/stat-shadow.c b/tools/perf/util/stat-shadow.c
> index 8fdef47005e6..fc9ac4b4218e 100644
> --- a/tools/perf/util/stat-shadow.c
> +++ b/tools/perf/util/stat-shadow.c
> @@ -773,9 +773,9 @@ static int prepare_metric(struct evsel **metric_events,
>                         *pn = 0;
>
>                 if (metric_total)
> -                       expr__add_id(pctx, n, metric_total);
> +                       expr__add_id_val(pctx, n, metric_total);
>                 else
> -                       expr__add_id(pctx, n, avg_stats(stats)*scale);
> +                       expr__add_id_val(pctx, n, avg_stats(stats)*scale);
>         }
>
>         return i;
> --
> 2.25.4
>

Re: [PATCH 02/18] perf metric: Add struct expr_id_data to keep expr value

[Ian Rogers]

On Sun, Jul 12, 2020 at 6:26 AM Jiri Olsa <jolsa@kernel.org> wrote:
>
> Adding struct expr_id_data to keep expr value
> instead of just simple double pointer, so we can
> store more data for ID in following changes.
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>

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

Thanks,
Ian

> ---
>  tools/perf/tests/expr.c       |  3 ++-
>  tools/perf/util/expr.c        | 22 +++++++++++-----------
>  tools/perf/util/expr.h        |  4 ++++
>  tools/perf/util/metricgroup.c |  2 +-
>  4 files changed, 18 insertions(+), 13 deletions(-)
>
> diff --git a/tools/perf/tests/expr.c b/tools/perf/tests/expr.c
> index d13fc1dfd5ef..4d01051951cd 100644
> --- a/tools/perf/tests/expr.c
> +++ b/tools/perf/tests/expr.c
> @@ -18,8 +18,9 @@ static int test(struct expr_parse_ctx *ctx, const char *e, double val2)
>
>  int test__expr(struct test *t __maybe_unused, int subtest __maybe_unused)
>  {
> +       struct expr_id_data *val_ptr;
>         const char *p;
> -       double val, *val_ptr;
> +       double val;
>         int ret;
>         struct expr_parse_ctx ctx;
>
> diff --git a/tools/perf/util/expr.c b/tools/perf/util/expr.c
> index 9116a3a01eea..5d05f9765ed8 100644
> --- a/tools/perf/util/expr.c
> +++ b/tools/perf/util/expr.c
> @@ -33,32 +33,32 @@ static bool key_equal(const void *key1, const void *key2,
>  }
>
>  /* Caller must make sure id is allocated */
> -int expr__add_id_val(struct expr_parse_ctx *ctx, const char *name, double val)
> +int expr__add_id_val(struct expr_parse_ctx *ctx, const char *id, double val)
>  {
> -       double *val_ptr = NULL, *old_val = NULL;
> +       struct expr_id_data *data_ptr = NULL, *old_data = NULL;
>         char *old_key = NULL;
>         int ret;
>
>         if (val != 0.0) {
> -               val_ptr = malloc(sizeof(double));
> -               if (!val_ptr)
> +               data_ptr = malloc(sizeof(*data_ptr));
> +               if (!data_ptr)
>                         return -ENOMEM;
> -               *val_ptr = val;
> +               data_ptr->val = val;
>         }
> -       ret = hashmap__set(&ctx->ids, name, val_ptr,
> -                          (const void **)&old_key, (void **)&old_val);
> +       ret = hashmap__set(&ctx->ids, id, data_ptr,
> +                          (const void **)&old_key, (void **)&old_data);
>         free(old_key);
> -       free(old_val);
> +       free(old_data);
>         return ret;
>  }
>
>  int expr__get_id(struct expr_parse_ctx *ctx, const char *id, double *val_ptr)
>  {
> -       double *data;
> +       struct expr_id_data *data;
>
>         if (!hashmap__find(&ctx->ids, id, (void **)&data))
>                 return -1;
> -       *val_ptr = (data == NULL) ?  0.0 : *data;
> +       *val_ptr = (data == NULL) ?  0.0 : data->val;
>         return 0;
>  }
>
> @@ -119,7 +119,7 @@ int expr__parse(double *final_val, struct expr_parse_ctx *ctx,
>  int expr__find_other(const char *expr, const char *one,
>                      struct expr_parse_ctx *ctx, int runtime)
>  {
> -       double *old_val = NULL;
> +       struct expr_id_data *old_val = NULL;
>         char *old_key = NULL;
>         int ret = __expr__parse(NULL, ctx, expr, EXPR_OTHER, runtime);
>
> diff --git a/tools/perf/util/expr.h b/tools/perf/util/expr.h
> index bb6bac836b48..21fe5bd85718 100644
> --- a/tools/perf/util/expr.h
> +++ b/tools/perf/util/expr.h
> @@ -15,6 +15,10 @@ struct expr_parse_ctx {
>         struct hashmap ids;
>  };
>
> +struct expr_id_data {
> +       double  val;
> +};
> +
>  struct expr_scanner_ctx {
>         int start_token;
>         int runtime;
> diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
> index 82fecb5a302d..df0356ec120d 100644
> --- a/tools/perf/util/metricgroup.c
> +++ b/tools/perf/util/metricgroup.c
> @@ -138,7 +138,7 @@ static struct evsel *find_evsel_group(struct evlist *perf_evlist,
>                                       unsigned long *evlist_used)
>  {
>         struct evsel *ev, *current_leader = NULL;
> -       double *val_ptr;
> +       struct expr_id_data *val_ptr;
>         int i = 0, matched_events = 0, events_to_match;
>         const int idnum = (int)hashmap__size(&pctx->ids);
>
> --
> 2.25.4
>

Re: [PATCH 05/18] perf metric: Add expr__del_id function

[Ian Rogers]

On Sun, Jul 12, 2020 at 6:27 AM Jiri Olsa <jolsa@kernel.org> wrote:
>
> Adding expr__del_id function to remove ID from hashmap.
> It will save us few lines in following changes.
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>

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

Thanks,
Ian

> ---
>  tools/perf/util/expr.c | 21 +++++++++++++--------
>  tools/perf/util/expr.h |  1 +
>  2 files changed, 14 insertions(+), 8 deletions(-)
>
> diff --git a/tools/perf/util/expr.c b/tools/perf/util/expr.c
> index 7d02d29286bc..91142d4c3419 100644
> --- a/tools/perf/util/expr.c
> +++ b/tools/perf/util/expr.c
> @@ -75,6 +75,17 @@ int expr__get_id(struct expr_parse_ctx *ctx, const char *id,
>         return hashmap__find(&ctx->ids, id, (void **)data) ? 0 : -1;
>  }
>
> +void expr__del_id(struct expr_parse_ctx *ctx, const char *id)
> +{
> +       struct expr_id_data *old_val = NULL;
> +       char *old_key = NULL;
> +
> +       hashmap__delete(&ctx->ids, id,
> +                       (const void **)&old_key, (void **)&old_val);
> +       free(old_key);
> +       free(old_val);
> +}
> +
>  void expr__ctx_init(struct expr_parse_ctx *ctx)
>  {
>         hashmap__init(&ctx->ids, key_hash, key_equal, NULL);
> @@ -132,16 +143,10 @@ int expr__parse(double *final_val, struct expr_parse_ctx *ctx,
>  int expr__find_other(const char *expr, const char *one,
>                      struct expr_parse_ctx *ctx, int runtime)
>  {
> -       struct expr_id_data *old_val = NULL;
> -       char *old_key = NULL;
>         int ret = __expr__parse(NULL, ctx, expr, EXPR_OTHER, runtime);
>
> -       if (one) {
> -               hashmap__delete(&ctx->ids, one,
> -                               (const void **)&old_key, (void **)&old_val);
> -               free(old_key);
> -               free(old_val);
> -       }
> +       if (one)
> +               expr__del_id(ctx, one);
>
>         return ret;
>  }
> diff --git a/tools/perf/util/expr.h b/tools/perf/util/expr.h
> index f38292fdab19..2462abd0ac65 100644
> --- a/tools/perf/util/expr.h
> +++ b/tools/perf/util/expr.h
> @@ -26,6 +26,7 @@ struct expr_scanner_ctx {
>
>  void expr__ctx_init(struct expr_parse_ctx *ctx);
>  void expr__ctx_clear(struct expr_parse_ctx *ctx);
> +void expr__del_id(struct expr_parse_ctx *ctx, const char *id);
>  int expr__add_id(struct expr_parse_ctx *ctx, const char *id);
>  int expr__add_id_val(struct expr_parse_ctx *ctx, const char *id, double val);
>  int expr__get_id(struct expr_parse_ctx *ctx, const char *id,
> --
> 2.25.4
>

Re: [PATCH 06/18] perf metric: Add find_metric function

[Ian Rogers]

On Sun, Jul 12, 2020 at 6:27 AM Jiri Olsa <jolsa@kernel.org> wrote:
>
> Decouple lookup metric logic into find_metric function,
> so it can be used from other places in following changes.
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>

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

Thanks,
Ian

> ---
>  tools/perf/util/metricgroup.c | 89 +++++++++++++++++++----------------
>  1 file changed, 48 insertions(+), 41 deletions(-)
>
> diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
> index df0356ec120d..72552608ff7d 100644
> --- a/tools/perf/util/metricgroup.c
> +++ b/tools/perf/util/metricgroup.c
> @@ -614,57 +614,64 @@ static int __metricgroup__add_metric(struct list_head *group_list,
>         return 0;
>  }
>
> -static int metricgroup__add_metric(const char *metric, bool metric_no_group,
> -                                  struct strbuf *events,
> -                                  struct list_head *group_list,
> -                                  struct pmu_events_map *map)
> +static struct pmu_event *find_metric(const char *metric, struct pmu_events_map *map)
>  {
>         struct pmu_event *pe;
> -       struct egroup *eg;
> -       int i, ret;
> -       bool has_match = false;
> +       int i;
>
>         for (i = 0; ; i++) {
>                 pe = &map->table[i];
> -
> -               if (!pe->name && !pe->metric_group && !pe->metric_name) {
> -                       /* End of pmu events. */
> -                       if (!has_match)
> -                               return -EINVAL;
> +               /* End of pmu events. */
> +               if (!pe->name && !pe->metric_group && !pe->metric_name)
>                         break;
> -               }
>                 if (!pe->metric_expr)
>                         continue;
>                 if (match_metric(pe->metric_group, metric) ||
> -                   match_metric(pe->metric_name, metric)) {
> -                       has_match = true;
> -                       pr_debug("metric expr %s for %s\n", pe->metric_expr, pe->metric_name);
> -
> -                       if (!strstr(pe->metric_expr, "?")) {
> -                               ret = __metricgroup__add_metric(group_list,
> -                                                               pe,
> -                                                               metric_no_group,
> -                                                               1);
> -                               if (ret)
> -                                       return ret;
> -                       } else {
> -                               int j, count;
> -
> -                               count = arch_get_runtimeparam();
> -
> -                               /* This loop is added to create multiple
> -                                * events depend on count value and add
> -                                * those events to group_list.
> -                                */
> +                   match_metric(pe->metric_name, metric))
> +                       return pe;
> +       }
>
> -                               for (j = 0; j < count; j++) {
> -                                       ret = __metricgroup__add_metric(
> -                                               group_list, pe,
> -                                               metric_no_group, j);
> -                                       if (ret)
> -                                               return ret;
> -                               }
> -                       }
> +       return NULL;
> +}
> +
> +static int metricgroup__add_metric(const char *metric, bool metric_no_group,
> +                                  struct strbuf *events,
> +                                  struct list_head *group_list,
> +                                  struct pmu_events_map *map)
> +{
> +       struct pmu_event *pe;
> +       struct egroup *eg;
> +       int ret;
> +
> +       pe = find_metric(metric, map);
> +       if (!pe)
> +               return -EINVAL;
> +
> +       pr_debug("metric expr %s for %s\n", pe->metric_expr, pe->metric_name);
> +
> +       if (!strstr(pe->metric_expr, "?")) {
> +               ret = __metricgroup__add_metric(group_list,
> +                                               pe,
> +                                               metric_no_group,
> +                                               1);
> +               if (ret)
> +                       return ret;
> +       } else {
> +               int j, count;
> +
> +               count = arch_get_runtimeparam();
> +
> +               /* This loop is added to create multiple
> +                * events depend on count value and add
> +                * those events to group_list.
> +                */
> +
> +               for (j = 0; j < count; j++) {
> +                       ret = __metricgroup__add_metric(
> +                               group_list, pe,
> +                               metric_no_group, j);
> +                       if (ret)
> +                               return ret;
>                 }
>         }
>         list_for_each_entry(eg, group_list, nd) {
> --
> 2.25.4
>

Re: [PATCH 07/18] perf metric: Add add_metric function

[Ian Rogers]

On Sun, Jul 12, 2020 at 6:27 AM Jiri Olsa <jolsa@kernel.org> wrote:
>
> Decouple metric adding login into add_metric function,

s/login/logging/ ?

> so it can be used from other places in following changes.
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>

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

Thanks,
Ian

> ---
>  tools/perf/util/metricgroup.c | 42 ++++++++++++++++++++++-------------
>  1 file changed, 26 insertions(+), 16 deletions(-)
>
> diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
> index 72552608ff7d..9a168f3df7a4 100644
> --- a/tools/perf/util/metricgroup.c
> +++ b/tools/perf/util/metricgroup.c
> @@ -634,18 +634,11 @@ static struct pmu_event *find_metric(const char *metric, struct pmu_events_map *
>         return NULL;
>  }
>
> -static int metricgroup__add_metric(const char *metric, bool metric_no_group,
> -                                  struct strbuf *events,
> -                                  struct list_head *group_list,
> -                                  struct pmu_events_map *map)
> +static int add_metric(struct list_head *group_list,
> +                     struct pmu_event *pe,
> +                     bool metric_no_group)
>  {
> -       struct pmu_event *pe;
> -       struct egroup *eg;
> -       int ret;
> -
> -       pe = find_metric(metric, map);
> -       if (!pe)
> -               return -EINVAL;
> +       int ret = 0;
>
>         pr_debug("metric expr %s for %s\n", pe->metric_expr, pe->metric_name);
>
> @@ -654,8 +647,6 @@ static int metricgroup__add_metric(const char *metric, bool metric_no_group,
>                                                 pe,
>                                                 metric_no_group,
>                                                 1);
> -               if (ret)
> -                       return ret;
>         } else {
>                 int j, count;
>
> @@ -666,14 +657,33 @@ static int metricgroup__add_metric(const char *metric, bool metric_no_group,
>                  * those events to group_list.
>                  */
>
> -               for (j = 0; j < count; j++) {
> +               for (j = 0; j < count && !ret; j++) {
>                         ret = __metricgroup__add_metric(
>                                 group_list, pe,
>                                 metric_no_group, j);
> -                       if (ret)
> -                               return ret;
>                 }
>         }
> +
> +       return ret;
> +}
> +
> +static int metricgroup__add_metric(const char *metric, bool metric_no_group,
> +                                  struct strbuf *events,
> +                                  struct list_head *group_list,
> +                                  struct pmu_events_map *map)
> +{
> +       struct pmu_event *pe;
> +       struct egroup *eg;
> +       int ret;
> +
> +       pe = find_metric(metric, map);
> +       if (!pe)
> +               return -EINVAL;
> +
> +       ret = add_metric(group_list, pe, metric_no_group);
> +       if (ret)
> +               return ret;
> +
>         list_for_each_entry(eg, group_list, nd) {
>                 if (events->len > 0)
>                         strbuf_addf(events, ",");
> --
> 2.25.4
>

Re: [PATCH 08/18] perf metric: Rename __metricgroup__add_metric to __add_metric

[Ian Rogers]

On Sun, Jul 12, 2020 at 6:27 AM Jiri Olsa <jolsa@kernel.org> wrote:
>
> Renaming __metricgroup__add_metric to __add_metric
> to fit in the current function names.
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>

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

Thanks,
Ian

> ---
>  tools/perf/util/metricgroup.c | 17 ++++++-----------
>  1 file changed, 6 insertions(+), 11 deletions(-)
>
> diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
> index 9a168f3df7a4..f0b0a053bfd2 100644
> --- a/tools/perf/util/metricgroup.c
> +++ b/tools/perf/util/metricgroup.c
> @@ -571,10 +571,10 @@ int __weak arch_get_runtimeparam(void)
>         return 1;
>  }
>
> -static int __metricgroup__add_metric(struct list_head *group_list,
> -                                    struct pmu_event *pe,
> -                                    bool metric_no_group,
> -                                    int runtime)
> +static int __add_metric(struct list_head *group_list,
> +                       struct pmu_event *pe,
> +                       bool metric_no_group,
> +                       int runtime)
>  {
>         struct egroup *eg;
>
> @@ -643,10 +643,7 @@ static int add_metric(struct list_head *group_list,
>         pr_debug("metric expr %s for %s\n", pe->metric_expr, pe->metric_name);
>
>         if (!strstr(pe->metric_expr, "?")) {
> -               ret = __metricgroup__add_metric(group_list,
> -                                               pe,
> -                                               metric_no_group,
> -                                               1);
> +               ret = __add_metric(group_list, pe, metric_no_group, 1);
>         } else {
>                 int j, count;
>
> @@ -658,9 +655,7 @@ static int add_metric(struct list_head *group_list,
>                  */
>
>                 for (j = 0; j < count && !ret; j++) {
> -                       ret = __metricgroup__add_metric(
> -                               group_list, pe,
> -                               metric_no_group, j);
> +                       ret = __add_metric(group_list, pe, metric_no_group, j);
>                 }
>         }
>
> --
> 2.25.4
>

Re: [PATCH 09/18] perf metric: Collect referenced metrics in struct metric_ref_node

[Ian Rogers]

On Sun, Jul 12, 2020 at 6:27 AM Jiri Olsa <jolsa@kernel.org> wrote:
>
> Collecting referenced metrics in struct metric_ref_node object,
> so we can process them later on.
>
> The change will parse nested metric names out of expression and
> 'resolve' them.
>
> All referenced metrics are dissolved into one context, meaning all
> nested metrics events and added to the parent context.
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  tools/perf/util/metricgroup.c | 147 ++++++++++++++++++++++++++++++----
>  1 file changed, 132 insertions(+), 15 deletions(-)
>
> diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
> index f0b0a053bfd2..9923eef1e2d4 100644
> --- a/tools/perf/util/metricgroup.c
> +++ b/tools/perf/util/metricgroup.c
> @@ -102,12 +102,20 @@ void metricgroup__rblist_exit(struct rblist *metric_events)
>         rblist__exit(metric_events);
>  }
>
> +struct metric_ref_node {
> +       const char *metric_name;
> +       const char *metric_expr;
> +       struct list_head list;
> +};

Perhaps a comment like this above:
/* A node in the list of referenced metrics. metric_expr is held as a
convenience to avoid a search through the metric list. */

> +
>  struct egroup {
>         struct list_head nd;
>         struct expr_parse_ctx pctx;
>         const char *metric_name;
>         const char *metric_expr;
>         const char *metric_unit;
> +       struct list_head refs;
> +       int refs_cnt;

A comment would be nice here as refs is a pretty overloaded term. For
example, is refs_cnt a reference count for memory management or the
length of the refs list? I'm unpopular and would use a long variable
name here of something like referenced_metrics for the list.

>         int runtime;
>         bool has_constraint;
>  };
> @@ -574,27 +582,66 @@ int __weak arch_get_runtimeparam(void)
>  static int __add_metric(struct list_head *group_list,
>                         struct pmu_event *pe,
>                         bool metric_no_group,
> -                       int runtime)
> +                       int runtime,
> +                       struct egroup **egp)

Rather than egp perhaps parent_eg or parent_metric?

>  {
> +       struct metric_ref_node *ref;
>         struct egroup *eg;
>
> -       eg = malloc(sizeof(*eg));
> -       if (!eg)
> -               return -ENOMEM;
> +       if (*egp == NULL) {
> +               /*
> +                * We got in here for the master group,
> +                * allocate it and put it on the list.
> +                */
> +               eg = malloc(sizeof(*eg));
> +               if (!eg)
> +                       return -ENOMEM;
> +
> +               expr__ctx_init(&eg->pctx);
> +               eg->metric_name = pe->metric_name;
> +               eg->metric_expr = pe->metric_expr;
> +               eg->metric_unit = pe->unit;
> +               eg->runtime = runtime;
> +               eg->has_constraint = metric_no_group || metricgroup__has_constraint(pe);
> +               INIT_LIST_HEAD(&eg->refs);
> +               eg->refs_cnt = 0;
> +               *egp = eg;
> +       } else {
> +               /*
> +                * We got here for the referenced metric, via the
> +                * recursive metricgroup__add_metric call, add
> +                * it to the master group.

Probably want to avoid the term master here and below:
https://www.kernel.org/doc/html/latest/process/coding-style.html?highlight=language%20master#naming
Perhaps parent or referencing metric?

> +                */
> +               eg = *egp;
> +
> +               ref = malloc(sizeof(*ref));
> +               if (!ref)
> +                       return -ENOMEM;
>
> -       expr__ctx_init(&eg->pctx);
> -       eg->metric_name = pe->metric_name;
> -       eg->metric_expr = pe->metric_expr;
> -       eg->metric_unit = pe->unit;
> -       eg->runtime = runtime;
> -       eg->has_constraint = metric_no_group || metricgroup__has_constraint(pe);
> +               ref->metric_name = pe->metric_name;
> +               ref->metric_expr = pe->metric_expr;
> +               list_add(&ref->list, &eg->refs);
> +               eg->refs_cnt++;
> +               eg->has_constraint |= metricgroup__has_constraint(pe);

Why not metric_no_group here? Perhaps use a function to avoid
duplication with the code above.

> +       }
>
> +       /*
> +        * For both the master and referenced metrics, we parse
> +        * all the metric's IDs and add it to the parent context.
> +        */
>         if (expr__find_other(pe->metric_expr, NULL, &eg->pctx, runtime) < 0) {
>                 expr__ctx_clear(&eg->pctx);
>                 free(eg);
>                 return -EINVAL;
>         }
>
> +       /*
> +        * We add new group only in the 'master' call,
> +        * so bail out for referenced metric case.
> +        */
> +       if (eg->refs_cnt)
> +               return 0;
> +
>         if (list_empty(group_list))
>                 list_add(&eg->nd, group_list);
>         else {
> @@ -636,14 +683,63 @@ static struct pmu_event *find_metric(const char *metric, struct pmu_events_map *
>
>  static int add_metric(struct list_head *group_list,
>                       struct pmu_event *pe,
> -                     bool metric_no_group)
> +                     bool metric_no_group,
> +                     struct egroup **egp);
> +
> +static int resolve_metric(struct egroup *eg,
> +                         bool metric_no_group,
> +                         struct list_head *group_list,
> +                         struct pmu_events_map *map)
> +{
> +       struct hashmap_entry *cur;
> +       size_t bkt;
> +       bool all;
> +       int ret;
> +
> +       /*
> +        * Iterate all the parsed IDs and if there's metric,
> +        * add it to the context.
> +        */

Does this mean that the ID doesn't need to begin "metric:" to
reference a different metric as per Andi Kleen's request?

> +       do {
> +               all = true;
> +               hashmap__for_each_entry((&eg->pctx.ids), cur, bkt) {
> +                       struct pmu_event *pe;
> +
> +                       pe = find_metric(cur->key, map);
> +                       if (!pe)
> +                               continue;
> +
> +                       all = false;
> +                       /* The metric key itself needs to go out.. */
> +                       expr__del_id(&eg->pctx, cur->key);
> +
> +                       /* ... and it gets resolved to the parent context. */
> +                       ret = add_metric(group_list, pe, metric_no_group, &eg);
> +                       if (ret)
> +                               return ret;
> +
> +                       /*
> +                        * We added new metric to hashmap, so we need
> +                        * to break the iteration and start over.
> +                        */
> +                       break;
> +               }
> +       } while (!all);
> +
> +       return 0;
> +}
> +
> +static int add_metric(struct list_head *group_list,
> +                     struct pmu_event *pe,
> +                     bool metric_no_group,
> +                     struct egroup **egp)
>  {
>         int ret = 0;
>
>         pr_debug("metric expr %s for %s\n", pe->metric_expr, pe->metric_name);
>
>         if (!strstr(pe->metric_expr, "?")) {
> -               ret = __add_metric(group_list, pe, metric_no_group, 1);
> +               ret = __add_metric(group_list, pe, metric_no_group, 1, egp);
>         } else {
>                 int j, count;
>
> @@ -655,7 +751,7 @@ static int add_metric(struct list_head *group_list,
>                  */
>
>                 for (j = 0; j < count && !ret; j++) {
> -                       ret = __add_metric(group_list, pe, metric_no_group, j);
> +                       ret = __add_metric(group_list, pe, metric_no_group, j, egp);
>                 }
>         }
>
> @@ -666,16 +762,26 @@ static int metricgroup__add_metric(const char *metric, bool metric_no_group,
>                                    struct strbuf *events,
>                                    struct list_head *group_list,
>                                    struct pmu_events_map *map)
> +
>  {
> +       struct egroup *eg = NULL;
>         struct pmu_event *pe;
> -       struct egroup *eg;
>         int ret;
>
>         pe = find_metric(metric, map);
>         if (!pe)
>                 return -EINVAL;
>
> -       ret = add_metric(group_list, pe, metric_no_group);
> +       ret = add_metric(group_list, pe, metric_no_group, &eg);
> +       if (ret)
> +               return ret;
> +
> +       /*
> +        * Process any possible referenced metrics
> +        * included in the expression.
> +        */
> +       ret = resolve_metric(eg, metric_no_group,
> +                            group_list, map);
>         if (ret)
>                 return ret;
>
> @@ -727,11 +833,22 @@ static int metricgroup__add_metric_list(const char *list, bool metric_no_group,
>         return ret;
>  }
>
> +static void egroup__free_refs(struct egroup *egroup)
> +{
> +       struct metric_ref_node *ref, *tmp;
> +
> +       list_for_each_entry_safe(ref, tmp, &egroup->refs, list) {
> +               list_del(&ref->list);
> +               free(ref);
> +       }
> +}
> +
>  static void metricgroup__free_egroups(struct list_head *group_list)
>  {
>         struct egroup *eg, *egtmp;
>
>         list_for_each_entry_safe (eg, egtmp, group_list, nd) {
> +               egroup__free_refs(eg);
>                 expr__ctx_clear(&eg->pctx);
>                 list_del_init(&eg->nd);
>                 free(eg);
> --
> 2.25.4
>

Re: [PATCH 10/18] perf metric: Collect referenced metrics in struct metric_expr

[Ian Rogers]

On Sun, Jul 12, 2020 at 6:27 AM Jiri Olsa <jolsa@kernel.org> wrote:
>
> Add referenced metrics into struct metric_expr object,
> so they are accessible when computing the metric.
>
> Storing just name and expression itself, so the metric
> can be resolved and computed.
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>

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

Thanks,
Ian

> ---
>  tools/perf/util/metricgroup.c | 26 ++++++++++++++++++++++++++
>  tools/perf/util/metricgroup.h |  6 ++++++
>  2 files changed, 32 insertions(+)
>
> diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
> index 9923eef1e2d4..8cbcc5e05fef 100644
> --- a/tools/perf/util/metricgroup.c
> +++ b/tools/perf/util/metricgroup.c
> @@ -83,6 +83,7 @@ static void metric_event_delete(struct rblist *rblist __maybe_unused,
>         struct metric_expr *expr, *tmp;
>
>         list_for_each_entry_safe(expr, tmp, &me->head, nd) {
> +               free(expr->metric_refs);
>                 free(expr);
>         }
>
> @@ -243,6 +244,7 @@ static int metricgroup__setup_events(struct list_head *groups,
>
>         list_for_each_entry (eg, groups, nd) {
>                 struct evsel **metric_events;
> +               struct metric_ref *metric_refs = NULL;
>
>                 metric_events = calloc(sizeof(void *),
>                                 hashmap__size(&eg->pctx.ids) + 1);
> @@ -274,6 +276,30 @@ static int metricgroup__setup_events(struct list_head *groups,
>                         free(metric_events);
>                         break;
>                 }
> +
> +               /*
> +                * Collect and store collected nested expressions
> +                * for metric processing.
> +                */
> +               if (eg->refs_cnt) {
> +                       struct metric_ref_node *ref;
> +
> +                       metric_refs = zalloc(sizeof(struct metric_ref) * (eg->refs_cnt + 1));
> +                       if (!metric_refs) {
> +                               ret = -ENOMEM;
> +                               free(metric_events);
> +                               break;
> +                       }
> +
> +                       i = 0;
> +                       list_for_each_entry(ref, &eg->refs, list) {
> +                               metric_refs[i].metric_name = ref->metric_name;
> +                               metric_refs[i].metric_expr = ref->metric_expr;
> +                               i++;
> +                       }
> +               };
> +
> +               expr->metric_refs = metric_refs;
>                 expr->metric_expr = eg->metric_expr;
>                 expr->metric_name = eg->metric_name;
>                 expr->metric_unit = eg->metric_unit;
> diff --git a/tools/perf/util/metricgroup.h b/tools/perf/util/metricgroup.h
> index 8315bd1a7da4..62623a39cbec 100644
> --- a/tools/perf/util/metricgroup.h
> +++ b/tools/perf/util/metricgroup.h
> @@ -18,12 +18,18 @@ struct metric_event {
>         struct list_head head; /* list of metric_expr */
>  };
>
> +struct metric_ref {
> +       const char *metric_name;
> +       const char *metric_expr;
> +};
> +
>  struct metric_expr {
>         struct list_head nd;
>         const char *metric_expr;
>         const char *metric_name;
>         const char *metric_unit;
>         struct evsel **metric_events;
> +       struct metric_ref *metric_refs;
>         int runtime;
>  };
>
> --
> 2.25.4
>

Re: [PATCH 12/18] perf metric: Compute referenced metrics

[Ian Rogers]

On Sun, Jul 12, 2020 at 6:27 AM Jiri Olsa <jolsa@kernel.org> wrote:
>
> Adding computation (expr__parse call) of referenced metric at
> the point when it needs to be resolved during the 'master'
> metric computation.

Same note on wrt master:
https://www.kernel.org/doc/html/latest/process/coding-style.html?highlight=language%20master#naming

> Once the inner metric is computed, the result is stored and
> used if there's another usage of that metric.
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>

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

Thanks,
Ian

> ---
>  tools/perf/util/expr.c | 31 +++++++++++++++++++++++++++++++
>  tools/perf/util/expr.h |  3 +++
>  tools/perf/util/expr.y |  4 ++--
>  3 files changed, 36 insertions(+), 2 deletions(-)
>
> diff --git a/tools/perf/util/expr.c b/tools/perf/util/expr.c
> index 6bf8a21f5c53..2784df38f8c2 100644
> --- a/tools/perf/util/expr.c
> +++ b/tools/perf/util/expr.c
> @@ -91,6 +91,7 @@ int expr__add_ref(struct expr_parse_ctx *ctx, struct metric_ref *ref)
>
>         data_ptr->ref.metric_name = ref->metric_name;
>         data_ptr->ref.metric_expr = ref->metric_expr;
> +       data_ptr->ref.counted = false;
>         data_ptr->is_ref = true;
>
>         ret = hashmap__set(&ctx->ids, name, data_ptr,
> @@ -110,6 +111,34 @@ int expr__get_id(struct expr_parse_ctx *ctx, const char *id,
>         return hashmap__find(&ctx->ids, id, (void **)data) ? 0 : -1;
>  }
>
> +int expr__resolve_id(struct expr_parse_ctx *ctx, const char *id,
> +                    struct expr_id_data **datap)
> +{
> +       struct expr_id_data *data;
> +
> +       if (expr__get_id(ctx, id, datap) || !*datap) {
> +               pr_debug("%s not found\n", id);
> +               return -1;
> +       }
> +
> +       data = *datap;
> +
> +       pr_debug2("lookup: is_ref %d, counted %d, val %f: %s\n",
> +                 data->is_ref, data->ref.counted, data->val, id);
> +
> +       if (data->is_ref && !data->ref.counted) {
> +               data->ref.counted = true;
> +               pr_debug("processing metric: %s ENTRY\n", id);
> +               if (expr__parse(&data->val, ctx, data->ref.metric_expr, 1)) {
> +                       pr_debug("%s failed to count\n", id);
> +                       return -1;
> +               }
> +               pr_debug("processing metric: %s EXIT: %f\n", id, data->val);
> +       }
> +
> +       return 0;
> +}
> +
>  void expr__del_id(struct expr_parse_ctx *ctx, const char *id)
>  {
>         struct expr_id_data *old_val = NULL;
> @@ -150,6 +179,8 @@ __expr__parse(double *val, struct expr_parse_ctx *ctx, const char *expr,
>         void *scanner;
>         int ret;
>
> +       pr_debug2("parsing metric: %s\n", expr);
> +
>         ret = expr_lex_init_extra(&scanner_ctx, &scanner);
>         if (ret)
>                 return ret;
> diff --git a/tools/perf/util/expr.h b/tools/perf/util/expr.h
> index d19e66915228..b6d1fba64eaa 100644
> --- a/tools/perf/util/expr.h
> +++ b/tools/perf/util/expr.h
> @@ -25,6 +25,7 @@ struct expr_id_data {
>                 struct {
>                         const char *metric_name;
>                         const char *metric_expr;
> +                       bool counted;
>                 } ref;
>         };
>  };
> @@ -42,6 +43,8 @@ int expr__add_id_val(struct expr_parse_ctx *ctx, const char *id, double val);
>  int expr__add_ref(struct expr_parse_ctx *ctx, struct metric_ref *ref);
>  int expr__get_id(struct expr_parse_ctx *ctx, const char *id,
>                  struct expr_id_data **data);
> +int expr__resolve_id(struct expr_parse_ctx *ctx, const char *id,
> +                    struct expr_id_data **datap);
>  int expr__parse(double *final_val, struct expr_parse_ctx *ctx,
>                 const char *expr, int runtime);
>  int expr__find_other(const char *expr, const char *one,
> diff --git a/tools/perf/util/expr.y b/tools/perf/util/expr.y
> index 0d4f5d324be7..d34b370391c6 100644
> --- a/tools/perf/util/expr.y
> +++ b/tools/perf/util/expr.y
> @@ -88,11 +88,11 @@ expr:         NUMBER
>         | ID                    {
>                                         struct expr_id_data *data;
>
> -                                       if (expr__get_id(ctx, $1, &data) || !data) {
> -                                               pr_debug("%s not found\n", $1);
> +                                       if (expr__resolve_id(ctx, $1, &data)) {
>                                                 free($1);
>                                                 YYABORT;
>                                         }
> +
>                                         $$ = data->val;
>                                         free($1);
>                                 }
> --
> 2.25.4
>

Re: [PATCH 13/18] perf metric: Add events for the current group

[Ian Rogers]

On Sun, Jul 12, 2020 at 6:27 AM Jiri Olsa <jolsa@kernel.org> wrote:
>
> There's no need to iterate the whole list of groups,
> when adding new events. The currently created group
> is the one we want to add.
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  tools/perf/util/metricgroup.c | 22 ++++++++++++----------
>  1 file changed, 12 insertions(+), 10 deletions(-)
>
> diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
> index 8cbcc5e05fef..66f25362702d 100644
> --- a/tools/perf/util/metricgroup.c
> +++ b/tools/perf/util/metricgroup.c
> @@ -811,17 +811,19 @@ static int metricgroup__add_metric(const char *metric, bool metric_no_group,

Could we add a function comment to describe the arguments here?
Currently events is an empty list out argument that is built up by
this code, now it will be incrementally updated. Except I don't think
I'm capturing the current state correctly, it is confusing that there
is the loop in the current code. It looks like events will be added
multiple times redundantly.

>         if (ret)
>                 return ret;
>
> -       list_for_each_entry(eg, group_list, nd) {
> -               if (events->len > 0)
> -                       strbuf_addf(events, ",");
> +       if (events->len > 0)
> +               strbuf_addf(events, ",");
>
> -               if (eg->has_constraint) {
> -                       metricgroup__add_metric_non_group(events,
> -                                                         &eg->pctx);
> -               } else {
> -                       metricgroup__add_metric_weak_group(events,
> -                                                          &eg->pctx);
> -               }
> +       /*
> +        * Even if we add multiple groups through the runtime
> +        * param, they share same events.
> +        */

I'm not clear what runtime param is here. Is it the \? arch runtime parameter?

Thanks,
Ian

> +       if (eg->has_constraint) {
> +               metricgroup__add_metric_non_group(events,
> +                                                 &eg->pctx);
> +       } else {
> +               metricgroup__add_metric_weak_group(events,
> +                                                  &eg->pctx);
>         }
>         return 0;
>  }
> --
> 2.25.4
>

Re: [PATCH 15/18] perf metric: Add DCache_L2 to metric parse test

[Ian Rogers]

On Sun, Jul 12, 2020 at 6:27 AM Jiri Olsa <jolsa@kernel.org> wrote:
>
> Adding test that compute DCache_L2 metrics with other related metrics in it.
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>

This feels familiar :-)

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

Thanks,
Ian


> ---
>  tools/perf/tests/parse-metric.c | 71 +++++++++++++++++++++++++++++++++
>  1 file changed, 71 insertions(+)
>
> diff --git a/tools/perf/tests/parse-metric.c b/tools/perf/tests/parse-metric.c
> index 28f33893338b..b50e2a3f3b73 100644
> --- a/tools/perf/tests/parse-metric.c
> +++ b/tools/perf/tests/parse-metric.c
> @@ -36,6 +36,27 @@ static struct pmu_event pme_test[] = {
>         .metric_expr    = "(dcache_miss_cpi + icache_miss_cycles)",
>         .metric_name    = "cache_miss_cycles",
>  },
> +{
> +       .metric_expr    = "L2_RQSTS.DEMAND_DATA_RD_HIT + L2_RQSTS.PF_HIT + L2_RQSTS.RFO_HIT",
> +       .metric_name    = "DCache_L2_All_Hits",
> +},
> +{
> +       .metric_expr    = "max(L2_RQSTS.ALL_DEMAND_DATA_RD - L2_RQSTS.DEMAND_DATA_RD_HIT, 0) + "
> +                         "L2_RQSTS.PF_MISS + L2_RQSTS.RFO_MISS",
> +       .metric_name    = "DCache_L2_All_Miss",
> +},
> +{
> +       .metric_expr    = "DCache_L2_All_Hits + DCache_L2_All_Miss",
> +       .metric_name    = "DCache_L2_All",
> +},
> +{
> +       .metric_expr    = "d_ratio(DCache_L2_All_Hits, DCache_L2_All)",
> +       .metric_name    = "DCache_L2_Hits",
> +},
> +{
> +       .metric_expr    = "d_ratio(DCache_L2_All_Miss, DCache_L2_All)",
> +       .metric_name    = "DCache_L2_Misses",
> +},
>  };
>
>  static struct pmu_events_map map = {
> @@ -194,10 +215,60 @@ static int test_cache_miss_cycles(void)
>         return 0;
>  }
>
> +
> +/*
> + * DCache_L2_All_Hits = L2_RQSTS.DEMAND_DATA_RD_HIT + L2_RQSTS.PF_HIT + L2_RQSTS.RFO_HI
> + * DCache_L2_All_Miss = MAX(L2_RQSTS.ALL_DEMAND_DATA_RD - L2_RQSTS.DEMAND_DATA_RD_HIT, 0) +
> + *                      L2_RQSTS.PF_MISS + L2_RQSTS.RFO_MISS
> + * DCache_L2_All      = DCache_L2_All_Hits + DCache_L2_All_Miss
> + * DCache_L2_Hits     = d_ratio(DCache_L2_All_Hits, DCache_L2_All)
> + * DCache_L2_Misses   = d_ratio(DCache_L2_All_Miss, DCache_L2_All)
> + *
> + * L2_RQSTS.DEMAND_DATA_RD_HIT = 100
> + * L2_RQSTS.PF_HIT             = 200
> + * L2_RQSTS.RFO_HI             = 300
> + * L2_RQSTS.ALL_DEMAND_DATA_RD = 400
> + * L2_RQSTS.PF_MISS            = 500
> + * L2_RQSTS.RFO_MISS           = 600
> + *
> + * DCache_L2_All_Hits = 600
> + * DCache_L2_All_Miss = MAX(400 - 100, 0) + 500 + 600 = 1400
> + * DCache_L2_All      = 600 + 1400  = 2000
> + * DCache_L2_Hits     = 600 / 2000  = 0.3
> + * DCache_L2_Misses   = 1400 / 2000 = 0.7
> + */
> +static int test_dcache_l2(void)
> +{
> +       double ratio;
> +       struct value vals[] = {
> +               { .event = "L2_RQSTS.DEMAND_DATA_RD_HIT", .val = 100 },
> +               { .event = "L2_RQSTS.PF_HIT",             .val = 200 },
> +               { .event = "L2_RQSTS.RFO_HIT",            .val = 300 },
> +               { .event = "L2_RQSTS.ALL_DEMAND_DATA_RD", .val = 400 },
> +               { .event = "L2_RQSTS.PF_MISS",            .val = 500 },
> +               { .event = "L2_RQSTS.RFO_MISS",           .val = 600 },
> +               { 0 },
> +       };
> +
> +       TEST_ASSERT_VAL("failed to compute metric",
> +                       compute_metric("DCache_L2_Hits", vals, &ratio) == 0);
> +
> +       TEST_ASSERT_VAL("DCache_L2_Hits failed, wrong ratio",
> +                       ratio == 0.3);
> +
> +       TEST_ASSERT_VAL("failed to compute metric",
> +                       compute_metric("DCache_L2_Misses", vals, &ratio) == 0);
> +
> +       TEST_ASSERT_VAL("DCache_L2_Misses failed, wrong ratio",
> +                       ratio == 0.7);
> +       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);
>         TEST_ASSERT_VAL("cache_miss_cycles failed", test_cache_miss_cycles() == 0);
> +       TEST_ASSERT_VAL("DCache_L2 failed", test_dcache_l2() == 0);
>         return 0;
>  }
> --
> 2.25.4
>

Re: [PATCH 14/18] perf metric: Add cache_miss_cycles to metric parse test

[Ian Rogers]

On Sun, Jul 12, 2020 at 6:27 AM Jiri Olsa <jolsa@kernel.org> wrote:
>
> Adding test that compute metric with other metrics in it.
>
>   cache_miss_cycles = metric:dcache_miss_cpi + metric:icache_miss_cycles
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>

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

Thanks!
Ian

> ---
>  tools/perf/tests/parse-metric.c | 33 +++++++++++++++++++++++++++++++++
>  1 file changed, 33 insertions(+)
>
> diff --git a/tools/perf/tests/parse-metric.c b/tools/perf/tests/parse-metric.c
> index 8c48251425e1..28f33893338b 100644
> --- a/tools/perf/tests/parse-metric.c
> +++ b/tools/perf/tests/parse-metric.c
> @@ -11,6 +11,8 @@
>  #include "debug.h"
>  #include "expr.h"
>  #include "stat.h"
> +#include <perf/cpumap.h>
> +#include <perf/evlist.h>
>
>  static struct pmu_event pme_test[] = {
>  {
> @@ -22,6 +24,18 @@ static struct pmu_event pme_test[] = {
>                           "( 1 + cpu_clk_unhalted.one_thread_active / cpu_clk_unhalted.ref_xclk ) )))",
>         .metric_name    = "Frontend_Bound_SMT",
>  },
> +{
> +       .metric_expr    = "l1d\\-loads\\-misses / inst_retired.any",
> +       .metric_name    = "dcache_miss_cpi",
> +},
> +{
> +       .metric_expr    = "l1i\\-loads\\-misses / inst_retired.any",
> +       .metric_name    = "icache_miss_cycles",
> +},
> +{
> +       .metric_expr    = "(dcache_miss_cpi + icache_miss_cycles)",
> +       .metric_name    = "cache_miss_cycles",
> +},
>  };
>
>  static struct pmu_events_map map = {
> @@ -162,9 +176,28 @@ static int test_frontend(void)
>         return 0;
>  }
>
> +static int test_cache_miss_cycles(void)
> +{
> +       double ratio;
> +       struct value vals[] = {
> +               { .event = "l1d-loads-misses",  .val = 300 },
> +               { .event = "l1i-loads-misses",  .val = 200 },
> +               { .event = "inst_retired.any",  .val = 400 },
> +               { 0 },
> +       };
> +
> +       TEST_ASSERT_VAL("failed to compute metric",
> +                       compute_metric("cache_miss_cycles", vals, &ratio) == 0);
> +
> +       TEST_ASSERT_VAL("cache_miss_cycles failed, wrong ratio",
> +                       ratio == 1.25);
> +       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);
> +       TEST_ASSERT_VAL("cache_miss_cycles failed", test_cache_miss_cycles() == 0);
>         return 0;
>  }
> --
> 2.25.4
>

Re: [PATCH 16/18] perf metric: Add recursion check when processing nested metrics

[Ian Rogers]

On Sun, Jul 12, 2020 at 6:27 AM Jiri Olsa <jolsa@kernel.org> wrote:
>
> Keeping the stack of nested metrics via 'struct expr_id' objecs

s/objecs/objects/

> and checking if we are in recursion via already processed metric.
>
> The stack is implemented as static array within the struct egroup
> with 100 entries, which should be enough nesting depth for any
> metric we have or plan to have at the moment.
>
> Adding test that simulates the recursion and checks we can
> detect it.
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  tools/perf/tests/parse-metric.c |  27 ++++++++-
>  tools/perf/util/expr.c          |   2 +
>  tools/perf/util/expr.h          |   9 ++-
>  tools/perf/util/metricgroup.c   | 101 +++++++++++++++++++++++++++++---
>  4 files changed, 128 insertions(+), 11 deletions(-)
>
> diff --git a/tools/perf/tests/parse-metric.c b/tools/perf/tests/parse-metric.c
> index b50e2a3f3b73..238a805edd55 100644
> --- a/tools/perf/tests/parse-metric.c
> +++ b/tools/perf/tests/parse-metric.c
> @@ -57,6 +57,14 @@ static struct pmu_event pme_test[] = {
>         .metric_expr    = "d_ratio(DCache_L2_All_Miss, DCache_L2_All)",
>         .metric_name    = "DCache_L2_Misses",
>  },
> +{
> +       .metric_expr    = "IPC + M2",
> +       .metric_name    = "M1",
> +},
> +{
> +       .metric_expr    = "IPC + M1",
> +       .metric_name    = "M2",
> +},
>  };

Perhaps add a test on simple recursion too:
{
  .metric_expr = "1/M1",
  .metric_name = "M1",
}

>  static struct pmu_events_map map = {
> @@ -139,8 +147,8 @@ static int compute_metric(const char *name, struct value *vals, double *ratio)
>         err = metricgroup__parse_groups_test(evlist, &map, name,
>                                              false, false,
>                                              &metric_events);
> -
> -       TEST_ASSERT_VAL("failed to parse metric", err == 0);
> +       if (err)
> +               return err;
>
>         if (perf_evlist__alloc_stats(evlist, false))
>                 return -1;
> @@ -264,11 +272,26 @@ static int test_dcache_l2(void)
>         return 0;
>  }
>
> +static int test_recursion_fail(void)
> +{
> +       double ratio;
> +       struct value vals[] = {
> +               { .event = "inst_retired.any",        .val = 300 },
> +               { .event = "cpu_clk_unhalted.thread", .val = 200 },
> +               { 0 },
> +       };
> +
> +       TEST_ASSERT_VAL("failed to find recursion",
> +                       compute_metric("M1", vals, &ratio) == -1);
> +       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);
>         TEST_ASSERT_VAL("cache_miss_cycles failed", test_cache_miss_cycles() == 0);
>         TEST_ASSERT_VAL("DCache_L2 failed", test_dcache_l2() == 0);
> +       TEST_ASSERT_VAL("recursion fail failed", test_recursion_fail() == 0);
>         return 0;
>  }
> diff --git a/tools/perf/util/expr.c b/tools/perf/util/expr.c
> index 2784df38f8c2..d23d8897bc61 100644
> --- a/tools/perf/util/expr.c
> +++ b/tools/perf/util/expr.c
> @@ -45,6 +45,8 @@ int expr__add_id(struct expr_parse_ctx *ctx, const char *id)
>         if (!data_ptr)
>                 return -ENOMEM;
>
> +       data_ptr->parent = ctx->parent;
> +
>         ret = hashmap__set(&ctx->ids, id, data_ptr,
>                            (const void **)&old_key, (void **)&old_data);
>         free(old_key);
> diff --git a/tools/perf/util/expr.h b/tools/perf/util/expr.h
> index b6d1fba64eaa..e5fdeab928c7 100644
> --- a/tools/perf/util/expr.h
> +++ b/tools/perf/util/expr.h
> @@ -13,8 +13,14 @@
>
>  struct metric_ref;
>
> +struct expr_id {
> +       char            *id;
> +       struct expr_id  *parent;
> +};
> +
>  struct expr_parse_ctx {
> -       struct hashmap ids;
> +       struct hashmap   ids;
> +       struct expr_id  *parent;
>  };
>
>  struct expr_id_data {
> @@ -27,6 +33,7 @@ struct expr_id_data {
>                         const char *metric_expr;
>                         bool counted;
>                 } ref;
> +               struct expr_id  *parent;
>         };
>  };
>
> diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
> index 66f25362702d..69ec20dd737b 100644
> --- a/tools/perf/util/metricgroup.c
> +++ b/tools/perf/util/metricgroup.c
> @@ -24,6 +24,7 @@
>  #include <subcmd/parse-options.h>
>  #include <api/fs/fs.h>
>  #include "util.h"
> +#include <asm/bug.h>
>
>  struct metric_event *metricgroup__lookup(struct rblist *metric_events,
>                                          struct evsel *evsel,
> @@ -109,6 +110,8 @@ struct metric_ref_node {
>         struct list_head list;
>  };
>
> +#define RECURSION_ID_MAX 100
> +
>  struct egroup {
>         struct list_head nd;
>         struct expr_parse_ctx pctx;
> @@ -119,6 +122,11 @@ struct egroup {
>         int refs_cnt;
>         int runtime;
>         bool has_constraint;
> +
> +       struct {
> +               struct expr_id  id[RECURSION_ID_MAX];
> +               int             cnt;
> +       } recursion;
>  };

Rather than place this in the egroup why not pass a "visited" array to
add metric. This would be more in keeping with Tarjan's algorithm
(although the SCC isn't needed in this case):
https://en.wikipedia.org/wiki/Tarjan%27s_strongly_connected_components_algorithm

Thanks,
Ian


>
>  /**
> @@ -609,7 +617,8 @@ static int __add_metric(struct list_head *group_list,
>                         struct pmu_event *pe,
>                         bool metric_no_group,
>                         int runtime,
> -                       struct egroup **egp)
> +                       struct egroup **egp,
> +                       struct expr_id *parent)
>  {
>         struct metric_ref_node *ref;
>         struct egroup *eg;
> @@ -619,7 +628,7 @@ static int __add_metric(struct list_head *group_list,
>                  * We got in here for the master group,
>                  * allocate it and put it on the list.
>                  */
> -               eg = malloc(sizeof(*eg));
> +               eg = zalloc(sizeof(*eg));
>                 if (!eg)
>                         return -ENOMEM;
>
> @@ -632,6 +641,16 @@ static int __add_metric(struct list_head *group_list,
>                 INIT_LIST_HEAD(&eg->refs);
>                 eg->refs_cnt = 0;
>                 *egp = eg;
> +
> +               /* Initialize first recursion exr id with base name. */
> +               parent = &eg->recursion.id[0];
> +               eg->recursion.cnt = 1;
> +
> +               parent->id = strdup(pe->metric_name);
> +               if (!parent->id) {
> +                       free(eg);
> +                       return -ENOMEM;
> +               }
>         } else {
>                 /*
>                  * We got here for the referenced metric, via the
> @@ -651,6 +670,10 @@ static int __add_metric(struct list_head *group_list,
>                 eg->has_constraint |= metricgroup__has_constraint(pe);
>         }
>
> +       /* Force all found IDs in metric to have us as parent ID. */
> +       WARN_ON_ONCE(!parent);
> +       eg->pctx.parent = parent;
> +
>         /*
>          * For both the master and referenced metrics, we parse
>          * all the metric's IDs and add it to the parent context.
> @@ -707,10 +730,56 @@ static struct pmu_event *find_metric(const char *metric, struct pmu_events_map *
>         return NULL;
>  }
>
> +static int recursion_check(struct egroup *eg, const char *id, struct expr_id **parent)
> +{
> +       struct expr_id_data *data;
> +       struct expr_id *p;
> +       int ret;
> +
> +       /*
> +        * We get the parent referenced by 'id' argument and
> +        * traverse through all the parent object IDs to check
> +        * if we already processed 'id', if we did, it's recursion
> +        * and we fail.
> +        */
> +       ret = expr__get_id(&eg->pctx, id, &data);
> +       if (ret)
> +               return ret;
> +
> +       p = data->parent;
> +
> +       while (p->parent) {
> +               if (!strcmp(p->id, id)) {
> +                       pr_err("failed: recursion detected for %s\n", id);
> +                       return -1;
> +               }
> +               p = p->parent;
> +       }
> +
> +       /*
> +        * If we are over the limit of static entris, the metric
> +        * is too difficult/nested to process, fail as well.
> +        */
> +       if (eg->recursion.cnt + 1 >= RECURSION_ID_MAX) {
> +               pr_err("failed: too many nested metrics\n");
> +               return -EINVAL;
> +       }
> +
> +       p = &eg->recursion.id[eg->recursion.cnt];
> +       eg->recursion.cnt++;
> +
> +       p->id     = strdup(id);
> +       p->parent = data->parent;
> +       *parent   = p;
> +
> +       return p->id ? 0 : -ENOMEM;
> +}
> +
>  static int add_metric(struct list_head *group_list,
>                       struct pmu_event *pe,
>                       bool metric_no_group,
> -                     struct egroup **egp);
> +                     struct egroup **egp,
> +                     struct expr_id *parent);
>
>  static int resolve_metric(struct egroup *eg,
>                           bool metric_no_group,
> @@ -729,18 +798,23 @@ static int resolve_metric(struct egroup *eg,
>         do {
>                 all = true;
>                 hashmap__for_each_entry((&eg->pctx.ids), cur, bkt) {
> +                       struct expr_id *parent;
>                         struct pmu_event *pe;
>
>                         pe = find_metric(cur->key, map);
>                         if (!pe)
>                                 continue;
>
> +                       ret = recursion_check(eg, cur->key, &parent);
> +                       if (ret)
> +                               return ret;
> +
>                         all = false;
>                         /* The metric key itself needs to go out.. */
>                         expr__del_id(&eg->pctx, cur->key);
>
>                         /* ... and it gets resolved to the parent context. */
> -                       ret = add_metric(group_list, pe, metric_no_group, &eg);
> +                       ret = add_metric(group_list, pe, metric_no_group, &eg, parent);
>                         if (ret)
>                                 return ret;
>
> @@ -758,14 +832,15 @@ static int resolve_metric(struct egroup *eg,
>  static int add_metric(struct list_head *group_list,
>                       struct pmu_event *pe,
>                       bool metric_no_group,
> -                     struct egroup **egp)
> +                     struct egroup **egp,
> +                     struct expr_id *parent)
>  {
>         int ret = 0;
>
>         pr_debug("metric expr %s for %s\n", pe->metric_expr, pe->metric_name);
>
>         if (!strstr(pe->metric_expr, "?")) {
> -               ret = __add_metric(group_list, pe, metric_no_group, 1, egp);
> +               ret = __add_metric(group_list, pe, metric_no_group, 1, egp, parent);
>         } else {
>                 int j, count;
>
> @@ -777,7 +852,7 @@ static int add_metric(struct list_head *group_list,
>                  */
>
>                 for (j = 0; j < count && !ret; j++) {
> -                       ret = __add_metric(group_list, pe, metric_no_group, j, egp);
> +                       ret = __add_metric(group_list, pe, metric_no_group, j, egp, parent);
>                 }
>         }
>
> @@ -798,7 +873,7 @@ static int metricgroup__add_metric(const char *metric, bool metric_no_group,
>         if (!pe)
>                 return -EINVAL;
>
> -       ret = add_metric(group_list, pe, metric_no_group, &eg);
> +       ret = add_metric(group_list, pe, metric_no_group, &eg, NULL);
>         if (ret)
>                 return ret;
>
> @@ -871,11 +946,21 @@ static void egroup__free_refs(struct egroup *egroup)
>         }
>  }
>
> +static void egroup__free_recursion(struct egroup *egroup)
> +{
> +       int i;
> +
> +       for (i = 0; i < egroup->recursion.cnt; i++) {
> +               free(egroup->recursion.id[i].id);
> +       }
> +}
> +
>  static void metricgroup__free_egroups(struct list_head *group_list)
>  {
>         struct egroup *eg, *egtmp;
>
>         list_for_each_entry_safe (eg, egtmp, group_list, nd) {
> +               egroup__free_recursion(eg);
>                 egroup__free_refs(eg);
>                 expr__ctx_clear(&eg->pctx);
>                 list_del_init(&eg->nd);
> --
> 2.25.4
>

Re: [PATCH 17/18] perf metric: Rename struct egroup to metric

[Ian Rogers]

On Sun, Jul 12, 2020 at 6:27 AM Jiri Olsa <jolsa@kernel.org> wrote:
>
> Renaming struct egroup to metric, because it seems
> to make more sense. Plus renaming all the variables
> that hold egroup to appropriate names.
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>

Awesome, thanks for improving readability!

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

> ---
>  tools/perf/util/metricgroup.c | 158 +++++++++++++++++-----------------
>  1 file changed, 79 insertions(+), 79 deletions(-)
>
> diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
> index 69ec20dd737b..9b880a5fb52b 100644
> --- a/tools/perf/util/metricgroup.c
> +++ b/tools/perf/util/metricgroup.c
> @@ -112,7 +112,7 @@ struct metric_ref_node {
>
>  #define RECURSION_ID_MAX 100
>
> -struct egroup {
> +struct metric {
>         struct list_head nd;
>         struct expr_parse_ctx pctx;
>         const char *metric_name;
> @@ -242,7 +242,7 @@ static int metricgroup__setup_events(struct list_head *groups,
>         struct metric_expr *expr;
>         int i = 0;
>         int ret = 0;
> -       struct egroup *eg;
> +       struct metric *m;
>         struct evsel *evsel, *tmp;
>         unsigned long *evlist_used;
>
> @@ -250,23 +250,23 @@ static int metricgroup__setup_events(struct list_head *groups,
>         if (!evlist_used)
>                 return -ENOMEM;
>
> -       list_for_each_entry (eg, groups, nd) {
> +       list_for_each_entry (m, groups, nd) {
>                 struct evsel **metric_events;
>                 struct metric_ref *metric_refs = NULL;
>
>                 metric_events = calloc(sizeof(void *),
> -                               hashmap__size(&eg->pctx.ids) + 1);
> +                               hashmap__size(&m->pctx.ids) + 1);
>                 if (!metric_events) {
>                         ret = -ENOMEM;
>                         break;
>                 }
> -               evsel = find_evsel_group(perf_evlist, &eg->pctx,
> +               evsel = find_evsel_group(perf_evlist, &m->pctx,
>                                          metric_no_merge,
> -                                        eg->has_constraint, metric_events,
> +                                        m->has_constraint, metric_events,
>                                          evlist_used);
>                 if (!evsel) {
>                         pr_debug("Cannot resolve %s: %s\n",
> -                                       eg->metric_name, eg->metric_expr);
> +                                       m->metric_name, m->metric_expr);
>                         free(metric_events);
>                         continue;
>                 }
> @@ -289,10 +289,10 @@ static int metricgroup__setup_events(struct list_head *groups,
>                  * Collect and store collected nested expressions
>                  * for metric processing.
>                  */
> -               if (eg->refs_cnt) {
> +               if (m->refs_cnt) {
>                         struct metric_ref_node *ref;
>
> -                       metric_refs = zalloc(sizeof(struct metric_ref) * (eg->refs_cnt + 1));
> +                       metric_refs = zalloc(sizeof(struct metric_ref) * (m->refs_cnt + 1));
>                         if (!metric_refs) {
>                                 ret = -ENOMEM;
>                                 free(metric_events);
> @@ -300,7 +300,7 @@ static int metricgroup__setup_events(struct list_head *groups,
>                         }
>
>                         i = 0;
> -                       list_for_each_entry(ref, &eg->refs, list) {
> +                       list_for_each_entry(ref, &m->refs, list) {
>                                 metric_refs[i].metric_name = ref->metric_name;
>                                 metric_refs[i].metric_expr = ref->metric_expr;
>                                 i++;
> @@ -308,11 +308,11 @@ static int metricgroup__setup_events(struct list_head *groups,
>                 };
>
>                 expr->metric_refs = metric_refs;
> -               expr->metric_expr = eg->metric_expr;
> -               expr->metric_name = eg->metric_name;
> -               expr->metric_unit = eg->metric_unit;
> +               expr->metric_expr = m->metric_expr;
> +               expr->metric_name = m->metric_name;
> +               expr->metric_unit = m->metric_unit;
>                 expr->metric_events = metric_events;
> -               expr->runtime = eg->runtime;
> +               expr->runtime = m->runtime;
>                 list_add(&expr->nd, &me->head);
>         }
>
> @@ -617,38 +617,38 @@ static int __add_metric(struct list_head *group_list,
>                         struct pmu_event *pe,
>                         bool metric_no_group,
>                         int runtime,
> -                       struct egroup **egp,
> +                       struct metric **mp,
>                         struct expr_id *parent)
>  {
>         struct metric_ref_node *ref;
> -       struct egroup *eg;
> +       struct metric *m;
>
> -       if (*egp == NULL) {
> +       if (*mp == NULL) {
>                 /*
>                  * We got in here for the master group,
>                  * allocate it and put it on the list.
>                  */
> -               eg = zalloc(sizeof(*eg));
> -               if (!eg)
> +               m = zalloc(sizeof(*m));
> +               if (!m)
>                         return -ENOMEM;
>
> -               expr__ctx_init(&eg->pctx);
> -               eg->metric_name = pe->metric_name;
> -               eg->metric_expr = pe->metric_expr;
> -               eg->metric_unit = pe->unit;
> -               eg->runtime = runtime;
> -               eg->has_constraint = metric_no_group || metricgroup__has_constraint(pe);
> -               INIT_LIST_HEAD(&eg->refs);
> -               eg->refs_cnt = 0;
> -               *egp = eg;
> +               expr__ctx_init(&m->pctx);
> +               m->metric_name = pe->metric_name;
> +               m->metric_expr = pe->metric_expr;
> +               m->metric_unit = pe->unit;
> +               m->runtime = runtime;
> +               m->has_constraint = metric_no_group || metricgroup__has_constraint(pe);
> +               INIT_LIST_HEAD(&m->refs);
> +               m->refs_cnt = 0;
> +               *mp = m;
>
>                 /* Initialize first recursion exr id with base name. */
> -               parent = &eg->recursion.id[0];
> -               eg->recursion.cnt = 1;
> +               parent = &m->recursion.id[0];
> +               m->recursion.cnt = 1;
>
>                 parent->id = strdup(pe->metric_name);
>                 if (!parent->id) {
> -                       free(eg);
> +                       free(m);
>                         return -ENOMEM;
>                 }
>         } else {
> @@ -657,7 +657,7 @@ static int __add_metric(struct list_head *group_list,
>                  * recursive metricgroup__add_metric call, add
>                  * it to the master group.
>                  */
> -               eg = *egp;
> +               m = *mp;
>
>                 ref = malloc(sizeof(*ref));
>                 if (!ref)
> @@ -665,22 +665,22 @@ static int __add_metric(struct list_head *group_list,
>
>                 ref->metric_name = pe->metric_name;
>                 ref->metric_expr = pe->metric_expr;
> -               list_add(&ref->list, &eg->refs);
> -               eg->refs_cnt++;
> -               eg->has_constraint |= metricgroup__has_constraint(pe);
> +               list_add(&ref->list, &m->refs);
> +               m->refs_cnt++;
> +               m->has_constraint |= metricgroup__has_constraint(pe);
>         }
>
>         /* Force all found IDs in metric to have us as parent ID. */
>         WARN_ON_ONCE(!parent);
> -       eg->pctx.parent = parent;
> +       m->pctx.parent = parent;
>
>         /*
>          * For both the master and referenced metrics, we parse
>          * all the metric's IDs and add it to the parent context.
>          */
> -       if (expr__find_other(pe->metric_expr, NULL, &eg->pctx, runtime) < 0) {
> -               expr__ctx_clear(&eg->pctx);
> -               free(eg);
> +       if (expr__find_other(pe->metric_expr, NULL, &m->pctx, runtime) < 0) {
> +               expr__ctx_clear(&m->pctx);
> +               free(m);
>                 return -EINVAL;
>         }
>
> @@ -688,23 +688,23 @@ static int __add_metric(struct list_head *group_list,
>          * We add new group only in the 'master' call,
>          * so bail out for referenced metric case.
>          */
> -       if (eg->refs_cnt)
> +       if (m->refs_cnt)
>                 return 0;
>
>         if (list_empty(group_list))
> -               list_add(&eg->nd, group_list);
> +               list_add(&m->nd, group_list);
>         else {
>                 struct list_head *pos;
>
>                 /* Place the largest groups at the front. */
>                 list_for_each_prev(pos, group_list) {
> -                       struct egroup *old = list_entry(pos, struct egroup, nd);
> +                       struct metric *old = list_entry(pos, struct metric, nd);
>
> -                       if (hashmap__size(&eg->pctx.ids) <=
> +                       if (hashmap__size(&m->pctx.ids) <=
>                             hashmap__size(&old->pctx.ids))
>                                 break;
>                 }
> -               list_add(&eg->nd, pos);
> +               list_add(&m->nd, pos);
>         }
>
>         return 0;
> @@ -730,7 +730,7 @@ static struct pmu_event *find_metric(const char *metric, struct pmu_events_map *
>         return NULL;
>  }
>
> -static int recursion_check(struct egroup *eg, const char *id, struct expr_id **parent)
> +static int recursion_check(struct metric *m, const char *id, struct expr_id **parent)
>  {
>         struct expr_id_data *data;
>         struct expr_id *p;
> @@ -742,7 +742,7 @@ static int recursion_check(struct egroup *eg, const char *id, struct expr_id **p
>          * if we already processed 'id', if we did, it's recursion
>          * and we fail.
>          */
> -       ret = expr__get_id(&eg->pctx, id, &data);
> +       ret = expr__get_id(&m->pctx, id, &data);
>         if (ret)
>                 return ret;
>
> @@ -760,13 +760,13 @@ static int recursion_check(struct egroup *eg, const char *id, struct expr_id **p
>          * If we are over the limit of static entris, the metric
>          * is too difficult/nested to process, fail as well.
>          */
> -       if (eg->recursion.cnt + 1 >= RECURSION_ID_MAX) {
> +       if (m->recursion.cnt + 1 >= RECURSION_ID_MAX) {
>                 pr_err("failed: too many nested metrics\n");
>                 return -EINVAL;
>         }
>
> -       p = &eg->recursion.id[eg->recursion.cnt];
> -       eg->recursion.cnt++;
> +       p = &m->recursion.id[m->recursion.cnt];
> +       m->recursion.cnt++;
>
>         p->id     = strdup(id);
>         p->parent = data->parent;
> @@ -778,10 +778,10 @@ static int recursion_check(struct egroup *eg, const char *id, struct expr_id **p
>  static int add_metric(struct list_head *group_list,
>                       struct pmu_event *pe,
>                       bool metric_no_group,
> -                     struct egroup **egp,
> +                     struct metric **mp,
>                       struct expr_id *parent);
>
> -static int resolve_metric(struct egroup *eg,
> +static int resolve_metric(struct metric *m,
>                           bool metric_no_group,
>                           struct list_head *group_list,
>                           struct pmu_events_map *map)
> @@ -797,7 +797,7 @@ static int resolve_metric(struct egroup *eg,
>          */
>         do {
>                 all = true;
> -               hashmap__for_each_entry((&eg->pctx.ids), cur, bkt) {
> +               hashmap__for_each_entry((&m->pctx.ids), cur, bkt) {
>                         struct expr_id *parent;
>                         struct pmu_event *pe;
>
> @@ -805,16 +805,16 @@ static int resolve_metric(struct egroup *eg,
>                         if (!pe)
>                                 continue;
>
> -                       ret = recursion_check(eg, cur->key, &parent);
> +                       ret = recursion_check(m, cur->key, &parent);
>                         if (ret)
>                                 return ret;
>
>                         all = false;
>                         /* The metric key itself needs to go out.. */
> -                       expr__del_id(&eg->pctx, cur->key);
> +                       expr__del_id(&m->pctx, cur->key);
>
>                         /* ... and it gets resolved to the parent context. */
> -                       ret = add_metric(group_list, pe, metric_no_group, &eg, parent);
> +                       ret = add_metric(group_list, pe, metric_no_group, &m, parent);
>                         if (ret)
>                                 return ret;
>
> @@ -832,7 +832,7 @@ static int resolve_metric(struct egroup *eg,
>  static int add_metric(struct list_head *group_list,
>                       struct pmu_event *pe,
>                       bool metric_no_group,
> -                     struct egroup **egp,
> +                     struct metric **m,
>                       struct expr_id *parent)
>  {
>         int ret = 0;
> @@ -840,7 +840,7 @@ static int add_metric(struct list_head *group_list,
>         pr_debug("metric expr %s for %s\n", pe->metric_expr, pe->metric_name);
>
>         if (!strstr(pe->metric_expr, "?")) {
> -               ret = __add_metric(group_list, pe, metric_no_group, 1, egp, parent);
> +               ret = __add_metric(group_list, pe, metric_no_group, 1, m, parent);
>         } else {
>                 int j, count;
>
> @@ -852,7 +852,7 @@ static int add_metric(struct list_head *group_list,
>                  */
>
>                 for (j = 0; j < count && !ret; j++) {
> -                       ret = __add_metric(group_list, pe, metric_no_group, j, egp, parent);
> +                       ret = __add_metric(group_list, pe, metric_no_group, j, m, parent);
>                 }
>         }
>
> @@ -865,7 +865,7 @@ static int metricgroup__add_metric(const char *metric, bool metric_no_group,
>                                    struct pmu_events_map *map)
>
>  {
> -       struct egroup *eg = NULL;
> +       struct metric *m = NULL;
>         struct pmu_event *pe;
>         int ret;
>
> @@ -873,7 +873,7 @@ static int metricgroup__add_metric(const char *metric, bool metric_no_group,
>         if (!pe)
>                 return -EINVAL;
>
> -       ret = add_metric(group_list, pe, metric_no_group, &eg, NULL);
> +       ret = add_metric(group_list, pe, metric_no_group, &m, NULL);
>         if (ret)
>                 return ret;
>
> @@ -881,7 +881,7 @@ static int metricgroup__add_metric(const char *metric, bool metric_no_group,
>          * Process any possible referenced metrics
>          * included in the expression.
>          */
> -       ret = resolve_metric(eg, metric_no_group,
> +       ret = resolve_metric(m, metric_no_group,
>                              group_list, map);
>         if (ret)
>                 return ret;
> @@ -893,12 +893,12 @@ static int metricgroup__add_metric(const char *metric, bool metric_no_group,
>          * Even if we add multiple groups through the runtime
>          * param, they share same events.
>          */
> -       if (eg->has_constraint) {
> +       if (m->has_constraint) {
>                 metricgroup__add_metric_non_group(events,
> -                                                 &eg->pctx);
> +                                                 &m->pctx);
>         } else {
>                 metricgroup__add_metric_weak_group(events,
> -                                                  &eg->pctx);
> +                                                  &m->pctx);
>         }
>         return 0;
>  }
> @@ -936,35 +936,35 @@ static int metricgroup__add_metric_list(const char *list, bool metric_no_group,
>         return ret;
>  }
>
> -static void egroup__free_refs(struct egroup *egroup)
> +static void metric__free_refs(struct metric *metric)
>  {
>         struct metric_ref_node *ref, *tmp;
>
> -       list_for_each_entry_safe(ref, tmp, &egroup->refs, list) {
> +       list_for_each_entry_safe(ref, tmp, &metric->refs, list) {
>                 list_del(&ref->list);
>                 free(ref);
>         }
>  }
>
> -static void egroup__free_recursion(struct egroup *egroup)
> +static void metric__free_recursion(struct metric *metric)
>  {
>         int i;
>
> -       for (i = 0; i < egroup->recursion.cnt; i++) {
> -               free(egroup->recursion.id[i].id);
> +       for (i = 0; i < metric->recursion.cnt; i++) {
> +               free(metric->recursion.id[i].id);
>         }
>  }
>
> -static void metricgroup__free_egroups(struct list_head *group_list)
> +static void metricgroup__free_metrics(struct list_head *group_list)
>  {
> -       struct egroup *eg, *egtmp;
> -
> -       list_for_each_entry_safe (eg, egtmp, group_list, nd) {
> -               egroup__free_recursion(eg);
> -               egroup__free_refs(eg);
> -               expr__ctx_clear(&eg->pctx);
> -               list_del_init(&eg->nd);
> -               free(eg);
> +       struct metric *m, *tmp;
> +
> +       list_for_each_entry_safe (m, tmp, group_list, nd) {
> +               metric__free_recursion(m);
> +               metric__free_refs(m);
> +               expr__ctx_clear(&m->pctx);
> +               list_del_init(&m->nd);
> +               free(m);
>         }
>  }
>
> @@ -997,7 +997,7 @@ static int parse_groups(struct evlist *perf_evlist, const char *str,
>         ret = metricgroup__setup_events(&group_list, metric_no_merge,
>                                         perf_evlist, metric_events);
>  out:
> -       metricgroup__free_egroups(&group_list);
> +       metricgroup__free_metrics(&group_list);
>         return ret;
>  }
>
> --
> 2.25.4
>

Re: [PATCH 18/18] perf metric: Rename group_list to list

[Ian Rogers]

On Sun, Jul 12, 2020 at 6:27 AM Jiri Olsa <jolsa@kernel.org> wrote:
>
> Following the previous change that rename egroup
> to metric, there's no reason to call the list
> 'group_list' anymore, renaming it to list.

List doesn't seem to be adding information beyond the data type. I
would prefer something like metric_list as it gives a clue what's in
the list. It's a shame we can't have templated lists, if we did then
my data type argument would apply and so maybe output_list would add
information. Probably to save space metric_list is best.

Thanks,
Ian

> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  tools/perf/util/metricgroup.c | 45 +++++++++++++++++------------------
>  1 file changed, 22 insertions(+), 23 deletions(-)
>
> diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
> index 9b880a5fb52b..0645ac1031fe 100644
> --- a/tools/perf/util/metricgroup.c
> +++ b/tools/perf/util/metricgroup.c
> @@ -613,7 +613,7 @@ int __weak arch_get_runtimeparam(void)
>         return 1;
>  }
>
> -static int __add_metric(struct list_head *group_list,
> +static int __add_metric(struct list_head *list,
>                         struct pmu_event *pe,
>                         bool metric_no_group,
>                         int runtime,
> @@ -691,13 +691,13 @@ static int __add_metric(struct list_head *group_list,
>         if (m->refs_cnt)
>                 return 0;
>
> -       if (list_empty(group_list))
> -               list_add(&m->nd, group_list);
> +       if (list_empty(list))
> +               list_add(&m->nd, list);
>         else {
>                 struct list_head *pos;
>
>                 /* Place the largest groups at the front. */
> -               list_for_each_prev(pos, group_list) {
> +               list_for_each_prev(pos, list) {
>                         struct metric *old = list_entry(pos, struct metric, nd);
>
>                         if (hashmap__size(&m->pctx.ids) <=
> @@ -775,7 +775,7 @@ static int recursion_check(struct metric *m, const char *id, struct expr_id **pa
>         return p->id ? 0 : -ENOMEM;
>  }
>
> -static int add_metric(struct list_head *group_list,
> +static int add_metric(struct list_head *list,
>                       struct pmu_event *pe,
>                       bool metric_no_group,
>                       struct metric **mp,
> @@ -783,7 +783,7 @@ static int add_metric(struct list_head *group_list,
>
>  static int resolve_metric(struct metric *m,
>                           bool metric_no_group,
> -                         struct list_head *group_list,
> +                         struct list_head *list,
>                           struct pmu_events_map *map)
>  {
>         struct hashmap_entry *cur;
> @@ -814,7 +814,7 @@ static int resolve_metric(struct metric *m,
>                         expr__del_id(&m->pctx, cur->key);
>
>                         /* ... and it gets resolved to the parent context. */
> -                       ret = add_metric(group_list, pe, metric_no_group, &m, parent);
> +                       ret = add_metric(list, pe, metric_no_group, &m, parent);
>                         if (ret)
>                                 return ret;
>
> @@ -829,7 +829,7 @@ static int resolve_metric(struct metric *m,
>         return 0;
>  }
>
> -static int add_metric(struct list_head *group_list,
> +static int add_metric(struct list_head *list,
>                       struct pmu_event *pe,
>                       bool metric_no_group,
>                       struct metric **m,
> @@ -840,7 +840,7 @@ static int add_metric(struct list_head *group_list,
>         pr_debug("metric expr %s for %s\n", pe->metric_expr, pe->metric_name);
>
>         if (!strstr(pe->metric_expr, "?")) {
> -               ret = __add_metric(group_list, pe, metric_no_group, 1, m, parent);
> +               ret = __add_metric(list, pe, metric_no_group, 1, m, parent);
>         } else {
>                 int j, count;
>
> @@ -848,11 +848,11 @@ static int add_metric(struct list_head *group_list,
>
>                 /* This loop is added to create multiple
>                  * events depend on count value and add
> -                * those events to group_list.
> +                * those events to list.
>                  */
>
>                 for (j = 0; j < count && !ret; j++) {
> -                       ret = __add_metric(group_list, pe, metric_no_group, j, m, parent);
> +                       ret = __add_metric(list, pe, metric_no_group, j, m, parent);
>                 }
>         }
>
> @@ -861,7 +861,7 @@ static int 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 *list,
>                                    struct pmu_events_map *map)
>
>  {
> @@ -873,7 +873,7 @@ static int metricgroup__add_metric(const char *metric, bool metric_no_group,
>         if (!pe)
>                 return -EINVAL;
>
> -       ret = add_metric(group_list, pe, metric_no_group, &m, NULL);
> +       ret = add_metric(list, pe, metric_no_group, &m, NULL);
>         if (ret)
>                 return ret;
>
> @@ -881,8 +881,7 @@ static int metricgroup__add_metric(const char *metric, bool metric_no_group,
>          * Process any possible referenced metrics
>          * included in the expression.
>          */
> -       ret = resolve_metric(m, metric_no_group,
> -                            group_list, map);
> +       ret = resolve_metric(m, metric_no_group, list, map);
>         if (ret)
>                 return ret;
>
> @@ -905,7 +904,7 @@ 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 *mlist,
>                                         struct pmu_events_map *map)
>  {
>         char *llist, *nlist, *p;
> @@ -921,7 +920,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, map);
> +                                             mlist, map);
>                 if (ret == -EINVAL) {
>                         fprintf(stderr, "Cannot find metric or group `%s'\n",
>                                         p);
> @@ -955,11 +954,11 @@ static void metric__free_recursion(struct metric *metric)
>         }
>  }
>
> -static void metricgroup__free_metrics(struct list_head *group_list)
> +static void metricgroup__free_metrics(struct list_head *list)
>  {
>         struct metric *m, *tmp;
>
> -       list_for_each_entry_safe (m, tmp, group_list, nd) {
> +       list_for_each_entry_safe (m, tmp, list, nd) {
>                 metric__free_recursion(m);
>                 metric__free_refs(m);
>                 expr__ctx_clear(&m->pctx);
> @@ -977,13 +976,13 @@ static int parse_groups(struct evlist *perf_evlist, const char *str,
>  {
>         struct parse_events_error parse_error;
>         struct strbuf extra_events;
> -       LIST_HEAD(group_list);
> +       LIST_HEAD(list);
>         int ret;
>
>         if (metric_events->nr_entries == 0)
>                 metricgroup__rblist_init(metric_events);
>         ret = metricgroup__add_metric_list(str, metric_no_group,
> -                                          &extra_events, &group_list, map);
> +                                          &extra_events, &list, map);
>         if (ret)
>                 return ret;
>         pr_debug("adding %s\n", extra_events.buf);
> @@ -994,10 +993,10 @@ static int parse_groups(struct evlist *perf_evlist, const char *str,
>                 goto out;
>         }
>         strbuf_release(&extra_events);
> -       ret = metricgroup__setup_events(&group_list, metric_no_merge,
> +       ret = metricgroup__setup_events(&list, metric_no_merge,
>                                         perf_evlist, metric_events);
>  out:
> -       metricgroup__free_metrics(&group_list);
> +       metricgroup__free_metrics(&list);
>         return ret;
>  }
>
> --
> 2.25.4
>

Re: [PATCH 11/18] perf metric: Add referenced metrics to hash data

[Ian Rogers]

On Sun, Jul 12, 2020 at 6:27 AM Jiri Olsa <jolsa@kernel.org> wrote:
>
> Adding referenced metrics to the parsing context so they
> can be resolved during the metric processing.
>
> Adding expr__add_ref function to store referenced metrics
> into parse context.
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  tools/perf/util/expr.c        | 35 +++++++++++++++++++++++++++++++++++
>  tools/perf/util/expr.h        | 13 ++++++++++++-
>  tools/perf/util/stat-shadow.c | 20 ++++++++++++++------
>  3 files changed, 61 insertions(+), 7 deletions(-)
>
> diff --git a/tools/perf/util/expr.c b/tools/perf/util/expr.c
> index 91142d4c3419..6bf8a21f5c53 100644
> --- a/tools/perf/util/expr.c
> +++ b/tools/perf/util/expr.c
> @@ -4,6 +4,8 @@
>  #include <errno.h>
>  #include <stdlib.h>
>  #include <string.h>
> +#include "metricgroup.h"
> +#include "debug.h"
>  #include "expr.h"
>  #include "expr-bison.h"
>  #include "expr-flex.h"
> @@ -61,6 +63,7 @@ int expr__add_id_val(struct expr_parse_ctx *ctx, const char *id, double val)
>         if (!data_ptr)
>                 return -ENOMEM;
>         data_ptr->val = val;
> +       data_ptr->is_ref = false;
>
>         ret = hashmap__set(&ctx->ids, id, data_ptr,
>                            (const void **)&old_key, (void **)&old_data);
> @@ -69,6 +72,38 @@ int expr__add_id_val(struct expr_parse_ctx *ctx, const char *id, double val)
>         return ret;
>  }
>
> +int expr__add_ref(struct expr_parse_ctx *ctx, struct metric_ref *ref)
> +{
> +       struct expr_id_data *data_ptr = NULL, *old_data = NULL;
> +       char *old_key = NULL;
> +       char *name;
> +       int ret;
> +
> +       data_ptr = zalloc(sizeof(*data_ptr));
> +       if (!data_ptr)
> +               return -ENOMEM;
> +
> +       name = strdup(ref->metric_name);
> +       if (!name) {
> +               free(data_ptr);
> +               return -ENOMEM;
> +       }
> +
> +       data_ptr->ref.metric_name = ref->metric_name;
> +       data_ptr->ref.metric_expr = ref->metric_expr;

Having one owned string and one unowned makes the memory management
here somewhat complicated. Perhaps dupe both?

> +       data_ptr->is_ref = true;
> +
> +       ret = hashmap__set(&ctx->ids, name, data_ptr,
> +                          (const void **)&old_key, (void **)&old_data);
> +
> +       pr_debug2("adding ref metric %s: %s\n",
> +                 ref->metric_name, ref->metric_expr);
> +
> +       free(old_key);
> +       free(old_data);
> +       return ret;
> +}
> +
>  int expr__get_id(struct expr_parse_ctx *ctx, const char *id,
>                  struct expr_id_data **data)
>  {
> diff --git a/tools/perf/util/expr.h b/tools/perf/util/expr.h
> index 2462abd0ac65..d19e66915228 100644
> --- a/tools/perf/util/expr.h
> +++ b/tools/perf/util/expr.h
> @@ -11,12 +11,22 @@
>  #include "util/hashmap.h"
>  //#endif
>
> +struct metric_ref;
> +
>  struct expr_parse_ctx {
>         struct hashmap ids;
>  };
>
>  struct expr_id_data {
> -       double  val;
> +       bool    is_ref;

nit: place at the end to avoid padding?

> +
> +       union {
> +               double  val;
> +               struct {
> +                       const char *metric_name;
> +                       const char *metric_expr;
> +               } ref;
> +       };
>  };
>
>  struct expr_scanner_ctx {
> @@ -29,6 +39,7 @@ void expr__ctx_clear(struct expr_parse_ctx *ctx);
>  void expr__del_id(struct expr_parse_ctx *ctx, const char *id);
>  int expr__add_id(struct expr_parse_ctx *ctx, const char *id);
>  int expr__add_id_val(struct expr_parse_ctx *ctx, const char *id, double val);
> +int expr__add_ref(struct expr_parse_ctx *ctx, struct metric_ref *ref);
>  int expr__get_id(struct expr_parse_ctx *ctx, const char *id,
>                  struct expr_id_data **data);
>  int expr__parse(double *final_val, struct expr_parse_ctx *ctx,
> diff --git a/tools/perf/util/stat-shadow.c b/tools/perf/util/stat-shadow.c
> index fc9ac4b4218e..e1ba6c1b916a 100644
> --- a/tools/perf/util/stat-shadow.c
> +++ b/tools/perf/util/stat-shadow.c
> @@ -731,13 +731,14 @@ static void print_smi_cost(struct perf_stat_config *config,
>  }
>
>  static int prepare_metric(struct evsel **metric_events,
> +                         struct metric_ref *metric_refs,

nit: the plural on refs confused me at first, perhaps a comment that
this is the array of referenced metrics.

Thanks,
Ian

>                           struct expr_parse_ctx *pctx,
>                           int cpu,
>                           struct runtime_stat *st)
>  {
>         double scale;
>         char *n, *pn;
> -       int i;
> +       int i, j, ret;
>
>         expr__ctx_init(pctx);
>         for (i = 0; metric_events[i]; i++) {
> @@ -778,12 +779,19 @@ static int prepare_metric(struct evsel **metric_events,
>                         expr__add_id_val(pctx, n, avg_stats(stats)*scale);
>         }
>
> +       for (j = 0; metric_refs && metric_refs[j].metric_name; j++) {
> +               ret = expr__add_ref(pctx, &metric_refs[j]);
> +               if (ret)
> +                       return ret;
> +       }
> +
>         return i;
>  }
>
>  static void generic_metric(struct perf_stat_config *config,
>                            const char *metric_expr,
>                            struct evsel **metric_events,
> +                          struct metric_ref *metric_refs,
>                            char *name,
>                            const char *metric_name,
>                            const char *metric_unit,
> @@ -798,7 +806,7 @@ static void generic_metric(struct perf_stat_config *config,
>         int i;
>         void *ctxp = out->ctx;
>
> -       i = prepare_metric(metric_events, &pctx, cpu, st);
> +       i = prepare_metric(metric_events, metric_refs, &pctx, cpu, st);
>         if (i < 0)
>                 return;
>
> @@ -847,7 +855,7 @@ double test_generic_metric(struct metric_expr *mexp, int cpu, struct runtime_sta
>         struct expr_parse_ctx pctx;
>         double ratio;
>
> -       if (prepare_metric(mexp->metric_events, &pctx, cpu, st) < 0)
> +       if (prepare_metric(mexp->metric_events, mexp->metric_refs, &pctx, cpu, st) < 0)
>                 return 0.;
>
>         if (expr__parse(&ratio, &pctx, mexp->metric_expr, 1))
> @@ -1064,8 +1072,8 @@ void perf_stat__print_shadow_stats(struct perf_stat_config *config,
>                 else
>                         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, cpu, out, st);
> +               generic_metric(config, evsel->metric_expr, evsel->metric_events, NULL,
> +                               evsel->name, 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];
> @@ -1093,7 +1101,7 @@ void perf_stat__print_shadow_stats(struct perf_stat_config *config,
>                         if (num++ > 0)
>                                 out->new_line(config, ctxp);
>                         generic_metric(config, mexp->metric_expr, mexp->metric_events,
> -                                       evsel->name, mexp->metric_name,
> +                                       mexp->metric_refs, evsel->name, mexp->metric_name,
>                                         mexp->metric_unit, mexp->runtime, cpu, out, st);
>                 }
>         }
> --
> 2.25.4
>

Re: [PATCH 00/18] perf metric: Add support to reuse metric

[Paul A. Clarke]

On Sun, Jul 12, 2020 at 03:26:16PM +0200, Jiri Olsa wrote:
> hi,
> this patchset is adding the support to reused metric in another 
> metric. The metric needs to be referenced by 'metric:' prefix.
> 
> For example, to define IPC by using CPI with change like:
> 
>          "BriefDescription": "Instructions Per Cycle (per Logical Processor)",
>  -       "MetricExpr": "INST_RETIRED.ANY / CPU_CLK_UNHALTED.THREAD",
>  +       "MetricExpr": "1/metric:CPI",
>          "MetricGroup": "TopDownL1",
>          "MetricName": "IPC"
> 
> I won't be able to find all the possible places we could
> use this at, so I wonder you guys (who was asking for this)
> would try it and come up with comments if there's something
> missing or we could already use it at some places.
> 
> It's based on Arnaldo's tmp.perf/core.
> 
> v2 changes:
>   - collected Ian's acks for few patches [Ian]
>   - renamed expr__add_id to expr__add_id_val [Ian]
>   - renamed expr_parse_data to expr_id_data [Ian]
>   - added recursion check [Ian]
>   - added metric test for DCache_L2 metric [Ian]
>   - added some renames as discussed in review [Ian]
> 
> Also available in here:
>   git://git.kernel.org/pub/scm/linux/kernel/git/jolsa/perf.git
>   perf/metric

I'm having trouble testing this.

I checked out this tree, and am able to build with a JSON metrics definition
file which uses other metrics.  I put this aside, though, because of the
following issue.

I built the kernel from this same tree and booted it successfully.
However, the metrics are not working correctly.  (I may very well be
doing something wrong.)

The base system is RHEL8, but it's now booted with the new kernel.
```
# uname -a
Linux system 5.8.0-rc4-g7dd02cf0b #1 SMP Wed Jul 15 12:31:45 EDT 2020 ppc64le ppc64le ppc64le GNU/Linux
# perf stat --metrics cpi_breakdown ./load
failed: way too many variables

 Performance counter stats for './load':

           569,884      pm_cmplu_stall_bru        #      0.0 bru_stall_cpi            (0.66%)
     5,662,892,875      pm_run_inst_cmpl                                              (0.66%)
                 0      pm_cmplu_stall_crypto     #      0.0 crypto_stall_cpi         (0.89%)
     6,728,401,038      pm_run_inst_cmpl                                              (0.89%)
        25,347,764      pm_cmplu_stall_dcache_miss #      0.0 dcache_miss_stall_cpi    (1.77%)
     6,727,267,383      pm_run_inst_cmpl                                              (1.77%)
                 0      pm_cmplu_stall_dflong     #      0.0 dflong_stall_cpi         (1.77%)
     6,719,648,242      pm_run_inst_cmpl                                              (1.77%)
                 0      pm_cmplu_stall_dfu        #      0.0 dfu_other_stall_cpi      (0.89%)
[...and LOTS more output...]

# ~/install/bin/perf stat --metrics cpi_breakdown ./load

 Performance counter stats for './load':

     6,729,588,550      pm_run_inst_cmpl          #     0.00 bru_stall_cpi          
            58,005      pm_cmplu_stall_bru                                          

       1.129524610 seconds time elapsed

       1.126914000 seconds user
       0.000955000 seconds sys

```

PC

Re: [PATCH 07/18] perf metric: Add add_metric function

[Jiri Olsa]

On Tue, Jul 14, 2020 at 02:11:02PM -0700, Ian Rogers wrote:
> On Sun, Jul 12, 2020 at 6:27 AM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > Decouple metric adding login into add_metric function,
> 
> s/login/logging/ ?

ugh yes

> 
> > so it can be used from other places in following changes.
> >
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> 
> Acked-by: Ian Rogers <irogers@google.com>

thanks,
jirka

Re: [PATCH 09/18] perf metric: Collect referenced metrics in struct metric_ref_node

[Jiri Olsa]

On Tue, Jul 14, 2020 at 05:07:04PM -0700, Ian Rogers wrote:
> On Sun, Jul 12, 2020 at 6:27 AM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > Collecting referenced metrics in struct metric_ref_node object,
> > so we can process them later on.
> >
> > The change will parse nested metric names out of expression and
> > 'resolve' them.
> >
> > All referenced metrics are dissolved into one context, meaning all
> > nested metrics events and added to the parent context.
> >
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> >  tools/perf/util/metricgroup.c | 147 ++++++++++++++++++++++++++++++----
> >  1 file changed, 132 insertions(+), 15 deletions(-)
> >
> > diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
> > index f0b0a053bfd2..9923eef1e2d4 100644
> > --- a/tools/perf/util/metricgroup.c
> > +++ b/tools/perf/util/metricgroup.c
> > @@ -102,12 +102,20 @@ void metricgroup__rblist_exit(struct rblist *metric_events)
> >         rblist__exit(metric_events);
> >  }
> >
> > +struct metric_ref_node {
> > +       const char *metric_name;
> > +       const char *metric_expr;
> > +       struct list_head list;
> > +};
> 
> Perhaps a comment like this above:
> /* A node in the list of referenced metrics. metric_expr is held as a
> convenience to avoid a search through the metric list. */

sounds good

> 
> > +
> >  struct egroup {
> >         struct list_head nd;
> >         struct expr_parse_ctx pctx;
> >         const char *metric_name;
> >         const char *metric_expr;
> >         const char *metric_unit;
> > +       struct list_head refs;
> > +       int refs_cnt;
> 
> A comment would be nice here as refs is a pretty overloaded term. For
> example, is refs_cnt a reference count for memory management or the
> length of the refs list? I'm unpopular and would use a long variable
> name here of something like referenced_metrics for the list.

how about metric_refs ? ;-)

> 
> >         int runtime;
> >         bool has_constraint;
> >  };
> > @@ -574,27 +582,66 @@ int __weak arch_get_runtimeparam(void)
> >  static int __add_metric(struct list_head *group_list,
> >                         struct pmu_event *pe,
> >                         bool metric_no_group,
> > -                       int runtime)
> > +                       int runtime,
> > +                       struct egroup **egp)
> 
> Rather than egp perhaps parent_eg or parent_metric?

I'm changing that later on, but perhaps not to parent_metric,
which sounds good, I'll check

> 
> >  {
> > +       struct metric_ref_node *ref;
> >         struct egroup *eg;
> >
> > -       eg = malloc(sizeof(*eg));
> > -       if (!eg)
> > -               return -ENOMEM;
> > +       if (*egp == NULL) {
> > +               /*
> > +                * We got in here for the master group,
> > +                * allocate it and put it on the list.
> > +                */
> > +               eg = malloc(sizeof(*eg));
> > +               if (!eg)
> > +                       return -ENOMEM;
> > +
> > +               expr__ctx_init(&eg->pctx);
> > +               eg->metric_name = pe->metric_name;
> > +               eg->metric_expr = pe->metric_expr;
> > +               eg->metric_unit = pe->unit;
> > +               eg->runtime = runtime;
> > +               eg->has_constraint = metric_no_group || metricgroup__has_constraint(pe);
> > +               INIT_LIST_HEAD(&eg->refs);
> > +               eg->refs_cnt = 0;
> > +               *egp = eg;
> > +       } else {
> > +               /*
> > +                * We got here for the referenced metric, via the
> > +                * recursive metricgroup__add_metric call, add
> > +                * it to the master group.
> 
> Probably want to avoid the term master here and below:
> https://www.kernel.org/doc/html/latest/process/coding-style.html?highlight=language%20master#naming
> Perhaps parent or referencing metric?

right.. will change

> 
> > +                */
> > +               eg = *egp;
> > +
> > +               ref = malloc(sizeof(*ref));
> > +               if (!ref)
> > +                       return -ENOMEM;
> >
> > -       expr__ctx_init(&eg->pctx);
> > -       eg->metric_name = pe->metric_name;
> > -       eg->metric_expr = pe->metric_expr;
> > -       eg->metric_unit = pe->unit;
> > -       eg->runtime = runtime;
> > -       eg->has_constraint = metric_no_group || metricgroup__has_constraint(pe);
> > +               ref->metric_name = pe->metric_name;
> > +               ref->metric_expr = pe->metric_expr;
> > +               list_add(&ref->list, &eg->refs);
> > +               eg->refs_cnt++;
> > +               eg->has_constraint |= metricgroup__has_constraint(pe);
> 
> Why not metric_no_group here? Perhaps use a function to avoid
> duplication with the code above.

oops, I think that has_constraint update should not be there at al,
just for the 'parent' metric, thanks!

> 
> > +       }
> >
> > +       /*
> > +        * For both the master and referenced metrics, we parse
> > +        * all the metric's IDs and add it to the parent context.
> > +        */
> >         if (expr__find_other(pe->metric_expr, NULL, &eg->pctx, runtime) < 0) {
> >                 expr__ctx_clear(&eg->pctx);
> >                 free(eg);
> >                 return -EINVAL;
> >         }
> >
> > +       /*
> > +        * We add new group only in the 'master' call,
> > +        * so bail out for referenced metric case.
> > +        */
> > +       if (eg->refs_cnt)
> > +               return 0;
> > +
> >         if (list_empty(group_list))
> >                 list_add(&eg->nd, group_list);
> >         else {
> > @@ -636,14 +683,63 @@ static struct pmu_event *find_metric(const char *metric, struct pmu_events_map *
> >
> >  static int add_metric(struct list_head *group_list,
> >                       struct pmu_event *pe,
> > -                     bool metric_no_group)
> > +                     bool metric_no_group,
> > +                     struct egroup **egp);
> > +
> > +static int resolve_metric(struct egroup *eg,
> > +                         bool metric_no_group,
> > +                         struct list_head *group_list,
> > +                         struct pmu_events_map *map)
> > +{
> > +       struct hashmap_entry *cur;
> > +       size_t bkt;
> > +       bool all;
> > +       int ret;
> > +
> > +       /*
> > +        * Iterate all the parsed IDs and if there's metric,
> > +        * add it to the context.
> > +        */
> 
> Does this mean that the ID doesn't need to begin "metric:" to
> reference a different metric as per Andi Kleen's request?

correct, that's why I separated find_metric function earlier,
now I see I did not put this to the changelog :-\ sry

> 
> > +       do {
> > +               all = true;
> > +               hashmap__for_each_entry((&eg->pctx.ids), cur, bkt) {
> > +                       struct pmu_event *pe;
> > +
> > +                       pe = find_metric(cur->key, map);
> > +                       if (!pe)
> > +                               continue;
> > +
> > +                       all = false;
> > +                       /* The metric key itself needs to go out.. */
> > +                       expr__del_id(&eg->pctx, cur->key);
> > +
> > +                       /* ... and it gets resolved to the parent context. */
> > +                       ret = add_metric(group_list, pe, metric_no_group, &eg);
> > +                       if (ret)
> > +                               return ret;
> > +
> > +                       /*
> > +                        * We added new metric to hashmap, so we need
> > +                        * to break the iteration and start over.
> > +                        */
> > +                       break;
> > +               }
> > +       } while (!all);
> > +
> > +       return 0;
> > +}

SNIP

Re: [PATCH 13/18] perf metric: Add events for the current group

[Jiri Olsa]

On Wed, Jul 15, 2020 at 10:00:09AM -0700, Ian Rogers wrote:
> On Sun, Jul 12, 2020 at 6:27 AM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > There's no need to iterate the whole list of groups,
> > when adding new events. The currently created group
> > is the one we want to add.
> >
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> >  tools/perf/util/metricgroup.c | 22 ++++++++++++----------
> >  1 file changed, 12 insertions(+), 10 deletions(-)
> >
> > diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
> > index 8cbcc5e05fef..66f25362702d 100644
> > --- a/tools/perf/util/metricgroup.c
> > +++ b/tools/perf/util/metricgroup.c
> > @@ -811,17 +811,19 @@ static int metricgroup__add_metric(const char *metric, bool metric_no_group,
> 
> Could we add a function comment to describe the arguments here?

ok

> Currently events is an empty list out argument that is built up by
> this code, now it will be incrementally updated. Except I don't think
> I'm capturing the current state correctly, it is confusing that there
> is the loop in the current code. It looks like events will be added
> multiple times redundantly.

oops, I meant to add the example of broken processing in here,
but forgot.. will update

> 
> >         if (ret)
> >                 return ret;
> >
> > -       list_for_each_entry(eg, group_list, nd) {
> > -               if (events->len > 0)
> > -                       strbuf_addf(events, ",");
> > +       if (events->len > 0)
> > +               strbuf_addf(events, ",");
> >
> > -               if (eg->has_constraint) {
> > -                       metricgroup__add_metric_non_group(events,
> > -                                                         &eg->pctx);
> > -               } else {
> > -                       metricgroup__add_metric_weak_group(events,
> > -                                                          &eg->pctx);
> > -               }
> > +       /*
> > +        * Even if we add multiple groups through the runtime
> > +        * param, they share same events.
> > +        */
> 
> I'm not clear what runtime param is here. Is it the \? arch runtime parameter?

yes, that's that ppc quirk.. adding extra same metrics based
on that runtime param.. for some reason ;-)

thanks,
jirka

Re: [PATCH 16/18] perf metric: Add recursion check when processing nested metrics

[Jiri Olsa]

On Wed, Jul 15, 2020 at 10:40:41AM -0700, Ian Rogers wrote:
> On Sun, Jul 12, 2020 at 6:27 AM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > Keeping the stack of nested metrics via 'struct expr_id' objecs
> 
> s/objecs/objects/
> 
> > and checking if we are in recursion via already processed metric.
> >
> > The stack is implemented as static array within the struct egroup
> > with 100 entries, which should be enough nesting depth for any
> > metric we have or plan to have at the moment.
> >
> > Adding test that simulates the recursion and checks we can
> > detect it.
> >
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> >  tools/perf/tests/parse-metric.c |  27 ++++++++-
> >  tools/perf/util/expr.c          |   2 +
> >  tools/perf/util/expr.h          |   9 ++-
> >  tools/perf/util/metricgroup.c   | 101 +++++++++++++++++++++++++++++---
> >  4 files changed, 128 insertions(+), 11 deletions(-)
> >
> > diff --git a/tools/perf/tests/parse-metric.c b/tools/perf/tests/parse-metric.c
> > index b50e2a3f3b73..238a805edd55 100644
> > --- a/tools/perf/tests/parse-metric.c
> > +++ b/tools/perf/tests/parse-metric.c
> > @@ -57,6 +57,14 @@ static struct pmu_event pme_test[] = {
> >         .metric_expr    = "d_ratio(DCache_L2_All_Miss, DCache_L2_All)",
> >         .metric_name    = "DCache_L2_Misses",
> >  },
> > +{
> > +       .metric_expr    = "IPC + M2",
> > +       .metric_name    = "M1",
> > +},
> > +{
> > +       .metric_expr    = "IPC + M1",
> > +       .metric_name    = "M2",
> > +},
> >  };
> 
> Perhaps add a test on simple recursion too:
> {
>   .metric_expr = "1/M1",
>   .metric_name = "M1",
> }

ok, will add

SNIP

> > diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
> > index 66f25362702d..69ec20dd737b 100644
> > --- a/tools/perf/util/metricgroup.c
> > +++ b/tools/perf/util/metricgroup.c
> > @@ -24,6 +24,7 @@
> >  #include <subcmd/parse-options.h>
> >  #include <api/fs/fs.h>
> >  #include "util.h"
> > +#include <asm/bug.h>
> >
> >  struct metric_event *metricgroup__lookup(struct rblist *metric_events,
> >                                          struct evsel *evsel,
> > @@ -109,6 +110,8 @@ struct metric_ref_node {
> >         struct list_head list;
> >  };
> >
> > +#define RECURSION_ID_MAX 100
> > +
> >  struct egroup {
> >         struct list_head nd;
> >         struct expr_parse_ctx pctx;
> > @@ -119,6 +122,11 @@ struct egroup {
> >         int refs_cnt;
> >         int runtime;
> >         bool has_constraint;
> > +
> > +       struct {
> > +               struct expr_id  id[RECURSION_ID_MAX];
> > +               int             cnt;
> > +       } recursion;
> >  };
> 
> Rather than place this in the egroup why not pass a "visited" array to
> add metric. This would be more in keeping with Tarjan's algorithm
> (although the SCC isn't needed in this case):
> https://en.wikipedia.org/wiki/Tarjan%27s_strongly_connected_components_algorithm

it's true that it's just source of the 'struct expr_id' objects and
one global array could be used in multiple metrics not event directly
nested ... seems good, will check

thanks,
jirka

Re: [PATCH 18/18] perf metric: Rename group_list to list

[Jiri Olsa]

On Wed, Jul 15, 2020 at 11:04:08AM -0700, Ian Rogers wrote:
> On Sun, Jul 12, 2020 at 6:27 AM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > Following the previous change that rename egroup
> > to metric, there's no reason to call the list
> > 'group_list' anymore, renaming it to list.
> 
> List doesn't seem to be adding information beyond the data type. I
> would prefer something like metric_list as it gives a clue what's in
> the list. It's a shame we can't have templated lists, if we did then
> my data type argument would apply and so maybe output_list would add
> information. Probably to save space metric_list is best.

ok, will change to metric_list

thanks,
jirka

Re: [PATCH 11/18] perf metric: Add referenced metrics to hash data

[Jiri Olsa]

On Wed, Jul 15, 2020 at 11:25:14AM -0700, Ian Rogers wrote:
> On Sun, Jul 12, 2020 at 6:27 AM Jiri Olsa <jolsa@kernel.org> wrote:

SNIP

> > +int expr__add_ref(struct expr_parse_ctx *ctx, struct metric_ref *ref)
> > +{
> > +       struct expr_id_data *data_ptr = NULL, *old_data = NULL;
> > +       char *old_key = NULL;
> > +       char *name;
> > +       int ret;
> > +
> > +       data_ptr = zalloc(sizeof(*data_ptr));
> > +       if (!data_ptr)
> > +               return -ENOMEM;
> > +
> > +       name = strdup(ref->metric_name);
> > +       if (!name) {
> > +               free(data_ptr);
> > +               return -ENOMEM;
> > +       }
> > +
> > +       data_ptr->ref.metric_name = ref->metric_name;
> > +       data_ptr->ref.metric_expr = ref->metric_expr;
> 
> Having one owned string and one unowned makes the memory management
> here somewhat complicated. Perhaps dupe both?

right, will check on this

> 
> > +       data_ptr->is_ref = true;
> > +
> > +       ret = hashmap__set(&ctx->ids, name, data_ptr,
> > +                          (const void **)&old_key, (void **)&old_data);
> > +
> > +       pr_debug2("adding ref metric %s: %s\n",
> > +                 ref->metric_name, ref->metric_expr);
> > +
> > +       free(old_key);
> > +       free(old_data);
> > +       return ret;
> > +}
> > +
> >  int expr__get_id(struct expr_parse_ctx *ctx, const char *id,
> >                  struct expr_id_data **data)
> >  {
> > diff --git a/tools/perf/util/expr.h b/tools/perf/util/expr.h
> > index 2462abd0ac65..d19e66915228 100644
> > --- a/tools/perf/util/expr.h
> > +++ b/tools/perf/util/expr.h
> > @@ -11,12 +11,22 @@
> >  #include "util/hashmap.h"
> >  //#endif
> >
> > +struct metric_ref;
> > +
> >  struct expr_parse_ctx {
> >         struct hashmap ids;
> >  };
> >
> >  struct expr_id_data {
> > -       double  val;
> > +       bool    is_ref;
> 
> nit: place at the end to avoid padding?

ok

> 
> > +
> > +       union {
> > +               double  val;
> > +               struct {
> > +                       const char *metric_name;
> > +                       const char *metric_expr;
> > +               } ref;
> > +       };
> >  };
> >
> >  struct expr_scanner_ctx {
> > @@ -29,6 +39,7 @@ void expr__ctx_clear(struct expr_parse_ctx *ctx);
> >  void expr__del_id(struct expr_parse_ctx *ctx, const char *id);
> >  int expr__add_id(struct expr_parse_ctx *ctx, const char *id);
> >  int expr__add_id_val(struct expr_parse_ctx *ctx, const char *id, double val);
> > +int expr__add_ref(struct expr_parse_ctx *ctx, struct metric_ref *ref);
> >  int expr__get_id(struct expr_parse_ctx *ctx, const char *id,
> >                  struct expr_id_data **data);
> >  int expr__parse(double *final_val, struct expr_parse_ctx *ctx,
> > diff --git a/tools/perf/util/stat-shadow.c b/tools/perf/util/stat-shadow.c
> > index fc9ac4b4218e..e1ba6c1b916a 100644
> > --- a/tools/perf/util/stat-shadow.c
> > +++ b/tools/perf/util/stat-shadow.c
> > @@ -731,13 +731,14 @@ static void print_smi_cost(struct perf_stat_config *config,
> >  }
> >
> >  static int prepare_metric(struct evsel **metric_events,
> > +                         struct metric_ref *metric_refs,
> 
> nit: the plural on refs confused me at first, perhaps a comment that
> this is the array of referenced metrics.

I'll add the comment

thanks,
jirka

Re: [PATCH 00/18] perf metric: Add support to reuse metric

[Jiri Olsa]

On Wed, Jul 15, 2020 at 01:33:27PM -0500, Paul A. Clarke wrote:
> On Sun, Jul 12, 2020 at 03:26:16PM +0200, Jiri Olsa wrote:
> > hi,
> > this patchset is adding the support to reused metric in another 
> > metric. The metric needs to be referenced by 'metric:' prefix.
> > 
> > For example, to define IPC by using CPI with change like:
> > 
> >          "BriefDescription": "Instructions Per Cycle (per Logical Processor)",
> >  -       "MetricExpr": "INST_RETIRED.ANY / CPU_CLK_UNHALTED.THREAD",
> >  +       "MetricExpr": "1/metric:CPI",
> >          "MetricGroup": "TopDownL1",
> >          "MetricName": "IPC"
> > 
> > I won't be able to find all the possible places we could
> > use this at, so I wonder you guys (who was asking for this)
> > would try it and come up with comments if there's something
> > missing or we could already use it at some places.
> > 
> > It's based on Arnaldo's tmp.perf/core.
> > 
> > v2 changes:
> >   - collected Ian's acks for few patches [Ian]
> >   - renamed expr__add_id to expr__add_id_val [Ian]
> >   - renamed expr_parse_data to expr_id_data [Ian]
> >   - added recursion check [Ian]
> >   - added metric test for DCache_L2 metric [Ian]
> >   - added some renames as discussed in review [Ian]
> > 
> > Also available in here:
> >   git://git.kernel.org/pub/scm/linux/kernel/git/jolsa/perf.git
> >   perf/metric
> 
> I'm having trouble testing this.
> 
> I checked out this tree, and am able to build with a JSON metrics definition
> file which uses other metrics.  I put this aside, though, because of the
> following issue.
> 
> I built the kernel from this same tree and booted it successfully.
> However, the metrics are not working correctly.  (I may very well be
> doing something wrong.)

if you'll share the metric change I can help debugging that

> 
> The base system is RHEL8, but it's now booted with the new kernel.
> ```
> # uname -a
> Linux system 5.8.0-rc4-g7dd02cf0b #1 SMP Wed Jul 15 12:31:45 EDT 2020 ppc64le ppc64le ppc64le GNU/Linux
> # perf stat --metrics cpi_breakdown ./load
> failed: way too many variables

hm, this ^^^ error string was removed in:
  43fe337c86a9 perf expr: Migrate expr ids table to a hashmap

looks like you're not running the correct perf binary

thanks for testing this,
jirka

Re: [PATCH 09/18] perf metric: Collect referenced metrics in struct metric_ref_node

[Ian Rogers]

On Wed, Jul 15, 2020 at 2:13 PM Jiri Olsa <jolsa@redhat.com> wrote:
>
> On Tue, Jul 14, 2020 at 05:07:04PM -0700, Ian Rogers wrote:
> > On Sun, Jul 12, 2020 at 6:27 AM Jiri Olsa <jolsa@kernel.org> wrote:
> > >
> > > Collecting referenced metrics in struct metric_ref_node object,
> > > so we can process them later on.
> > >
> > > The change will parse nested metric names out of expression and
> > > 'resolve' them.
> > >
> > > All referenced metrics are dissolved into one context, meaning all
> > > nested metrics events and added to the parent context.
> > >
> > > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > > ---
> > >  tools/perf/util/metricgroup.c | 147 ++++++++++++++++++++++++++++++----
> > >  1 file changed, 132 insertions(+), 15 deletions(-)
> > >
> > > diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
> > > index f0b0a053bfd2..9923eef1e2d4 100644
> > > --- a/tools/perf/util/metricgroup.c
> > > +++ b/tools/perf/util/metricgroup.c
> > > @@ -102,12 +102,20 @@ void metricgroup__rblist_exit(struct rblist *metric_events)
> > >         rblist__exit(metric_events);
> > >  }
> > >
> > > +struct metric_ref_node {
> > > +       const char *metric_name;
> > > +       const char *metric_expr;
> > > +       struct list_head list;
> > > +};
> >
> > Perhaps a comment like this above:
> > /* A node in the list of referenced metrics. metric_expr is held as a
> > convenience to avoid a search through the metric list. */
>
> sounds good
>
> >
> > > +
> > >  struct egroup {
> > >         struct list_head nd;
> > >         struct expr_parse_ctx pctx;
> > >         const char *metric_name;
> > >         const char *metric_expr;
> > >         const char *metric_unit;
> > > +       struct list_head refs;
> > > +       int refs_cnt;
> >
> > A comment would be nice here as refs is a pretty overloaded term. For
> > example, is refs_cnt a reference count for memory management or the
> > length of the refs list? I'm unpopular and would use a long variable
> > name here of something like referenced_metrics for the list.
>
> how about metric_refs ? ;-)

Sounds great, thanks!
Ian

> >
> > >         int runtime;
> > >         bool has_constraint;
> > >  };
> > > @@ -574,27 +582,66 @@ int __weak arch_get_runtimeparam(void)
> > >  static int __add_metric(struct list_head *group_list,
> > >                         struct pmu_event *pe,
> > >                         bool metric_no_group,
> > > -                       int runtime)
> > > +                       int runtime,
> > > +                       struct egroup **egp)
> >
> > Rather than egp perhaps parent_eg or parent_metric?
>
> I'm changing that later on, but perhaps not to parent_metric,
> which sounds good, I'll check
>
> >
> > >  {
> > > +       struct metric_ref_node *ref;
> > >         struct egroup *eg;
> > >
> > > -       eg = malloc(sizeof(*eg));
> > > -       if (!eg)
> > > -               return -ENOMEM;
> > > +       if (*egp == NULL) {
> > > +               /*
> > > +                * We got in here for the master group,
> > > +                * allocate it and put it on the list.
> > > +                */
> > > +               eg = malloc(sizeof(*eg));
> > > +               if (!eg)
> > > +                       return -ENOMEM;
> > > +
> > > +               expr__ctx_init(&eg->pctx);
> > > +               eg->metric_name = pe->metric_name;
> > > +               eg->metric_expr = pe->metric_expr;
> > > +               eg->metric_unit = pe->unit;
> > > +               eg->runtime = runtime;
> > > +               eg->has_constraint = metric_no_group || metricgroup__has_constraint(pe);
> > > +               INIT_LIST_HEAD(&eg->refs);
> > > +               eg->refs_cnt = 0;
> > > +               *egp = eg;
> > > +       } else {
> > > +               /*
> > > +                * We got here for the referenced metric, via the
> > > +                * recursive metricgroup__add_metric call, add
> > > +                * it to the master group.
> >
> > Probably want to avoid the term master here and below:
> > https://www.kernel.org/doc/html/latest/process/coding-style.html?highlight=language%20master#naming
> > Perhaps parent or referencing metric?
>
> right.. will change
>
> >
> > > +                */
> > > +               eg = *egp;
> > > +
> > > +               ref = malloc(sizeof(*ref));
> > > +               if (!ref)
> > > +                       return -ENOMEM;
> > >
> > > -       expr__ctx_init(&eg->pctx);
> > > -       eg->metric_name = pe->metric_name;
> > > -       eg->metric_expr = pe->metric_expr;
> > > -       eg->metric_unit = pe->unit;
> > > -       eg->runtime = runtime;
> > > -       eg->has_constraint = metric_no_group || metricgroup__has_constraint(pe);
> > > +               ref->metric_name = pe->metric_name;
> > > +               ref->metric_expr = pe->metric_expr;
> > > +               list_add(&ref->list, &eg->refs);
> > > +               eg->refs_cnt++;
> > > +               eg->has_constraint |= metricgroup__has_constraint(pe);
> >
> > Why not metric_no_group here? Perhaps use a function to avoid
> > duplication with the code above.
>
> oops, I think that has_constraint update should not be there at al,
> just for the 'parent' metric, thanks!
>
> >
> > > +       }
> > >
> > > +       /*
> > > +        * For both the master and referenced metrics, we parse
> > > +        * all the metric's IDs and add it to the parent context.
> > > +        */
> > >         if (expr__find_other(pe->metric_expr, NULL, &eg->pctx, runtime) < 0) {
> > >                 expr__ctx_clear(&eg->pctx);
> > >                 free(eg);
> > >                 return -EINVAL;
> > >         }
> > >
> > > +       /*
> > > +        * We add new group only in the 'master' call,
> > > +        * so bail out for referenced metric case.
> > > +        */
> > > +       if (eg->refs_cnt)
> > > +               return 0;
> > > +
> > >         if (list_empty(group_list))
> > >                 list_add(&eg->nd, group_list);
> > >         else {
> > > @@ -636,14 +683,63 @@ static struct pmu_event *find_metric(const char *metric, struct pmu_events_map *
> > >
> > >  static int add_metric(struct list_head *group_list,
> > >                       struct pmu_event *pe,
> > > -                     bool metric_no_group)
> > > +                     bool metric_no_group,
> > > +                     struct egroup **egp);
> > > +
> > > +static int resolve_metric(struct egroup *eg,
> > > +                         bool metric_no_group,
> > > +                         struct list_head *group_list,
> > > +                         struct pmu_events_map *map)
> > > +{
> > > +       struct hashmap_entry *cur;
> > > +       size_t bkt;
> > > +       bool all;
> > > +       int ret;
> > > +
> > > +       /*
> > > +        * Iterate all the parsed IDs and if there's metric,
> > > +        * add it to the context.
> > > +        */
> >
> > Does this mean that the ID doesn't need to begin "metric:" to
> > reference a different metric as per Andi Kleen's request?
>
> correct, that's why I separated find_metric function earlier,
> now I see I did not put this to the changelog :-\ sry
>
> >
> > > +       do {
> > > +               all = true;
> > > +               hashmap__for_each_entry((&eg->pctx.ids), cur, bkt) {
> > > +                       struct pmu_event *pe;
> > > +
> > > +                       pe = find_metric(cur->key, map);
> > > +                       if (!pe)
> > > +                               continue;
> > > +
> > > +                       all = false;
> > > +                       /* The metric key itself needs to go out.. */
> > > +                       expr__del_id(&eg->pctx, cur->key);
> > > +
> > > +                       /* ... and it gets resolved to the parent context. */
> > > +                       ret = add_metric(group_list, pe, metric_no_group, &eg);
> > > +                       if (ret)
> > > +                               return ret;
> > > +
> > > +                       /*
> > > +                        * We added new metric to hashmap, so we need
> > > +                        * to break the iteration and start over.
> > > +                        */
> > > +                       break;
> > > +               }
> > > +       } while (!all);
> > > +
> > > +       return 0;
> > > +}
>
> SNIP
>

Re: [PATCH 13/18] perf metric: Add events for the current group

[Ian Rogers]

On Wed, Jul 15, 2020 at 2:19 PM Jiri Olsa <jolsa@redhat.com> wrote:
>
> On Wed, Jul 15, 2020 at 10:00:09AM -0700, Ian Rogers wrote:
> > On Sun, Jul 12, 2020 at 6:27 AM Jiri Olsa <jolsa@kernel.org> wrote:
> > >
> > > There's no need to iterate the whole list of groups,
> > > when adding new events. The currently created group
> > > is the one we want to add.
> > >
> > > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > > ---
> > >  tools/perf/util/metricgroup.c | 22 ++++++++++++----------
> > >  1 file changed, 12 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
> > > index 8cbcc5e05fef..66f25362702d 100644
> > > --- a/tools/perf/util/metricgroup.c
> > > +++ b/tools/perf/util/metricgroup.c
> > > @@ -811,17 +811,19 @@ static int metricgroup__add_metric(const char *metric, bool metric_no_group,
> >
> > Could we add a function comment to describe the arguments here?
>
> ok
>
> > Currently events is an empty list out argument that is built up by
> > this code, now it will be incrementally updated. Except I don't think
> > I'm capturing the current state correctly, it is confusing that there
> > is the loop in the current code. It looks like events will be added
> > multiple times redundantly.
>
> oops, I meant to add the example of broken processing in here,
> but forgot.. will update
>
> >
> > >         if (ret)
> > >                 return ret;
> > >
> > > -       list_for_each_entry(eg, group_list, nd) {
> > > -               if (events->len > 0)
> > > -                       strbuf_addf(events, ",");
> > > +       if (events->len > 0)
> > > +               strbuf_addf(events, ",");
> > >
> > > -               if (eg->has_constraint) {
> > > -                       metricgroup__add_metric_non_group(events,
> > > -                                                         &eg->pctx);
> > > -               } else {
> > > -                       metricgroup__add_metric_weak_group(events,
> > > -                                                          &eg->pctx);
> > > -               }
> > > +       /*
> > > +        * Even if we add multiple groups through the runtime
> > > +        * param, they share same events.
> > > +        */
> >
> > I'm not clear what runtime param is here. Is it the \? arch runtime parameter?
>
> yes, that's that ppc quirk.. adding extra same metrics based
> on that runtime param.. for some reason ;-)

Do they share the same event? I thought the "?" was substituted in a
loop for a value 0...arch_runtime_value and so you got an event for
each of those values.

Thanks,
Ian

> thanks,
> jirka
>

RE: [PATCH 00/18] perf metric: Add support to reuse metric

[Paul A. Clarke]

On Wed, Jul 15, 2020 at 11:41:34PM +0200, Jiri Olsa wrote:
> On Wed, Jul 15, 2020 at 01:33:27PM -0500, Paul A. Clarke wrote:
> > On Sun, Jul 12, 2020 at 03:26:16PM +0200, Jiri Olsa wrote:
> > > hi,
> > > this patchset is adding the support to reused metric in another 
> > > metric. The metric needs to be referenced by 'metric:' prefix.
> > > 
> > > For example, to define IPC by using CPI with change like:
> > > 
> > >          "BriefDescription": "Instructions Per Cycle (per Logical Processor)",
> > >  -       "MetricExpr": "INST_RETIRED.ANY / CPU_CLK_UNHALTED.THREAD",
> > >  +       "MetricExpr": "1/metric:CPI",
> > >          "MetricGroup": "TopDownL1",
> > >          "MetricName": "IPC"
> > > 
> > > I won't be able to find all the possible places we could
> > > use this at, so I wonder you guys (who was asking for this)
> > > would try it and come up with comments if there's something
> > > missing or we could already use it at some places.
> > > 
> > > It's based on Arnaldo's tmp.perf/core.
> > > 
> > > v2 changes:
> > >   - collected Ian's acks for few patches [Ian]
> > >   - renamed expr__add_id to expr__add_id_val [Ian]
> > >   - renamed expr_parse_data to expr_id_data [Ian]
> > >   - added recursion check [Ian]
> > >   - added metric test for DCache_L2 metric [Ian]
> > >   - added some renames as discussed in review [Ian]
> > > 
> > > Also available in here:
> > >   git://git.kernel.org/pub/scm/linux/kernel/git/jolsa/perf.git
> > >   perf/metric
> > 
> > I'm having trouble testing this.
> > 
> > I checked out this tree, and am able to build with a JSON metrics definition
> > file which uses other metrics.  I put this aside, though, because of the
> > following issue.
> > 
> > I built the kernel from this same tree and booted it successfully.
> > However, the metrics are not working correctly.  (I may very well be
> > doing something wrong.)
> 
> if you'll share the metric change I can help debugging that

It fails as described below without any changes from me.

> > The base system is RHEL8, but it's now booted with the new kernel.
> > ```
> > # uname -a
> > Linux system 5.8.0-rc4-g7dd02cf0b #1 SMP Wed Jul 15 12:31:45 EDT 2020 ppc64le ppc64le ppc64le GNU/Linux
> > # perf stat --metrics cpi_breakdown ./load
> > failed: way too many variables
> 
> hm, this ^^^ error string was removed in:
>   43fe337c86a9 perf expr: Migrate expr ids table to a hashmap
> 
> looks like you're not running the correct perf binary

I should've explained this better in my post, but there are two runs of
perf there.  The first is above, and is the perf that comes with the
distribution.  I'll repeat that here:
```
# perf --version
perf version 4.18.0-214.el8.ppc64le
# perf stat --metrics cpi_breakdown ./load >/dev/null             
failed: way too many variables                                                               
 Performance counter stats for './load':                                                     
                                                                                             
           818,130      pm_cmplu_stall_bru        #      0.0 bru_stall_cpi            (0.45%)
     5,013,082,026      pm_run_inst_cmpl                                              (0.45%)
                 0      pm_cmplu_stall_crypto     #      0.0 crypto_stall_cpi         (0.89%)
     6,580,655,094      pm_run_inst_cmpl                                              (0.89%)
        25,729,751      pm_cmplu_stall_dcache_miss #      0.0 dcache_miss_stall_cpi    (1.77%)
     6,690,035,175      pm_run_inst_cmpl                                              (1.77%)
                 0      pm_cmplu_stall_dflong     #      0.0 dflong_stall_cpi         (1.77%)
     6,769,854,632      pm_run_inst_cmpl                                              (1.77%)
                 0      pm_cmplu_stall_dfu        #      0.0 dfu_other_stall_cpi      (0.89%)
[...and LOTS more...]
```

The second example in my previous post was from
git://git.kernel.org/pub/scm/linux/kernel/git/jolsa/perf.git
branch perf/metric
```
commit 7dd02cf0b9f04ca5339fa97f9a2280ebdd60b1db (grafted, HEAD -> perf/metric, origin/perf/metric)
Author: Jiri Olsa <jolsa@kernel.org>
Date:   Thu Jul 9 13:45:30 2020 +0200

    perf metric: Rename group_list to list
```

This build of `perf` fails:
```
# ~/install/bin/perf --version
perf version 5.8.rc4.g7dd02cf0b9f0
# ~/install/bin/perf stat --metrics cpi_breakdown ./load >/dev/null

 Performance counter stats for './load':

     6,729,400,541      pm_run_inst_cmpl          #     0.00 bru_stall_cpi          
            57,953      pm_cmplu_stall_bru                                          

       1.127319209 seconds time elapsed

       1.124906000 seconds user
       0.000890000 seconds sys
```

PC

Re: [PATCH 00/18] perf metric: Add support to reuse metric

[John Garry]

On 12/07/2020 14:26, Jiri Olsa wrote:
> hi,
> this patchset is adding the support to reused metric in another
> metric. The metric needs to be referenced by 'metric:' prefix.
> 
> For example, to define IPC by using CPI with change like:
> 
>           "BriefDescription": "Instructions Per Cycle (per Logical Processor)",
>   -       "MetricExpr": "INST_RETIRED.ANY / CPU_CLK_UNHALTED.THREAD",
>   +       "MetricExpr": "1/metric:CPI",
>           "MetricGroup": "TopDownL1",
>           "MetricName": "IPC"
> 
> I won't be able to find all the possible places we could
> use this at, so I wonder you guys (who was asking for this)
> would try it and come up with comments if there's something
> missing or we could already use it at some places.
> 
> It's based on Arnaldo's tmp.perf/core.
> 
> v2 changes:
>    - collected Ian's acks for few patches [Ian]
>    - renamed expr__add_id to expr__add_id_val [Ian]
>    - renamed expr_parse_data to expr_id_data [Ian]
>    - added recursion check [Ian]
>    - added metric test for DCache_L2 metric [Ian]
>    - added some renames as discussed in review [Ian]
> 
> Also available in here:
>    git://git.kernel.org/pub/scm/linux/kernel/git/jolsa/perf.git
>    perf/metric
> 
> thanks,
> jirka
> 
> 
> ---
> Jiri Olsa (18):
>        perf metric: Rename expr__add_id to expr__add_val
>        perf metric: Add struct expr_id_data to keep expr value
>        perf metric: Add expr__add_id function
>        perf metric: Change expr__get_id to return struct expr_id_data
>        perf metric: Add expr__del_id function
>        perf metric: Add find_metric function
>        perf metric: Add add_metric function
>        perf metric: Rename __metricgroup__add_metric to __add_metric
>        perf metric: Collect referenced metrics in struct metric_ref_node
>        perf metric: Collect referenced metrics in struct metric_expr
>        perf metric: Add referenced metrics to hash data
>        perf metric: Compute referenced metrics
>        perf metric: Add events for the current group
>        perf metric: Add cache_miss_cycles to metric parse test
>        perf metric: Add DCache_L2 to metric parse test
>        perf metric: Add recursion check when processing nested metrics
>        perf metric: Rename struct egroup to metric
>        perf metric: Rename group_list to list

I was thinking that a test metric using this reuse feature could be 
added in pmu-events/arch/test/test_cpu. But since no relevant parsing is 
done in jevents, maybe not a lot of value. Just for a bit more completeness.

Thanks,
John

> 
>   tools/perf/tests/expr.c         |   7 +-
>   tools/perf/tests/parse-metric.c | 131 +++++++++++++++++++++++++++++++++++-
>   tools/perf/tests/pmu-events.c   |   4 +-
>   tools/perf/util/expr.c          | 130 ++++++++++++++++++++++++++++++------
>   tools/perf/util/expr.h          |  34 +++++++++-
>   tools/perf/util/expr.y          |  16 +++--
>   tools/perf/util/metricgroup.c   | 435 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++---------------------------
>   tools/perf/util/metricgroup.h   |   6 ++
>   tools/perf/util/stat-shadow.c   |  24 ++++---
>   9 files changed, 644 insertions(+), 143 deletions(-)
> 
> .
> 

Re: [PATCH 13/18] perf metric: Add events for the current group

[Jiri Olsa]

On Wed, Jul 15, 2020 at 03:14:37PM -0700, Ian Rogers wrote:
> On Wed, Jul 15, 2020 at 2:19 PM Jiri Olsa <jolsa@redhat.com> wrote:
> >
> > On Wed, Jul 15, 2020 at 10:00:09AM -0700, Ian Rogers wrote:
> > > On Sun, Jul 12, 2020 at 6:27 AM Jiri Olsa <jolsa@kernel.org> wrote:
> > > >
> > > > There's no need to iterate the whole list of groups,
> > > > when adding new events. The currently created group
> > > > is the one we want to add.
> > > >
> > > > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > > > ---
> > > >  tools/perf/util/metricgroup.c | 22 ++++++++++++----------
> > > >  1 file changed, 12 insertions(+), 10 deletions(-)
> > > >
> > > > diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
> > > > index 8cbcc5e05fef..66f25362702d 100644
> > > > --- a/tools/perf/util/metricgroup.c
> > > > +++ b/tools/perf/util/metricgroup.c
> > > > @@ -811,17 +811,19 @@ static int metricgroup__add_metric(const char *metric, bool metric_no_group,
> > >
> > > Could we add a function comment to describe the arguments here?
> >
> > ok
> >
> > > Currently events is an empty list out argument that is built up by
> > > this code, now it will be incrementally updated. Except I don't think
> > > I'm capturing the current state correctly, it is confusing that there
> > > is the loop in the current code. It looks like events will be added
> > > multiple times redundantly.
> >
> > oops, I meant to add the example of broken processing in here,
> > but forgot.. will update
> >
> > >
> > > >         if (ret)
> > > >                 return ret;
> > > >
> > > > -       list_for_each_entry(eg, group_list, nd) {
> > > > -               if (events->len > 0)
> > > > -                       strbuf_addf(events, ",");
> > > > +       if (events->len > 0)
> > > > +               strbuf_addf(events, ",");
> > > >
> > > > -               if (eg->has_constraint) {
> > > > -                       metricgroup__add_metric_non_group(events,
> > > > -                                                         &eg->pctx);
> > > > -               } else {
> > > > -                       metricgroup__add_metric_weak_group(events,
> > > > -                                                          &eg->pctx);
> > > > -               }
> > > > +       /*
> > > > +        * Even if we add multiple groups through the runtime
> > > > +        * param, they share same events.
> > > > +        */
> > >
> > > I'm not clear what runtime param is here. Is it the \? arch runtime parameter?
> >
> > yes, that's that ppc quirk.. adding extra same metrics based
> > on that runtime param.. for some reason ;-)
> 
> Do they share the same event? I thought the "?" was substituted in a
> loop for a value 0...arch_runtime_value and so you got an event for
> each of those values.

that's right.. I need to rethink this part, I think this change broke
the runtime param stuff, we need to keep some temp list for the ppc case..
but it was broken already with adding all the events from the group_list,
which could contain other metrics events

thanks,
jirka

Re: [PATCH 00/18] perf metric: Add support to reuse metric

[Jiri Olsa]

On Wed, Jul 15, 2020 at 08:25:04PM -0500, Paul A. Clarke wrote:

SNIP

> # perf --version
> perf version 4.18.0-214.el8.ppc64le
> # perf stat --metrics cpi_breakdown ./load >/dev/null             
> failed: way too many variables                                                               
>  Performance counter stats for './load':                                                     
>                                                                                              
>            818,130      pm_cmplu_stall_bru        #      0.0 bru_stall_cpi            (0.45%)
>      5,013,082,026      pm_run_inst_cmpl                                              (0.45%)
>                  0      pm_cmplu_stall_crypto     #      0.0 crypto_stall_cpi         (0.89%)
>      6,580,655,094      pm_run_inst_cmpl                                              (0.89%)
>         25,729,751      pm_cmplu_stall_dcache_miss #      0.0 dcache_miss_stall_cpi    (1.77%)
>      6,690,035,175      pm_run_inst_cmpl                                              (1.77%)
>                  0      pm_cmplu_stall_dflong     #      0.0 dflong_stall_cpi         (1.77%)
>      6,769,854,632      pm_run_inst_cmpl                                              (1.77%)
>                  0      pm_cmplu_stall_dfu        #      0.0 dfu_other_stall_cpi      (0.89%)
> [...and LOTS more...]
> ```
> 
> The second example in my previous post was from
> git://git.kernel.org/pub/scm/linux/kernel/git/jolsa/perf.git
> branch perf/metric
> ```
> commit 7dd02cf0b9f04ca5339fa97f9a2280ebdd60b1db (grafted, HEAD -> perf/metric, origin/perf/metric)
> Author: Jiri Olsa <jolsa@kernel.org>
> Date:   Thu Jul 9 13:45:30 2020 +0200
> 
>     perf metric: Rename group_list to list
> ```
> 
> This build of `perf` fails:
> ```
> # ~/install/bin/perf --version
> perf version 5.8.rc4.g7dd02cf0b9f0
> # ~/install/bin/perf stat --metrics cpi_breakdown ./load >/dev/null
> 
>  Performance counter stats for './load':
> 
>      6,729,400,541      pm_run_inst_cmpl          #     0.00 bru_stall_cpi          
>             57,953      pm_cmplu_stall_bru                                          
> 
>        1.127319209 seconds time elapsed
> 
>        1.124906000 seconds user
>        0.000890000 seconds sys
> ```

ok, I can see that as well.. will fix in next version

thanks,
jirka

Re: [PATCH 00/18] perf metric: Add support to reuse metric

[Jiri Olsa]

On Thu, Jul 16, 2020 at 08:50:56AM +0100, John Garry wrote:
> On 12/07/2020 14:26, Jiri Olsa wrote:
> > hi,
> > this patchset is adding the support to reused metric in another
> > metric. The metric needs to be referenced by 'metric:' prefix.
> > 
> > For example, to define IPC by using CPI with change like:
> > 
> >           "BriefDescription": "Instructions Per Cycle (per Logical Processor)",
> >   -       "MetricExpr": "INST_RETIRED.ANY / CPU_CLK_UNHALTED.THREAD",
> >   +       "MetricExpr": "1/metric:CPI",
> >           "MetricGroup": "TopDownL1",
> >           "MetricName": "IPC"
> > 
> > I won't be able to find all the possible places we could
> > use this at, so I wonder you guys (who was asking for this)
> > would try it and come up with comments if there's something
> > missing or we could already use it at some places.
> > 
> > It's based on Arnaldo's tmp.perf/core.
> > 
> > v2 changes:
> >    - collected Ian's acks for few patches [Ian]
> >    - renamed expr__add_id to expr__add_id_val [Ian]
> >    - renamed expr_parse_data to expr_id_data [Ian]
> >    - added recursion check [Ian]
> >    - added metric test for DCache_L2 metric [Ian]
> >    - added some renames as discussed in review [Ian]
> > 
> > Also available in here:
> >    git://git.kernel.org/pub/scm/linux/kernel/git/jolsa/perf.git
> >    perf/metric
> > 
> > thanks,
> > jirka
> > 
> > 
> > ---
> > Jiri Olsa (18):
> >        perf metric: Rename expr__add_id to expr__add_val
> >        perf metric: Add struct expr_id_data to keep expr value
> >        perf metric: Add expr__add_id function
> >        perf metric: Change expr__get_id to return struct expr_id_data
> >        perf metric: Add expr__del_id function
> >        perf metric: Add find_metric function
> >        perf metric: Add add_metric function
> >        perf metric: Rename __metricgroup__add_metric to __add_metric
> >        perf metric: Collect referenced metrics in struct metric_ref_node
> >        perf metric: Collect referenced metrics in struct metric_expr
> >        perf metric: Add referenced metrics to hash data
> >        perf metric: Compute referenced metrics
> >        perf metric: Add events for the current group
> >        perf metric: Add cache_miss_cycles to metric parse test
> >        perf metric: Add DCache_L2 to metric parse test
> >        perf metric: Add recursion check when processing nested metrics
> >        perf metric: Rename struct egroup to metric
> >        perf metric: Rename group_list to list
> 
> I was thinking that a test metric using this reuse feature could be added in
> pmu-events/arch/test/test_cpu. But since no relevant parsing is done in
> jevents, maybe not a lot of value. Just for a bit more completeness.

ok, I think it's a good idea

thanks,
jirka

Re: [PATCH 01/18] perf metric: Rename expr__add_id to expr__add_val

[Arnaldo Carvalho de Melo]

Em Tue, Jul 14, 2020 at 02:05:36PM -0700, Ian Rogers escreveu:
> On Sun, Jul 12, 2020 at 6:26 AM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > Renaming expr__add_id to expr__add_val so we can use
> > expr__add_id to actually add just id without any value
> > in following changes.
> >
> > There's no functional change.
> >
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> 
> Acked-by: Ian Rogers <irogers@google.com>

Thanks, applied.

- Arnaldo
 
> Thanks,
> Ian
> 
> > ---
> >  tools/perf/tests/expr.c       | 4 ++--
> >  tools/perf/tests/pmu-events.c | 4 ++--
> >  tools/perf/util/expr.c        | 2 +-
> >  tools/perf/util/expr.h        | 2 +-
> >  tools/perf/util/expr.y        | 2 +-
> >  tools/perf/util/stat-shadow.c | 4 ++--
> >  6 files changed, 9 insertions(+), 9 deletions(-)
> >
> > diff --git a/tools/perf/tests/expr.c b/tools/perf/tests/expr.c
> > index b7e5ef3007fc..d13fc1dfd5ef 100644
> > --- a/tools/perf/tests/expr.c
> > +++ b/tools/perf/tests/expr.c
> > @@ -24,8 +24,8 @@ int test__expr(struct test *t __maybe_unused, int subtest __maybe_unused)
> >         struct expr_parse_ctx ctx;
> >
> >         expr__ctx_init(&ctx);
> > -       expr__add_id(&ctx, strdup("FOO"), 1);
> > -       expr__add_id(&ctx, strdup("BAR"), 2);
> > +       expr__add_id_val(&ctx, strdup("FOO"), 1);
> > +       expr__add_id_val(&ctx, strdup("BAR"), 2);
> >
> >         ret = test(&ctx, "1+1", 2);
> >         ret |= test(&ctx, "FOO+BAR", 3);
> > diff --git a/tools/perf/tests/pmu-events.c b/tools/perf/tests/pmu-events.c
> > index b66b021476ec..eb19f9a0bc15 100644
> > --- a/tools/perf/tests/pmu-events.c
> > +++ b/tools/perf/tests/pmu-events.c
> > @@ -492,7 +492,7 @@ static int test_parsing(void)
> >                          */
> >                         k = 1;
> >                         hashmap__for_each_entry((&ctx.ids), cur, bkt)
> > -                               expr__add_id(&ctx, strdup(cur->key), k++);
> > +                               expr__add_id_val(&ctx, strdup(cur->key), k++);
> >
> >                         hashmap__for_each_entry((&ctx.ids), cur, bkt) {
> >                                 if (check_parse_cpu(cur->key, map == cpus_map,
> > @@ -547,7 +547,7 @@ static int metric_parse_fake(const char *str)
> >          */
> >         i = 1;
> >         hashmap__for_each_entry((&ctx.ids), cur, bkt)
> > -               expr__add_id(&ctx, strdup(cur->key), i++);
> > +               expr__add_id_val(&ctx, strdup(cur->key), i++);
> >
> >         hashmap__for_each_entry((&ctx.ids), cur, bkt) {
> >                 if (check_parse_fake(cur->key)) {
> > diff --git a/tools/perf/util/expr.c b/tools/perf/util/expr.c
> > index e8f777830a23..9116a3a01eea 100644
> > --- a/tools/perf/util/expr.c
> > +++ b/tools/perf/util/expr.c
> > @@ -33,7 +33,7 @@ static bool key_equal(const void *key1, const void *key2,
> >  }
> >
> >  /* Caller must make sure id is allocated */
> > -int expr__add_id(struct expr_parse_ctx *ctx, const char *name, double val)
> > +int expr__add_id_val(struct expr_parse_ctx *ctx, const char *name, double val)
> >  {
> >         double *val_ptr = NULL, *old_val = NULL;
> >         char *old_key = NULL;
> > diff --git a/tools/perf/util/expr.h b/tools/perf/util/expr.h
> > index 8a2c1074f90f..bb6bac836b48 100644
> > --- a/tools/perf/util/expr.h
> > +++ b/tools/perf/util/expr.h
> > @@ -22,7 +22,7 @@ struct expr_scanner_ctx {
> >
> >  void expr__ctx_init(struct expr_parse_ctx *ctx);
> >  void expr__ctx_clear(struct expr_parse_ctx *ctx);
> > -int expr__add_id(struct expr_parse_ctx *ctx, const char *id, double val);
> > +int expr__add_id_val(struct expr_parse_ctx *ctx, const char *id, double val);
> >  int expr__get_id(struct expr_parse_ctx *ctx, const char *id, double *val_ptr);
> >  int expr__parse(double *final_val, struct expr_parse_ctx *ctx,
> >                 const char *expr, int runtime);
> > diff --git a/tools/perf/util/expr.y b/tools/perf/util/expr.y
> > index 5fcb98800f9c..b2b3420ea6ec 100644
> > --- a/tools/perf/util/expr.y
> > +++ b/tools/perf/util/expr.y
> > @@ -69,7 +69,7 @@ all_other: all_other other
> >
> >  other: ID
> >  {
> > -       expr__add_id(ctx, $1, 0.0);
> > +       expr__add_id_val(ctx, $1, 0.0);
> >  }
> >  |
> >  MIN | MAX | IF | ELSE | SMT_ON | NUMBER | '|' | '^' | '&' | '-' | '+' | '*' | '/' | '%' | '(' | ')' | ','
> > diff --git a/tools/perf/util/stat-shadow.c b/tools/perf/util/stat-shadow.c
> > index 8fdef47005e6..fc9ac4b4218e 100644
> > --- a/tools/perf/util/stat-shadow.c
> > +++ b/tools/perf/util/stat-shadow.c
> > @@ -773,9 +773,9 @@ static int prepare_metric(struct evsel **metric_events,
> >                         *pn = 0;
> >
> >                 if (metric_total)
> > -                       expr__add_id(pctx, n, metric_total);
> > +                       expr__add_id_val(pctx, n, metric_total);
> >                 else
> > -                       expr__add_id(pctx, n, avg_stats(stats)*scale);
> > +                       expr__add_id_val(pctx, n, avg_stats(stats)*scale);
> >         }
> >
> >         return i;
> > --
> > 2.25.4
> >

-- 

- Arnaldo

Re: [PATCH 02/18] perf metric: Add struct expr_id_data to keep expr value

[Arnaldo Carvalho de Melo]

Em Tue, Jul 14, 2020 at 02:06:23PM -0700, Ian Rogers escreveu:
> On Sun, Jul 12, 2020 at 6:26 AM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > Adding struct expr_id_data to keep expr value
> > instead of just simple double pointer, so we can
> > store more data for ID in following changes.
> >
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> 
> Acked-by: Ian Rogers <irogers@google.com>

Thanks, applied.

- Arnaldo
 
> Thanks,
> Ian
> 
> > ---
> >  tools/perf/tests/expr.c       |  3 ++-
> >  tools/perf/util/expr.c        | 22 +++++++++++-----------
> >  tools/perf/util/expr.h        |  4 ++++
> >  tools/perf/util/metricgroup.c |  2 +-
> >  4 files changed, 18 insertions(+), 13 deletions(-)
> >
> > diff --git a/tools/perf/tests/expr.c b/tools/perf/tests/expr.c
> > index d13fc1dfd5ef..4d01051951cd 100644
> > --- a/tools/perf/tests/expr.c
> > +++ b/tools/perf/tests/expr.c
> > @@ -18,8 +18,9 @@ static int test(struct expr_parse_ctx *ctx, const char *e, double val2)
> >
> >  int test__expr(struct test *t __maybe_unused, int subtest __maybe_unused)
> >  {
> > +       struct expr_id_data *val_ptr;
> >         const char *p;
> > -       double val, *val_ptr;
> > +       double val;
> >         int ret;
> >         struct expr_parse_ctx ctx;
> >
> > diff --git a/tools/perf/util/expr.c b/tools/perf/util/expr.c
> > index 9116a3a01eea..5d05f9765ed8 100644
> > --- a/tools/perf/util/expr.c
> > +++ b/tools/perf/util/expr.c
> > @@ -33,32 +33,32 @@ static bool key_equal(const void *key1, const void *key2,
> >  }
> >
> >  /* Caller must make sure id is allocated */
> > -int expr__add_id_val(struct expr_parse_ctx *ctx, const char *name, double val)
> > +int expr__add_id_val(struct expr_parse_ctx *ctx, const char *id, double val)
> >  {
> > -       double *val_ptr = NULL, *old_val = NULL;
> > +       struct expr_id_data *data_ptr = NULL, *old_data = NULL;
> >         char *old_key = NULL;
> >         int ret;
> >
> >         if (val != 0.0) {
> > -               val_ptr = malloc(sizeof(double));
> > -               if (!val_ptr)
> > +               data_ptr = malloc(sizeof(*data_ptr));
> > +               if (!data_ptr)
> >                         return -ENOMEM;
> > -               *val_ptr = val;
> > +               data_ptr->val = val;
> >         }
> > -       ret = hashmap__set(&ctx->ids, name, val_ptr,
> > -                          (const void **)&old_key, (void **)&old_val);
> > +       ret = hashmap__set(&ctx->ids, id, data_ptr,
> > +                          (const void **)&old_key, (void **)&old_data);
> >         free(old_key);
> > -       free(old_val);
> > +       free(old_data);
> >         return ret;
> >  }
> >
> >  int expr__get_id(struct expr_parse_ctx *ctx, const char *id, double *val_ptr)
> >  {
> > -       double *data;
> > +       struct expr_id_data *data;
> >
> >         if (!hashmap__find(&ctx->ids, id, (void **)&data))
> >                 return -1;
> > -       *val_ptr = (data == NULL) ?  0.0 : *data;
> > +       *val_ptr = (data == NULL) ?  0.0 : data->val;
> >         return 0;
> >  }
> >
> > @@ -119,7 +119,7 @@ int expr__parse(double *final_val, struct expr_parse_ctx *ctx,
> >  int expr__find_other(const char *expr, const char *one,
> >                      struct expr_parse_ctx *ctx, int runtime)
> >  {
> > -       double *old_val = NULL;
> > +       struct expr_id_data *old_val = NULL;
> >         char *old_key = NULL;
> >         int ret = __expr__parse(NULL, ctx, expr, EXPR_OTHER, runtime);
> >
> > diff --git a/tools/perf/util/expr.h b/tools/perf/util/expr.h
> > index bb6bac836b48..21fe5bd85718 100644
> > --- a/tools/perf/util/expr.h
> > +++ b/tools/perf/util/expr.h
> > @@ -15,6 +15,10 @@ struct expr_parse_ctx {
> >         struct hashmap ids;
> >  };
> >
> > +struct expr_id_data {
> > +       double  val;
> > +};
> > +
> >  struct expr_scanner_ctx {
> >         int start_token;
> >         int runtime;
> > diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
> > index 82fecb5a302d..df0356ec120d 100644
> > --- a/tools/perf/util/metricgroup.c
> > +++ b/tools/perf/util/metricgroup.c
> > @@ -138,7 +138,7 @@ static struct evsel *find_evsel_group(struct evlist *perf_evlist,
> >                                       unsigned long *evlist_used)
> >  {
> >         struct evsel *ev, *current_leader = NULL;
> > -       double *val_ptr;
> > +       struct expr_id_data *val_ptr;
> >         int i = 0, matched_events = 0, events_to_match;
> >         const int idnum = (int)hashmap__size(&pctx->ids);
> >
> > --
> > 2.25.4
> >

-- 

- Arnaldo

Re: [PATCH 03/18] perf metric: Add expr__add_id function

[Arnaldo Carvalho de Melo]

Em Sun, Jul 12, 2020 at 03:26:19PM +0200, Jiri Olsa escreveu:
> Adding expr__add_id function to data for ID
> with zero value, which is used when scanning
> the expression for IDs.
> 
> Acked-by: Ian Rogers <irogers@google.com>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  tools/perf/util/expr.c | 29 +++++++++++++++++++++++------
>  tools/perf/util/expr.h |  1 +
>  tools/perf/util/expr.y |  2 +-
>  3 files changed, 25 insertions(+), 7 deletions(-)
> 
> diff --git a/tools/perf/util/expr.c b/tools/perf/util/expr.c
> index 5d05f9765ed8..4b8953605aed 100644
> --- a/tools/perf/util/expr.c
> +++ b/tools/perf/util/expr.c
> @@ -32,6 +32,24 @@ static bool key_equal(const void *key1, const void *key2,
>  	return !strcmp((const char *)key1, (const char *)key2);
>  }
>  
> +/* Caller must make sure id is allocated */
> +int expr__add_id(struct expr_parse_ctx *ctx, const char *id)
> +{
> +	struct expr_id_data *data_ptr = NULL, *old_data = NULL;
> +	char *old_key = NULL;
> +	int ret;
> +
> +	data_ptr = malloc(sizeof(*data_ptr));
> +	if (!data_ptr)
> +		return -ENOMEM;
> +
> +	ret = hashmap__set(&ctx->ids, id, data_ptr,
> +			   (const void **)&old_key, (void **)&old_data);
> +	free(old_key);
> +	free(old_data);

Hey, if hashmap__set() fails then we leak data_ptr?

- Arnaldo

> +	return ret;
> +}
> +
>  /* Caller must make sure id is allocated */
>  int expr__add_id_val(struct expr_parse_ctx *ctx, const char *id, double val)
>  {
> @@ -39,12 +57,11 @@ int expr__add_id_val(struct expr_parse_ctx *ctx, const char *id, double val)
>  	char *old_key = NULL;
>  	int ret;
>  
> -	if (val != 0.0) {
> -		data_ptr = malloc(sizeof(*data_ptr));
> -		if (!data_ptr)
> -			return -ENOMEM;
> -		data_ptr->val = val;
> -	}
> +	data_ptr = malloc(sizeof(*data_ptr));
> +	if (!data_ptr)
> +		return -ENOMEM;
> +	data_ptr->val = val;
> +
>  	ret = hashmap__set(&ctx->ids, id, data_ptr,
>  			   (const void **)&old_key, (void **)&old_data);
>  	free(old_key);
> diff --git a/tools/perf/util/expr.h b/tools/perf/util/expr.h
> index 21fe5bd85718..ac32cda42006 100644
> --- a/tools/perf/util/expr.h
> +++ b/tools/perf/util/expr.h
> @@ -26,6 +26,7 @@ struct expr_scanner_ctx {
>  
>  void expr__ctx_init(struct expr_parse_ctx *ctx);
>  void expr__ctx_clear(struct expr_parse_ctx *ctx);
> +int expr__add_id(struct expr_parse_ctx *ctx, const char *id);
>  int expr__add_id_val(struct expr_parse_ctx *ctx, const char *id, double val);
>  int expr__get_id(struct expr_parse_ctx *ctx, const char *id, double *val_ptr);
>  int expr__parse(double *final_val, struct expr_parse_ctx *ctx,
> diff --git a/tools/perf/util/expr.y b/tools/perf/util/expr.y
> index b2b3420ea6ec..8befe4a46f87 100644
> --- a/tools/perf/util/expr.y
> +++ b/tools/perf/util/expr.y
> @@ -69,7 +69,7 @@ all_other: all_other other
>  
>  other: ID
>  {
> -	expr__add_id_val(ctx, $1, 0.0);
> +	expr__add_id(ctx, $1);
>  }
>  |
>  MIN | MAX | IF | ELSE | SMT_ON | NUMBER | '|' | '^' | '&' | '-' | '+' | '*' | '/' | '%' | '(' | ')' | ','
> -- 
> 2.25.4
> 

-- 

- Arnaldo

Re: [PATCH 03/18] perf metric: Add expr__add_id function

[Jiri Olsa]

On Thu, Jul 16, 2020 at 12:29:06PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Sun, Jul 12, 2020 at 03:26:19PM +0200, Jiri Olsa escreveu:
> > Adding expr__add_id function to data for ID
> > with zero value, which is used when scanning
> > the expression for IDs.
> > 
> > Acked-by: Ian Rogers <irogers@google.com>
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> >  tools/perf/util/expr.c | 29 +++++++++++++++++++++++------
> >  tools/perf/util/expr.h |  1 +
> >  tools/perf/util/expr.y |  2 +-
> >  3 files changed, 25 insertions(+), 7 deletions(-)
> > 
> > diff --git a/tools/perf/util/expr.c b/tools/perf/util/expr.c
> > index 5d05f9765ed8..4b8953605aed 100644
> > --- a/tools/perf/util/expr.c
> > +++ b/tools/perf/util/expr.c
> > @@ -32,6 +32,24 @@ static bool key_equal(const void *key1, const void *key2,
> >  	return !strcmp((const char *)key1, (const char *)key2);
> >  }
> >  
> > +/* Caller must make sure id is allocated */
> > +int expr__add_id(struct expr_parse_ctx *ctx, const char *id)
> > +{
> > +	struct expr_id_data *data_ptr = NULL, *old_data = NULL;
> > +	char *old_key = NULL;
> > +	int ret;
> > +
> > +	data_ptr = malloc(sizeof(*data_ptr));
> > +	if (!data_ptr)
> > +		return -ENOMEM;
> > +
> > +	ret = hashmap__set(&ctx->ids, id, data_ptr,
> > +			   (const void **)&old_key, (void **)&old_data);
> > +	free(old_key);
> > +	free(old_data);
> 
> Hey, if hashmap__set() fails then we leak data_ptr?

right, good catch, will fix

thanks,
jirka

Re: [PATCH 11/18] perf metric: Add referenced metrics to hash data

[Jiri Olsa]

On Wed, Jul 15, 2020 at 11:36:09PM +0200, Jiri Olsa wrote:
> On Wed, Jul 15, 2020 at 11:25:14AM -0700, Ian Rogers wrote:
> > On Sun, Jul 12, 2020 at 6:27 AM Jiri Olsa <jolsa@kernel.org> wrote:
> 
> SNIP
> 
> > > +int expr__add_ref(struct expr_parse_ctx *ctx, struct metric_ref *ref)
> > > +{
> > > +       struct expr_id_data *data_ptr = NULL, *old_data = NULL;
> > > +       char *old_key = NULL;
> > > +       char *name;
> > > +       int ret;
> > > +
> > > +       data_ptr = zalloc(sizeof(*data_ptr));
> > > +       if (!data_ptr)
> > > +               return -ENOMEM;
> > > +
> > > +       name = strdup(ref->metric_name);
> > > +       if (!name) {
> > > +               free(data_ptr);
> > > +               return -ENOMEM;
> > > +       }
> > > +
> > > +       data_ptr->ref.metric_name = ref->metric_name;
> > > +       data_ptr->ref.metric_expr = ref->metric_expr;
> > 
> > Having one owned string and one unowned makes the memory management
> > here somewhat complicated. Perhaps dupe both?
> 
> right, will check on this

actualy, both of them are pointers to const char strings from struct pmu_event,
so they don't need to be freed

the journey starts at __add_metric function, where we
get the struct pmu_event pointers:

	ref->metric_name = pe->metric_name;
	ref->metric_expr = pe->metric_expr;

then it's passed to struct metric_ref in metricgroup__setup_events:

	metric_refs[i].metric_name = ref->metric_name;
	metric_refs[i].metric_expr = ref->metric_expr;

still 'const char*'.. and ending up as part of the value in
expr__add_ref function:

	data_ptr->ref.metric_name = ref->metric_name;
	data_ptr->ref.metric_expr = ref->metric_expr;

I'll put comments in all those places so it's evident that it's intentional

jirka

Re: [PATCH 00/18] perf metric: Add support to reuse metric

[Jiri Olsa]

On Thu, Jul 16, 2020 at 01:32:04PM +0200, Jiri Olsa wrote:

SNIP

> > > ---
> > > Jiri Olsa (18):
> > >        perf metric: Rename expr__add_id to expr__add_val
> > >        perf metric: Add struct expr_id_data to keep expr value
> > >        perf metric: Add expr__add_id function
> > >        perf metric: Change expr__get_id to return struct expr_id_data
> > >        perf metric: Add expr__del_id function
> > >        perf metric: Add find_metric function
> > >        perf metric: Add add_metric function
> > >        perf metric: Rename __metricgroup__add_metric to __add_metric
> > >        perf metric: Collect referenced metrics in struct metric_ref_node
> > >        perf metric: Collect referenced metrics in struct metric_expr
> > >        perf metric: Add referenced metrics to hash data
> > >        perf metric: Compute referenced metrics
> > >        perf metric: Add events for the current group
> > >        perf metric: Add cache_miss_cycles to metric parse test
> > >        perf metric: Add DCache_L2 to metric parse test
> > >        perf metric: Add recursion check when processing nested metrics
> > >        perf metric: Rename struct egroup to metric
> > >        perf metric: Rename group_list to list
> > 
> > I was thinking that a test metric using this reuse feature could be added in
> > pmu-events/arch/test/test_cpu. But since no relevant parsing is done in
> > jevents, maybe not a lot of value. Just for a bit more completeness.
> 
> ok, I think it's a good idea

actually those tests treats all IDs in metrics as events,
so what will happen is that the nested metric ID will be
considered as event with assigned value

so I don't see too much value to add nested metrics there,
I think tests/parse-metric.c will cover all metrics usage

jirka