| 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=fU+TN6FiRYc_g2etdBeXBujKtHA2z=NE3k3mBrz2bUS1Q@mail.gmail.com>
Date: Tue, 24 Jun 2025 16:58:11 -0700
From: Ian Rogers <irogers@...gle.com>
To: Chun-Tse Shao <ctshao@...gle.com>
Cc: linux-kernel@...r.kernel.org, wy.shih90@...il.com, peterz@...radead.org,
mingo@...hat.com, acme@...nel.org, namhyung@...nel.org, mark.rutland@....com,
alexander.shishkin@...ux.intel.com, jolsa@...nel.org, adrian.hunter@...el.com,
kan.liang@...ux.intel.com, james.clark@...aro.org,
linux-perf-users@...r.kernel.org
Subject: Re: [PATCH v3] perf stat: Fix uncore aggregation number
On Tue, Jun 24, 2025 at 3:15 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. Change the name of the column to `ctrs` and `counters` for
> json mode.
>
> 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 ctrs counts unit events
> 1.001026071 S0 12 9619677996 clockticks
> 1.001026071 S1 12 9618612614 clockticks
> perf stat -e clockticks -I 1000 --per-node
> # time node ctrs counts unit events
> 1.001027449 N0 4 3207251859 clockticks
> 1.001027449 N1 4 3207315930 clockticks
> 1.001027449 N2 4 3206981828 clockticks
> 1.001027449 N3 4 3206566126 clockticks
> 1.001027449 N4 4 3206032609 clockticks
> 1.001027449 N5 4 3205651355 clockticks
>
> Suggested-by: Ian Rogers <irogers@...gle.com>
> Signed-off-by: Chun-Tse Shao <ctshao@...gle.com>
> ---
> v3:
> Rename the column to `ctrs` and `counters` in json mode.
>
> v2: https://lore.kernel.org/20250612225324.3315450-1-ctshao@google.com/
> Rename the column to `aggr_nr`.
> Remove unnecessary comment.
>
> v1: https://lore.kernel.org/20250611233239.3098064-1-ctshao@google.com/
>
> tools/perf/util/stat-display.c | 34 +++++++++++++++++-----------------
> tools/perf/util/stat.c | 2 +-
> 2 files changed, 18 insertions(+), 18 deletions(-)
>
> diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c
> index 729ad5cd52cb..9cb5245a92aa 100644
> --- a/tools/perf/util/stat-display.c
> +++ b/tools/perf/util/stat-display.c
> @@ -50,15 +50,15 @@ static int aggr_header_lens[] = {
> };
>
> static const char *aggr_header_csv[] = {
> - [AGGR_CORE] = "core,cpus,",
> - [AGGR_CACHE] = "cache,cpus,",
> - [AGGR_CLUSTER] = "cluster,cpus,",
> - [AGGR_DIE] = "die,cpus,",
> - [AGGR_SOCKET] = "socket,cpus,",
> - [AGGR_NONE] = "cpu,",
> - [AGGR_THREAD] = "comm-pid,",
> - [AGGR_NODE] = "node,",
> - [AGGR_GLOBAL] = ""
> + [AGGR_CORE] = "core,ctrs,",
> + [AGGR_CACHE] = "cache,ctrs,",
> + [AGGR_CLUSTER] = "cluster,ctrs,",
> + [AGGR_DIE] = "die,ctrs,",
> + [AGGR_SOCKET] = "socket,ctrs,",
> + [AGGR_NONE] = "cpu,",
> + [AGGR_THREAD] = "comm-pid,",
> + [AGGR_NODE] = "node,",
> + [AGGR_GLOBAL] = ""
> };
>
> static const char *aggr_header_std[] = {
> @@ -304,7 +304,7 @@ static void print_aggr_id_std(struct perf_stat_config *config,
> return;
> }
>
> - fprintf(output, "%-*s %*d ", aggr_header_lens[idx], buf, 4, aggr_nr);
> + fprintf(output, "%-*s %*d ", aggr_header_lens[idx], buf, /*strlen("ctrs")*/ 4, aggr_nr);
> }
>
> static void print_aggr_id_csv(struct perf_stat_config *config,
> @@ -366,27 +366,27 @@ static void print_aggr_id_json(struct perf_stat_config *config, struct outstate
> {
> switch (config->aggr_mode) {
> case AGGR_CORE:
> - json_out(os, "\"core\" : \"S%d-D%d-C%d\", \"aggregate-number\" : %d",
> + json_out(os, "\"core\" : \"S%d-D%d-C%d\", \"counters\" : %d",
Do we need to change the test?
https://web.git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/tests/shell/lib/perf_json_output_lint.py?h=perf-tools-next#n48
I think we can also document the value in the man page:
https://web.git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/Documentation/perf-stat.txt?h=perf-tools-next#n637
I think because it was ambiguous before we hadn't done this.
Thanks,
Ian
> id.socket, id.die, id.core, aggr_nr);
> break;
> case AGGR_CACHE:
> - json_out(os, "\"cache\" : \"S%d-D%d-L%d-ID%d\", \"aggregate-number\" : %d",
> + json_out(os, "\"cache\" : \"S%d-D%d-L%d-ID%d\", \"counters\" : %d",
> id.socket, id.die, id.cache_lvl, id.cache, aggr_nr);
> break;
> case AGGR_CLUSTER:
> - json_out(os, "\"cluster\" : \"S%d-D%d-CLS%d\", \"aggregate-number\" : %d",
> + json_out(os, "\"cluster\" : \"S%d-D%d-CLS%d\", \"counters\" : %d",
> id.socket, id.die, id.cluster, aggr_nr);
> break;
> case AGGR_DIE:
> - json_out(os, "\"die\" : \"S%d-D%d\", \"aggregate-number\" : %d",
> + json_out(os, "\"die\" : \"S%d-D%d\", \"counters\" : %d",
> id.socket, id.die, aggr_nr);
> break;
> case AGGR_SOCKET:
> - json_out(os, "\"socket\" : \"S%d\", \"aggregate-number\" : %d",
> + json_out(os, "\"socket\" : \"S%d\", \"counters\" : %d",
> id.socket, aggr_nr);
> break;
> case AGGR_NODE:
> - json_out(os, "\"node\" : \"N%d\", \"aggregate-number\" : %d",
> + json_out(os, "\"node\" : \"N%d\", \"counters\" : %d",
> id.node, aggr_nr);
> break;
> case AGGR_NONE:
> @@ -1317,7 +1317,7 @@ static void print_header_interval_std(struct perf_stat_config *config,
> case AGGR_CLUSTER:
> case AGGR_CACHE:
> case AGGR_CORE:
> - fprintf(output, "#%*s %-*s cpus",
> + fprintf(output, "#%*s %-*s ctrs",
> INTERVAL_LEN - 1, "time",
> aggr_header_lens[config->aggr_mode],
> aggr_header_std[config->aggr_mode]);
> diff --git a/tools/perf/util/stat.c b/tools/perf/util/stat.c
> index 355a7d5c8ab8..b0205e99a4c9 100644
> --- a/tools/perf/util/stat.c
> +++ b/tools/perf/util/stat.c
> @@ -526,7 +526,7 @@ static int evsel__merge_aggr_counters(struct evsel *evsel, struct evsel *alias)
> struct perf_counts_values *aggr_counts_a = &ps_a->aggr[i].counts;
> 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.714.g196bf9f422-goog
>
Powered by blists - more mailing lists