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(-)
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 <[email protected]>
Signed-off-by: Jiri Olsa <[email protected]>
---
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
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 <[email protected]>
Signed-off-by: Jiri Olsa <[email protected]>
---
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
Adding expr__del_id function to remove ID from hashmap.
It will save us few lines in following changes.
Signed-off-by: Jiri Olsa <[email protected]>
---
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
Decouple lookup metric logic into find_metric function,
so it can be used from other places in following changes.
Signed-off-by: Jiri Olsa <[email protected]>
---
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
Decouple metric adding login into add_metric function,
so it can be used from other places in following changes.
Signed-off-by: Jiri Olsa <[email protected]>
---
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
Renaming __metricgroup__add_metric to __add_metric
to fit in the current function names.
Signed-off-by: Jiri Olsa <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
Adding test that compute DCache_L2 metrics with other related metrics in it.
Signed-off-by: Jiri Olsa <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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
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
On Sun, Jul 12, 2020 at 6:27 AM Jiri Olsa <[email protected]> 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 <[email protected]>
Acked-by: Ian Rogers <[email protected]>
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
>
On Sun, Jul 12, 2020 at 6:27 AM Jiri Olsa <[email protected]> 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 <[email protected]>
Acked-by: Ian Rogers <[email protected]>
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
>
On Sun, Jul 12, 2020 at 6:27 AM Jiri Olsa <[email protected]> wrote:
>
> Renaming __metricgroup__add_metric to __add_metric
> to fit in the current function names.
>
> Signed-off-by: Jiri Olsa <[email protected]>
Acked-by: Ian Rogers <[email protected]>
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
>
On Sun, Jul 12, 2020 at 6:27 AM Jiri Olsa <[email protected]> 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 <[email protected]>
Acked-by: Ian Rogers <[email protected]>
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
>
On Sun, Jul 12, 2020 at 6:27 AM Jiri Olsa <[email protected]> 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 <[email protected]>
> ---
> 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
>
On Sun, Jul 12, 2020 at 6:27 AM Jiri Olsa <[email protected]> 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 <[email protected]>
Acked-by: Ian Rogers <[email protected]>
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
>
On Sun, Jul 12, 2020 at 6:27 AM Jiri Olsa <[email protected]> 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 <[email protected]>
Acked-by: Ian Rogers <[email protected]>
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
>
On Sun, Jul 12, 2020 at 6:27 AM Jiri Olsa <[email protected]> wrote:
>
> Adding test that compute DCache_L2 metrics with other related metrics in it.
>
> Signed-off-by: Jiri Olsa <[email protected]>
This feels familiar :-)
Acked-by: Ian Rogers <[email protected]>
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
>
On Sun, Jul 12, 2020 at 6:27 AM Jiri Olsa <[email protected]> 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 <[email protected]>
> ---
> 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
>
On Sun, Jul 12, 2020 at 6:27 AM Jiri Olsa <[email protected]> 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 <[email protected]>
Acked-by: Ian Rogers <[email protected]>
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
>
On Sun, Jul 12, 2020 at 6:27 AM Jiri Olsa <[email protected]> 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 <[email protected]>
> ---
> 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
>
On Sun, Jul 12, 2020 at 6:27 AM Jiri Olsa <[email protected]> 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 <[email protected]>
Awesome, thanks for improving readability!
Acked-by: Ian Rogers <[email protected]>
> ---
> 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
>
On Sun, Jul 12, 2020 at 6:27 AM Jiri Olsa <[email protected]> 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 <[email protected]>
> ---
> 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
>
On Sun, Jul 12, 2020 at 6:27 AM Jiri Olsa <[email protected]> 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 <[email protected]>
> ---
> 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
>
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
On Tue, Jul 14, 2020 at 02:11:02PM -0700, Ian Rogers wrote:
> On Sun, Jul 12, 2020 at 6:27 AM Jiri Olsa <[email protected]> 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 <[email protected]>
>
> Acked-by: Ian Rogers <[email protected]>
thanks,
jirka
On Tue, Jul 14, 2020 at 05:07:04PM -0700, Ian Rogers wrote:
> On Sun, Jul 12, 2020 at 6:27 AM Jiri Olsa <[email protected]> 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 <[email protected]>
> > ---
> > 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
On Wed, Jul 15, 2020 at 10:00:09AM -0700, Ian Rogers wrote:
> On Sun, Jul 12, 2020 at 6:27 AM Jiri Olsa <[email protected]> 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 <[email protected]>
> > ---
> > 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
On Wed, Jul 15, 2020 at 10:40:41AM -0700, Ian Rogers wrote:
> On Sun, Jul 12, 2020 at 6:27 AM Jiri Olsa <[email protected]> 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 <[email protected]>
> > ---
> > 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
On Wed, Jul 15, 2020 at 11:04:08AM -0700, Ian Rogers wrote:
> On Sun, Jul 12, 2020 at 6:27 AM Jiri Olsa <[email protected]> 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
On Wed, Jul 15, 2020 at 11:25:14AM -0700, Ian Rogers wrote:
> On Sun, Jul 12, 2020 at 6:27 AM Jiri Olsa <[email protected]> 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
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
On Wed, Jul 15, 2020 at 2:13 PM Jiri Olsa <[email protected]> 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 <[email protected]> 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 <[email protected]>
> > > ---
> > > 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
>
On Wed, Jul 15, 2020 at 2:19 PM Jiri Olsa <[email protected]> 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 <[email protected]> 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 <[email protected]>
> > > ---
> > > 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
>
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 <[email protected]>
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
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(-)
>
> .
>
On Wed, Jul 15, 2020 at 03:14:37PM -0700, Ian Rogers wrote:
> On Wed, Jul 15, 2020 at 2:19 PM Jiri Olsa <[email protected]> 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 <[email protected]> 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 <[email protected]>
> > > > ---
> > > > 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
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 <[email protected]>
> 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
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
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 <[email protected]>
> Signed-off-by: Jiri Olsa <[email protected]>
> ---
> 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
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 <[email protected]>
> > Signed-off-by: Jiri Olsa <[email protected]>
> > ---
> > 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
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 <[email protected]> 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
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