Received-SPF: pass (google.com: domain of linux-kernel+bounces-54177-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Precedence: bulk
MIME-Version: 1.0
References: <20240202022512.467636-1-irogers@google.com> <20240202022512.467636-2-irogers@google.com>
In-Reply-To: <20240202022512.467636-2-irogers@google.com>
From: Namhyung Kim <namhyung@kernel.org>
Date: Mon, 5 Feb 2024 18:02:40 -0800
Message-ID: <CAM9d7cgv3zPNohz6eWwH7qaQyY0QMRK_sqtJdumeGrD5tMW9Yg@mail.gmail.com>
Subject: Re: [PATCH v1 2/3] perf metrics: Compute unmerged uncore metrics individually
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:
>
> 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/metricgroup.=
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 rblist =
*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-shadow.=
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 *conf=
ig,
>  }
>
>  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_event=
s[i]->stats;
> -                       struct perf_stat_aggr *aggr =3D &ps->aggr[aggr_id=
x];
> +                       struct perf_stat_aggr *aggr;
>
> +                       /*
> +                        * If there are multiple uncore PMUs and we're no=
t
> +                        * reading the leader's stats, determine the stat=
s 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->met=
ric_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.

Thanks,
Namhyung


> +                               struct evsel *pos;
> +
> +                               evlist__for_each_entry(evsel->evlist, pos=
) {
> +                                       if (pos->pmu !=3D evsel->pmu)
> +                                               continue;
> +                                       if (pos->metric_leader !=3D mexp-=
>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 *m=
exp,
>                                  * metric.
>                                  */
>                                 val =3D aggr->counts.val * (1.0 / mexp->m=
etric_events[i]->scale);
> -                               source_count =3D evsel__source_count(mexp=
->metric_events[i]);
> +                               if (!source_count)
> +                                       source_count =3D evsel__source_co=
unt(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 *c=
onfig,
>                 pctx->sctx.user_requested_cpu_list =3D strdup(config->use=
r_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
>