| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAP-5=fW39O4=fu4CkcZSJWMA-5gkPaixGhWZecqMnrQt_vYsCg@mail.gmail.com>
Date: Mon, 23 Jun 2025 11:17:20 -0700
From: Ian Rogers <irogers@...gle.com>
To: Namhyung Kim <namhyung@...nel.org>
Cc: Chun-Tse Shao <ctshao@...gle.com>, linux-kernel@...r.kernel.org, peterz@...radead.org,
mingo@...hat.com, acme@...nel.org, mark.rutland@....com,
alexander.shishkin@...ux.intel.com, jolsa@...nel.org, adrian.hunter@...el.com,
kan.liang@...ux.intel.com, linux-perf-users@...r.kernel.org
Subject: Re: [PATCH v1] perf stat: Fix uncore aggregation number
On Fri, Jun 20, 2025 at 3:12 PM Namhyung Kim <namhyung@...nel.org> wrote:
>
> Hi CT,
>
> On Thu, Jun 12, 2025 at 03:55:59PM -0700, Chun-Tse Shao wrote:
> > Hi Ian, I actually renamed it to `aggr_nr` in v2 patch so it should be
> > better aligned to json mode, which is using `aggregate-nunber`. But
> > anyway I think any name other than `cpus` is better.
> > v2 patch: lore.kernel.org/20250612225324.3315450-1-ctshao@...gle.com
>
> I think "aggregation-count" is a better name, maybe abbreviated to
> "ag_cnt". Can we rename the JSON as well? I'm not sure if it's
> documented somewhere.
Fwiw, I still think "counters" is a clearer, more intention revealing
definition than "aggregation count" or "aggregation number". The term
counter appears in the perf-stat and perf_event_open man page. In the
perf-stat man page we have a pattern of:
--per-XXX
Aggregate counts per XXX for system-wide mode measurements. This is a
useful mode to .... To enable this mode, use --per-XXX
in addition to -a. (system-wide). The output includes the ...
This is useful to gauge the amount of aggregation.
It seems that the aggregated value of the counters could be confused
for being an "aggregation count" or "aggregation number" given the man
page definition. I think "counters" to some extent avoids this, we
giving the number of counters aggregated. The perf-stat man page could
certainly be clearer :-)
Thanks,
Ian
> Thanks,
> Namhyung
>
> >
> >
> > On Wed, Jun 11, 2025 at 10:12 PM Ian Rogers <irogers@...gle.com> wrote:
> > >
> > > On Wed, Jun 11, 2025 at 8:18 PM Chun-Tse Shao <ctshao@...gle.com> wrote:
> > > >
> > > > Thanks for your test, Ian!
> > > >
> > > > I wonder if `nr_pmus` makes sense, since the column would be shared
> > > > with multiple different pmus. WDYT?
> > >
> > > So each PMU in sysfs has a cpumask that specifies which CPUs perf
> > > should pass to perf_event_open. For example, on a two socket machine
> > > the cpumask will typically have the first CPU of each socket. If the
> > > cpumask (or cpus) file isn't present then the cpumask is implicitly
> > > all online CPUs. Given that the aggregation number is the number of
> > > CPUs in the cpumask multiplied by the number of PMUs, I think the most
> > > neutral name is probably "counters" possibly shortened down to "ctrs".
> > > I suspect others have better suggestions :-)
> > >
> > > Thanks,
> > > Ian
> > >
> > > > -CT
> > > >
> > > > On Wed, Jun 11, 2025 at 5:16 PM Ian Rogers <irogers@...gle.com> wrote:
> > > > >
> > > > > On Wed, Jun 11, 2025 at 4:36 PM Chun-Tse Shao <ctshao@...gle.com> wrote:
> > > > > >
> > > > > > Follow up:
> > > > > > lore.kernel.org/CAP-5=fVDF4-qYL1Lm7efgiHk7X=_nw_nEFMBZFMcsnOOJgX4Kg@...l.gmail.com/
> > > > > >
> > > > > > The patch adds unit aggregation during evsel merge the aggregated uncore
> > > > > > counters.
> > > > > >
> > > > > > Tested on a 2-socket machine with SNC3, uncore_imc_[0-11] and
> > > > > > cpumask="0,120"
> > > > > > Before:
> > > > > > perf stat -e clockticks -I 1000 --per-socket
> > > > > > # time socket cpus counts unit events
> > > > > > 1.001085024 S0 1 9615386315 clockticks
> > > > > > 1.001085024 S1 1 9614287448 clockticks
> > > > > > perf stat -e clockticks -I 1000 --per-node
> > > > > > # time node cpus counts unit events
> > > > > > 1.001029867 N0 1 3205726984 clockticks
> > > > > > 1.001029867 N1 1 3205444421 clockticks
> > > > > > 1.001029867 N2 1 3205234018 clockticks
> > > > > > 1.001029867 N3 1 3205224660 clockticks
> > > > > > 1.001029867 N4 1 3205207213 clockticks
> > > > > > 1.001029867 N5 1 3205528246 clockticks
> > > > > > After:
> > > > > > perf stat -e clockticks -I 1000 --per-socket
> > > > > > # time socket cpus counts unit events
> > > > >
> > > > > I wonder if there is a better column heading than "cpus" given that
> > > > > these are imc PMUs.
> > > > >
> > > > > > 1.001022937 S0 12 9621463177 clockticks
> > > > > > 1.001022937 S1 12 9619804949 clockticks
> > > > > > perf stat -e clockticks -I 1000 --per-node
> > > > > > # time node cpus counts unit events
> > > > > > 1.001029867 N0 4 3206782080 clockticks
> > > > > > 1.001029867 N1 4 3207025354 clockticks
> > > > > > 1.001029867 N2 4 3207067946 clockticks
> > > > > > 1.001029867 N3 4 3206871733 clockticks
> > > > > > 1.001029867 N4 4 3206199005 clockticks
> > > > > > 1.001029867 N5 4 3205525058 clockticks
> > > > > >
> > > > > > Suggested-by: Ian Rogers <irogers@...gle.com>
> > > > > > Signed-off-by: Chun-Tse Shao <ctshao@...gle.com>
> > > >
> > > > Added Namhyung's ack from the previous email.
> > > > Acked-by: Namhyung Kim <namhyung@...nel.org>
> > > >
> > > > >
> > > > > Tested-by: Ian Rogers <irogers@...gle.com>
> > > > >
> > > > > Thanks,
> > > > > Ian
> > > > >
> > > > > > ---
> > > > > > tools/perf/util/stat.c | 1 +
> > > > > > 1 file changed, 1 insertion(+)
> > > > > >
> > > > > > diff --git a/tools/perf/util/stat.c b/tools/perf/util/stat.c
> > > > > > index 355a7d5c8ab8..52266d773353 100644
> > > > > > --- a/tools/perf/util/stat.c
> > > > > > +++ b/tools/perf/util/stat.c
> > > > > > @@ -527,6 +527,7 @@ static int evsel__merge_aggr_counters(struct evsel *evsel, struct evsel *alias)
> > > > > > struct perf_counts_values *aggr_counts_b = &ps_b->aggr[i].counts;
> > > > > >
> > > > > > /* NB: don't increase aggr.nr for aliases */
> > > > > > + ps_a->aggr[i].nr += ps_b->aggr[i].nr;
> > > > > >
> > > > > > aggr_counts_a->val += aggr_counts_b->val;
> > > > > > aggr_counts_a->ena += aggr_counts_b->ena;
> > > > > > --
> > > > > > 2.50.0.rc1.591.g9c95f17f64-goog
> > > > > >
Powered by blists - more mailing lists