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.
Also available in here:
git://git.kernel.org/pub/scm/linux/kernel/git/jolsa/perf.git
perf/metric
thanks,
jirka
---
Jiri Olsa (10):
perf tools: Rename expr__add_id to expr__add_val
perf tools: Add struct expr_parse_data to keep expr value
perf tools: Add expr__add_id function
perf tools: Change expr__get_id to return struct expr_parse_data
perf tools: Add expr__del_id function
perf tools: Collect other metrics in struct egroup
perf tools: Collect other metrics in struct metric_expr
perf tools: Add other metrics to hash data
perf tools: Compute other metrics
perf tests: Add cache_miss_cycles to metric parse test
tools/perf/pmu-events/arch/x86/skylake/skl-metrics.json | 2 +-
tools/perf/tests/expr.c | 7 ++--
tools/perf/tests/parse-metric.c | 33 +++++++++++++++++
tools/perf/tests/pmu-events.c | 4 +--
tools/perf/util/expr.c | 115 +++++++++++++++++++++++++++++++++++++++++++++-------------
tools/perf/util/expr.h | 24 +++++++++++--
tools/perf/util/expr.y | 34 ++++++++++++++----
tools/perf/util/metricgroup.c | 187 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--------
tools/perf/util/metricgroup.h | 6 ++++
tools/perf/util/stat-shadow.c | 23 +++++++-----
10 files changed, 374 insertions(+), 61 deletions(-)
Adding struct expr_parse_data to keep expr value
instead of just simple double pointer, so we can
store more data for ID in following changes.
Signed-off-by: Jiri Olsa <[email protected]>
---
tools/perf/tests/expr.c | 3 ++-
tools/perf/util/expr.c | 20 ++++++++++----------
tools/perf/util/expr.h | 4 ++++
tools/perf/util/metricgroup.c | 2 +-
4 files changed, 17 insertions(+), 12 deletions(-)
diff --git a/tools/perf/tests/expr.c b/tools/perf/tests/expr.c
index 82aa32fcab64..e64461f1a24c 100644
--- a/tools/perf/tests/expr.c
+++ b/tools/perf/tests/expr.c
@@ -18,8 +18,9 @@ static int test(struct expr_parse_ctx *ctx, const char *e, double val2)
int test__expr(struct test *t __maybe_unused, int subtest __maybe_unused)
{
+ struct expr_parse_data *val_ptr;
const char *p;
- double val, *val_ptr;
+ double val;
int ret;
struct expr_parse_ctx ctx;
diff --git a/tools/perf/util/expr.c b/tools/perf/util/expr.c
index a02937e5f3ac..7573b21e73df 100644
--- a/tools/perf/util/expr.c
+++ b/tools/perf/util/expr.c
@@ -35,30 +35,30 @@ static bool key_equal(const void *key1, const void *key2,
/* Caller must make sure id is allocated */
int expr__add_val(struct expr_parse_ctx *ctx, const char *name, double val)
{
- double *val_ptr = NULL, *old_val = NULL;
+ struct expr_parse_data *data_ptr = NULL, *old_data = NULL;
char *old_key = NULL;
int ret;
if (val != 0.0) {
- val_ptr = malloc(sizeof(double));
- if (!val_ptr)
+ data_ptr = malloc(sizeof(*data_ptr));
+ if (!data_ptr)
return -ENOMEM;
- *val_ptr = val;
+ data_ptr->val = val;
}
- ret = hashmap__set(&ctx->ids, name, val_ptr,
- (const void **)&old_key, (void **)&old_val);
+ ret = hashmap__set(&ctx->ids, name, data_ptr,
+ (const void **)&old_key, (void **)&old_data);
free(old_key);
- free(old_val);
+ free(old_data);
return ret;
}
int expr__get_id(struct expr_parse_ctx *ctx, const char *id, double *val_ptr)
{
- double *data;
+ struct expr_parse_data *data;
if (!hashmap__find(&ctx->ids, id, (void **)&data))
return -1;
- *val_ptr = (data == NULL) ? 0.0 : *data;
+ *val_ptr = (data == NULL) ? 0.0 : data->val;
return 0;
}
@@ -119,7 +119,7 @@ int expr__parse(double *final_val, struct expr_parse_ctx *ctx,
int expr__find_other(const char *expr, const char *one,
struct expr_parse_ctx *ctx, int runtime)
{
- double *old_val = NULL;
+ struct expr_parse_data *old_val = NULL;
char *old_key = NULL;
int ret = __expr__parse(NULL, ctx, expr, EXPR_OTHER, runtime);
diff --git a/tools/perf/util/expr.h b/tools/perf/util/expr.h
index 35bdc609cf55..f9f16efe76bc 100644
--- a/tools/perf/util/expr.h
+++ b/tools/perf/util/expr.h
@@ -15,6 +15,10 @@ struct expr_parse_ctx {
struct hashmap ids;
};
+struct expr_parse_data {
+ double val;
+};
+
struct expr_scanner_ctx {
int start_token;
int runtime;
diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
index 82fecb5a302d..85e7fa2e2707 100644
--- a/tools/perf/util/metricgroup.c
+++ b/tools/perf/util/metricgroup.c
@@ -138,7 +138,7 @@ static struct evsel *find_evsel_group(struct evlist *perf_evlist,
unsigned long *evlist_used)
{
struct evsel *ev, *current_leader = NULL;
- double *val_ptr;
+ struct expr_parse_data *val_ptr;
int i = 0, matched_events = 0, events_to_match;
const int idnum = (int)hashmap__size(&pctx->ids);
--
2.25.4
Adding expr__add_id function to data for ID
with zero value, which is used when scanning
the expression for IDs.
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 7573b21e73df..0b6d3a6ce88e 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 *name)
+{
+ struct expr_parse_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, name, 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_val(struct expr_parse_ctx *ctx, const char *name, double val)
{
@@ -39,12 +57,11 @@ int expr__add_val(struct expr_parse_ctx *ctx, const char *name, 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, name, 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 f9f16efe76bc..5452e641acf4 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 *name);
int expr__add_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 ff5e5f6e170d..ac4b119877e0 100644
--- a/tools/perf/util/expr.y
+++ b/tools/perf/util/expr.y
@@ -71,7 +71,7 @@ all_other: all_other other
other: ID
{
- expr__add_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_parse_data
pointer as value for the ID. This way we can access data other
than value for given ID in following changes.
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 0b6d3a6ce88e..29cdef18849c 100644
--- a/tools/perf/util/expr.c
+++ b/tools/perf/util/expr.c
@@ -69,14 +69,10 @@ int expr__add_val(struct expr_parse_ctx *ctx, const char *name, 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_parse_data **data)
{
- struct expr_parse_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 5452e641acf4..0af5b887b6c7 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 *name);
int expr__add_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_parse_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 ac4b119877e0..6252d9f6cfc8 100644
--- a/tools/perf/util/expr.y
+++ b/tools/perf/util/expr.y
@@ -87,12 +87,16 @@ if_expr:
;
expr: NUMBER
- | ID { if (expr__get_id(ctx, $1, &$$)) {
- pr_debug("%s not found\n", $1);
+ | ID {
+ struct expr_parse_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 29cdef18849c..aa14c7111ecc 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_parse_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_parse_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 0af5b887b6c7..1a76b002c576 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 *name);
int expr__add_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
Collecting other metrics in struct egroup object,
so we can process them later on.
The change will parse or 'other' metric names out of
expression and 'resolve' them.
Every used expression needs to have 'metric:' prefix,
like:
cache_miss_cycles = metric:dcache_miss_cpi + metric:icache_miss_cycles
All 'other' metrics are disolved into one context,
meaning all 'other' metrics events and addded to
the parent context.
Signed-off-by: Jiri Olsa <[email protected]>
---
.../arch/x86/skylake/skl-metrics.json | 2 +-
tools/perf/util/expr.c | 11 ++
tools/perf/util/expr.h | 1 +
tools/perf/util/metricgroup.c | 158 ++++++++++++++++--
4 files changed, 157 insertions(+), 15 deletions(-)
diff --git a/tools/perf/pmu-events/arch/x86/skylake/skl-metrics.json b/tools/perf/pmu-events/arch/x86/skylake/skl-metrics.json
index 8704efeb8d31..71e5a2b471ac 100644
--- a/tools/perf/pmu-events/arch/x86/skylake/skl-metrics.json
+++ b/tools/perf/pmu-events/arch/x86/skylake/skl-metrics.json
@@ -57,7 +57,7 @@
},
{
"BriefDescription": "Instructions Per Cycle (per Logical Processor)",
- "MetricExpr": "INST_RETIRED.ANY / CPU_CLK_UNHALTED.THREAD",
+ "MetricExpr": "1/metric:CPI",
"MetricGroup": "TopDownL1",
"MetricName": "IPC"
},
diff --git a/tools/perf/util/expr.c b/tools/perf/util/expr.c
index aa14c7111ecc..cd73dae4588c 100644
--- a/tools/perf/util/expr.c
+++ b/tools/perf/util/expr.c
@@ -150,3 +150,14 @@ int expr__find_other(const char *expr, const char *one,
return ret;
}
+
+#define METRIC "metric:"
+
+bool expr__is_metric(const char *name, const char **metric)
+{
+ int ret = !strncmp(name, METRIC, sizeof(METRIC) - 1);
+
+ if (ret && metric)
+ *metric = name + sizeof(METRIC) - 1;
+ return ret;
+}
diff --git a/tools/perf/util/expr.h b/tools/perf/util/expr.h
index 1a76b002c576..fd924bb4e5cd 100644
--- a/tools/perf/util/expr.h
+++ b/tools/perf/util/expr.h
@@ -35,5 +35,6 @@ 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,
struct expr_parse_ctx *ids, int runtime);
+bool expr__is_metric(const char *name, const char **metric);
#endif
diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
index 85e7fa2e2707..f88fd667cc78 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 eother {
+ 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 other;
+ int other_cnt;
int runtime;
bool has_constraint;
};
@@ -574,27 +582,66 @@ int __weak arch_get_runtimeparam(void)
static int __metricgroup__add_metric(struct list_head *group_list,
struct pmu_event *pe,
bool metric_no_group,
- int runtime)
+ int runtime,
+ struct egroup **egp)
{
+ struct eother *eo;
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->other);
+ eg->other_cnt = 0;
+ *egp = eg;
+ } else {
+ /*
+ * We got here for the 'other' metric, via the
+ * recursive metricgroup__add_metric call, add
+ * it to the master group.
+ */
+ eg = *egp;
- 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);
+ eo = malloc(sizeof(*eo));
+ if (!eo)
+ return -ENOMEM;
+ eo->metric_name = pe->metric_name;
+ eo->metric_expr = pe->metric_expr;
+ list_add(&eo->list, &eg->other);
+ eg->other_cnt++;
+ eg->has_constraint |= metricgroup__has_constraint(pe);
+ }
+
+ /*
+ * For both the master and other 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 'other' metric case.
+ */
+ if (eg->other_cnt)
+ return 0;
+
if (list_empty(group_list))
list_add(&eg->nd, group_list);
else {
@@ -617,12 +664,67 @@ static int __metricgroup__add_metric(struct list_head *group_list,
static int metricgroup__add_metric(const char *metric, bool metric_no_group,
struct strbuf *events,
struct list_head *group_list,
- struct pmu_events_map *map)
+ struct pmu_events_map *map,
+ struct egroup *egp);
+
+static int resolve_group(struct egroup *eg,
+ bool metric_no_group,
+ struct strbuf *events,
+ 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 same context - recursive call to
+ * metricgroup__add_metric and remove the metric ID
+ * from the context.
+ */
+ do {
+ all = true;
+ hashmap__for_each_entry((&eg->pctx.ids), cur, bkt) {
+ const char *name;
+ char *m;
+
+ if (!expr__is_metric(cur->key, &name))
+ continue;
+
+ all = false;
+
+ m = strdup(name);
+ if (!m)
+ return -ENOMEM;
+
+ /* The metric:* key itself needs to go out.. */
+ expr__del_id(&eg->pctx, cur->key);
+
+ /* ... and it gets resolved to the parent context. */
+ ret = metricgroup__add_metric(m, metric_no_group, events,
+ group_list, map, eg);
+ if (ret)
+ return ret;
+ break;
+ }
+ } while (!all);
+
+ 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,
+ struct egroup *egp)
{
struct pmu_event *pe;
struct egroup *eg;
int i, ret;
bool has_match = false;
+ bool base = egp == NULL;
for (i = 0; ; i++) {
pe = &map->table[i];
@@ -644,7 +746,7 @@ static int metricgroup__add_metric(const char *metric, bool metric_no_group,
ret = __metricgroup__add_metric(group_list,
pe,
metric_no_group,
- 1);
+ 1, &egp);
if (ret)
return ret;
} else {
@@ -660,13 +762,30 @@ static int metricgroup__add_metric(const char *metric, bool metric_no_group,
for (j = 0; j < count; j++) {
ret = __metricgroup__add_metric(
group_list, pe,
- metric_no_group, j);
+ metric_no_group, j, &egp);
if (ret)
return ret;
}
}
+
+ /*
+ * Process any possible other metrics
+ * included in the expression.
+ */
+ ret = resolve_group(egp, metric_no_group, events,
+ group_list, map);
+ if (ret)
+ return ret;
}
}
+
+ /*
+ * All the IDs are added to the base context,
+ * so we add events only in the 'base' call.
+ */
+ if (!base)
+ return 0;
+
list_for_each_entry(eg, group_list, nd) {
if (events->len > 0)
strbuf_addf(events, ",");
@@ -700,7 +819,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);
+ group_list, map, NULL);
if (ret == -EINVAL) {
fprintf(stderr, "Cannot find metric or group `%s'\n",
p);
@@ -715,11 +834,22 @@ static int metricgroup__add_metric_list(const char *list, bool metric_no_group,
return ret;
}
+static void egroup__free_other(struct egroup *egroup)
+{
+ struct eother *eo, *oetmp;
+
+ list_for_each_entry_safe(eo, oetmp, &egroup->other, list) {
+ list_del(&eo->list);
+ free(eo);
+ }
+}
+
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_other(eg);
expr__ctx_clear(&eg->pctx);
list_del_init(&eg->nd);
free(eg);
--
2.25.4
Adding other metrics to the parsing context so they
can be resolved during the metric processing.
Adding expr__add_other function to store 'other' 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 | 19 +++++++++++++------
3 files changed, 60 insertions(+), 7 deletions(-)
diff --git a/tools/perf/util/expr.c b/tools/perf/util/expr.c
index cd73dae4588c..32f7acac7c19 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_val(struct expr_parse_ctx *ctx, const char *name, double val)
if (!data_ptr)
return -ENOMEM;
data_ptr->val = val;
+ data_ptr->is_other = false;
ret = hashmap__set(&ctx->ids, name, data_ptr,
(const void **)&old_key, (void **)&old_data);
@@ -69,6 +72,38 @@ int expr__add_val(struct expr_parse_ctx *ctx, const char *name, double val)
return ret;
}
+int expr__add_other(struct expr_parse_ctx *ctx, struct metric_other *other)
+{
+ struct expr_parse_data *data_ptr = NULL, *old_data = NULL;
+ char *old_key = NULL;
+ char *name;
+ int ret;
+
+ data_ptr = malloc(sizeof(*data_ptr));
+ if (!data_ptr)
+ return -ENOMEM;
+
+ name = strdup(other->metric_name);
+ if (!name) {
+ free(data_ptr);
+ return -ENOMEM;
+ }
+
+ data_ptr->other.metric_name = other->metric_name;
+ data_ptr->other.metric_expr = other->metric_expr;
+ data_ptr->is_other = true;
+
+ ret = hashmap__set(&ctx->ids, name, data_ptr,
+ (const void **)&old_key, (void **)&old_data);
+
+ pr_debug2("adding other metric %s: %s\n",
+ other->metric_name, other->metric_expr);
+
+ free(old_key);
+ free(old_data);
+ return ret;
+}
+
int expr__get_id(struct expr_parse_ctx *ctx, const char *id,
struct expr_parse_data **data)
{
diff --git a/tools/perf/util/expr.h b/tools/perf/util/expr.h
index fd924bb4e5cd..ed60f9227b43 100644
--- a/tools/perf/util/expr.h
+++ b/tools/perf/util/expr.h
@@ -11,12 +11,22 @@
#include "util/hashmap.h"
//#endif
+struct metric_other;
+
struct expr_parse_ctx {
struct hashmap ids;
};
struct expr_parse_data {
- double val;
+ bool is_other;
+
+ union {
+ double val;
+ struct {
+ const char *metric_name;
+ const char *metric_expr;
+ } other;
+ };
};
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 *name);
int expr__add_val(struct expr_parse_ctx *ctx, const char *id, double val);
+int expr__add_other(struct expr_parse_ctx *ctx, struct metric_other *other);
int expr__get_id(struct expr_parse_ctx *ctx, const char *id,
struct expr_parse_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 0a991016f848..434382410170 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_other *metric_other,
struct expr_parse_ctx *pctx,
int cpu,
struct runtime_stat *st)
{
double scale;
char *n, *pn;
- int i;
+ int i, j;
expr__ctx_init(pctx);
for (i = 0; metric_events[i]; i++) {
@@ -778,12 +779,18 @@ static int prepare_metric(struct evsel **metric_events,
expr__add_val(pctx, n, avg_stats(stats)*scale);
}
+ for (j = 0; metric_other && metric_other[j].metric_name; j++) {
+ if (expr__add_other(pctx, &metric_other[j]))
+ return -ENOMEM;
+ }
+
return i;
}
static void generic_metric(struct perf_stat_config *config,
const char *metric_expr,
struct evsel **metric_events,
+ struct metric_other *metric_other,
char *name,
const char *metric_name,
const char *metric_unit,
@@ -798,7 +805,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_other, &pctx, cpu, st);
if (i < 0)
return;
@@ -847,7 +854,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_other, &pctx, cpu, st) < 0)
return 0.;
if (expr__parse(&ratio, &pctx, mexp->metric_expr, 1))
@@ -1064,8 +1071,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 +1100,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_other, evsel->name, mexp->metric_name,
mexp->metric_unit, mexp->runtime, cpu, out, st);
}
}
--
2.25.4
Adding computation (expr__parse call) of 'other' 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 | 3 +++
tools/perf/util/expr.h | 1 +
tools/perf/util/expr.y | 20 +++++++++++++++++++-
3 files changed, 23 insertions(+), 1 deletion(-)
diff --git a/tools/perf/util/expr.c b/tools/perf/util/expr.c
index 32f7acac7c19..1b6d550cec5f 100644
--- a/tools/perf/util/expr.c
+++ b/tools/perf/util/expr.c
@@ -91,6 +91,7 @@ int expr__add_other(struct expr_parse_ctx *ctx, struct metric_other *other)
data_ptr->other.metric_name = other->metric_name;
data_ptr->other.metric_expr = other->metric_expr;
+ data_ptr->other.counted = false;
data_ptr->is_other = true;
ret = hashmap__set(&ctx->ids, name, data_ptr,
@@ -150,6 +151,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 ed60f9227b43..f85f3941eda5 100644
--- a/tools/perf/util/expr.h
+++ b/tools/perf/util/expr.h
@@ -25,6 +25,7 @@ struct expr_parse_data {
struct {
const char *metric_name;
const char *metric_expr;
+ bool counted;
} other;
};
};
diff --git a/tools/perf/util/expr.y b/tools/perf/util/expr.y
index 6252d9f6cfc8..cca423331f65 100644
--- a/tools/perf/util/expr.y
+++ b/tools/perf/util/expr.y
@@ -89,12 +89,30 @@ if_expr:
expr: NUMBER
| ID {
struct expr_parse_data *data;
+ char *lookup = $1;
+ const char *name;
- if (expr__get_id(ctx, $1, &data) || !data) {
+ if (expr__is_metric($1, &name))
+ lookup = name;
+
+ if (expr__get_id(ctx, lookup, &data) || !data) {
pr_debug("%s not found\n", $1);
free($1);
YYABORT;
}
+
+ pr_debug2("lookup: is_other %d, counted %d: %s\n",
+ data->is_other, data->other.counted, lookup);
+
+ if (data->is_other && !data->other.counted) {
+ data->other.counted = true;
+ if (expr__parse(&data->val, ctx, data->other.metric_expr, 1)) {
+ pr_debug("%s failed to count\n", $1);
+ free($1);
+ YYABORT;
+ }
+ }
+
$$ = data->val;
free($1);
}
--
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..feb97f7c90c8 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 = "(metric:dcache_miss_cpi + metric: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
Add 'other' 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 | 27 +++++++++++++++++++++++++++
tools/perf/util/metricgroup.h | 6 ++++++
2 files changed, 33 insertions(+)
diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
index f88fd667cc78..a5d5dcc1b805 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_other);
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_other *other = NULL;
metric_events = calloc(sizeof(void *),
hashmap__size(&eg->pctx.ids) + 1);
@@ -274,6 +276,31 @@ static int metricgroup__setup_events(struct list_head *groups,
free(metric_events);
break;
}
+
+ /*
+ * Collect and store collected 'other' expressions
+ * for metric processing.
+ */
+ if (eg->other_cnt) {
+ struct eother *eo;
+
+ other = zalloc(sizeof(struct metric_other) * (eg->other_cnt + 1));
+ if (!other) {
+ ret = -ENOMEM;
+ free(metric_events);
+ free(other);
+ break;
+ }
+
+ i = 0;
+ list_for_each_entry(eo, &eg->other, list) {
+ other[i].metric_name = eo->metric_name;
+ other[i].metric_expr = eo->metric_expr;
+ i++;
+ }
+ };
+
+ expr->metric_other = other;
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..3a1e320cb2d3 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_other {
+ 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_other *metric_other;
int runtime;
};
--
2.25.4
On Fri, Jun 26, 2020 at 12:47 PM Jiri Olsa <[email protected]> wrote:
>
> Adding struct expr_parse_data to keep expr value
> instead of just simple double pointer, so we can
> store more data for ID in following changes.
Nit, expr_parse_data sounds a bit like data that is created just at
parse time. Perhaps id_data, for data associated with an id?
Thanks,
Ian
> Signed-off-by: Jiri Olsa <[email protected]>
> ---
> tools/perf/tests/expr.c | 3 ++-
> tools/perf/util/expr.c | 20 ++++++++++----------
> tools/perf/util/expr.h | 4 ++++
> tools/perf/util/metricgroup.c | 2 +-
> 4 files changed, 17 insertions(+), 12 deletions(-)
>
> diff --git a/tools/perf/tests/expr.c b/tools/perf/tests/expr.c
> index 82aa32fcab64..e64461f1a24c 100644
> --- a/tools/perf/tests/expr.c
> +++ b/tools/perf/tests/expr.c
> @@ -18,8 +18,9 @@ static int test(struct expr_parse_ctx *ctx, const char *e, double val2)
>
> int test__expr(struct test *t __maybe_unused, int subtest __maybe_unused)
> {
> + struct expr_parse_data *val_ptr;
> const char *p;
> - double val, *val_ptr;
> + double val;
> int ret;
> struct expr_parse_ctx ctx;
>
> diff --git a/tools/perf/util/expr.c b/tools/perf/util/expr.c
> index a02937e5f3ac..7573b21e73df 100644
> --- a/tools/perf/util/expr.c
> +++ b/tools/perf/util/expr.c
> @@ -35,30 +35,30 @@ static bool key_equal(const void *key1, const void *key2,
> /* Caller must make sure id is allocated */
> int expr__add_val(struct expr_parse_ctx *ctx, const char *name, double val)
> {
> - double *val_ptr = NULL, *old_val = NULL;
> + struct expr_parse_data *data_ptr = NULL, *old_data = NULL;
> char *old_key = NULL;
> int ret;
>
> if (val != 0.0) {
> - val_ptr = malloc(sizeof(double));
> - if (!val_ptr)
> + data_ptr = malloc(sizeof(*data_ptr));
> + if (!data_ptr)
> return -ENOMEM;
> - *val_ptr = val;
> + data_ptr->val = val;
> }
> - ret = hashmap__set(&ctx->ids, name, val_ptr,
> - (const void **)&old_key, (void **)&old_val);
> + ret = hashmap__set(&ctx->ids, name, data_ptr,
> + (const void **)&old_key, (void **)&old_data);
> free(old_key);
> - free(old_val);
> + free(old_data);
> return ret;
> }
>
> int expr__get_id(struct expr_parse_ctx *ctx, const char *id, double *val_ptr)
> {
> - double *data;
> + struct expr_parse_data *data;
>
> if (!hashmap__find(&ctx->ids, id, (void **)&data))
> return -1;
> - *val_ptr = (data == NULL) ? 0.0 : *data;
> + *val_ptr = (data == NULL) ? 0.0 : data->val;
> return 0;
> }
>
> @@ -119,7 +119,7 @@ int expr__parse(double *final_val, struct expr_parse_ctx *ctx,
> int expr__find_other(const char *expr, const char *one,
> struct expr_parse_ctx *ctx, int runtime)
> {
> - double *old_val = NULL;
> + struct expr_parse_data *old_val = NULL;
> char *old_key = NULL;
> int ret = __expr__parse(NULL, ctx, expr, EXPR_OTHER, runtime);
>
> diff --git a/tools/perf/util/expr.h b/tools/perf/util/expr.h
> index 35bdc609cf55..f9f16efe76bc 100644
> --- a/tools/perf/util/expr.h
> +++ b/tools/perf/util/expr.h
> @@ -15,6 +15,10 @@ struct expr_parse_ctx {
> struct hashmap ids;
> };
>
> +struct expr_parse_data {
> + double val;
> +};
> +
> struct expr_scanner_ctx {
> int start_token;
> int runtime;
> diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
> index 82fecb5a302d..85e7fa2e2707 100644
> --- a/tools/perf/util/metricgroup.c
> +++ b/tools/perf/util/metricgroup.c
> @@ -138,7 +138,7 @@ static struct evsel *find_evsel_group(struct evlist *perf_evlist,
> unsigned long *evlist_used)
> {
> struct evsel *ev, *current_leader = NULL;
> - double *val_ptr;
> + struct expr_parse_data *val_ptr;
> int i = 0, matched_events = 0, events_to_match;
> const int idnum = (int)hashmap__size(&pctx->ids);
>
> --
> 2.25.4
>
On Fri, Jun 26, 2020 at 12:47 PM Jiri Olsa <[email protected]> wrote:
>
> Adding expr__add_id function to data for ID
> with zero value, which is used when scanning
> the expression for IDs.
>
> 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 7573b21e73df..0b6d3a6ce88e 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 *name)
Nit, perhaps "id" is more consistent than "name". Perhaps also change
add_val below.
Acked-by: Ian Rogers <[email protected]>
> +{
> + struct expr_parse_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, name, 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_val(struct expr_parse_ctx *ctx, const char *name, double val)
> {
> @@ -39,12 +57,11 @@ int expr__add_val(struct expr_parse_ctx *ctx, const char *name, 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, name, 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 f9f16efe76bc..5452e641acf4 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 *name);
> int expr__add_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 ff5e5f6e170d..ac4b119877e0 100644
> --- a/tools/perf/util/expr.y
> +++ b/tools/perf/util/expr.y
> @@ -71,7 +71,7 @@ all_other: all_other other
>
> other: ID
> {
> - expr__add_val(ctx, $1, 0.0);
> + expr__add_id(ctx, $1);
> }
> |
> MIN | MAX | IF | ELSE | SMT_ON | NUMBER | '|' | '^' | '&' | '-' | '+' | '*' | '/' | '%' | '(' | ')' | ','
> --
> 2.25.4
>
On Fri, Jun 26, 2020 at 12:47 PM Jiri Olsa <[email protected]> wrote:
>
> Changing expr__get_id to use and return struct expr_parse_data
> pointer as value for the ID. This way we can access data other
> than value for given ID in following changes.
>
> 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 0b6d3a6ce88e..29cdef18849c 100644
> --- a/tools/perf/util/expr.c
> +++ b/tools/perf/util/expr.c
> @@ -69,14 +69,10 @@ int expr__add_val(struct expr_parse_ctx *ctx, const char *name, 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_parse_data **data)
> {
> - struct expr_parse_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;
> }
Nit, the 0 vs -1 here is a bit sad given hashmap__find is using true
to mean present. I'm also guilty of not trying to clean this up.
Acked-by: Ian Rogers <[email protected]>
> void expr__ctx_init(struct expr_parse_ctx *ctx)
> diff --git a/tools/perf/util/expr.h b/tools/perf/util/expr.h
> index 5452e641acf4..0af5b887b6c7 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 *name);
> int expr__add_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_parse_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 ac4b119877e0..6252d9f6cfc8 100644
> --- a/tools/perf/util/expr.y
> +++ b/tools/perf/util/expr.y
> @@ -87,12 +87,16 @@ if_expr:
> ;
>
> expr: NUMBER
> - | ID { if (expr__get_id(ctx, $1, &$$)) {
> - pr_debug("%s not found\n", $1);
> + | ID {
> + struct expr_parse_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
>
On Fri, Jun 26, 2020 at 12:47 PM 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]>
> ---
> 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 29cdef18849c..aa14c7111ecc 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_parse_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_parse_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);
Nit, I always have to read the code to know why we have "one" as an
argument. Could we remove it as an argument and have the caller use
expr__del_id?
Thanks,
Ian
> return ret;
> }
> diff --git a/tools/perf/util/expr.h b/tools/perf/util/expr.h
> index 0af5b887b6c7..1a76b002c576 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 *name);
> int expr__add_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 Fri, Jun 26, 2020 at 12:47 PM Jiri Olsa <[email protected]> wrote:
>
> Collecting other metrics in struct egroup object,
> so we can process them later on.
>
> The change will parse or 'other' metric names out of
> expression and 'resolve' them.
>
> Every used expression needs to have 'metric:' prefix,
> like:
> cache_miss_cycles = metric:dcache_miss_cpi + metric:icache_miss_cycles
>
> All 'other' metrics are disolved into one context,
> meaning all 'other' metrics events and addded to
> the parent context.
>
> Signed-off-by: Jiri Olsa <[email protected]>
> ---
> .../arch/x86/skylake/skl-metrics.json | 2 +-
> tools/perf/util/expr.c | 11 ++
> tools/perf/util/expr.h | 1 +
> tools/perf/util/metricgroup.c | 158 ++++++++++++++++--
> 4 files changed, 157 insertions(+), 15 deletions(-)
>
> diff --git a/tools/perf/pmu-events/arch/x86/skylake/skl-metrics.json b/tools/perf/pmu-events/arch/x86/skylake/skl-metrics.json
> index 8704efeb8d31..71e5a2b471ac 100644
> --- a/tools/perf/pmu-events/arch/x86/skylake/skl-metrics.json
> +++ b/tools/perf/pmu-events/arch/x86/skylake/skl-metrics.json
> @@ -57,7 +57,7 @@
> },
> {
> "BriefDescription": "Instructions Per Cycle (per Logical Processor)",
> - "MetricExpr": "INST_RETIRED.ANY / CPU_CLK_UNHALTED.THREAD",
> + "MetricExpr": "1/metric:CPI",
> "MetricGroup": "TopDownL1",
> "MetricName": "IPC"
> },
> diff --git a/tools/perf/util/expr.c b/tools/perf/util/expr.c
> index aa14c7111ecc..cd73dae4588c 100644
> --- a/tools/perf/util/expr.c
> +++ b/tools/perf/util/expr.c
> @@ -150,3 +150,14 @@ int expr__find_other(const char *expr, const char *one,
>
> return ret;
> }
> +
> +#define METRIC "metric:"
> +
> +bool expr__is_metric(const char *name, const char **metric)
> +{
> + int ret = !strncmp(name, METRIC, sizeof(METRIC) - 1);
> +
> + if (ret && metric)
> + *metric = name + sizeof(METRIC) - 1;
> + return ret;
> +}
> diff --git a/tools/perf/util/expr.h b/tools/perf/util/expr.h
> index 1a76b002c576..fd924bb4e5cd 100644
> --- a/tools/perf/util/expr.h
> +++ b/tools/perf/util/expr.h
> @@ -35,5 +35,6 @@ 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,
> struct expr_parse_ctx *ids, int runtime);
> +bool expr__is_metric(const char *name, const char **metric);
>
> #endif
> diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
> index 85e7fa2e2707..f88fd667cc78 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 eother {
> + 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 other;
> + int other_cnt;
> int runtime;
> bool has_constraint;
> };
Nit, could we do better than egroup and eother for struct names?
egroup are nodes within the metric group with their associated values,
so would metric_group_node be more intention revealing? eother and
other are metrics referred to by the metric_group_node. Presumably the
metrics are on the same list as egroup? Perhaps eother could be
referenced_metrics?
Thanks,
Ian
> @@ -574,27 +582,66 @@ int __weak arch_get_runtimeparam(void)
> static int __metricgroup__add_metric(struct list_head *group_list,
> struct pmu_event *pe,
> bool metric_no_group,
> - int runtime)
> + int runtime,
> + struct egroup **egp)
> {
> + struct eother *eo;
> 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->other);
> + eg->other_cnt = 0;
> + *egp = eg;
> + } else {
> + /*
> + * We got here for the 'other' metric, via the
> + * recursive metricgroup__add_metric call, add
> + * it to the master group.
> + */
> + eg = *egp;
>
> - 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);
> + eo = malloc(sizeof(*eo));
> + if (!eo)
> + return -ENOMEM;
>
> + eo->metric_name = pe->metric_name;
> + eo->metric_expr = pe->metric_expr;
> + list_add(&eo->list, &eg->other);
> + eg->other_cnt++;
> + eg->has_constraint |= metricgroup__has_constraint(pe);
> + }
> +
> + /*
> + * For both the master and other 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 'other' metric case.
> + */
> + if (eg->other_cnt)
> + return 0;
> +
> if (list_empty(group_list))
> list_add(&eg->nd, group_list);
> else {
> @@ -617,12 +664,67 @@ static int __metricgroup__add_metric(struct list_head *group_list,
> static int metricgroup__add_metric(const char *metric, bool metric_no_group,
> struct strbuf *events,
> struct list_head *group_list,
> - struct pmu_events_map *map)
> + struct pmu_events_map *map,
> + struct egroup *egp);
> +
> +static int resolve_group(struct egroup *eg,
> + bool metric_no_group,
> + struct strbuf *events,
> + 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 same context - recursive call to
> + * metricgroup__add_metric and remove the metric ID
> + * from the context.
> + */
> + do {
> + all = true;
> + hashmap__for_each_entry((&eg->pctx.ids), cur, bkt) {
> + const char *name;
> + char *m;
> +
> + if (!expr__is_metric(cur->key, &name))
> + continue;
> +
> + all = false;
> +
> + m = strdup(name);
> + if (!m)
> + return -ENOMEM;
> +
> + /* The metric:* key itself needs to go out.. */
> + expr__del_id(&eg->pctx, cur->key);
> +
> + /* ... and it gets resolved to the parent context. */
> + ret = metricgroup__add_metric(m, metric_no_group, events,
> + group_list, map, eg);
> + if (ret)
> + return ret;
> + break;
> + }
> + } while (!all);
> +
> + 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,
> + struct egroup *egp)
> {
> struct pmu_event *pe;
> struct egroup *eg;
> int i, ret;
> bool has_match = false;
> + bool base = egp == NULL;
>
> for (i = 0; ; i++) {
> pe = &map->table[i];
> @@ -644,7 +746,7 @@ static int metricgroup__add_metric(const char *metric, bool metric_no_group,
> ret = __metricgroup__add_metric(group_list,
> pe,
> metric_no_group,
> - 1);
> + 1, &egp);
> if (ret)
> return ret;
> } else {
> @@ -660,13 +762,30 @@ static int metricgroup__add_metric(const char *metric, bool metric_no_group,
> for (j = 0; j < count; j++) {
> ret = __metricgroup__add_metric(
> group_list, pe,
> - metric_no_group, j);
> + metric_no_group, j, &egp);
> if (ret)
> return ret;
> }
> }
> +
> + /*
> + * Process any possible other metrics
> + * included in the expression.
> + */
> + ret = resolve_group(egp, metric_no_group, events,
> + group_list, map);
> + if (ret)
> + return ret;
> }
> }
> +
> + /*
> + * All the IDs are added to the base context,
> + * so we add events only in the 'base' call.
> + */
> + if (!base)
> + return 0;
> +
> list_for_each_entry(eg, group_list, nd) {
> if (events->len > 0)
> strbuf_addf(events, ",");
> @@ -700,7 +819,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);
> + group_list, map, NULL);
> if (ret == -EINVAL) {
> fprintf(stderr, "Cannot find metric or group `%s'\n",
> p);
> @@ -715,11 +834,22 @@ static int metricgroup__add_metric_list(const char *list, bool metric_no_group,
> return ret;
> }
>
> +static void egroup__free_other(struct egroup *egroup)
> +{
> + struct eother *eo, *oetmp;
> +
> + list_for_each_entry_safe(eo, oetmp, &egroup->other, list) {
> + list_del(&eo->list);
> + free(eo);
> + }
> +}
> +
> 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_other(eg);
> expr__ctx_clear(&eg->pctx);
> list_del_init(&eg->nd);
> free(eg);
> --
> 2.25.4
>
On Fri, Jun 26, 2020 at 12:48 PM Jiri Olsa <[email protected]> wrote:
>
> Add 'other' 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.
Nit, other vs something like referenced_metric but otherwise lgtm.
Acked-by: Ian Rogers
> Signed-off-by: Jiri Olsa <[email protected]>
> ---
> tools/perf/util/metricgroup.c | 27 +++++++++++++++++++++++++++
> tools/perf/util/metricgroup.h | 6 ++++++
> 2 files changed, 33 insertions(+)
>
> diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
> index f88fd667cc78..a5d5dcc1b805 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_other);
> 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_other *other = NULL;
>
> metric_events = calloc(sizeof(void *),
> hashmap__size(&eg->pctx.ids) + 1);
> @@ -274,6 +276,31 @@ static int metricgroup__setup_events(struct list_head *groups,
> free(metric_events);
> break;
> }
> +
> + /*
> + * Collect and store collected 'other' expressions
> + * for metric processing.
> + */
> + if (eg->other_cnt) {
> + struct eother *eo;
> +
> + other = zalloc(sizeof(struct metric_other) * (eg->other_cnt + 1));
> + if (!other) {
> + ret = -ENOMEM;
> + free(metric_events);
> + free(other);
> + break;
> + }
> +
> + i = 0;
> + list_for_each_entry(eo, &eg->other, list) {
> + other[i].metric_name = eo->metric_name;
> + other[i].metric_expr = eo->metric_expr;
> + i++;
> + }
> + };
> +
> + expr->metric_other = other;
> 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..3a1e320cb2d3 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_other {
> + 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_other *metric_other;
> int runtime;
> };
>
> --
> 2.25.4
>
On Fri, Jun 26, 2020 at 12:48 PM Jiri Olsa <[email protected]> wrote:
>
> Adding other metrics to the parsing context so they
> can be resolved during the metric processing.
>
> Adding expr__add_other function to store 'other' 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 | 19 +++++++++++++------
> 3 files changed, 60 insertions(+), 7 deletions(-)
>
> diff --git a/tools/perf/util/expr.c b/tools/perf/util/expr.c
> index cd73dae4588c..32f7acac7c19 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_val(struct expr_parse_ctx *ctx, const char *name, double val)
> if (!data_ptr)
> return -ENOMEM;
> data_ptr->val = val;
> + data_ptr->is_other = false;
>
> ret = hashmap__set(&ctx->ids, name, data_ptr,
> (const void **)&old_key, (void **)&old_data);
> @@ -69,6 +72,38 @@ int expr__add_val(struct expr_parse_ctx *ctx, const char *name, double val)
> return ret;
> }
>
> +int expr__add_other(struct expr_parse_ctx *ctx, struct metric_other *other)
> +{
> + struct expr_parse_data *data_ptr = NULL, *old_data = NULL;
> + char *old_key = NULL;
> + char *name;
> + int ret;
> +
> + data_ptr = malloc(sizeof(*data_ptr));
> + if (!data_ptr)
> + return -ENOMEM;
> +
> + name = strdup(other->metric_name);
> + if (!name) {
> + free(data_ptr);
> + return -ENOMEM;
> + }
> +
> + data_ptr->other.metric_name = other->metric_name;
> + data_ptr->other.metric_expr = other->metric_expr;
> + data_ptr->is_other = true;
> +
> + ret = hashmap__set(&ctx->ids, name, data_ptr,
> + (const void **)&old_key, (void **)&old_data);
> +
> + pr_debug2("adding other metric %s: %s\n",
> + other->metric_name, other->metric_expr);
> +
> + free(old_key);
> + free(old_data);
> + return ret;
> +}
> +
> int expr__get_id(struct expr_parse_ctx *ctx, const char *id,
> struct expr_parse_data **data)
> {
> diff --git a/tools/perf/util/expr.h b/tools/perf/util/expr.h
> index fd924bb4e5cd..ed60f9227b43 100644
> --- a/tools/perf/util/expr.h
> +++ b/tools/perf/util/expr.h
> @@ -11,12 +11,22 @@
> #include "util/hashmap.h"
> //#endif
>
> +struct metric_other;
> +
> struct expr_parse_ctx {
> struct hashmap ids;
> };
>
> struct expr_parse_data {
> - double val;
> + bool is_other;
> +
> + union {
> + double val;
> + struct {
> + const char *metric_name;
> + const char *metric_expr;
It is probably worth a comment why both the metric_name and the
metric's expression are required here? The parse and other data for
the metric won't be here.
Thanks,
Ian
> + } other;
> + };
> };
>
> 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 *name);
> int expr__add_val(struct expr_parse_ctx *ctx, const char *id, double val);
> +int expr__add_other(struct expr_parse_ctx *ctx, struct metric_other *other);
> int expr__get_id(struct expr_parse_ctx *ctx, const char *id,
> struct expr_parse_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 0a991016f848..434382410170 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_other *metric_other,
> struct expr_parse_ctx *pctx,
> int cpu,
> struct runtime_stat *st)
> {
> double scale;
> char *n, *pn;
> - int i;
> + int i, j;
>
> expr__ctx_init(pctx);
> for (i = 0; metric_events[i]; i++) {
> @@ -778,12 +779,18 @@ static int prepare_metric(struct evsel **metric_events,
> expr__add_val(pctx, n, avg_stats(stats)*scale);
> }
>
> + for (j = 0; metric_other && metric_other[j].metric_name; j++) {
> + if (expr__add_other(pctx, &metric_other[j]))
> + return -ENOMEM;
> + }
> +
> return i;
> }
>
> static void generic_metric(struct perf_stat_config *config,
> const char *metric_expr,
> struct evsel **metric_events,
> + struct metric_other *metric_other,
> char *name,
> const char *metric_name,
> const char *metric_unit,
> @@ -798,7 +805,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_other, &pctx, cpu, st);
> if (i < 0)
> return;
>
> @@ -847,7 +854,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_other, &pctx, cpu, st) < 0)
> return 0.;
>
> if (expr__parse(&ratio, &pctx, mexp->metric_expr, 1))
> @@ -1064,8 +1071,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 +1100,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_other, evsel->name, mexp->metric_name,
> mexp->metric_unit, mexp->runtime, cpu, out, st);
> }
> }
> --
> 2.25.4
>
On Fri, Jun 26, 2020 at 12:48 PM Jiri Olsa <[email protected]> wrote:
>
> Adding computation (expr__parse call) of 'other' 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 | 3 +++
> tools/perf/util/expr.h | 1 +
> tools/perf/util/expr.y | 20 +++++++++++++++++++-
> 3 files changed, 23 insertions(+), 1 deletion(-)
>
> diff --git a/tools/perf/util/expr.c b/tools/perf/util/expr.c
> index 32f7acac7c19..1b6d550cec5f 100644
> --- a/tools/perf/util/expr.c
> +++ b/tools/perf/util/expr.c
> @@ -91,6 +91,7 @@ int expr__add_other(struct expr_parse_ctx *ctx, struct metric_other *other)
>
> data_ptr->other.metric_name = other->metric_name;
> data_ptr->other.metric_expr = other->metric_expr;
> + data_ptr->other.counted = false;
> data_ptr->is_other = true;
>
> ret = hashmap__set(&ctx->ids, name, data_ptr,
> @@ -150,6 +151,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 ed60f9227b43..f85f3941eda5 100644
> --- a/tools/perf/util/expr.h
> +++ b/tools/perf/util/expr.h
> @@ -25,6 +25,7 @@ struct expr_parse_data {
> struct {
> const char *metric_name;
> const char *metric_expr;
> + bool counted;
> } other;
> };
> };
> diff --git a/tools/perf/util/expr.y b/tools/perf/util/expr.y
> index 6252d9f6cfc8..cca423331f65 100644
> --- a/tools/perf/util/expr.y
> +++ b/tools/perf/util/expr.y
> @@ -89,12 +89,30 @@ if_expr:
> expr: NUMBER
> | ID {
> struct expr_parse_data *data;
> + char *lookup = $1;
> + const char *name;
>
> - if (expr__get_id(ctx, $1, &data) || !data) {
> + if (expr__is_metric($1, &name))
> + lookup = name;
> +
> + if (expr__get_id(ctx, lookup, &data) || !data) {
> pr_debug("%s not found\n", $1);
> free($1);
> YYABORT;
> }
> +
> + pr_debug2("lookup: is_other %d, counted %d: %s\n",
> + data->is_other, data->other.counted, lookup);
> +
> + if (data->is_other && !data->other.counted) {
> + data->other.counted = true;
> + if (expr__parse(&data->val, ctx, data->other.metric_expr, 1)) {
Ah, so this handles the problem the referenced metric isn't calculated
and calculates it - with the sharing of events this doesn't impose
extra pmu cost. Do we need to worry about detecting recursion? For
example:
"MetricName": "Foo",
"MetricExpr": "1/metric:Foo",
It seems unfortunate to have the MetricExpr calculated twice, but it
is understandable. Is it also a property that referenced/other metrics
won't be reported individually? Perhaps these are sub-metrics?
Thanks,
Ian
> + pr_debug("%s failed to count\n", $1);
> + free($1);
> + YYABORT;
> + }
> + }
> +
> $$ = data->val;
> free($1);
> }
> --
> 2.25.4
>
On Fri, Jun 26, 2020 at 09:47:10PM +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.
Why is the prefix needed?
Could just look it up without prefix.
-Andi
On Fri, Jun 26, 2020 at 12:48 PM 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
>
This is really nice! Do we need to do anything in tests/pmu-events.c?
Presumably if a metric is referencing another metric the call to
expr__parse there will test the other metric is parseable. Just wanted
to sanity check.
Acked-by: Ian Rogers <[email protected]>
> 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..feb97f7c90c8 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 = "(metric:dcache_miss_cpi + metric: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 Fri, Jun 26, 2020 at 2:25 PM Andi Kleen <[email protected]> wrote:
>
> On Fri, Jun 26, 2020 at 09:47:10PM +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.
>
> Why is the prefix needed?
>
> Could just look it up without prefix.
The name could be a metric or an event, the logic for each is quite
different. You could look up an event and when it fails assume it was
a metric, but I like the simplicity of this approach. Maybe this
change could be adopted more widely with something like "perf stat -e
metric:IPC -a -I 1000" rather than the current "perf stat -M IPC -a -I
1000".
Thanks,
Ian
> -Andi
On Fri, Jun 26, 2020 at 12:47 PM Jiri Olsa <[email protected]> wrote:
>
> Collecting other metrics in struct egroup object,
> so we can process them later on.
>
> The change will parse or 'other' metric names out of
> expression and 'resolve' them.
>
> Every used expression needs to have 'metric:' prefix,
> like:
> cache_miss_cycles = metric:dcache_miss_cpi + metric:icache_miss_cycles
>
> All 'other' metrics are disolved into one context,
> meaning all 'other' metrics events and addded to
> the parent context.
>
> Signed-off-by: Jiri Olsa <[email protected]>
> ---
> .../arch/x86/skylake/skl-metrics.json | 2 +-
> tools/perf/util/expr.c | 11 ++
> tools/perf/util/expr.h | 1 +
> tools/perf/util/metricgroup.c | 158 ++++++++++++++++--
> 4 files changed, 157 insertions(+), 15 deletions(-)
>
> diff --git a/tools/perf/pmu-events/arch/x86/skylake/skl-metrics.json b/tools/perf/pmu-events/arch/x86/skylake/skl-metrics.json
> index 8704efeb8d31..71e5a2b471ac 100644
> --- a/tools/perf/pmu-events/arch/x86/skylake/skl-metrics.json
> +++ b/tools/perf/pmu-events/arch/x86/skylake/skl-metrics.json
> @@ -57,7 +57,7 @@
> },
> {
> "BriefDescription": "Instructions Per Cycle (per Logical Processor)",
> - "MetricExpr": "INST_RETIRED.ANY / CPU_CLK_UNHALTED.THREAD",
> + "MetricExpr": "1/metric:CPI",
> "MetricGroup": "TopDownL1",
> "MetricName": "IPC"
> },
> diff --git a/tools/perf/util/expr.c b/tools/perf/util/expr.c
> index aa14c7111ecc..cd73dae4588c 100644
> --- a/tools/perf/util/expr.c
> +++ b/tools/perf/util/expr.c
> @@ -150,3 +150,14 @@ int expr__find_other(const char *expr, const char *one,
>
> return ret;
> }
> +
> +#define METRIC "metric:"
> +
> +bool expr__is_metric(const char *name, const char **metric)
> +{
> + int ret = !strncmp(name, METRIC, sizeof(METRIC) - 1);
> +
> + if (ret && metric)
> + *metric = name + sizeof(METRIC) - 1;
> + return ret;
> +}
Should expr.l recognize metric:... as a different kind of token rather
than an ID?
Thanks,
Ian
> diff --git a/tools/perf/util/expr.h b/tools/perf/util/expr.h
> index 1a76b002c576..fd924bb4e5cd 100644
> --- a/tools/perf/util/expr.h
> +++ b/tools/perf/util/expr.h
> @@ -35,5 +35,6 @@ 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,
> struct expr_parse_ctx *ids, int runtime);
> +bool expr__is_metric(const char *name, const char **metric);
>
> #endif
> diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
> index 85e7fa2e2707..f88fd667cc78 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 eother {
> + 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 other;
> + int other_cnt;
> int runtime;
> bool has_constraint;
> };
> @@ -574,27 +582,66 @@ int __weak arch_get_runtimeparam(void)
> static int __metricgroup__add_metric(struct list_head *group_list,
> struct pmu_event *pe,
> bool metric_no_group,
> - int runtime)
> + int runtime,
> + struct egroup **egp)
> {
> + struct eother *eo;
> 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->other);
> + eg->other_cnt = 0;
> + *egp = eg;
> + } else {
> + /*
> + * We got here for the 'other' metric, via the
> + * recursive metricgroup__add_metric call, add
> + * it to the master group.
> + */
> + eg = *egp;
>
> - 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);
> + eo = malloc(sizeof(*eo));
> + if (!eo)
> + return -ENOMEM;
>
> + eo->metric_name = pe->metric_name;
> + eo->metric_expr = pe->metric_expr;
> + list_add(&eo->list, &eg->other);
> + eg->other_cnt++;
> + eg->has_constraint |= metricgroup__has_constraint(pe);
> + }
> +
> + /*
> + * For both the master and other 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 'other' metric case.
> + */
> + if (eg->other_cnt)
> + return 0;
> +
> if (list_empty(group_list))
> list_add(&eg->nd, group_list);
> else {
> @@ -617,12 +664,67 @@ static int __metricgroup__add_metric(struct list_head *group_list,
> static int metricgroup__add_metric(const char *metric, bool metric_no_group,
> struct strbuf *events,
> struct list_head *group_list,
> - struct pmu_events_map *map)
> + struct pmu_events_map *map,
> + struct egroup *egp);
> +
> +static int resolve_group(struct egroup *eg,
> + bool metric_no_group,
> + struct strbuf *events,
> + 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 same context - recursive call to
> + * metricgroup__add_metric and remove the metric ID
> + * from the context.
> + */
> + do {
> + all = true;
> + hashmap__for_each_entry((&eg->pctx.ids), cur, bkt) {
> + const char *name;
> + char *m;
> +
> + if (!expr__is_metric(cur->key, &name))
> + continue;
> +
> + all = false;
> +
> + m = strdup(name);
> + if (!m)
> + return -ENOMEM;
> +
> + /* The metric:* key itself needs to go out.. */
> + expr__del_id(&eg->pctx, cur->key);
> +
> + /* ... and it gets resolved to the parent context. */
> + ret = metricgroup__add_metric(m, metric_no_group, events,
> + group_list, map, eg);
> + if (ret)
> + return ret;
> + break;
> + }
> + } while (!all);
> +
> + 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,
> + struct egroup *egp)
> {
> struct pmu_event *pe;
> struct egroup *eg;
> int i, ret;
> bool has_match = false;
> + bool base = egp == NULL;
>
> for (i = 0; ; i++) {
> pe = &map->table[i];
> @@ -644,7 +746,7 @@ static int metricgroup__add_metric(const char *metric, bool metric_no_group,
> ret = __metricgroup__add_metric(group_list,
> pe,
> metric_no_group,
> - 1);
> + 1, &egp);
> if (ret)
> return ret;
> } else {
> @@ -660,13 +762,30 @@ static int metricgroup__add_metric(const char *metric, bool metric_no_group,
> for (j = 0; j < count; j++) {
> ret = __metricgroup__add_metric(
> group_list, pe,
> - metric_no_group, j);
> + metric_no_group, j, &egp);
> if (ret)
> return ret;
> }
> }
> +
> + /*
> + * Process any possible other metrics
> + * included in the expression.
> + */
> + ret = resolve_group(egp, metric_no_group, events,
> + group_list, map);
> + if (ret)
> + return ret;
> }
> }
> +
> + /*
> + * All the IDs are added to the base context,
> + * so we add events only in the 'base' call.
> + */
> + if (!base)
> + return 0;
> +
> list_for_each_entry(eg, group_list, nd) {
> if (events->len > 0)
> strbuf_addf(events, ",");
> @@ -700,7 +819,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);
> + group_list, map, NULL);
> if (ret == -EINVAL) {
> fprintf(stderr, "Cannot find metric or group `%s'\n",
> p);
> @@ -715,11 +834,22 @@ static int metricgroup__add_metric_list(const char *list, bool metric_no_group,
> return ret;
> }
>
> +static void egroup__free_other(struct egroup *egroup)
> +{
> + struct eother *eo, *oetmp;
> +
> + list_for_each_entry_safe(eo, oetmp, &egroup->other, list) {
> + list_del(&eo->list);
> + free(eo);
> + }
> +}
> +
> 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_other(eg);
> expr__ctx_clear(&eg->pctx);
> list_del_init(&eg->nd);
> free(eg);
> --
> 2.25.4
>
> The name could be a metric or an event, the logic for each is quite
I would say collisions are unlikely. Event names follow quite structured
patterns.
> different. You could look up an event and when it fails assume it was
> a metric, but I like the simplicity of this approach.
I don't think it's simpler for the user.
> Maybe this
> change could be adopted more widely with something like "perf stat -e
> metric:IPC -a -I 1000" rather than the current "perf stat -M IPC -a -I
> 1000".
I thought about just adding metrics to -e, without metric: of course.
-Andi
On 26/06/2020 20:47, 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.
>
I notice that there is much repetition in the x86 metric JSONs between CPUs.
So I know it's not the same as what you propose here, but jevents
standard arch events feature could be used to reduce the repetition.
I'm guessing that those metric JSONs are human generated, so would be
suitable; unlike the regular JSONs, which are automated.
Thanks,
John
> 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.
>
> Also available in here:
> git://git.kernel.org/pub/scm/linux/kernel/git/jolsa/perf.git
> perf/metric
>
> thanks,
> jirka
>
>
> ---
> Jiri Olsa (10):
> perf tools: Rename expr__add_id to expr__add_val
> perf tools: Add struct expr_parse_data to keep expr value
> perf tools: Add expr__add_id function
> perf tools: Change expr__get_id to return struct expr_parse_data
> perf tools: Add expr__del_id function
> perf tools: Collect other metrics in struct egroup
> perf tools: Collect other metrics in struct metric_expr
> perf tools: Add other metrics to hash data
> perf tools: Compute other metrics
> perf tests: Add cache_miss_cycles to metric parse test
>
> tools/perf/pmu-events/arch/x86/skylake/skl-metrics.json | 2 +-
> tools/perf/tests/expr.c | 7 ++--
> tools/perf/tests/parse-metric.c | 33 +++++++++++++++++
> tools/perf/tests/pmu-events.c | 4 +--
> tools/perf/util/expr.c | 115 +++++++++++++++++++++++++++++++++++++++++++++-------------
> tools/perf/util/expr.h | 24 +++++++++++--
> tools/perf/util/expr.y | 34 ++++++++++++++----
> tools/perf/util/metricgroup.c | 187 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--------
> tools/perf/util/metricgroup.h | 6 ++++
> tools/perf/util/stat-shadow.c | 23 +++++++-----
> 10 files changed, 374 insertions(+), 61 deletions(-)
>
> .
>
Em Fri, Jun 26, 2020 at 02:57:59PM -0700, Andi Kleen escreveu:
> > The name could be a metric or an event, the logic for each is quite
>
> I would say collisions are unlikely. Event names follow quite structured
> patterns.
And when introducing a new metric the build process can detect that
clash and fail.
> > different. You could look up an event and when it fails assume it was
> > a metric, but I like the simplicity of this approach.
> I don't think it's simpler for the user.
Agreed.
> > Maybe this
> > change could be adopted more widely with something like "perf stat -e
> > metric:IPC -a -I 1000" rather than the current "perf stat -M IPC -a -I
> > 1000".
>
> I thought about just adding metrics to -e, without metric: of course.
Ditto.
- Arnaldo
Em Fri, Jun 26, 2020 at 02:44:14PM -0700, Ian Rogers escreveu:
> On Fri, Jun 26, 2020 at 2:25 PM Andi Kleen <[email protected]> wrote:
> > On Fri, Jun 26, 2020 at 09:47:10PM +0200, Jiri Olsa wrote:
> > > this patchset is adding the support to reused metric in another
> > > metric. The metric needs to be referenced by 'metric:' prefix.
> > Why is the prefix needed?
> > Could just look it up without prefix.
> The name could be a metric or an event, the logic for each is quite
> different. You could look up an event and when it fails assume it was
> a metric, but I like the simplicity of this approach. Maybe this
> change could be adopted more widely with something like "perf stat -e
> metric:IPC -a -I 1000" rather than the current "perf stat -M IPC -a -I
> 1000".
Humm, the more concise, the better, so I think that we should use
metric: when we notice ambiguity, i.e. we should first lookup the
provided name as an event, and even if it resolves, look it up as well
as a metric, if both lookups work, then one need to disambiguate.
But then, why should we pick a name for a metric that is also a name for
an event? Can you think about a concrete case? Can't we detect this at
build time, when introducing the new metric and bail out?
- Arnaldo
On Sat, Jun 27, 2020 at 5:48 AM Arnaldo Carvalho de Melo
<[email protected]> wrote:
>
> Em Fri, Jun 26, 2020 at 02:57:59PM -0700, Andi Kleen escreveu:
> > > The name could be a metric or an event, the logic for each is quite
> >
> > I would say collisions are unlikely. Event names follow quite structured
> > patterns.
>
> And when introducing a new metric the build process can detect that
> clash and fail.
>
> > > different. You could look up an event and when it fails assume it was
> > > a metric, but I like the simplicity of this approach.
>
> > I don't think it's simpler for the user.
>
> Agreed.
>
> > > Maybe this
> > > change could be adopted more widely with something like "perf stat -e
> > > metric:IPC -a -I 1000" rather than the current "perf stat -M IPC -a -I
> > > 1000".
> >
> > I thought about just adding metrics to -e, without metric: of course.
>
> Ditto.
Thanks, while we're thinking about this I'd like there to be support
for flags on metrics. Such as 'perf stat -M IPC:u ...' where the ':u'
is specifying user only as with events.
Fwiw, another point of pain is lining up events with cgroups. Being
able to have the cgroup be a flag on an event or metric would be nice.
Ian
> - Arnaldo
On Fri, Jun 26, 2020 at 01:04:41PM -0700, Ian Rogers wrote:
> On Fri, Jun 26, 2020 at 12:47 PM Jiri Olsa <[email protected]> wrote:
> >
> > Adding struct expr_parse_data to keep expr value
> > instead of just simple double pointer, so we can
> > store more data for ID in following changes.
>
> Nit, expr_parse_data sounds a bit like data that is created just at
> parse time. Perhaps id_data, for data associated with an id?
we should keep the expr prefix, expr_id_data ?
jirka
On Fri, Jun 26, 2020 at 01:07:24PM -0700, Ian Rogers wrote:
> On Fri, Jun 26, 2020 at 12:47 PM Jiri Olsa <[email protected]> wrote:
> >
> > Adding expr__add_id function to data for ID
> > with zero value, which is used when scanning
> > the expression for IDs.
> >
> > 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 7573b21e73df..0b6d3a6ce88e 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 *name)
>
> Nit, perhaps "id" is more consistent than "name". Perhaps also change
> add_val below.
ok, will change
thanks,
jirka
>
> Acked-by: Ian Rogers <[email protected]>
>
> > +{
> > + struct expr_parse_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, name, 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_val(struct expr_parse_ctx *ctx, const char *name, double val)
> > {
> > @@ -39,12 +57,11 @@ int expr__add_val(struct expr_parse_ctx *ctx, const char *name, 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, name, 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 f9f16efe76bc..5452e641acf4 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 *name);
> > int expr__add_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 ff5e5f6e170d..ac4b119877e0 100644
> > --- a/tools/perf/util/expr.y
> > +++ b/tools/perf/util/expr.y
> > @@ -71,7 +71,7 @@ all_other: all_other other
> >
> > other: ID
> > {
> > - expr__add_val(ctx, $1, 0.0);
> > + expr__add_id(ctx, $1);
> > }
> > |
> > MIN | MAX | IF | ELSE | SMT_ON | NUMBER | '|' | '^' | '&' | '-' | '+' | '*' | '/' | '%' | '(' | ')' | ','
> > --
> > 2.25.4
> >
>
On Fri, Jun 26, 2020 at 01:55:37PM -0700, Ian Rogers wrote:
> On Fri, Jun 26, 2020 at 12:47 PM 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]>
> > ---
> > 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 29cdef18849c..aa14c7111ecc 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_parse_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_parse_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);
>
> Nit, I always have to read the code to know why we have "one" as an
> argument. Could we remove it as an argument and have the caller use
> expr__del_id?
I'll check sounds like good thing to do
thanks,
jirka
On Fri, Jun 26, 2020 at 02:10:57PM -0700, Ian Rogers wrote:
> On Fri, Jun 26, 2020 at 12:48 PM Jiri Olsa <[email protected]> wrote:
> >
> > Add 'other' 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.
>
> Nit, other vs something like referenced_metric but otherwise lgtm.
I'd like to keep metric prefix
struct metric_ref ?
jirka
On Fri, Jun 26, 2020 at 02:16:30PM -0700, Ian Rogers wrote:
> On Fri, Jun 26, 2020 at 12:48 PM Jiri Olsa <[email protected]> wrote:
> >
> > Adding other metrics to the parsing context so they
> > can be resolved during the metric processing.
> >
> > Adding expr__add_other function to store 'other' 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 | 19 +++++++++++++------
> > 3 files changed, 60 insertions(+), 7 deletions(-)
> >
> > diff --git a/tools/perf/util/expr.c b/tools/perf/util/expr.c
> > index cd73dae4588c..32f7acac7c19 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_val(struct expr_parse_ctx *ctx, const char *name, double val)
> > if (!data_ptr)
> > return -ENOMEM;
> > data_ptr->val = val;
> > + data_ptr->is_other = false;
> >
> > ret = hashmap__set(&ctx->ids, name, data_ptr,
> > (const void **)&old_key, (void **)&old_data);
> > @@ -69,6 +72,38 @@ int expr__add_val(struct expr_parse_ctx *ctx, const char *name, double val)
> > return ret;
> > }
> >
> > +int expr__add_other(struct expr_parse_ctx *ctx, struct metric_other *other)
> > +{
> > + struct expr_parse_data *data_ptr = NULL, *old_data = NULL;
> > + char *old_key = NULL;
> > + char *name;
> > + int ret;
> > +
> > + data_ptr = malloc(sizeof(*data_ptr));
> > + if (!data_ptr)
> > + return -ENOMEM;
> > +
> > + name = strdup(other->metric_name);
> > + if (!name) {
> > + free(data_ptr);
> > + return -ENOMEM;
> > + }
> > +
> > + data_ptr->other.metric_name = other->metric_name;
> > + data_ptr->other.metric_expr = other->metric_expr;
> > + data_ptr->is_other = true;
> > +
> > + ret = hashmap__set(&ctx->ids, name, data_ptr,
> > + (const void **)&old_key, (void **)&old_data);
> > +
> > + pr_debug2("adding other metric %s: %s\n",
> > + other->metric_name, other->metric_expr);
> > +
> > + free(old_key);
> > + free(old_data);
> > + return ret;
> > +}
> > +
> > int expr__get_id(struct expr_parse_ctx *ctx, const char *id,
> > struct expr_parse_data **data)
> > {
> > diff --git a/tools/perf/util/expr.h b/tools/perf/util/expr.h
> > index fd924bb4e5cd..ed60f9227b43 100644
> > --- a/tools/perf/util/expr.h
> > +++ b/tools/perf/util/expr.h
> > @@ -11,12 +11,22 @@
> > #include "util/hashmap.h"
> > //#endif
> >
> > +struct metric_other;
> > +
> > struct expr_parse_ctx {
> > struct hashmap ids;
> > };
> >
> > struct expr_parse_data {
> > - double val;
> > + bool is_other;
> > +
> > + union {
> > + double val;
> > + struct {
> > + const char *metric_name;
> > + const char *metric_expr;
>
> It is probably worth a comment why both the metric_name and the
> metric's expression are required here? The parse and other data for
> the metric won't be here.
it's there to have it ready when processing the metric,
I'll add some more comments in here
thanks,
jirka
On Fri, Jun 26, 2020 at 02:24:38PM -0700, Ian Rogers wrote:
SNIP
> > +
> > + if (expr__get_id(ctx, lookup, &data) || !data) {
> > pr_debug("%s not found\n", $1);
> > free($1);
> > YYABORT;
> > }
> > +
> > + pr_debug2("lookup: is_other %d, counted %d: %s\n",
> > + data->is_other, data->other.counted, lookup);
> > +
> > + if (data->is_other && !data->other.counted) {
> > + data->other.counted = true;
> > + if (expr__parse(&data->val, ctx, data->other.metric_expr, 1)) {
>
> Ah, so this handles the problem the referenced metric isn't calculated
> and calculates it - with the sharing of events this doesn't impose
> extra pmu cost. Do we need to worry about detecting recursion? For
> example:
>
> "MetricName": "Foo",
> "MetricExpr": "1/metric:Foo",
right, we should add some recursion check,
I'll lcheck on it
>
> It seems unfortunate to have the MetricExpr calculated twice, but it
hum, not sure what you mean by twice.. we do that just once for
each involved metric and store the value.. the metric is also
processed before for 'other' metrics
jirka
> is understandable. Is it also a property that referenced/other metrics
> won't be reported individually? Perhaps these are sub-metrics?
>
> Thanks,
> Ian
>
> > + pr_debug("%s failed to count\n", $1);
> > + free($1);
> > + YYABORT;
> > + }
> > + }
> > +
> > $$ = data->val;
> > free($1);
> > }
> > --
> > 2.25.4
> >
>
On Fri, Jun 26, 2020 at 02:40:34PM -0700, Ian Rogers wrote:
> On Fri, Jun 26, 2020 at 12:48 PM 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
> >
>
> This is really nice! Do we need to do anything in tests/pmu-events.c?
> Presumably if a metric is referencing another metric the call to
> expr__parse there will test the other metric is parseable. Just wanted
> to sanity check.
right, I did not realize that, but that test is passing for me,
so I guess it's fine.. I'll double check
thanks,
jirka
>
> Acked-by: Ian Rogers <[email protected]>
>
> > 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..feb97f7c90c8 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 = "(metric:dcache_miss_cpi + metric: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 Fri, Jun 26, 2020 at 02:06:47PM -0700, Ian Rogers wrote:
SNIP
> > diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
> > index 85e7fa2e2707..f88fd667cc78 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 eother {
> > + 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 other;
> > + int other_cnt;
> > int runtime;
> > bool has_constraint;
> > };
>
> Nit, could we do better than egroup and eother for struct names?
> egroup are nodes within the metric group with their associated values,
> so would metric_group_node be more intention revealing? eother and
> other are metrics referred to by the metric_group_node. Presumably the
> metrics are on the same list as egroup? Perhaps eother could be
> referenced_metrics?
ok, how about:
struct metric_group_node
struct metric_ref
jirka
On Fri, Jun 26, 2020 at 02:48:02PM -0700, Ian Rogers wrote:
> On Fri, Jun 26, 2020 at 12:47 PM Jiri Olsa <[email protected]> wrote:
> >
> > Collecting other metrics in struct egroup object,
> > so we can process them later on.
> >
> > The change will parse or 'other' metric names out of
> > expression and 'resolve' them.
> >
> > Every used expression needs to have 'metric:' prefix,
> > like:
> > cache_miss_cycles = metric:dcache_miss_cpi + metric:icache_miss_cycles
> >
> > All 'other' metrics are disolved into one context,
> > meaning all 'other' metrics events and addded to
> > the parent context.
> >
> > Signed-off-by: Jiri Olsa <[email protected]>
> > ---
> > .../arch/x86/skylake/skl-metrics.json | 2 +-
> > tools/perf/util/expr.c | 11 ++
> > tools/perf/util/expr.h | 1 +
> > tools/perf/util/metricgroup.c | 158 ++++++++++++++++--
> > 4 files changed, 157 insertions(+), 15 deletions(-)
> >
> > diff --git a/tools/perf/pmu-events/arch/x86/skylake/skl-metrics.json b/tools/perf/pmu-events/arch/x86/skylake/skl-metrics.json
> > index 8704efeb8d31..71e5a2b471ac 100644
> > --- a/tools/perf/pmu-events/arch/x86/skylake/skl-metrics.json
> > +++ b/tools/perf/pmu-events/arch/x86/skylake/skl-metrics.json
> > @@ -57,7 +57,7 @@
> > },
> > {
> > "BriefDescription": "Instructions Per Cycle (per Logical Processor)",
> > - "MetricExpr": "INST_RETIRED.ANY / CPU_CLK_UNHALTED.THREAD",
> > + "MetricExpr": "1/metric:CPI",
> > "MetricGroup": "TopDownL1",
> > "MetricName": "IPC"
> > },
> > diff --git a/tools/perf/util/expr.c b/tools/perf/util/expr.c
> > index aa14c7111ecc..cd73dae4588c 100644
> > --- a/tools/perf/util/expr.c
> > +++ b/tools/perf/util/expr.c
> > @@ -150,3 +150,14 @@ int expr__find_other(const char *expr, const char *one,
> >
> > return ret;
> > }
> > +
> > +#define METRIC "metric:"
> > +
> > +bool expr__is_metric(const char *name, const char **metric)
> > +{
> > + int ret = !strncmp(name, METRIC, sizeof(METRIC) - 1);
> > +
> > + if (ret && metric)
> > + *metric = name + sizeof(METRIC) - 1;
> > + return ret;
> > +}
>
> Should expr.l recognize metric:... as a different kind of token rather
> than an ID?
hm, we still want it to be returned as ID token, and the processing
code needs a way to distinguish between event and metric, so I'd think
we need to keep it, but I'll double check
thanks,
jirka
On Sat, Jun 27, 2020 at 09:48:21AM -0300, Arnaldo Carvalho de Melo wrote:
> Em Fri, Jun 26, 2020 at 02:57:59PM -0700, Andi Kleen escreveu:
> > > The name could be a metric or an event, the logic for each is quite
> >
> > I would say collisions are unlikely. Event names follow quite structured
> > patterns.
>
> And when introducing a new metric the build process can detect that
> clash and fail.
>
> > > different. You could look up an event and when it fails assume it was
> > > a metric, but I like the simplicity of this approach.
>
> > I don't think it's simpler for the user.
>
> Agreed.
>
> > > Maybe this
> > > change could be adopted more widely with something like "perf stat -e
> > > metric:IPC -a -I 1000" rather than the current "perf stat -M IPC -a -I
> > > 1000".
> >
> > I thought about just adding metrics to -e, without metric: of course.
>
> Ditto.
>
> - Arnaldo
>
I guess I wanted to clearly separate other metrics from the expression,
also running through the whole lists of metrics for each id did not
seem good.. but it's actualy not that bad (compared to other things we
do ;-), and if you guys prefer not using a prefix I think it's ok
thanks,
jirka
On Fri, 26 Jun 2020, Andi Kleen wrote:
> > The name could be a metric or an event, the logic for each is quite
>
> I would say collisions are unlikely. Event names follow quite structured
> patterns.
But across various architectures? I guess event names can be arbitrary.
In perftool-testsuite, I use the following regexp to match event names:
[\w\-\:\/_=,]+
>
> > different. You could look up an event and when it fails assume it was
> > a metric, but I like the simplicity of this approach.
>
> I don't think it's simpler for the user.
>
I think it should be clear at the user level whether they're using an event
or a metric (basically a couple of events together). I don't hiding too
much of details from users is any good.
> > Maybe this
> > change could be adopted more widely with something like "perf stat -e
> > metric:IPC -a -I 1000" rather than the current "perf stat -M IPC -a -I
> > 1000".
>
> I thought about just adding metrics to -e, without metric: of course.
>
> -Andi
>
>
On Sun, Jun 28, 2020 at 3:00 PM Jiri Olsa <[email protected]> wrote:
>
> On Fri, Jun 26, 2020 at 02:24:38PM -0700, Ian Rogers wrote:
>
> SNIP
>
> > > +
> > > + if (expr__get_id(ctx, lookup, &data) || !data) {
> > > pr_debug("%s not found\n", $1);
> > > free($1);
> > > YYABORT;
> > > }
> > > +
> > > + pr_debug2("lookup: is_other %d, counted %d: %s\n",
> > > + data->is_other, data->other.counted, lookup);
> > > +
> > > + if (data->is_other && !data->other.counted) {
> > > + data->other.counted = true;
> > > + if (expr__parse(&data->val, ctx, data->other.metric_expr, 1)) {
> >
> > Ah, so this handles the problem the referenced metric isn't calculated
> > and calculates it - with the sharing of events this doesn't impose
> > extra pmu cost. Do we need to worry about detecting recursion? For
> > example:
> >
> > "MetricName": "Foo",
> > "MetricExpr": "1/metric:Foo",
>
> right, we should add some recursion check,
> I'll lcheck on it
>
> >
> > It seems unfortunate to have the MetricExpr calculated twice, but it
>
> hum, not sure what you mean by twice.. we do that just once for
> each involved metric and store the value.. the metric is also
> processed before for 'other' metrics
So I'm thinking out loud. Here is an example from Skylake:
{
"BriefDescription": "All L2 hit counts",
"MetricExpr": "L2_RQSTS.DEMAND_DATA_RD_HIT + L2_RQSTS.PF_HIT +
L2_RQSTS.RFO_HIT",
"MetricName": "DCache_L2_All_Hits",
}
{
"BriefDescription": "All L2 miss counts",
"MetricExpr": "MAX(L2_RQSTS.ALL_DEMAND_DATA_RD -
L2_RQSTS.DEMAND_DATA_RD_HIT, 0) + L2_RQSTS.PF_MISS +
L2_RQSTS.RFO_MISS",
"MetricName": "DCache_L2_All_Miss",
}
{
"BriefDescription": "All L2 counts",
"MetricExpr": "metric:DCache_L2_All_Hits + metric:DCache_L2_All_Miss",
"MetricName": "DCache_L2_All",
}
{
"BriefDescription": "DCache L2 hit rate",
"MetricExpr": "d_ratio(metric:DCache_L2_All_Hits, metric:DCache_L2_All)",
"MetricName": "DCache_L2_Hits",
"MetricGroup": "DCache_L2",
"ScaleUnit": "100%",
},
{
"BriefDescription": "DCache L2 miss rate",
"MetricExpr": "d_ratio(metric:DCache_L2_All_Miss, metric:DCache_L2_All)",
"MetricName": "DCache_L2_Misses",
"MetricGroup": "DCache_L2",
"ScaleUnit": "100%",
},
Firstly, it should be clear that having this change makes the json far
more readable! The current approach is to copy and paste resulting in
100s of characters wide expressions. This is a great improvement!
With these metrics the hope would be that 'perf stat -M DCache_L2 ...'
is going to report just DCache_L2_Hits and DCache_L2_Misses. To
compute these two metrics, as an example, DCache_L2_All_Hits is needed
three times. My comment was meant to mean that it seems a little
unfortunate to keep repeatedly evaluating the expression rather than
to compute it once and reuse the result.
Thanks,
Ian
> jirka
>
> > is understandable. Is it also a property that referenced/other metrics
> > won't be reported individually? Perhaps these are sub-metrics?
>
> >
> > Thanks,
> > Ian
> >
> > > + pr_debug("%s failed to count\n", $1);
> > > + free($1);
> > > + YYABORT;
> > > + }
> > > + }
> > > +
> > > $$ = data->val;
> > > free($1);
> > > }
> > > --
> > > 2.25.4
> > >
> >
>
On Mon, Jun 29, 2020 at 09:35:12AM -0700, Ian Rogers wrote:
SNIP
>
> {
> "BriefDescription": "All L2 hit counts",
> "MetricExpr": "L2_RQSTS.DEMAND_DATA_RD_HIT + L2_RQSTS.PF_HIT +
> L2_RQSTS.RFO_HIT",
> "MetricName": "DCache_L2_All_Hits",
> }
> {
> "BriefDescription": "All L2 miss counts",
> "MetricExpr": "MAX(L2_RQSTS.ALL_DEMAND_DATA_RD -
> L2_RQSTS.DEMAND_DATA_RD_HIT, 0) + L2_RQSTS.PF_MISS +
> L2_RQSTS.RFO_MISS",
> "MetricName": "DCache_L2_All_Miss",
> }
> {
> "BriefDescription": "All L2 counts",
> "MetricExpr": "metric:DCache_L2_All_Hits + metric:DCache_L2_All_Miss",
> "MetricName": "DCache_L2_All",
> }
> {
> "BriefDescription": "DCache L2 hit rate",
> "MetricExpr": "d_ratio(metric:DCache_L2_All_Hits, metric:DCache_L2_All)",
> "MetricName": "DCache_L2_Hits",
> "MetricGroup": "DCache_L2",
> "ScaleUnit": "100%",
> },
> {
> "BriefDescription": "DCache L2 miss rate",
> "MetricExpr": "d_ratio(metric:DCache_L2_All_Miss, metric:DCache_L2_All)",
> "MetricName": "DCache_L2_Misses",
> "MetricGroup": "DCache_L2",
> "ScaleUnit": "100%",
> },
>
> Firstly, it should be clear that having this change makes the json far
> more readable! The current approach is to copy and paste resulting in
> 100s of characters wide expressions. This is a great improvement!
>
> With these metrics the hope would be that 'perf stat -M DCache_L2 ...'
> is going to report just DCache_L2_Hits and DCache_L2_Misses. To
> compute these two metrics, as an example, DCache_L2_All_Hits is needed
> three times. My comment was meant to mean that it seems a little
> unfortunate to keep repeatedly evaluating the expression rather than
> to compute it once and reuse the result.
nice example, the code should evaluate the expression just once as long
as it's under same name.. I'll prepare new version and verify that's the
case with the example above
thanks,
jirka
On Sun, Jun 28, 2020 at 2:25 PM Jiri Olsa <[email protected]> wrote:
>
> On Fri, Jun 26, 2020 at 01:04:41PM -0700, Ian Rogers wrote:
> > On Fri, Jun 26, 2020 at 12:47 PM Jiri Olsa <[email protected]> wrote:
> > >
> > > Adding struct expr_parse_data to keep expr value
> > > instead of just simple double pointer, so we can
> > > store more data for ID in following changes.
> >
> > Nit, expr_parse_data sounds a bit like data that is created just at
> > parse time. Perhaps id_data, for data associated with an id?
>
> we should keep the expr prefix, expr_id_data ?
Sounds good to me. Thanks,
Ian
> jirka
>
On Sun, Jun 28, 2020 at 2:55 PM Jiri Olsa <[email protected]> wrote:
>
> On Fri, Jun 26, 2020 at 02:10:57PM -0700, Ian Rogers wrote:
> > On Fri, Jun 26, 2020 at 12:48 PM Jiri Olsa <[email protected]> wrote:
> > >
> > > Add 'other' 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.
> >
> > Nit, other vs something like referenced_metric but otherwise lgtm.
>
> I'd like to keep metric prefix
>
> struct metric_ref ?
Sounds good to me. Thanks,
Ian
> jirka
>
On Sun, Jun 28, 2020 at 3:06 PM Jiri Olsa <[email protected]> wrote:
>
> On Fri, Jun 26, 2020 at 02:48:02PM -0700, Ian Rogers wrote:
> > On Fri, Jun 26, 2020 at 12:47 PM Jiri Olsa <[email protected]> wrote:
> > >
> > > Collecting other metrics in struct egroup object,
> > > so we can process them later on.
> > >
> > > The change will parse or 'other' metric names out of
> > > expression and 'resolve' them.
> > >
> > > Every used expression needs to have 'metric:' prefix,
> > > like:
> > > cache_miss_cycles = metric:dcache_miss_cpi + metric:icache_miss_cycles
> > >
> > > All 'other' metrics are disolved into one context,
> > > meaning all 'other' metrics events and addded to
> > > the parent context.
> > >
> > > Signed-off-by: Jiri Olsa <[email protected]>
> > > ---
> > > .../arch/x86/skylake/skl-metrics.json | 2 +-
> > > tools/perf/util/expr.c | 11 ++
> > > tools/perf/util/expr.h | 1 +
> > > tools/perf/util/metricgroup.c | 158 ++++++++++++++++--
> > > 4 files changed, 157 insertions(+), 15 deletions(-)
> > >
> > > diff --git a/tools/perf/pmu-events/arch/x86/skylake/skl-metrics.json b/tools/perf/pmu-events/arch/x86/skylake/skl-metrics.json
> > > index 8704efeb8d31..71e5a2b471ac 100644
> > > --- a/tools/perf/pmu-events/arch/x86/skylake/skl-metrics.json
> > > +++ b/tools/perf/pmu-events/arch/x86/skylake/skl-metrics.json
> > > @@ -57,7 +57,7 @@
> > > },
> > > {
> > > "BriefDescription": "Instructions Per Cycle (per Logical Processor)",
> > > - "MetricExpr": "INST_RETIRED.ANY / CPU_CLK_UNHALTED.THREAD",
> > > + "MetricExpr": "1/metric:CPI",
> > > "MetricGroup": "TopDownL1",
> > > "MetricName": "IPC"
> > > },
> > > diff --git a/tools/perf/util/expr.c b/tools/perf/util/expr.c
> > > index aa14c7111ecc..cd73dae4588c 100644
> > > --- a/tools/perf/util/expr.c
> > > +++ b/tools/perf/util/expr.c
> > > @@ -150,3 +150,14 @@ int expr__find_other(const char *expr, const char *one,
> > >
> > > return ret;
> > > }
> > > +
> > > +#define METRIC "metric:"
> > > +
> > > +bool expr__is_metric(const char *name, const char **metric)
> > > +{
> > > + int ret = !strncmp(name, METRIC, sizeof(METRIC) - 1);
> > > +
> > > + if (ret && metric)
> > > + *metric = name + sizeof(METRIC) - 1;
> > > + return ret;
> > > +}
> >
> > Should expr.l recognize metric:... as a different kind of token rather
> > than an ID?
>
> hm, we still want it to be returned as ID token, and the processing
> code needs a way to distinguish between event and metric, so I'd think
> we need to keep it, but I'll double check
Thanks, the struct names sound good. I suggested using a token as it
is a little strange that we have layers of parsing and this would be a
chance to avoid one layer. It isn't a big deal, the event parsing is
far more complex :-)
Ian
> thanks,
> jirka
>
> I'm guessing that those metric JSONs are human generated, so would be
> suitable; unlike the regular JSONs, which are automated.
The x86 metrics are auto generated too.
That's why you see the repetition.
-Andi