Received-SPF: pass (google.com: domain of linux-kernel+bounces-54173-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Precedence: bulk
MIME-Version: 1.0
References: <20240202022512.467636-1-irogers@google.com>
In-Reply-To: <20240202022512.467636-1-irogers@google.com>
From: Namhyung Kim <namhyung@kernel.org>
Date: Mon, 5 Feb 2024 17:59:09 -0800
Message-ID: <CAM9d7chAU=arK2y7RDHtxfRcQ80Az6dzGB0_+01iZ346tSob3w@mail.gmail.com>
Subject: Re: [PATCH v1 1/3] perf stat: Pass fewer metric arguments
To: Ian Rogers <irogers@google.com>
Cc: Peter Zijlstra <peterz@infradead.org>, Ingo Molnar <mingo@redhat.com>, 
	Arnaldo Carvalho de Melo <acme@kernel.org>, Mark Rutland <mark.rutland@arm.com>, 
	Alexander Shishkin <alexander.shishkin@linux.intel.com>, Jiri Olsa <jolsa@kernel.org>, 
	Adrian Hunter <adrian.hunter@intel.com>, Kan Liang <kan.liang@linux.intel.com>, 
	Kajol Jain <kjain@linux.ibm.com>, John Garry <john.g.garry@oracle.com>, Kaige Ye <ye@kaige.org>, 
	K Prateek Nayak <kprateek.nayak@amd.com>, linux-perf-users@vger.kernel.org, 
	linux-kernel@vger.kernel.org, Stephane Eranian <eranian@google.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Thu, Feb 1, 2024 at 6:25=E2=80=AFPM Ian Rogers <irogers@google.com> wrot=
e:
>
> 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@google.com>
> ---
>  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 *co=
nfig,
>                 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 =3D 0; metric_events[i]; i++) {
> +       for (i =3D 0; mexp->metric_events[i]; i++) {
>                 char *n;
>                 double val;
>                 int source_count =3D 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 =3D &walltime_nsecs_stats;
>                                 scale =3D 1e-9;
> @@ -391,19 +390,20 @@ static int prepare_metric(struct evsel **metric_eve=
nts,
>                                 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 =3D avg_stats(stats) * scale;
>                         source_count =3D 1;
>                 } else {
> -                       struct perf_stat_evsel *ps =3D metric_events[i]->=
stats;
> +                       struct perf_stat_evsel *ps =3D mexp->metric_event=
s[i]->stats;
>                         struct perf_stat_aggr *aggr =3D &ps->aggr[aggr_id=
x];
>
>                         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_eve=
nts,
>                                  * reverse the scale before computing the
>                                  * metric.
>                                  */
> -                               val =3D aggr->counts.val * (1.0 / metric_=
events[i]->scale);
> -                               source_count =3D evsel__source_count(metr=
ic_events[i]);
> +                               val =3D aggr->counts.val * (1.0 / mexp->m=
etric_events[i]->scale);
> +                               source_count =3D evsel__source_count(mexp=
->metric_events[i]);
>                         }
>                 }
> -               n =3D strdup(evsel__metric_id(metric_events[i]));
> +               n =3D 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 =3D 0; metric_refs && metric_refs[j].metric_name; j++)=
 {
> -               int ret =3D expr__add_ref(pctx, &metric_refs[j]);
> +       for (int j =3D 0; mexp->metric_refs && mexp->metric_refs[j].metri=
c_name; j++) {
> +               int ret =3D expr__add_ref(pctx, &mexp->metric_refs[j]);
>
>                 if (ret)
>                         return ret;
> @@ -441,14 +441,8 @@ static int prepare_metric(struct evsel **metric_even=
ts,
>  }
>
>  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 =3D strdup(config->use=
r_requested_cpu_list);
> -       pctx->sctx.runtime =3D runtime;
> +       pctx->sctx.runtime =3D mexp->runtime;
>         pctx->sctx.system_wide =3D config->system_wide;
> -       i =3D prepare_metric(metric_events, metric_refs, pctx, aggr_idx);
> +       i =3D 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) =3D=3D 0) {
> +       if (!mexp->metric_events[i]) {
> +               if (expr__parse(&ratio, pctx, mexp->metric_expr) =3D=3D 0=
) {
>                         char *unit;
>                         char metric_bf[64];
>
> -                       if (metric_threshold &&
> -                           expr__parse(&threshold, pctx, metric_threshol=
d) =3D=3D 0 &&
> +                       if (mexp->metric_threshold &&
> +                           expr__parse(&threshold, pctx, mexp->metric_th=
reshold) =3D=3D 0 &&
>                             !isnan(threshold)) {
>                                 color =3D fpclassify(threshold) =3D=3D FP=
_ZERO
>                                         ? PERF_COLOR_GREEN : PERF_COLOR_R=
ED;
>                         }
>
> -                       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) >=3D 0) {
>                                         ratio *=3D scale;
>                                 }
> -                               if (strstr(metric_expr, "?"))
> +                               if (strstr(mexp->metric_expr, "?"))
>                                         scnprintf(metric_bf, sizeof(metri=
c_bf),
> -                                         "%s  %s_%d", unit, metric_name,=
 runtime);
> +                                         "%s  %s_%d", unit, mexp->metric=
_name, mexp->runtime);
>                                 else
>                                         scnprintf(metric_bf, sizeof(metri=
c_bf),
> -                                         "%s  %s", unit, metric_name);
> +                                         "%s  %s", unit, mexp->metric_na=
me);
>
>                                 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=3D*/NULL=
,
>                                      out->force_header ?
> -                                    (metric_name ? metric_name : name) :=
 "", 0);
> +                                    (mexp->metric_name ?: evsel->name) :=
 "", 0);
>                 }
>         } else {
>                 print_metric(config, ctxp, color, /*unit=3D*/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(stru=
ct perf_stat_config *config,
>
>                 if ((*num)++ > 0)
>                         out->new_line(config, ctxp);
> -               generic_metric(config, mexp->metric_expr, mexp->metric_th=
reshold,
> -                              mexp->metric_events, mexp->metric_refs, ev=
sel->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
>