Re: [PATCH 3/4] perf stat: Add --metrics-file option

From: Ian Rogers
Date: Wed May 13 2020 - 03:05:09 EST

Next message: Kalle Valo: "Re: [PATCH] ath9k: release allocated buffer if timed out"
Previous message: Geert Uytterhoeven: "Re: [PATCH 5/5] dt-bindings: Fix incorrect 'reg' property sizes"
In reply to: Jiri Olsa: "[PATCH 3/4] perf stat: Add --metrics-file option"
Next in thread: Jiri Olsa: "Re: [PATCH 3/4] perf stat: Add --metrics-file option"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Mon, May 11, 2020 at 1:54 PM Jiri Olsa <jolsa@xxxxxxxxxx> wrote:
>
> Adding --metrics-file option that allows to specify metrics
> in the file.
>
> It's now possible to define metrics in file and use them like:
>
> $ cat metrics
> // IPC
> mine1 = inst_retired.any / cpu_clk_unhalted.thread;
>
> /* DECODED_ICACHE_UOPS% */
> mine2 = 100 * (idq.dsb_uops / (idq.ms_uops + idq.mite_uops + idq.dsb_uops + lsd.uops));
>
> $ sudo perf stat --metrics-file ./metrics -M mine1,mine2 -a -I 1000 --metric-only
> # time mine1 mine2
> 1.000997184 0.41 18.47
> 2.002479737 0.57 22.46
> 3.003932935 0.40 17.52
> ...
>
> Signed-off-by: Jiri Olsa <jolsa@xxxxxxxxxx>
> ---
> tools/perf/Documentation/perf-stat.txt | 77 ++++++++++++++++++++++++++
> tools/perf/builtin-stat.c | 7 ++-
> tools/perf/util/metricgroup.c | 69 ++++++++++++++++++++---
> tools/perf/util/metricgroup.h | 3 +-
> tools/perf/util/stat.h | 1 +
> 5 files changed, 147 insertions(+), 10 deletions(-)
>
> diff --git a/tools/perf/Documentation/perf-stat.txt b/tools/perf/Documentation/perf-stat.txt
> index 3fb5028aef08..9f0a646d719c 100644
> --- a/tools/perf/Documentation/perf-stat.txt
> +++ b/tools/perf/Documentation/perf-stat.txt
> @@ -266,6 +266,10 @@ For a group all metrics from the group are added.
> The events from the metrics are automatically measured.
> See perf list output for the possble metrics and metricgroups.
>
> +--metrics-file file::
> +Read metrics definitions from file in addition to compiled in metrics.
> +Please check the file's syntax details in METRICS FILE section below.
> +
> -A::
> --no-aggr::
> Do not aggregate counts across all monitored CPUs.
> @@ -404,6 +408,79 @@ The fields are in this order:
>
> Additional metrics may be printed with all earlier fields being empty.
>
> +METRICS FILE
> +------------
> +The file with metrics has following syntax:
> +
> + NAME = EXPRESSION ;
> + NAME = EXPRESSION ;
> + ...
> +
> +where NAME is unique identifier of the metric, which is later used in
> +perf stat as -M option argument (see below).
> +
> +The EXPRESSION is the metric's formula with following grammar:
> +
> + EXPR: EVENT
> + EXPR: EXPR if EXPR else EXPR

Not introduced by this patch, but this patch is exposing it as an API.
This notion of if-else is really weird. For one thing there are no
comparison operators. The unit test doesn't really help:
ret |= test(&ctx, "1+1 if 3*4 else 0", 2);
What kind of comparison is "3*4"? If 0.0 causes the else clause then will -0.0?
A typical expression I see written in C is to give a ratio such:
value = denom == 0 ? 0 : nom / denom;
I've worked around encoding this by extending expr.y locally.

> + EXPR: NUMBER
> + EXPR: EXPR | EXPR
> + EXPR: EXPR & EXPR
> + EXPR: EXPR ^ EXPR

Again, it's odd that these cast the double to a long and then assign
the result back to a double.

