Received-SPF: pass (google.com: domain of linux-kernel+bounces-54196-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Precedence: bulk
MIME-Version: 1.0
References: <20240202022512.467636-1-irogers@google.com> <20240202022512.467636-2-irogers@google.com>
 <CAM9d7cgv3zPNohz6eWwH7qaQyY0QMRK_sqtJdumeGrD5tMW9Yg@mail.gmail.com>
In-Reply-To: <CAM9d7cgv3zPNohz6eWwH7qaQyY0QMRK_sqtJdumeGrD5tMW9Yg@mail.gmail.com>
From: Ian Rogers <irogers@google.com>
Date: Mon, 5 Feb 2024 18:21:02 -0800
Message-ID: <CAP-5=fUThAUD6yJ+iDBbKvw4mFWbN3-j=bcBHwu-aY3_tMnQnQ@mail.gmail.com>
Subject: Re: [PATCH v1 2/3] perf metrics: Compute unmerged uncore metrics individually
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 6:02=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:
> >
> > When merging counts from multiple uncore PMUs the metric is only
> > computed for the metric leader. When merging/aggregation is disabled,
> > prior to this patch just the leader's metric would be computed. Fix
> > this by computing the metric for each PMU.
> >
> > On a SkylakeX:
> > Before:
> > ```
> > $ perf stat -A -M memory_bandwidth_total -a sleep 1
> >
> >  Performance counter stats for 'system wide':
> >
> > CPU0               82,217      UNC_M_CAS_COUNT.RD [uncore_imc_0] #     =
 9.2 MB/s  memory_bandwidth_total
> > CPU18                   0      UNC_M_CAS_COUNT.RD [uncore_imc_0] #     =
 0.0 MB/s  memory_bandwidth_total
> > CPU0               61,395      UNC_M_CAS_COUNT.WR [uncore_imc_0]
> > CPU18                   0      UNC_M_CAS_COUNT.WR [uncore_imc_0]
> > CPU0                    0      UNC_M_CAS_COUNT.RD [uncore_imc_1]
> > CPU18                   0      UNC_M_CAS_COUNT.RD [uncore_imc_1]
> > CPU0                    0      UNC_M_CAS_COUNT.WR [uncore_imc_1]
> > CPU18                   0      UNC_M_CAS_COUNT.WR [uncore_imc_1]
> > CPU0               81,570      UNC_M_CAS_COUNT.RD [uncore_imc_2]
> > CPU18             113,886      UNC_M_CAS_COUNT.RD [uncore_imc_2]
> > CPU0               62,330      UNC_M_CAS_COUNT.WR [uncore_imc_2]
> > CPU18              66,942      UNC_M_CAS_COUNT.WR [uncore_imc_2]
> > CPU0               75,489      UNC_M_CAS_COUNT.RD [uncore_imc_3]
> > CPU18              27,958      UNC_M_CAS_COUNT.RD [uncore_imc_3]
> > CPU0               55,864      UNC_M_CAS_COUNT.WR [uncore_imc_3]
> > CPU18              38,727      UNC_M_CAS_COUNT.WR [uncore_imc_3]
> > CPU0                    0      UNC_M_CAS_COUNT.RD [uncore_imc_4]
> > CPU18                   0      UNC_M_CAS_COUNT.RD [uncore_imc_4]
> > CPU0                    0      UNC_M_CAS_COUNT.WR [uncore_imc_4]
> > CPU18                   0      UNC_M_CAS_COUNT.WR [uncore_imc_4]
> > CPU0               75,423      UNC_M_CAS_COUNT.RD [uncore_imc_5]
> > CPU18             104,527      UNC_M_CAS_COUNT.RD [uncore_imc_5]
> > CPU0               57,596      UNC_M_CAS_COUNT.WR [uncore_imc_5]
> > CPU18              56,777      UNC_M_CAS_COUNT.WR [uncore_imc_5]
> > CPU0        1,003,440,851 ns   duration_time
> >
> >        1.003440851 seconds time elapsed
> > ```
> >
> > After:
> > ```
> > $ perf stat -A -M memory_bandwidth_total -a sleep 1
> >
> >  Performance counter stats for 'system wide':
> >
> > CPU0               88,968      UNC_M_CAS_COUNT.RD [uncore_imc_0] #     =
 9.5 MB/s  memory_bandwidth_total
> > CPU18                   0      UNC_M_CAS_COUNT.RD [uncore_imc_0] #     =
 0.0 MB/s  memory_bandwidth_total
> > CPU0               59,498      UNC_M_CAS_COUNT.WR [uncore_imc_0]
> > CPU18                   0      UNC_M_CAS_COUNT.WR [uncore_imc_0]
> > CPU0                    0      UNC_M_CAS_COUNT.RD [uncore_imc_1] #     =
 0.0 MB/s  memory_bandwidth_total
> > CPU18                   0      UNC_M_CAS_COUNT.RD [uncore_imc_1] #     =
 0.0 MB/s  memory_bandwidth_total
> > CPU0                    0      UNC_M_CAS_COUNT.WR [uncore_imc_1]
> > CPU18                   0      UNC_M_CAS_COUNT.WR [uncore_imc_1]
> > CPU0               88,635      UNC_M_CAS_COUNT.RD [uncore_imc_2] #     =
 9.5 MB/s  memory_bandwidth_total
> > CPU18             117,975      UNC_M_CAS_COUNT.RD [uncore_imc_2] #     =
11.5 MB/s  memory_bandwidth_total
> > CPU0               60,829      UNC_M_CAS_COUNT.WR [uncore_imc_2]
> > CPU18              62,105      UNC_M_CAS_COUNT.WR [uncore_imc_2]
> > CPU0               82,238      UNC_M_CAS_COUNT.RD [uncore_imc_3] #     =
 8.7 MB/s  memory_bandwidth_total
> > CPU18              22,906      UNC_M_CAS_COUNT.RD [uncore_imc_3] #     =
 3.6 MB/s  memory_bandwidth_total
> > CPU0               53,959      UNC_M_CAS_COUNT.WR [uncore_imc_3]
> > CPU18              32,990      UNC_M_CAS_COUNT.WR [uncore_imc_3]
> > CPU0                    0      UNC_M_CAS_COUNT.RD [uncore_imc_4] #     =
 0.0 MB/s  memory_bandwidth_total
> > CPU18                   0      UNC_M_CAS_COUNT.RD [uncore_imc_4] #     =
 0.0 MB/s  memory_bandwidth_total
> > CPU0                    0      UNC_M_CAS_COUNT.WR [uncore_imc_4]
> > CPU18                   0      UNC_M_CAS_COUNT.WR [uncore_imc_4]
> > CPU0               83,595      UNC_M_CAS_COUNT.RD [uncore_imc_5] #     =
 8.9 MB/s  memory_bandwidth_total
> > CPU18             110,151      UNC_M_CAS_COUNT.RD [uncore_imc_5] #     =
10.5 MB/s  memory_bandwidth_total
> > CPU0               56,540      UNC_M_CAS_COUNT.WR [uncore_imc_5]
> > CPU18              53,816      UNC_M_CAS_COUNT.WR [uncore_imc_5]
> > CPU0        1,003,353,416 ns   duration_time
> > ```
> >
> > Signed-off-by: Ian Rogers <irogers@google.com>
> > ---
> >  tools/perf/util/metricgroup.c |  2 ++
> >  tools/perf/util/stat-shadow.c | 31 +++++++++++++++++++++++++++----
> >  2 files changed, 29 insertions(+), 4 deletions(-)
> >
> > diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgrou=
p.c
> > index ca3e0404f187..c33ffee837ca 100644
> > --- a/tools/perf/util/metricgroup.c
> > +++ b/tools/perf/util/metricgroup.c
> > @@ -44,6 +44,8 @@ struct metric_event *metricgroup__lookup(struct rblis=
t *metric_events,
> >         if (!metric_events)
> >                 return NULL;
> >
> > +       if (evsel->metric_leader)
> > +               me.evsel =3D evsel->metric_leader;
> >         nd =3D rblist__find(metric_events, &me);
> >         if (nd)
> >                 return container_of(nd, struct metric_event, nd);
> > diff --git a/tools/perf/util/stat-shadow.c b/tools/perf/util/stat-shado=
w.c
> > index f6c9d2f98835..1be23b0eee2f 100644
> > --- a/tools/perf/util/stat-shadow.c
> > +++ b/tools/perf/util/stat-shadow.c
> > @@ -356,6 +356,7 @@ static void print_nsecs(struct perf_stat_config *co=
nfig,
> >  }
> >
> >  static int prepare_metric(const struct metric_expr *mexp,
> > +                         const struct evsel *evsel,
> >                           struct expr_parse_ctx *pctx,
> >                           int aggr_idx)
> >  {
> > @@ -398,8 +399,29 @@ static int prepare_metric(const struct metric_expr=
 *mexp,
> >                         source_count =3D 1;
> >                 } else {
> >                         struct perf_stat_evsel *ps =3D mexp->metric_eve=
nts[i]->stats;
> > -                       struct perf_stat_aggr *aggr =3D &ps->aggr[aggr_=
idx];
> > +                       struct perf_stat_aggr *aggr;
> >
> > +                       /*
> > +                        * If there are multiple uncore PMUs and we're =
not
> > +                        * reading the leader's stats, determine the st=
ats for
> > +                        * the appropriate uncore PMU.
> > +                        */
> > +                       if (evsel && evsel->metric_leader &&
> > +                           evsel->pmu !=3D evsel->metric_leader->pmu &=
&
> > +                           mexp->metric_events[i]->pmu =3D=3D evsel->m=
etric_leader->pmu) {
>
> Is it just to check we're in --no-aggr (or --no-merge)?
> Then it'd be simpler to use stat_config->aggr_mode.

For most metrics the events will be on the same PMU, but there is
nothing stopping mixing events from different PMUs (grouping can be
disabled). There may also be software and tool evsels.

Thanks,
Ian

> Thanks,
> Namhyung
>
>
> > +                               struct evsel *pos;
> > +
> > +                               evlist__for_each_entry(evsel->evlist, p=
os) {
> > +                                       if (pos->pmu !=3D evsel->pmu)
> > +                                               continue;
> > +                                       if (pos->metric_leader !=3D mex=
p->metric_events[i])
> > +                                               continue;
> > +                                       ps =3D pos->stats;
> > +                                       source_count =3D 1;
> > +                                       break;
> > +                               }
> > +                       }
> > +                       aggr =3D &ps->aggr[aggr_idx];
> >                         if (!aggr)
> >                                 break;
> >
> > @@ -420,7 +442,8 @@ static int prepare_metric(const struct metric_expr =
*mexp,
> >                                  * metric.
> >                                  */
> >                                 val =3D aggr->counts.val * (1.0 / mexp-=
>metric_events[i]->scale);
> > -                               source_count =3D evsel__source_count(me=
xp->metric_events[i]);
> > +                               if (!source_count)
> > +                                       source_count =3D evsel__source_=
count(mexp->metric_events[i]);
> >                         }
> >                 }
> >                 n =3D strdup(evsel__metric_id(mexp->metric_events[i]));
> > @@ -461,7 +484,7 @@ static void generic_metric(struct perf_stat_config =
*config,
> >                 pctx->sctx.user_requested_cpu_list =3D strdup(config->u=
ser_requested_cpu_list);
> >         pctx->sctx.runtime =3D mexp->runtime;
> >         pctx->sctx.system_wide =3D config->system_wide;
> > -       i =3D prepare_metric(mexp, pctx, aggr_idx);
> > +       i =3D prepare_metric(mexp, evsel, pctx, aggr_idx);
> >         if (i < 0) {
> >                 expr__ctx_free(pctx);
> >                 return;
> > @@ -522,7 +545,7 @@ double test_generic_metric(struct metric_expr *mexp=
, int aggr_idx)
> >         if (!pctx)
> >                 return NAN;
> >
> > -       if (prepare_metric(mexp, pctx, aggr_idx) < 0)
> > +       if (prepare_metric(mexp, /*evsel=3D*/NULL, pctx, aggr_idx) < 0)
> >                 goto out;
> >
> >         if (expr__parse(&ratio, pctx, mexp->metric_expr))
> > --
> > 2.43.0.594.gd9cf4e227d-goog
> >