hi,
Joe asked for possibility to add user defined metrics. Given that
we already have metrics support, I added --metrics-file option that
allows to specify custom metrics.
$ cat metrics
# IPC
mine1 = instructions / cycles;
/* 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 --metric-only -a -I 1000
# time insn per cycle mine1 mine2
1.000536263 0.71 0.7 41.4
2.002069025 0.31 0.3 14.1
3.003427684 0.27 0.3 14.8
4.004807132 0.25 0.2 12.1
...
v3 changes:
- added doc for metrics file in perf stat man page
- reporting error line number now
- changed '#' style comment to C way with '//'
v2 changes:
- add new --metrics-file option
- rebased on current perf/core expression bison/flex enhancements
Also available in:
git://git.kernel.org/pub/scm/linux/kernel/git/jolsa/perf.git
perf/metric
thanks,
jirka
---
Jiri Olsa (4):
perf expr: Add parsing support for multiple expressions
perf expr: Allow comments in custom metric file
perf stat: Add --metrics-file option
perf expr: Report line number with error
tools/perf/Documentation/perf-stat.txt | 77 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
tools/perf/builtin-stat.c | 7 +++++--
tools/perf/tests/expr.c | 18 ++++++++++++++++++
tools/perf/util/expr.c | 6 ++++++
tools/perf/util/expr.h | 21 +++++++++++++++++++--
tools/perf/util/expr.l | 34 ++++++++++++++++++++++++++++++++++
tools/perf/util/expr.y | 21 +++++++++++++++++----
tools/perf/util/metricgroup.c | 70 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-------
tools/perf/util/metricgroup.h | 3 ++-
tools/perf/util/stat.h | 1 +
10 files changed, 242 insertions(+), 16 deletions(-)
Adding support to use comments within the custom metric file
with both the shell '# ... ' and C '/* ... */ styles.
Signed-off-by: Jiri Olsa <[email protected]>
---
tools/perf/util/expr.l | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/tools/perf/util/expr.l b/tools/perf/util/expr.l
index c6a930ed22e6..e9b294ba09fc 100644
--- a/tools/perf/util/expr.l
+++ b/tools/perf/util/expr.l
@@ -82,6 +82,8 @@ static int str(yyscan_t scanner, int token, int runtime)
%}
%x custom
+%x comment_single
+%x comment_multi
number [0-9]*\.?[0-9]+
@@ -116,6 +118,16 @@ else { return ELSE; }
#smt_on { return SMT_ON; }
{number} { return value(yyscanner); }
{symbol} { return str(yyscanner, ID, sctx->runtime); }
+
+"//" { BEGIN(comment_single); }
+<comment_single>\n { BEGIN(INITIAL); }
+<comment_single>. { }
+
+"/*" { BEGIN(comment_multi); }
+<comment_multi>"*/" { BEGIN(INITIAL); }
+<comment_multi>\n { }
+<comment_multi>. { }
+
"|" { return '|'; }
"^" { return '^'; }
"&" { return '&'; }
--
2.25.4
Adding support to parse metric difinitions in following form:
NAME = EXPRESSION ;
NAME = EXPRESSION ;
...
The parsed NAME and EXPRESSION will be used in following
changes to feed the metric code with definitions from
custom file.
Signed-off-by: Jiri Olsa <[email protected]>
---
tools/perf/tests/expr.c | 13 +++++++++++++
tools/perf/util/expr.c | 6 ++++++
tools/perf/util/expr.h | 19 +++++++++++++++++--
tools/perf/util/expr.l | 12 ++++++++++++
tools/perf/util/expr.y | 13 ++++++++++++-
5 files changed, 60 insertions(+), 3 deletions(-)
diff --git a/tools/perf/tests/expr.c b/tools/perf/tests/expr.c
index f9e8e5628836..c62e122fe719 100644
--- a/tools/perf/tests/expr.c
+++ b/tools/perf/tests/expr.c
@@ -71,5 +71,18 @@ int test__expr(struct test *t __maybe_unused, int subtest __maybe_unused)
zfree(&other[i]);
free((void *)other);
+ expr__ctx_init(&ctx);
+ ret = expr__parse_custom(&ctx, "IPC=INSTRUCTIONS / CYCLES; CPI=CYCLES / INSTRUCTIONS;");
+ TEST_ASSERT_VAL("parse custom failed", ret == 0);
+ TEST_ASSERT_VAL("parse custom count", ctx.num_custom == 2);
+ TEST_ASSERT_VAL("parse custom name", !strcmp(ctx.custom[0].name, "IPC"));
+ TEST_ASSERT_VAL("parse custom name", !strcmp(ctx.custom[1].name, "CPI"));
+ TEST_ASSERT_VAL("parse custom expr", !strcmp(ctx.custom[0].expr, "INSTRUCTIONS / CYCLES"));
+ TEST_ASSERT_VAL("parse custom expr", !strcmp(ctx.custom[1].expr, "CYCLES / INSTRUCTIONS"));
+
+ for (i = 0; i < ctx.num_custom; i++) {
+ zfree(&ctx.custom[i].name);
+ zfree(&ctx.custom[i].expr);
+ }
return 0;
}
diff --git a/tools/perf/util/expr.c b/tools/perf/util/expr.c
index 8b4ce704a68d..d744cb15c1d4 100644
--- a/tools/perf/util/expr.c
+++ b/tools/perf/util/expr.c
@@ -23,6 +23,7 @@ void expr__add_id(struct expr_parse_ctx *ctx, const char *name, double val)
void expr__ctx_init(struct expr_parse_ctx *ctx)
{
ctx->num_ids = 0;
+ ctx->num_custom = 0;
}
static int
@@ -61,6 +62,11 @@ int expr__parse(double *final_val, struct expr_parse_ctx *ctx, const char *expr,
return __expr__parse(final_val, ctx, expr, EXPR_PARSE, runtime) ? -1 : 0;
}
+int expr__parse_custom(struct expr_parse_ctx *ctx, const char *expr)
+{
+ return __expr__parse(NULL, ctx, expr, EXPR_CUSTOM, 0);
+}
+
static bool
already_seen(const char *val, const char *one, const char **other,
int num_other)
diff --git a/tools/perf/util/expr.h b/tools/perf/util/expr.h
index 40fc452b0f2b..ef116b58a5d4 100644
--- a/tools/perf/util/expr.h
+++ b/tools/perf/util/expr.h
@@ -4,15 +4,29 @@
#define EXPR_MAX_OTHER 64
#define MAX_PARSE_ID EXPR_MAX_OTHER
+#define EXPR_MAX 20
struct expr_parse_id {
const char *name;
double val;
};
+struct expr_parse_custom {
+ const char *name;
+ const char *expr;
+};
+
struct expr_parse_ctx {
- int num_ids;
- struct expr_parse_id ids[MAX_PARSE_ID];
+ union {
+ struct {
+ int num_ids;
+ struct expr_parse_id ids[MAX_PARSE_ID];
+ };
+ struct {
+ int num_custom;
+ struct expr_parse_custom custom[EXPR_MAX];
+ };
+ };
};
struct expr_scanner_ctx {
@@ -23,6 +37,7 @@ struct expr_scanner_ctx {
void expr__ctx_init(struct expr_parse_ctx *ctx);
void expr__add_id(struct expr_parse_ctx *ctx, const char *id, double val);
int expr__parse(double *final_val, struct expr_parse_ctx *ctx, const char *expr, int runtime);
+int expr__parse_custom(struct expr_parse_ctx *ctx, const char *expr);
int expr__find_other(const char *expr, const char *one, const char ***other,
int *num_other, int runtime);
diff --git a/tools/perf/util/expr.l b/tools/perf/util/expr.l
index ceab11bea6f9..c6a930ed22e6 100644
--- a/tools/perf/util/expr.l
+++ b/tools/perf/util/expr.l
@@ -81,12 +81,15 @@ static int str(yyscan_t scanner, int token, int runtime)
}
%}
+%x custom
+
number [0-9]*\.?[0-9]+
sch [-,=]
spec \\{sch}
sym [0-9a-zA-Z_\.:@?]+
symbol ({spec}|{sym})+
+all [^;]+
%%
struct expr_scanner_ctx *sctx = expr_get_extra(yyscanner);
@@ -100,6 +103,12 @@ symbol ({spec}|{sym})+
}
}
+<custom>{
+
+{all} { BEGIN(INITIAL); return str(yyscanner, ALL, sctx->runtime); }
+
+}
+
max { return MAX; }
min { return MIN; }
if { return IF; }
@@ -118,6 +127,9 @@ else { return ELSE; }
"(" { return '('; }
")" { return ')'; }
"," { return ','; }
+";" { return ';'; }
+"=" { BEGIN(custom); return '='; }
+\n { }
. { }
%%
diff --git a/tools/perf/util/expr.y b/tools/perf/util/expr.y
index 21e82a1e11a2..0521e48fa5e3 100644
--- a/tools/perf/util/expr.y
+++ b/tools/perf/util/expr.y
@@ -24,9 +24,10 @@
char *str;
}
-%token EXPR_PARSE EXPR_OTHER EXPR_ERROR
+%token EXPR_PARSE EXPR_OTHER EXPR_CUSTOM EXPR_ERROR
%token <num> NUMBER
%token <str> ID
+%token <str> ALL
%token MIN MAX IF ELSE SMT_ON
%left MIN MAX IF
%left '|'
@@ -66,6 +67,16 @@ start:
EXPR_PARSE all_expr
|
EXPR_OTHER all_other
+|
+EXPR_CUSTOM all_custom
+
+all_custom: all_custom ID '=' ALL ';'
+{
+ ctx->custom[ctx->num_custom].name = $2;
+ ctx->custom[ctx->num_custom].expr = $4;
+ ctx->num_custom++;
+}
+|
all_other: all_other other
|
--
2.25.4
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 <[email protected]>
---
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
+ EXPR: NUMBER
+ EXPR: EXPR | EXPR
+ EXPR: EXPR & EXPR
+ EXPR: EXPR ^ EXPR
+ 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
+ ...
+
+
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
Display line number on when parsing custom metrics file, like:
$ cat metrics
// IPC
mine1 = inst_retired.any / cpu_clk_unhalted.thread;
krava
$ sudo perf stat --metrics-file ./metrics -M mine1 -a -I 1000 --metric-only
failed to parse metrics file: ./metrics:4
Please note that because the grammar is flexible on new lines,
the syntax could be broken on the next 'not fitting' item and
not the first wrong word, like:
$ cat metrics
// IPC
krava
mine1 = inst_retired.any / cpu_clk_unhalted.thread;
$ sudo perf stat --metrics-file ./metrics -M mine1 -a -I 1000 --metric-only
failed to parse metrics file: ./metrics:3
Signed-off-by: Jiri Olsa <[email protected]>
---
tools/perf/tests/expr.c | 5 +++++
tools/perf/util/expr.h | 2 ++
tools/perf/util/expr.l | 10 ++++++++++
tools/perf/util/expr.y | 8 +++++---
tools/perf/util/metricgroup.c | 3 ++-
5 files changed, 24 insertions(+), 4 deletions(-)
diff --git a/tools/perf/tests/expr.c b/tools/perf/tests/expr.c
index c62e122fe719..3bffcd9f8397 100644
--- a/tools/perf/tests/expr.c
+++ b/tools/perf/tests/expr.c
@@ -84,5 +84,10 @@ int test__expr(struct test *t __maybe_unused, int subtest __maybe_unused)
zfree(&ctx.custom[i].name);
zfree(&ctx.custom[i].expr);
}
+
+ expr__ctx_init(&ctx);
+ ret = expr__parse_custom(&ctx, "IPC=INSTRUCTIONS / CYCLES;\n error");
+ TEST_ASSERT_VAL("parse custom failed", ret != 0);
+ TEST_ASSERT_VAL("parse custom count", ctx.lineno == 2);
return 0;
}
diff --git a/tools/perf/util/expr.h b/tools/perf/util/expr.h
index ef116b58a5d4..ce95dfd2ad5a 100644
--- a/tools/perf/util/expr.h
+++ b/tools/perf/util/expr.h
@@ -27,6 +27,8 @@ struct expr_parse_ctx {
struct expr_parse_custom custom[EXPR_MAX];
};
};
+ /* Set on error for custom metrics. */
+ int lineno;
};
struct expr_scanner_ctx {
diff --git a/tools/perf/util/expr.l b/tools/perf/util/expr.l
index e9b294ba09fc..718aac4316d7 100644
--- a/tools/perf/util/expr.l
+++ b/tools/perf/util/expr.l
@@ -1,6 +1,8 @@
%option prefix="expr_"
%option reentrant
%option bison-bridge
+%option bison-locations
+%option yylineno
%{
#include <linux/compiler.h>
@@ -79,6 +81,13 @@ static int str(yyscan_t scanner, int token, int runtime)
yylval->str = normalize(yylval->str, runtime);
return token;
}
+
+#define YY_USER_ACTION \
+do { \
+ yylloc->last_line = yylloc->first_line; \
+ yylloc->first_line = yylineno; \
+} while (0);
+
%}
%x custom
@@ -101,6 +110,7 @@ all [^;]+
if (sctx->start_token) {
sctx->start_token = 0;
+ yylineno = 1;
return start_token;
}
}
diff --git a/tools/perf/util/expr.y b/tools/perf/util/expr.y
index 0521e48fa5e3..812d893bcd31 100644
--- a/tools/perf/util/expr.y
+++ b/tools/perf/util/expr.y
@@ -18,6 +18,7 @@
%parse-param { struct expr_parse_ctx *ctx }
%parse-param {void *scanner}
%lex-param {void* scanner}
+%locations
%union {
double num;
@@ -39,12 +40,13 @@
%type <num> expr if_expr
%{
-static void expr_error(double *final_val __maybe_unused,
- struct expr_parse_ctx *ctx __maybe_unused,
+static void expr_error(YYLTYPE *loc, double *final_val __maybe_unused,
+ struct expr_parse_ctx *ctx,
void *scanner,
const char *s)
{
- pr_debug("%s\n", s);
+ pr_debug("%s, line %d\n", s, loc->last_line);
+ ctx->lineno = loc->last_line;
}
static int lookup_id(struct expr_parse_ctx *ctx, char *id, double *val)
diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
index 3b4d5bdb5ac6..36df43546e84 100644
--- a/tools/perf/util/metricgroup.c
+++ b/tools/perf/util/metricgroup.c
@@ -615,7 +615,8 @@ static int metricgroup__add_metric_list(const char *list, struct strbuf *events,
ret = expr__parse_custom(&ctx, buf);
free(buf);
if (ret) {
- pr_err("failed to parse metrics file: %s\n", metrics_file);
+ pr_err("failed to parse metrics file: %s:%d\n",
+ metrics_file, ctx.lineno);
return -1;
}
}
--
2.25.4
On Mon, May 11, 2020 at 1:54 PM Jiri Olsa <[email protected]> wrote:
>
> Adding support to parse metric difinitions in following form:
Typo on definitions.
> NAME = EXPRESSION ;
> NAME = EXPRESSION ;
> ...
>
> The parsed NAME and EXPRESSION will be used in following
> changes to feed the metric code with definitions from
> custom file.
>
> Signed-off-by: Jiri Olsa <[email protected]>
> ---
> tools/perf/tests/expr.c | 13 +++++++++++++
> tools/perf/util/expr.c | 6 ++++++
> tools/perf/util/expr.h | 19 +++++++++++++++++--
> tools/perf/util/expr.l | 12 ++++++++++++
> tools/perf/util/expr.y | 13 ++++++++++++-
> 5 files changed, 60 insertions(+), 3 deletions(-)
>
> diff --git a/tools/perf/tests/expr.c b/tools/perf/tests/expr.c
> index f9e8e5628836..c62e122fe719 100644
> --- a/tools/perf/tests/expr.c
> +++ b/tools/perf/tests/expr.c
> @@ -71,5 +71,18 @@ int test__expr(struct test *t __maybe_unused, int subtest __maybe_unused)
> zfree(&other[i]);
> free((void *)other);
>
> + expr__ctx_init(&ctx);
> + ret = expr__parse_custom(&ctx, "IPC=INSTRUCTIONS / CYCLES; CPI=CYCLES / INSTRUCTIONS;");
> + TEST_ASSERT_VAL("parse custom failed", ret == 0);
> + TEST_ASSERT_VAL("parse custom count", ctx.num_custom == 2);
> + TEST_ASSERT_VAL("parse custom name", !strcmp(ctx.custom[0].name, "IPC"));
> + TEST_ASSERT_VAL("parse custom name", !strcmp(ctx.custom[1].name, "CPI"));
> + TEST_ASSERT_VAL("parse custom expr", !strcmp(ctx.custom[0].expr, "INSTRUCTIONS / CYCLES"));
> + TEST_ASSERT_VAL("parse custom expr", !strcmp(ctx.custom[1].expr, "CYCLES / INSTRUCTIONS"));
> +
> + for (i = 0; i < ctx.num_custom; i++) {
> + zfree(&ctx.custom[i].name);
> + zfree(&ctx.custom[i].expr);
> + }
> return 0;
> }
> diff --git a/tools/perf/util/expr.c b/tools/perf/util/expr.c
> index 8b4ce704a68d..d744cb15c1d4 100644
> --- a/tools/perf/util/expr.c
> +++ b/tools/perf/util/expr.c
> @@ -23,6 +23,7 @@ void expr__add_id(struct expr_parse_ctx *ctx, const char *name, double val)
> void expr__ctx_init(struct expr_parse_ctx *ctx)
> {
> ctx->num_ids = 0;
> + ctx->num_custom = 0;
> }
>
> static int
> @@ -61,6 +62,11 @@ int expr__parse(double *final_val, struct expr_parse_ctx *ctx, const char *expr,
> return __expr__parse(final_val, ctx, expr, EXPR_PARSE, runtime) ? -1 : 0;
> }
>
> +int expr__parse_custom(struct expr_parse_ctx *ctx, const char *expr)
> +{
> + return __expr__parse(NULL, ctx, expr, EXPR_CUSTOM, 0);
> +}
> +
> static bool
> already_seen(const char *val, const char *one, const char **other,
> int num_other)
> diff --git a/tools/perf/util/expr.h b/tools/perf/util/expr.h
> index 40fc452b0f2b..ef116b58a5d4 100644
> --- a/tools/perf/util/expr.h
> +++ b/tools/perf/util/expr.h
> @@ -4,15 +4,29 @@
>
> #define EXPR_MAX_OTHER 64
> #define MAX_PARSE_ID EXPR_MAX_OTHER
> +#define EXPR_MAX 20
Currently deduplication of ids is done after rather than during
expression pasing, meaning hitting these limits is quite easy. This is
fixed in:
https://lore.kernel.org/lkml/[email protected]/
But not for custom expressions being added here. I plan to rebase that
work and clone hashmap from libbpf into libapi to workaround the
dependency issue.
That patch also adds expr__ctx_clear as a convenience for cleaning up
the context, and passes the context around inside of metricgroup
rather than ids.
> struct expr_parse_id {
> const char *name;
> double val;
> };
>
> +struct expr_parse_custom {
> + const char *name;
> + const char *expr;
> +};
> +
> struct expr_parse_ctx {
> - int num_ids;
> - struct expr_parse_id ids[MAX_PARSE_ID];
> + union {
> + struct {
> + int num_ids;
> + struct expr_parse_id ids[MAX_PARSE_ID];
> + };
> + struct {
> + int num_custom;
> + struct expr_parse_custom custom[EXPR_MAX];
> + };
> + };
> };
>
> struct expr_scanner_ctx {
> @@ -23,6 +37,7 @@ struct expr_scanner_ctx {
> void expr__ctx_init(struct expr_parse_ctx *ctx);
> void expr__add_id(struct expr_parse_ctx *ctx, const char *id, double val);
> int expr__parse(double *final_val, struct expr_parse_ctx *ctx, const char *expr, int runtime);
> +int expr__parse_custom(struct expr_parse_ctx *ctx, const char *expr);
> int expr__find_other(const char *expr, const char *one, const char ***other,
> int *num_other, int runtime);
>
> diff --git a/tools/perf/util/expr.l b/tools/perf/util/expr.l
> index ceab11bea6f9..c6a930ed22e6 100644
> --- a/tools/perf/util/expr.l
> +++ b/tools/perf/util/expr.l
> @@ -81,12 +81,15 @@ static int str(yyscan_t scanner, int token, int runtime)
> }
> %}
>
> +%x custom
> +
> number [0-9]*\.?[0-9]+
>
> sch [-,=]
> spec \\{sch}
> sym [0-9a-zA-Z_\.:@?]+
> symbol ({spec}|{sym})+
> +all [^;]+
>
> %%
> struct expr_scanner_ctx *sctx = expr_get_extra(yyscanner);
> @@ -100,6 +103,12 @@ symbol ({spec}|{sym})+
> }
> }
>
> +<custom>{
> +
> +{all} { BEGIN(INITIAL); return str(yyscanner, ALL, sctx->runtime); }
> +
> +}
> +
> max { return MAX; }
> min { return MIN; }
> if { return IF; }
> @@ -118,6 +127,9 @@ else { return ELSE; }
> "(" { return '('; }
> ")" { return ')'; }
> "," { return ','; }
> +";" { return ';'; }
> +"=" { BEGIN(custom); return '='; }
Will this interfere with the \\= encoded in MetricExpr? Could you test with:
https://lore.kernel.org/lkml/[email protected]/
> +\n { }
> . { }
> %%
>
> diff --git a/tools/perf/util/expr.y b/tools/perf/util/expr.y
> index 21e82a1e11a2..0521e48fa5e3 100644
> --- a/tools/perf/util/expr.y
> +++ b/tools/perf/util/expr.y
> @@ -24,9 +24,10 @@
> char *str;
> }
>
> -%token EXPR_PARSE EXPR_OTHER EXPR_ERROR
> +%token EXPR_PARSE EXPR_OTHER EXPR_CUSTOM EXPR_ERROR
> %token <num> NUMBER
> %token <str> ID
> +%token <str> ALL
Missing %destructor, fix is here:
https://lore.kernel.org/lkml/[email protected]/
Thanks,
Ian
> %token MIN MAX IF ELSE SMT_ON
> %left MIN MAX IF
> %left '|'
> @@ -66,6 +67,16 @@ start:
> EXPR_PARSE all_expr
> |
> EXPR_OTHER all_other
> +|
> +EXPR_CUSTOM all_custom
> +
> +all_custom: all_custom ID '=' ALL ';'
> +{
> + ctx->custom[ctx->num_custom].name = $2;
> + ctx->custom[ctx->num_custom].expr = $4;
> + ctx->num_custom++;
> +}
> +|
>
> all_other: all_other other
> |
> --
> 2.25.4
>
On Mon, May 11, 2020 at 1:54 PM Jiri Olsa <[email protected]> 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 <[email protected]>
> ---
> 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
>
On Mon, May 11, 2020 at 1:54 PM Jiri Olsa <[email protected]> wrote:
>
> Display line number on when parsing custom metrics file, like:
>
> $ cat metrics
> // IPC
> mine1 = inst_retired.any / cpu_clk_unhalted.thread;
>
> krava
> $ sudo perf stat --metrics-file ./metrics -M mine1 -a -I 1000 --metric-only
> failed to parse metrics file: ./metrics:4
>
> Please note that because the grammar is flexible on new lines,
> the syntax could be broken on the next 'not fitting' item and
> not the first wrong word, like:
>
> $ cat metrics
> // IPC
> krava
> mine1 = inst_retired.any / cpu_clk_unhalted.thread;
> $ sudo perf stat --metrics-file ./metrics -M mine1 -a -I 1000 --metric-only
> failed to parse metrics file: ./metrics:3
A line number is better than nothing :-) It'd be nice to be told about
broken events and more information about what's broken in the line. A
common failure is @ vs / encoding and also no-use or misuse of \\.
Perhaps expand the test coverage.
Thanks,
Ian
> Signed-off-by: Jiri Olsa <[email protected]>
> ---
> tools/perf/tests/expr.c | 5 +++++
> tools/perf/util/expr.h | 2 ++
> tools/perf/util/expr.l | 10 ++++++++++
> tools/perf/util/expr.y | 8 +++++---
> tools/perf/util/metricgroup.c | 3 ++-
> 5 files changed, 24 insertions(+), 4 deletions(-)
>
> diff --git a/tools/perf/tests/expr.c b/tools/perf/tests/expr.c
> index c62e122fe719..3bffcd9f8397 100644
> --- a/tools/perf/tests/expr.c
> +++ b/tools/perf/tests/expr.c
> @@ -84,5 +84,10 @@ int test__expr(struct test *t __maybe_unused, int subtest __maybe_unused)
> zfree(&ctx.custom[i].name);
> zfree(&ctx.custom[i].expr);
> }
> +
> + expr__ctx_init(&ctx);
> + ret = expr__parse_custom(&ctx, "IPC=INSTRUCTIONS / CYCLES;\n error");
> + TEST_ASSERT_VAL("parse custom failed", ret != 0);
> + TEST_ASSERT_VAL("parse custom count", ctx.lineno == 2);
> return 0;
> }
> diff --git a/tools/perf/util/expr.h b/tools/perf/util/expr.h
> index ef116b58a5d4..ce95dfd2ad5a 100644
> --- a/tools/perf/util/expr.h
> +++ b/tools/perf/util/expr.h
> @@ -27,6 +27,8 @@ struct expr_parse_ctx {
> struct expr_parse_custom custom[EXPR_MAX];
> };
> };
> + /* Set on error for custom metrics. */
> + int lineno;
> };
>
> struct expr_scanner_ctx {
> diff --git a/tools/perf/util/expr.l b/tools/perf/util/expr.l
> index e9b294ba09fc..718aac4316d7 100644
> --- a/tools/perf/util/expr.l
> +++ b/tools/perf/util/expr.l
> @@ -1,6 +1,8 @@
> %option prefix="expr_"
> %option reentrant
> %option bison-bridge
> +%option bison-locations
> +%option yylineno
>
> %{
> #include <linux/compiler.h>
> @@ -79,6 +81,13 @@ static int str(yyscan_t scanner, int token, int runtime)
> yylval->str = normalize(yylval->str, runtime);
> return token;
> }
> +
> +#define YY_USER_ACTION \
> +do { \
> + yylloc->last_line = yylloc->first_line; \
> + yylloc->first_line = yylineno; \
> +} while (0);
> +
> %}
>
> %x custom
> @@ -101,6 +110,7 @@ all [^;]+
>
> if (sctx->start_token) {
> sctx->start_token = 0;
> + yylineno = 1;
> return start_token;
> }
> }
> diff --git a/tools/perf/util/expr.y b/tools/perf/util/expr.y
> index 0521e48fa5e3..812d893bcd31 100644
> --- a/tools/perf/util/expr.y
> +++ b/tools/perf/util/expr.y
> @@ -18,6 +18,7 @@
> %parse-param { struct expr_parse_ctx *ctx }
> %parse-param {void *scanner}
> %lex-param {void* scanner}
> +%locations
>
> %union {
> double num;
> @@ -39,12 +40,13 @@
> %type <num> expr if_expr
>
> %{
> -static void expr_error(double *final_val __maybe_unused,
> - struct expr_parse_ctx *ctx __maybe_unused,
> +static void expr_error(YYLTYPE *loc, double *final_val __maybe_unused,
> + struct expr_parse_ctx *ctx,
> void *scanner,
> const char *s)
> {
> - pr_debug("%s\n", s);
> + pr_debug("%s, line %d\n", s, loc->last_line);
> + ctx->lineno = loc->last_line;
> }
>
> static int lookup_id(struct expr_parse_ctx *ctx, char *id, double *val)
> diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
> index 3b4d5bdb5ac6..36df43546e84 100644
> --- a/tools/perf/util/metricgroup.c
> +++ b/tools/perf/util/metricgroup.c
> @@ -615,7 +615,8 @@ static int metricgroup__add_metric_list(const char *list, struct strbuf *events,
> ret = expr__parse_custom(&ctx, buf);
> free(buf);
> if (ret) {
> - pr_err("failed to parse metrics file: %s\n", metrics_file);
> + pr_err("failed to parse metrics file: %s:%d\n",
> + metrics_file, ctx.lineno);
> return -1;
> }
> }
> --
> 2.25.4
>
On Tue, May 12, 2020 at 11:50:18PM -0700, Ian Rogers wrote:
> On Mon, May 11, 2020 at 1:54 PM Jiri Olsa <[email protected]> wrote:
> >
> > Adding support to parse metric difinitions in following form:
>
> Typo on definitions.
right
SNIP
> > +int expr__parse_custom(struct expr_parse_ctx *ctx, const char *expr)
> > +{
> > + return __expr__parse(NULL, ctx, expr, EXPR_CUSTOM, 0);
> > +}
> > +
> > static bool
> > already_seen(const char *val, const char *one, const char **other,
> > int num_other)
> > diff --git a/tools/perf/util/expr.h b/tools/perf/util/expr.h
> > index 40fc452b0f2b..ef116b58a5d4 100644
> > --- a/tools/perf/util/expr.h
> > +++ b/tools/perf/util/expr.h
> > @@ -4,15 +4,29 @@
> >
> > #define EXPR_MAX_OTHER 64
> > #define MAX_PARSE_ID EXPR_MAX_OTHER
> > +#define EXPR_MAX 20
>
> Currently deduplication of ids is done after rather than during
> expression pasing, meaning hitting these limits is quite easy. This is
> fixed in:
> https://lore.kernel.org/lkml/[email protected]/
> But not for custom expressions being added here. I plan to rebase that
> work and clone hashmap from libbpf into libapi to workaround the
> dependency issue.
> That patch also adds expr__ctx_clear as a convenience for cleaning up
> the context, and passes the context around inside of metricgroup
> rather than ids.
ok
SNIP
> > symbol ({spec}|{sym})+
> > +all [^;]+
> >
> > %%
> > struct expr_scanner_ctx *sctx = expr_get_extra(yyscanner);
> > @@ -100,6 +103,12 @@ symbol ({spec}|{sym})+
> > }
> > }
> >
> > +<custom>{
> > +
> > +{all} { BEGIN(INITIAL); return str(yyscanner, ALL, sctx->runtime); }
> > +
> > +}
> > +
> > max { return MAX; }
> > min { return MIN; }
> > if { return IF; }
> > @@ -118,6 +127,9 @@ else { return ELSE; }
> > "(" { return '('; }
> > ")" { return ')'; }
> > "," { return ','; }
> > +";" { return ';'; }
> > +"=" { BEGIN(custom); return '='; }
>
> Will this interfere with the \\= encoded in MetricExpr? Could you test with:
> https://lore.kernel.org/lkml/[email protected]/
it shouldn't, the escape is matched first.. but I'll put it on top
of the new tests to be sure
>
> > +\n { }
> > . { }
> > %%
> >
> > diff --git a/tools/perf/util/expr.y b/tools/perf/util/expr.y
> > index 21e82a1e11a2..0521e48fa5e3 100644
> > --- a/tools/perf/util/expr.y
> > +++ b/tools/perf/util/expr.y
> > @@ -24,9 +24,10 @@
> > char *str;
> > }
> >
> > -%token EXPR_PARSE EXPR_OTHER EXPR_ERROR
> > +%token EXPR_PARSE EXPR_OTHER EXPR_CUSTOM EXPR_ERROR
> > %token <num> NUMBER
> > %token <str> ID
> > +%token <str> ALL
>
> Missing %destructor, fix is here:
> https://lore.kernel.org/lkml/[email protected]/
oops, right.. thanks
jirka
On Wed, May 13, 2020 at 12:04:55AM -0700, Ian Rogers wrote:
SNIP
> > +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.
yea, I was thinking about this and I think we will put a disclaimer in
here that this is not an API and the interface can change.. it's really
mostly intended to help out with running a custom metric which is not
compiled in ... I don't want to be commited to support old 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.
AFAICS it's used only with #SMT_on in the condition, aybe we could limit
the condition only for #SMT_on term?
>
> > + 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.
is this even used anywhere? perhaps it was added just to be complete
SNIP
> > + 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.
nice idea, but let's finish straighten up what we have first ;-)
I'll try to go through all the fixes/tests you posted and let's
get it in first
thanks,
jirka
Em Wed, May 13, 2020 at 01:34:24PM +0200, Jiri Olsa escreveu:
> On Wed, May 13, 2020 at 12:09:30AM -0700, Ian Rogers wrote:
> > On Mon, May 11, 2020 at 1:54 PM Jiri Olsa <[email protected]> wrote:
> > >
> > > Display line number on when parsing custom metrics file, like:
> > >
> > > $ cat metrics
> > > // IPC
> > > mine1 = inst_retired.any / cpu_clk_unhalted.thread;
> > >
> > > krava
> > > $ sudo perf stat --metrics-file ./metrics -M mine1 -a -I 1000 --metric-only
> > > failed to parse metrics file: ./metrics:4
> > >
> > > Please note that because the grammar is flexible on new lines,
> > > the syntax could be broken on the next 'not fitting' item and
> > > not the first wrong word, like:
> > >
> > > $ cat metrics
> > > // IPC
> > > krava
> > > mine1 = inst_retired.any / cpu_clk_unhalted.thread;
> > > $ sudo perf stat --metrics-file ./metrics -M mine1 -a -I 1000 --metric-only
> > > failed to parse metrics file: ./metrics:3
> >
> > A line number is better than nothing :-) It'd be nice to be told about
> > broken events and more information about what's broken in the line. A
> > common failure is @ vs / encoding and also no-use or misuse of \\.
> > Perhaps expand the test coverage.
>
> yep, error reporting needs more changes.. but the line is crucial ;-)
So I had started processing this patchkit, I assume you will send a v2
and I should drop what I had processed, is that ok?
- Arnaldo
On Wed, May 13, 2020 at 12:09:30AM -0700, Ian Rogers wrote:
> On Mon, May 11, 2020 at 1:54 PM Jiri Olsa <[email protected]> wrote:
> >
> > Display line number on when parsing custom metrics file, like:
> >
> > $ cat metrics
> > // IPC
> > mine1 = inst_retired.any / cpu_clk_unhalted.thread;
> >
> > krava
> > $ sudo perf stat --metrics-file ./metrics -M mine1 -a -I 1000 --metric-only
> > failed to parse metrics file: ./metrics:4
> >
> > Please note that because the grammar is flexible on new lines,
> > the syntax could be broken on the next 'not fitting' item and
> > not the first wrong word, like:
> >
> > $ cat metrics
> > // IPC
> > krava
> > mine1 = inst_retired.any / cpu_clk_unhalted.thread;
> > $ sudo perf stat --metrics-file ./metrics -M mine1 -a -I 1000 --metric-only
> > failed to parse metrics file: ./metrics:3
>
> A line number is better than nothing :-) It'd be nice to be told about
> broken events and more information about what's broken in the line. A
> common failure is @ vs / encoding and also no-use or misuse of \\.
> Perhaps expand the test coverage.
yep, error reporting needs more changes.. but the line is crucial ;-)
jirka
On Wed, May 13, 2020 at 11:08:25AM -0300, Arnaldo Carvalho de Melo wrote:
> Em Wed, May 13, 2020 at 01:34:24PM +0200, Jiri Olsa escreveu:
> > On Wed, May 13, 2020 at 12:09:30AM -0700, Ian Rogers wrote:
> > > On Mon, May 11, 2020 at 1:54 PM Jiri Olsa <[email protected]> wrote:
> > > >
> > > > Display line number on when parsing custom metrics file, like:
> > > >
> > > > $ cat metrics
> > > > // IPC
> > > > mine1 = inst_retired.any / cpu_clk_unhalted.thread;
> > > >
> > > > krava
> > > > $ sudo perf stat --metrics-file ./metrics -M mine1 -a -I 1000 --metric-only
> > > > failed to parse metrics file: ./metrics:4
> > > >
> > > > Please note that because the grammar is flexible on new lines,
> > > > the syntax could be broken on the next 'not fitting' item and
> > > > not the first wrong word, like:
> > > >
> > > > $ cat metrics
> > > > // IPC
> > > > krava
> > > > mine1 = inst_retired.any / cpu_clk_unhalted.thread;
> > > > $ sudo perf stat --metrics-file ./metrics -M mine1 -a -I 1000 --metric-only
> > > > failed to parse metrics file: ./metrics:3
> > >
> > > A line number is better than nothing :-) It'd be nice to be told about
> > > broken events and more information about what's broken in the line. A
> > > common failure is @ vs / encoding and also no-use or misuse of \\.
> > > Perhaps expand the test coverage.
> >
> > yep, error reporting needs more changes.. but the line is crucial ;-)
>
> So I had started processing this patchkit, I assume you will send a v2
> and I should drop what I had processed, is that ok?
yes, I will resubmit on top of the other expr changes
we have now pending
jirka
Em Wed, May 13, 2020 at 04:46:10PM +0200, Jiri Olsa escreveu:
> On Wed, May 13, 2020 at 11:08:25AM -0300, Arnaldo Carvalho de Melo wrote:
> > Em Wed, May 13, 2020 at 01:34:24PM +0200, Jiri Olsa escreveu:
> > > On Wed, May 13, 2020 at 12:09:30AM -0700, Ian Rogers wrote:
> > > > On Mon, May 11, 2020 at 1:54 PM Jiri Olsa <[email protected]> wrote:
> > > > >
> > > > > Display line number on when parsing custom metrics file, like:
> > > > >
> > > > > $ cat metrics
> > > > > // IPC
> > > > > mine1 = inst_retired.any / cpu_clk_unhalted.thread;
> > > > >
> > > > > krava
> > > > > $ sudo perf stat --metrics-file ./metrics -M mine1 -a -I 1000 --metric-only
> > > > > failed to parse metrics file: ./metrics:4
> > > > >
> > > > > Please note that because the grammar is flexible on new lines,
> > > > > the syntax could be broken on the next 'not fitting' item and
> > > > > not the first wrong word, like:
> > > > >
> > > > > $ cat metrics
> > > > > // IPC
> > > > > krava
> > > > > mine1 = inst_retired.any / cpu_clk_unhalted.thread;
> > > > > $ sudo perf stat --metrics-file ./metrics -M mine1 -a -I 1000 --metric-only
> > > > > failed to parse metrics file: ./metrics:3
> > > >
> > > > A line number is better than nothing :-) It'd be nice to be told about
> > > > broken events and more information about what's broken in the line. A
> > > > common failure is @ vs / encoding and also no-use or misuse of \\.
> > > > Perhaps expand the test coverage.
> > >
> > > yep, error reporting needs more changes.. but the line is crucial ;-)
> >
> > So I had started processing this patchkit, I assume you will send a v2
> > and I should drop what I had processed, is that ok?
>
> yes, I will resubmit on top of the other expr changes
> we have now pending
Ok, I removed those from my local branch, and pushed what I have, which
includes a few more fixes from Ian and others, continuing to process
other remaining patches now.
- Arnaldo
On Wed, May 13, 2020 at 4:33 AM Jiri Olsa <[email protected]> wrote:
>
> On Wed, May 13, 2020 at 12:04:55AM -0700, Ian Rogers wrote:
>
> SNIP
>
> > > +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.
>
> yea, I was thinking about this and I think we will put a disclaimer in
> here that this is not an API and the interface can change.. it's really
> mostly intended to help out with running a custom metric which is not
> compiled in ... I don't want to be commited to support old 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.
>
> AFAICS it's used only with #SMT_on in the condition, aybe we could limit
> the condition only for #SMT_on term?
>
>
> >
> > > + 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.
>
> is this even used anywhere? perhaps it was added just to be complete
I don't believe they are used and checked with the pmu parsing test
that removing them doesn't cause any x86 expressions to break. I'd
prefer it if we could remove the unused operators and avoid
advertising here.
Thanks,
Ian
> SNIP
>
> > > + 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.
>
> nice idea, but let's finish straighten up what we have first ;-)
>
> I'll try to go through all the fixes/tests you posted and let's
> get it in first
>
> thanks,
> jirka
>
On 11/05/2020 21:53, Jiri Olsa wrote:
+
Hi jirka,
Did you a plan to continue to work? I don't think that this support was
ever merged.
We have a requirement to be able to tune parameters of metrics, and
support here seems suitable.
Thanks,
John
> hi,
> Joe asked for possibility to add user defined metrics. Given that
> we already have metrics support, I added --metrics-file option that
> allows to specify custom metrics.
>
> $ cat metrics
> # IPC
> mine1 = instructions / cycles;
> /* 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 --metric-only -a -I 1000
> # time insn per cycle mine1 mine2
> 1.000536263 0.71 0.7 41.4
> 2.002069025 0.31 0.3 14.1
> 3.003427684 0.27 0.3 14.8
> 4.004807132 0.25 0.2 12.1
> ...
>
> v3 changes:
> - added doc for metrics file in perf stat man page
> - reporting error line number now
> - changed '#' style comment to C way with '//'
>
> v2 changes:
> - add new --metrics-file option
> - rebased on current perf/core expression bison/flex enhancements
>
> Also available in:
> git://git.kernel.org/pub/scm/linux/kernel/git/jolsa/perf.git
> perf/metric
>
> thanks,
> jirka
>
>
> ---
> Jiri Olsa (4):
> perf expr: Add parsing support for multiple expressions
> perf expr: Allow comments in custom metric file
> perf stat: Add --metrics-file option
> perf expr: Report line number with error
>
> tools/perf/Documentation/perf-stat.txt | 77 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> tools/perf/builtin-stat.c | 7 +++++--
> tools/perf/tests/expr.c | 18 ++++++++++++++++++
> tools/perf/util/expr.c | 6 ++++++
> tools/perf/util/expr.h | 21 +++++++++++++++++++--
> tools/perf/util/expr.l | 34 ++++++++++++++++++++++++++++++++++
> tools/perf/util/expr.y | 21 +++++++++++++++++----
> tools/perf/util/metricgroup.c | 70 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-------
> tools/perf/util/metricgroup.h | 3 ++-
> tools/perf/util/stat.h | 1 +
> 10 files changed, 242 insertions(+), 16 deletions(-)
>
> .
>
On January 25, 2022 9:34:19 AM GMT-03:00, John Garry <[email protected]> wrote:
>On 11/05/2020 21:53, Jiri Olsa wrote:
>
>+
>
>Hi jirka,
>
>Did you a plan to continue to work? I don't think that this support was
>ever merged.
>
>We have a requirement to be able to tune parameters of metrics, and
>support here seems suitable.
John,
Have you tried to apply that series to see if it still applies?
- Arnaldo
>
>Thanks,
>John
>
>> hi,
>> Joe asked for possibility to add user defined metrics. Given that
>> we already have metrics support, I added --metrics-file option that
>> allows to specify custom metrics.
>>
>> $ cat metrics
>> # IPC
>> mine1 = instructions / cycles;
>> /* 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 --metric-only -a -I 1000
>> # time insn per cycle mine1 mine2
>> 1.000536263 0.71 0.7 41.4
>> 2.002069025 0.31 0.3 14.1
>> 3.003427684 0.27 0.3 14.8
>> 4.004807132 0.25 0.2 12.1
>> ...
>>
>> v3 changes:
>> - added doc for metrics file in perf stat man page
>> - reporting error line number now
>> - changed '#' style comment to C way with '//'
>>
>> v2 changes:
>> - add new --metrics-file option
>> - rebased on current perf/core expression bison/flex enhancements
>>
>> Also available in:
>> git://git.kernel.org/pub/scm/linux/kernel/git/jolsa/perf.git
>> perf/metric
>>
>> thanks,
>> jirka
>>
>>
>> ---
>> Jiri Olsa (4):
>> perf expr: Add parsing support for multiple expressions
>> perf expr: Allow comments in custom metric file
>> perf stat: Add --metrics-file option
>> perf expr: Report line number with error
>>
>> tools/perf/Documentation/perf-stat.txt | 77 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>> tools/perf/builtin-stat.c | 7 +++++--
>> tools/perf/tests/expr.c | 18 ++++++++++++++++++
>> tools/perf/util/expr.c | 6 ++++++
>> tools/perf/util/expr.h | 21 +++++++++++++++++++--
>> tools/perf/util/expr.l | 34 ++++++++++++++++++++++++++++++++++
>> tools/perf/util/expr.y | 21 +++++++++++++++++----
>> tools/perf/util/metricgroup.c | 70 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-------
>> tools/perf/util/metricgroup.h | 3 ++-
>> tools/perf/util/stat.h | 1 +
>> 10 files changed, 242 insertions(+), 16 deletions(-)
>>
>> .
>>
>