Received-SPF: pass (google.com: domain of linux-kernel+bounces-54201-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240202022512.467636-1-irogers@google.com> <CAM9d7chAU=arK2y7RDHtxfRcQ80Az6dzGB0_+01iZ346tSob3w@mail.gmail.com>
In-Reply-To: <CAM9d7chAU=arK2y7RDHtxfRcQ80Az6dzGB0_+01iZ346tSob3w@mail.gmail.com>
From: Ian Rogers <irogers@google.com>
Date: Mon, 5 Feb 2024 18:23:51 -0800
Message-ID: <CAP-5=fXqi05ohSe7fH=7zo=DMCjBR1OcjdHETHZJPNAMvsmJaw@mail.gmail.com>
Subject: Re: [PATCH v1 1/3] perf stat: Pass fewer metric arguments
To: Namhyung Kim <namhyung@kernel.org>
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 Mon, Feb 5, 2024 at 5:59=E2=80=AFPM Namhyung Kim <namhyung@kernel.org> w=
rote:
>
> On Thu, Feb 1, 2024 at 6:25=E2=80=AFPM Ian Rogers <irogers@google.com> wr=
ote:
> >
> > 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-shado=
w.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 =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_e=
vents,
> >                                 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_eve=
nts[i]->stats;
> >                         struct perf_stat_aggr *aggr =3D &ps->aggr[aggr_=
idx];
> >
> >                         if (!aggr)
> >                                 break;
> >
> > -                        if (!metric_events[i]->supported) {
> > +                       if (!mexp->metric_events[i]->supported) {
> >                                 /*
> >                                  * Not supported events will have a cou=
nt of 0,
> >                                  * which can be confusing in a
> > @@ -419,19 +419,19 @@ static int prepare_metric(struct evsel **metric_e=
vents,
> >                                  * reverse the scale before computing t=
he
> >                                  * metric.
> >                                  */
> > -                               val =3D aggr->counts.val * (1.0 / metri=
c_events[i]->scale);
> > -                               source_count =3D evsel__source_count(me=
tric_events[i]);
> > +                               val =3D aggr->counts.val * (1.0 / mexp-=
>metric_events[i]->scale);
> > +                               source_count =3D evsel__source_count(me=
xp->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_coun=
t);
> >         }
> >
> > -       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].met=
ric_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_ev=
ents,
> >  }
> >
> >  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_confi=
g *config,
> >
> >         if (config->user_requested_cpu_list)
> >                 pctx->sctx.user_requested_cpu_list =3D strdup(config->u=
ser_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_thresh=
old) =3D=3D 0 &&
> > +                       if (mexp->metric_threshold &&
> > +                           expr__parse(&threshold, pctx, mexp->metric_=
threshold) =3D=3D 0 &&
> >                             !isnan(threshold)) {
> >                                 color =3D fpclassify(threshold) =3D=3D =
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->metri=
c_unit,
> >                                         &unit, &scale) >=3D 0) {
> >                                         ratio *=3D scale;
> >                                 }
> > -                               if (strstr(metric_expr, "?"))
> > +                               if (strstr(mexp->metric_expr, "?"))
> >                                         scnprintf(metric_bf, sizeof(met=
ric_bf),
> > -                                         "%s  %s_%d", unit, metric_nam=
e, runtime);
> > +                                         "%s  %s_%d", unit, mexp->metr=
ic_name, mexp->runtime);
> >                                 else
> >                                         scnprintf(metric_bf, sizeof(met=
ric_bf),
> > -                                         "%s  %s", unit, metric_name);
> > +                                         "%s  %s", unit, mexp->metric_=
name);
> >
> >                                 print_metric(config, ctxp, color, "%8.1=
f",
> >                                              metric_bf, ratio);
> >                         } else {
> >                                 print_metric(config, ctxp, color, "%8.2=
f",
> > -                                       metric_name ?
> > -                                       metric_name :
> > -                                       out->force_header ?  name : "",
> > +                                       mexp->metric_name ?
> > +                                       mexp->metric_name :
> > +                                       out->force_header ?  evsel->nam=
e : "",
> >                                         ratio);
> >                         }
> >                 } else {
> >                         print_metric(config, ctxp, color, /*unit=3D*/NU=
LL,
> >                                      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(st=
ruct 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, me=
xp->runtime,
> > -                              aggr_idx, out);
> > +               generic_metric(config, mexp, evsel, aggr_idx, out);
> >         }
> >
> >         return NULL;
> > --
> > 2.43.0.594.gd9cf4e227d-goog
> >