> + EXPR: EXPR + EXPR
> + EXPR: EXPR - EXPR
> + EXPR: EXPR * EXPR
> + EXPR: EXPR / EXPR
> + EXPR: EXPR % EXPR
> + EXPR: - EXPR
> + EXPR: ( EXPR )
> + EXPR: min( EXPR, EXPR )
> + EXPR: max( EXPR, EXPR )
> + EXPR: #smt_on
> +
> +where:
> +
> + EVENT is monitored event name
> + min returns smaller of 2 expressions
> + max returns bigger of 2 expressions
> + #smt_on returns true if SMT is enabled
> +
> +The metric's definition can be spread across multiple lines and it's finished
> +with ';' character.
> +
> +The syntax allows for C style comments:
> +
> + // single line
> + /* multiple
> + lines */
> +
> +Metrics file example with 2 custom metrics mine1 and mine2:
> +
> + $ cat metrics
> + // IPC
> + mine1 = inst_retired.any / cpu_clk_unhalted.thread;
> +
> + /* DECODED_ICACHE_UOPS% */
> + mine2 = 100 * (idq.dsb_uops / (idq.ms_uops + idq.mite_uops + idq.dsb_uops + lsd.uops));
> +
> + $ sudo perf stat --metrics-file ./metrics -M mine1,mine2 -a -I 1000 --metric-only
> + # time mine1 mine2
> + 1.000997184 0.41 18.47
> + 2.002479737 0.57 22.46
> + 3.003932935 0.40 17.52
> + ...
> +
> +
> +It's possible to mix custom metrics with compiled-in metrics in one -M argument:
> +
> + $ sudo perf stat --metrics-file ./metrics -M mine1,mine2,IPC -a -I 1000 --metric-only
> + # time mine1 mine2 IPC
> + 1.001142007 0.85 22.33 0.85
> + 2.002460174 0.86 23.37 0.86
> + 3.003969795 1.03 23.93 1.03
> + ...

A feature request would be to allow metrics in terms of other metrics,
not just events :-) For example, it is common to sum all cache
hit/miss events. It is laborious to copy that expression for hit rate,
miss rate, etc.

Perhaps the expression parsing code should be folded into the event
parsing code.

Thanks,
Ian

