Re: [PATCH v1 1/3] perf stat: Pass fewer metric arguments

From: Ian Rogers
Date: Mon Feb 05 2024 - 21:24:12 EST

Next message: SeongJae Park: "Re: [PATCH] mm/swap: fix race condition in direct swapin path"
Previous message: Chengming Zhou: "Re: [PATCH] mm/zswap: invalidate old entry when store fail or !zswap_enabled"
In reply to: Namhyung Kim: "Re: [PATCH v1 1/3] perf stat: Pass fewer metric arguments"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Mon, Feb 5, 2024 at 5:59 PM Namhyung Kim <namhyung@xxxxxxxxxx> wrote:
>
> On Thu, Feb 1, 2024 at 6:25 PM Ian Rogers <irogers@xxxxxxxxxx> wrote:
> >
> > Pass metric_expr and evsel rather than specific variables from the
> > struct, thereby reducing the number of arguments. This will enable
> > later fixes.
> >
> > Signed-off-by: Ian Rogers <irogers@xxxxxxxxxx>
> > ---
> > tools/perf/util/stat-shadow.c | 75 +++++++++++++++--------------------
> > 1 file changed, 33 insertions(+), 42 deletions(-)
> >
> > diff --git a/tools/perf/util/stat-shadow.c b/tools/perf/util/stat-shadow.c
> > index e31426167852..f6c9d2f98835 100644
> > --- a/tools/perf/util/stat-shadow.c
> > +++ b/tools/perf/util/stat-shadow.c
> > @@ -355,23 +355,22 @@ static void print_nsecs(struct perf_stat_config *config,
> > print_metric(config, ctxp, NULL, NULL, "CPUs utilized", 0);
> > }
> >
> > -static int prepare_metric(struct evsel **metric_events,
> > - struct metric_ref *metric_refs,
> > +static int prepare_metric(const struct metric_expr *mexp,
> > struct expr_parse_ctx *pctx,
> > int aggr_idx)
> > {
> > int i;
>
> You may add local variables with the same name to reduce
> the amount of diff.

I'll see what I can do in v2.

Thanks,
Ian

> Thanks,
> Namhyung
>
> >
> > - for (i = 0; metric_events[i]; i++) {
> > + for (i = 0; mexp->metric_events[i]; i++) {
> > char *n;
> > double val;
> > int source_count = 0;
> >
> > - if (evsel__is_tool(metric_events[i])) {
> > + if (evsel__is_tool(mexp->metric_events[i])) {
> > struct stats *stats;
> > double scale;
> >
> > - switch (metric_events[i]->tool_event) {
> > + switch (mexp->metric_events[i]->tool_event) {
> > case PERF_TOOL_DURATION_TIME:
> > stats = &walltime_nsecs_stats;
> > scale = 1e-9;
> > @@ -391,19 +390,20 @@ static int prepare_metric(struct evsel **metric_events,
> > pr_err("Invalid tool event 'max'");
> > abort();
> > default:
> > - pr_err("Unknown tool event '%s'", evsel__name(metric_events[i]));
> > + pr_err("Unknown tool event '%s'",
> > + evsel__name(mexp->metric_events[i]));
> > abort();
> > }
> > val = avg_stats(stats) * scale;
> > source_count = 1;
> > } else {
> > - struct perf_stat_evsel *ps = metric_events[i]->stats;
> > + struct perf_stat_evsel *ps = mexp->metric_events[i]->stats;
> > struct perf_stat_aggr *aggr = &ps->aggr[aggr_idx];
> >
> > if (!aggr)
> > break;
> >
> > - if (!metric_events[i]->supported) {
> > + if (!mexp->metric_events[i]->supported) {
> > /*
> > * Not supported events will have a count of 0,
> > * which can be confusing in a
> > @@ -419,19 +419,19 @@ static int prepare_metric(struct evsel **metric_events,
> > * reverse the scale before computing the
> > * metric.
> > */
> > - val = aggr->counts.val * (1.0 / metric_events[i]->scale);
> > - source_count = evsel__source_count(metric_events[i]);
> > + val = aggr->counts.val * (1.0 / mexp->metric_events[i]->scale);
> > + source_count = evsel__source_count(mexp->metric_events[i]);
> > }
> > }
> > - n = strdup(evsel__metric_id(metric_events[i]));
> > + n = strdup(evsel__metric_id(mexp->metric_events[i]));
> > if (!n)
> > return -ENOMEM;
> >
> > expr__add_id_val_source_count(pctx, n, val, source_count);
> > }
> >
> > - for (int j = 0; metric_refs && metric_refs[j].metric_name; j++) {
> > - int ret = expr__add_ref(pctx, &metric_refs[j]);
> > + for (int j = 0; mexp->metric_refs && mexp->metric_refs[j].metric_name; j++) {
> > + int ret = expr__add_ref(pctx, &mexp->metric_refs[j]);
> >
> > if (ret)
> > return ret;
> > @@ -441,14 +441,8 @@ static int prepare_metric(struct evsel **metric_events,
> > }
> >
> > static void generic_metric(struct perf_stat_config *config,
> > - const char *metric_expr,
> > - const char *metric_threshold,
> > - struct evsel **metric_events,
> > - struct metric_ref *metric_refs,
> > - char *name,
> > - const char *metric_name,
> > - const char *metric_unit,
> > - int runtime,
> > + struct metric_expr *mexp,
> > + struct evsel *evsel,
> > int aggr_idx,
> > struct perf_stat_output_ctx *out)
> > {
> > @@ -465,55 +459,55 @@ static void generic_metric(struct perf_stat_config *config,
> >
> > if (config->user_requested_cpu_list)
> > pctx->sctx.user_requested_cpu_list = strdup(config->user_requested_cpu_list);
> > - pctx->sctx.runtime = runtime;
> > + pctx->sctx.runtime = mexp->runtime;
> > pctx->sctx.system_wide = config->system_wide;
> > - i = prepare_metric(metric_events, metric_refs, pctx, aggr_idx);
> > + i = prepare_metric(mexp, pctx, aggr_idx);
> > if (i < 0) {
> > expr__ctx_free(pctx);
> > return;
> > }
> > - if (!metric_events[i]) {
> > - if (expr__parse(&ratio, pctx, metric_expr) == 0) {
> > + if (!mexp->metric_events[i]) {
> > + if (expr__parse(&ratio, pctx, mexp->metric_expr) == 0) {
> > char *unit;
> > char metric_bf[64];
> >
> > - if (metric_threshold &&
> > - expr__parse(&threshold, pctx, metric_threshold) == 0 &&
> > + if (mexp->metric_threshold &&
> > + expr__parse(&threshold, pctx, mexp->metric_threshold) == 0 &&
> > !isnan(threshold)) {
> > color = fpclassify(threshold) == FP_ZERO
> > ? PERF_COLOR_GREEN : PERF_COLOR_RED;
> > }
> >
> > - if (metric_unit && metric_name) {
> > - if (perf_pmu__convert_scale(metric_unit,
> > + if (mexp->metric_unit && mexp->metric_name) {
> > + if (perf_pmu__convert_scale(mexp->metric_unit,
> > &unit, &scale) >= 0) {
> > ratio *= scale;
> > }
> > - if (strstr(metric_expr, "?"))
> > + if (strstr(mexp->metric_expr, "?"))
> > scnprintf(metric_bf, sizeof(metric_bf),
> > - "%s %s_%d", unit, metric_name, runtime);
> > + "%s %s_%d", unit, mexp->metric_name, mexp->runtime);
> > else
> > scnprintf(metric_bf, sizeof(metric_bf),
> > - "%s %s", unit, metric_name);
> > + "%s %s", unit, mexp->metric_name);
> >
> > print_metric(config, ctxp, color, "%8.1f",
> > metric_bf, ratio);
> > } else {
> > print_metric(config, ctxp, color, "%8.2f",
> > - metric_name ?
> > - metric_name :
> > - out->force_header ? name : "",
> > + mexp->metric_name ?
> > + mexp->metric_name :
> > + out->force_header ? evsel->name : "",
> > ratio);
> > }
> > } else {
> > print_metric(config, ctxp, color, /*unit=*/NULL,
> > out->force_header ?
> > - (metric_name ? metric_name : name) : "", 0);
> > + (mexp->metric_name ?: evsel->name) : "", 0);
> > }
> > } else {
> > print_metric(config, ctxp, color, /*unit=*/NULL,
> > out->force_header ?
> > - (metric_name ? metric_name : name) : "", 0);
> > + (mexp->metric_name ?: evsel->name) : "", 0);
> > }
> >
> > expr__ctx_free(pctx);
> > @@ -528,7 +522,7 @@ double test_generic_metric(struct metric_expr *mexp, int aggr_idx)
> > if (!pctx)
> > return NAN;
> >
> > - if (prepare_metric(mexp->metric_events, mexp->metric_refs, pctx, aggr_idx) < 0)
> > + if (prepare_metric(mexp, pctx, aggr_idx) < 0)
> > goto out;
> >
> > if (expr__parse(&ratio, pctx, mexp->metric_expr))
> > @@ -630,10 +624,7 @@ void *perf_stat__print_shadow_stats_metricgroup(struct perf_stat_config *config,
> >
> > if ((*num)++ > 0)
> > out->new_line(config, ctxp);
> > - generic_metric(config, mexp->metric_expr, mexp->metric_threshold,
> > - mexp->metric_events, mexp->metric_refs, evsel->name,
> > - mexp->metric_name, mexp->metric_unit, mexp->runtime,
> > - aggr_idx, out);
> > + generic_metric(config, mexp, evsel, aggr_idx, out);
> > }
> >
> > return NULL;
> > --
> > 2.43.0.594.gd9cf4e227d-goog
> >

Next message: SeongJae Park: "Re: [PATCH] mm/swap: fix race condition in direct swapin path"
Previous message: Chengming Zhou: "Re: [PATCH] mm/zswap: invalidate old entry when store fail or !zswap_enabled"
In reply to: Namhyung Kim: "Re: [PATCH v1 1/3] perf stat: Pass fewer metric arguments"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]