Messages in this thread | | | Subject | Re: [PATCH v2 05/21] perf metric: Move runtime value to the expr context | From | kajoljain <> | Date | Tue, 26 Oct 2021 13:40:48 +0530 |
| |
On 10/15/21 10:51 PM, Ian Rogers wrote: > The runtime value is needed when recursively parsing metrics, currently > a value of 1 is passed which is incorrect. Rather than add more > arguments to the bison parser, add runtime to the context. Fix call > sites not to pass a value. The runtime value is defaulted to 0, which is > arbitrary. In some places this replaces a value of 1, which was also > arbitrary. This shouldn't affect anything other than PPC. The use of > 0 or 1 shouldn't matter as a proper runtime value would be needed in a > case that it did matter. > > Acked-by: Andi Kleen <ak@linux.intel.com> > Signed-off-by: Ian Rogers <irogers@google.com>
Patch looks good to me.
Reviewed-by: Kajol Jain<kjain@linux.ibm.com>
Thanks, Kajol Jain
> --- > tools/perf/tests/expr.c | 15 ++++++++------- > tools/perf/tests/pmu-events.c | 10 +++++----- > tools/perf/util/expr.c | 15 ++++++++------- > tools/perf/util/expr.h | 5 +++-- > tools/perf/util/metricgroup.c | 7 +++---- > tools/perf/util/stat-shadow.c | 7 ++++--- > 6 files changed, 31 insertions(+), 28 deletions(-) > > diff --git a/tools/perf/tests/expr.c b/tools/perf/tests/expr.c > index f1d8411fce12..3c16f3df1980 100644 > --- a/tools/perf/tests/expr.c > +++ b/tools/perf/tests/expr.c > @@ -56,7 +56,7 @@ static int test(struct expr_parse_ctx *ctx, const char *e, double val2) > { > double val; > > - if (expr__parse(&val, ctx, e, 1)) > + if (expr__parse(&val, ctx, e)) > TEST_ASSERT_VAL("parse test failed", 0); > TEST_ASSERT_VAL("unexpected value", val == val2); > return 0; > @@ -104,17 +104,17 @@ int test__expr(struct test *t __maybe_unused, int subtest __maybe_unused) > } > > p = "FOO/0"; > - ret = expr__parse(&val, ctx, p, 1); > + ret = expr__parse(&val, ctx, p); > TEST_ASSERT_VAL("division by zero", ret == -1); > > p = "BAR/"; > - ret = expr__parse(&val, ctx, p, 1); > + ret = expr__parse(&val, ctx, p); > TEST_ASSERT_VAL("missing operand", ret == -1); > > expr__ctx_clear(ctx); > TEST_ASSERT_VAL("find ids", > expr__find_ids("FOO + BAR + BAZ + BOZO", "FOO", > - ctx, 1) == 0); > + ctx) == 0); > TEST_ASSERT_VAL("find ids", hashmap__size(ctx->ids) == 3); > TEST_ASSERT_VAL("find ids", hashmap__find(ctx->ids, "BAR", > (void **)&val_ptr)); > @@ -124,9 +124,10 @@ int test__expr(struct test *t __maybe_unused, int subtest __maybe_unused) > (void **)&val_ptr)); > > expr__ctx_clear(ctx); > + ctx->runtime = 3; > TEST_ASSERT_VAL("find ids", > expr__find_ids("EVENT1\\,param\\=?@ + EVENT2\\,param\\=?@", > - NULL, ctx, 3) == 0); > + NULL, ctx) == 0); > TEST_ASSERT_VAL("find ids", hashmap__size(ctx->ids) == 2); > TEST_ASSERT_VAL("find ids", hashmap__find(ctx->ids, "EVENT1,param=3/", > (void **)&val_ptr)); > @@ -137,7 +138,7 @@ int test__expr(struct test *t __maybe_unused, int subtest __maybe_unused) > expr__ctx_clear(ctx); > TEST_ASSERT_VAL("find ids", > expr__find_ids("EVENT1 if #smt_on else EVENT2", > - NULL, ctx, 0) == 0); > + NULL, ctx) == 0); > TEST_ASSERT_VAL("find ids", hashmap__size(ctx->ids) == 1); > TEST_ASSERT_VAL("find ids", hashmap__find(ctx->ids, > smt_on() ? "EVENT1" : "EVENT2", > @@ -147,7 +148,7 @@ int test__expr(struct test *t __maybe_unused, int subtest __maybe_unused) > expr__ctx_clear(ctx); > TEST_ASSERT_VAL("find ids", > expr__find_ids("1.0 if EVENT1 > 100.0 else 1.0", > - NULL, ctx, 0) == 0); > + NULL, ctx) == 0); > TEST_ASSERT_VAL("find ids", hashmap__size(ctx->ids) == 0); > > expr__ctx_free(ctx); > diff --git a/tools/perf/tests/pmu-events.c b/tools/perf/tests/pmu-events.c > index cc5cea141beb..71b08c296410 100644 > --- a/tools/perf/tests/pmu-events.c > +++ b/tools/perf/tests/pmu-events.c > @@ -866,7 +866,7 @@ static int resolve_metric_simple(struct expr_parse_ctx *pctx, > ref->metric_expr = pe->metric_expr; > list_add_tail(&metric->list, compound_list); > > - rc = expr__find_ids(pe->metric_expr, NULL, pctx, 0); > + rc = expr__find_ids(pe->metric_expr, NULL, pctx); > if (rc) > goto out_err; > break; /* The hashmap has been modified, so restart */ > @@ -916,7 +916,7 @@ static int test_parsing(void) > if (!pe->metric_expr) > continue; > expr__ctx_clear(ctx); > - if (expr__find_ids(pe->metric_expr, NULL, ctx, 0) < 0) { > + if (expr__find_ids(pe->metric_expr, NULL, ctx) < 0) { > expr_failure("Parse find ids failed", map, pe); > ret++; > continue; > @@ -949,7 +949,7 @@ static int test_parsing(void) > free(metric); > } > > - if (expr__parse(&result, ctx, pe->metric_expr, 0)) { > + if (expr__parse(&result, ctx, pe->metric_expr)) { > expr_failure("Parse failed", map, pe); > ret++; > } > @@ -989,7 +989,7 @@ static int metric_parse_fake(const char *str) > pr_debug("expr__ctx_new failed"); > return TEST_FAIL; > } > - if (expr__find_ids(str, NULL, ctx, 0) < 0) { > + if (expr__find_ids(str, NULL, ctx) < 0) { > pr_err("expr__find_ids failed\n"); > return -1; > } > @@ -1010,7 +1010,7 @@ static int metric_parse_fake(const char *str) > } > } > > - if (expr__parse(&result, ctx, str, 0)) > + if (expr__parse(&result, ctx, str)) > pr_err("expr__parse failed\n"); > else > ret = 0; > diff --git a/tools/perf/util/expr.c b/tools/perf/util/expr.c > index db2445677c8c..62fb39fd4d9d 100644 > --- a/tools/perf/util/expr.c > +++ b/tools/perf/util/expr.c > @@ -246,7 +246,7 @@ int expr__resolve_id(struct expr_parse_ctx *ctx, const char *id, > data->ref.metric_name); > pr_debug("processing metric: %s ENTRY\n", id); > data->kind = EXPR_ID_DATA__REF_VALUE; > - if (expr__parse(&data->ref.val, ctx, data->ref.metric_expr, 1)) { > + if (expr__parse(&data->ref.val, ctx, data->ref.metric_expr)) { > pr_debug("%s failed to count\n", id); > return -1; > } > @@ -284,6 +284,7 @@ struct expr_parse_ctx *expr__ctx_new(void) > > ctx->ids = hashmap__new(key_hash, key_equal, NULL); > ctx->parent = NULL; > + ctx->runtime = 0; > return ctx; > } > > @@ -314,10 +315,10 @@ void expr__ctx_free(struct expr_parse_ctx *ctx) > > static int > __expr__parse(double *val, struct expr_parse_ctx *ctx, const char *expr, > - bool compute_ids, int runtime) > + bool compute_ids) > { > struct expr_scanner_ctx scanner_ctx = { > - .runtime = runtime, > + .runtime = ctx->runtime, > }; > YY_BUFFER_STATE buffer; > void *scanner; > @@ -345,15 +346,15 @@ __expr__parse(double *val, struct expr_parse_ctx *ctx, const char *expr, > } > > int expr__parse(double *final_val, struct expr_parse_ctx *ctx, > - const char *expr, int runtime) > + const char *expr) > { > - return __expr__parse(final_val, ctx, expr, /*compute_ids=*/false, runtime) ? -1 : 0; > + return __expr__parse(final_val, ctx, expr, /*compute_ids=*/false) ? -1 : 0; > } > > int expr__find_ids(const char *expr, const char *one, > - struct expr_parse_ctx *ctx, int runtime) > + struct expr_parse_ctx *ctx) > { > - int ret = __expr__parse(NULL, ctx, expr, /*compute_ids=*/true, runtime); > + int ret = __expr__parse(NULL, ctx, expr, /*compute_ids=*/true); > > if (one) > expr__del_id(ctx, one); > diff --git a/tools/perf/util/expr.h b/tools/perf/util/expr.h > index b20513f0ae59..124475a4f245 100644 > --- a/tools/perf/util/expr.h > +++ b/tools/perf/util/expr.h > @@ -21,6 +21,7 @@ struct expr_id { > struct expr_parse_ctx { > struct hashmap *ids; > struct expr_id *parent; > + int runtime; > }; > > struct expr_id_data; > @@ -52,10 +53,10 @@ int expr__resolve_id(struct expr_parse_ctx *ctx, const char *id, > struct expr_id_data **datap); > > int expr__parse(double *final_val, struct expr_parse_ctx *ctx, > - const char *expr, int runtime); > + const char *expr); > > int expr__find_ids(const char *expr, const char *one, > - struct expr_parse_ctx *ids, int runtime); > + struct expr_parse_ctx *ids); > > double expr_id_data__value(const struct expr_id_data *data); > struct expr_id *expr_id_data__parent(struct expr_id_data *data); > diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c > index b60ccbbf0829..139f4a793f92 100644 > --- a/tools/perf/util/metricgroup.c > +++ b/tools/perf/util/metricgroup.c > @@ -124,7 +124,6 @@ struct metric { > const char *metric_unit; > struct list_head metric_refs; > int metric_refs_cnt; > - int runtime; > bool has_constraint; > }; > > @@ -391,7 +390,7 @@ static int metricgroup__setup_events(struct list_head *groups, > expr->metric_name = m->metric_name; > expr->metric_unit = m->metric_unit; > expr->metric_events = metric_events; > - expr->runtime = m->runtime; > + expr->runtime = m->pctx->runtime; > list_add(&expr->nd, &me->head); > } > > @@ -812,7 +811,7 @@ static int __add_metric(struct list_head *metric_list, > m->metric_name = pe->metric_name; > m->metric_expr = pe->metric_expr; > m->metric_unit = pe->unit; > - m->runtime = runtime; > + m->pctx->runtime = runtime; > m->has_constraint = metric_no_group || metricgroup__has_constraint(pe); > INIT_LIST_HEAD(&m->metric_refs); > m->metric_refs_cnt = 0; > @@ -862,7 +861,7 @@ static int __add_metric(struct list_head *metric_list, > * For both the parent and referenced metrics, we parse > * all the metric's IDs and add it to the parent context. > */ > - if (expr__find_ids(pe->metric_expr, NULL, m->pctx, runtime) < 0) { > + if (expr__find_ids(pe->metric_expr, NULL, m->pctx) < 0) { > if (m->metric_refs_cnt == 0) { > expr__ctx_free(m->pctx); > free(m); > diff --git a/tools/perf/util/stat-shadow.c b/tools/perf/util/stat-shadow.c > index 9bc841e09a0c..20f1b9d0f272 100644 > --- a/tools/perf/util/stat-shadow.c > +++ b/tools/perf/util/stat-shadow.c > @@ -394,7 +394,7 @@ void perf_stat__collect_metric_expr(struct evlist *evsel_list) > if (!metric_events) { > if (expr__find_ids(counter->metric_expr, > counter->name, > - ctx, 1) < 0) > + ctx) < 0) > continue; > > metric_events = calloc(sizeof(struct evsel *), > @@ -894,13 +894,14 @@ static void generic_metric(struct perf_stat_config *config, > if (!pctx) > return; > > + pctx->runtime = runtime; > i = prepare_metric(metric_events, metric_refs, pctx, cpu, st); > if (i < 0) { > expr__ctx_free(pctx); > return; > } > if (!metric_events[i]) { > - if (expr__parse(&ratio, pctx, metric_expr, runtime) == 0) { > + if (expr__parse(&ratio, pctx, metric_expr) == 0) { > char *unit; > char metric_bf[64]; > > @@ -951,7 +952,7 @@ double test_generic_metric(struct metric_expr *mexp, int cpu, struct runtime_sta > if (prepare_metric(mexp->metric_events, mexp->metric_refs, pctx, cpu, st) < 0) > goto out; > > - if (expr__parse(&ratio, pctx, mexp->metric_expr, 1)) > + if (expr__parse(&ratio, pctx, mexp->metric_expr)) > ratio = 0.0; > > out: >
| |