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

From: Namhyung Kim
Date: Mon Feb 05 2024 - 20:59:36 EST

Next message: Martin K. Petersen: "Re: [PATCH] scsi: tcm_loop: make tcm_loop_lld_bus const"
Previous message: Joe Damato: "Re: [PATCH net-next] eth: mlx5: link NAPI instances to queues and IRQs"
In reply to: Ian Rogers: "[PATCH v1 3/3] perf stat: Fix metric-only aggregation index"
Next in thread: Ian Rogers: "Re: [PATCH v1 1/3] perf stat: Pass fewer metric arguments"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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.

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: Martin K. Petersen: "Re: [PATCH] scsi: tcm_loop: make tcm_loop_lld_bus const"
Previous message: Joe Damato: "Re: [PATCH net-next] eth: mlx5: link NAPI instances to queues and IRQs"
In reply to: Ian Rogers: "[PATCH v1 3/3] perf stat: Fix metric-only aggregation index"
Next in thread: Ian Rogers: "Re: [PATCH v1 1/3] perf stat: Pass fewer metric arguments"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]