Messages in this thread | | | From | Ian Rogers <> | Date | Tue, 12 May 2020 23:50:18 -0700 | Subject | Re: [PATCH 1/4] perf expr: Add parsing support for multiple expressions |
| |
On Mon, May 11, 2020 at 1:54 PM Jiri Olsa <jolsa@kernel.org> 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 <jolsa@kernel.org> > --- > 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/20200508053629.210324-8-irogers@google.com/ 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/20200513062752.3681-2-irogers@google.com/
> +\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/20200513000318.15166-1-irogers@google.com/
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 >
| |