> +
> SEE ALSO
> --------
> linkperf:perf-top[1], linkperf:perf-list[1]
> diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
> index e0c1ad23c768..5eda298654a8 100644
> --- a/tools/perf/builtin-stat.c
> +++ b/tools/perf/builtin-stat.c
> @@ -840,7 +840,8 @@ static int parse_metric_groups(const struct option *opt,
> const char *str,
> int unset __maybe_unused)
> {
> - return metricgroup__parse_groups(opt, str, &stat_config.metric_events);
> + return metricgroup__parse_groups(opt, str, &stat_config.metric_events,
> + stat_config.metrics_file);
> }
>
> static struct option stat_options[] = {
> @@ -925,6 +926,8 @@ static struct option stat_options[] = {
> OPT_CALLBACK('M', "metrics", &evsel_list, "metric/metric group list",
> "monitor specified metrics or metric groups (separated by ,)",
> parse_metric_groups),
> + OPT_STRING(0, "metrics-file", &stat_config.metrics_file, "file path",
> + "file with metrics definitions"),
> OPT_BOOLEAN_FLAG(0, "all-kernel", &stat_config.all_kernel,
> "Configure all used events to run in kernel space.",
> PARSE_OPT_EXCLUSIVE),
> @@ -1442,7 +1445,7 @@ static int add_default_attributes(void)
> struct option opt = { .value = &evsel_list };
>
> return metricgroup__parse_groups(&opt, "transaction",
> - &stat_config.metric_events);
> + &stat_config.metric_events, NULL);
> }
>
> if (pmu_have_event("cpu", "cycles-ct") &&
> diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
> index b071df373f8b..3b4d5bdb5ac6 100644
> --- a/tools/perf/util/metricgroup.c
> +++ b/tools/perf/util/metricgroup.c
> @@ -527,17 +527,17 @@ static int __metricgroup__add_metric(struct strbuf *events,
> }
>
> static int metricgroup__add_metric(const char *metric, struct strbuf *events,
> - struct list_head *group_list)
> + struct list_head *group_list,
> + struct expr_parse_ctx *ctx)
> {
> struct pmu_events_map *map = perf_pmu__find_map(NULL);
> - struct pmu_event *pe;
> int i, ret = -EINVAL;
>
> if (!map)
> return 0;
>
> for (i = 0; ; i++) {
> - pe = &map->table[i];
> + struct pmu_event *pe = &map->table[i];
>
> if (!pe->name && !pe->metric_group && !pe->metric_name)
> break;
> @@ -567,15 +567,59 @@ static int metricgroup__add_metric(const char *metric, struct strbuf *events,
> break;
> }
> }
> +
> + if (!ctx->num_custom || ret == -ENOMEM)
> + return ret;
> +
> + for (i = 0; i < ctx->num_custom; i++) {
> + struct pmu_event pe = { 0 };
> +
> + if (!match_metric(ctx->custom[i].name, metric))
> + continue;
> +
> + pe.metric_name = strdup(ctx->custom[i].name);
> + pe.metric_expr = strdup(ctx->custom[i].expr);
> +
> + if (!pe.metric_name && !pe.metric_expr)
> + return -ENOMEM;
> +
> + ret = __metricgroup__add_metric(events, group_list, &pe, 1);
> + if (ret) {
> + free((char *) pe.metric_name);
> + free((char *) pe.metric_expr);
> + break;
> + }
> + }
> +
> return ret;
> }
>
> static int metricgroup__add_metric_list(const char *list, struct strbuf *events,
> - struct list_head *group_list)
> + struct list_head *group_list,
> + const char *metrics_file)
> {
> + struct expr_parse_ctx ctx = { 0 };
> char *llist, *nlist, *p;
> int ret = -EINVAL;
>
> + if (metrics_file) {
> + size_t size;
> + char *buf;
> +
> + if (filename__read_str(metrics_file, &buf, &size)) {
> + pr_err("failed to read metrics file: %s\n", metrics_file);
> + return -1;
> + }
> +
> + expr__ctx_init(&ctx);
> + ret = expr__parse_custom(&ctx, buf);
> + free(buf);
> + if (ret) {
> + pr_err("failed to parse metrics file: %s\n", metrics_file);
> + return -1;
> + }
> + }
> +
> nlist = strdup(list);
> if (!nlist)
> return -ENOMEM;
> @@ -585,7 +629,7 @@ static int metricgroup__add_metric_list(const char *list, struct strbuf *events,
> strbuf_addf(events, "%s", "");
>
> while ((p = strsep(&llist, ",")) != NULL) {
> - ret = metricgroup__add_metric(p, events, group_list);
> + ret = metricgroup__add_metric(p, events, group_list, &ctx);
> if (ret == -EINVAL) {
> fprintf(stderr, "Cannot find metric or group `%s'\n",
> p);
> @@ -597,6 +641,15 @@ static int metricgroup__add_metric_list(const char *list, struct strbuf *events,
> if (!ret)
> metricgroup___watchdog_constraint_hint(NULL, true);
>
> + if (metrics_file) {
> + int i;
> +
> + for (i = 0; i < ctx.num_custom; i++) {
> + zfree(&ctx.custom[i].name);
> + zfree(&ctx.custom[i].expr);
> + }
> + }
> +
> return ret;
> }
>
> @@ -616,7 +669,8 @@ static void metricgroup__free_egroups(struct list_head *group_list)
>
> int metricgroup__parse_groups(const struct option *opt,
> const char *str,
> - struct rblist *metric_events)
> + struct rblist *metric_events,
> + const char *metrics_file)
> {
> struct parse_events_error parse_error;
> struct evlist *perf_evlist = *(struct evlist **)opt->value;
> @@ -626,7 +680,8 @@ int metricgroup__parse_groups(const struct option *opt,
>
> if (metric_events->nr_entries == 0)
> metricgroup__rblist_init(metric_events);
> - ret = metricgroup__add_metric_list(str, &extra_events, &group_list);
> + ret = metricgroup__add_metric_list(str, &extra_events, &group_list,
> + metrics_file);
> if (ret)
> return ret;
> pr_debug("adding %s\n", extra_events.buf);
> diff --git a/tools/perf/util/metricgroup.h b/tools/perf/util/metricgroup.h
> index 6b09eb30b4ec..33dcdcad15dd 100644
> --- a/tools/perf/util/metricgroup.h
> +++ b/tools/perf/util/metricgroup.h
> @@ -30,7 +30,8 @@ struct metric_event *metricgroup__lookup(struct rblist *metric_events,
> bool create);
> int metricgroup__parse_groups(const struct option *opt,
> const char *str,
> - struct rblist *metric_events);
> + struct rblist *metric_events,
> + const char *metrics_file);
>
> void metricgroup__print(bool metrics, bool groups, char *filter,
> bool raw, bool details);
> diff --git a/tools/perf/util/stat.h b/tools/perf/util/stat.h
> index b4fdfaa7f2c0..57c4c7695aaf 100644
> --- a/tools/perf/util/stat.h
> +++ b/tools/perf/util/stat.h
> @@ -123,6 +123,7 @@ struct perf_stat_config {
> struct runtime_stat *stats;
> int stats_num;
> const char *csv_sep;
> + const char *metrics_file;
> struct stats *walltime_nsecs_stats;
> struct rusage ru_data;
> struct perf_cpu_map *aggr_map;
> --
> 2.25.4
>

Next message: Kalle Valo: "Re: [PATCH] ath9k: release allocated buffer if timed out"
Previous message: Geert Uytterhoeven: "Re: [PATCH 5/5] dt-bindings: Fix incorrect 'reg' property sizes"
In reply to: Jiri Olsa: "[PATCH 3/4] perf stat: Add --metrics-file option"
Next in thread: Jiri Olsa: "Re: [PATCH 3/4] perf stat: Add --metrics-file option"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]