| 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=fWSpM7NL7UjXZBN8WHNAE7hGe1ghQ6_DqFe2VjTCGoA7A@mail.gmail.com>
Date: Thu, 27 Jun 2024 13:48:22 -0700
From: Ian Rogers <irogers@...gle.com>
To: Namhyung Kim <namhyung@...nel.org>
Cc: Arnaldo Carvalho de Melo <acme@...nel.org>, Kan Liang <kan.liang@...ux.intel.com>,
Jiri Olsa <jolsa@...nel.org>, Adrian Hunter <adrian.hunter@...el.com>,
Peter Zijlstra <peterz@...radead.org>, Ingo Molnar <mingo@...nel.org>,
LKML <linux-kernel@...r.kernel.org>, linux-perf-users@...r.kernel.org
Subject: Re: [PATCH 1/2] perf stat: Use field separator in the metric header
On Thu, Jun 27, 2024 at 1:03 PM Namhyung Kim <namhyung@...nel.org> wrote:
>
> It didn't use the passed field separator (using -x option) when it
> prints the metric headers and always put "," between the fields.
>
> Before:
> $ sudo ./perf stat -a -x : --per-core -M tma_core_bound --metric-only true
> core,cpus,% tma_core_bound: <<<--- here: "core,cpus," but ":" expected
> S0-D0-C0:2:10.5:
> S0-D0-C1:2:14.8:
> S0-D0-C2:2:9.9:
> S0-D0-C3:2:13.2:
>
> After:
> $ sudo ./perf stat -a -x : --per-core -M tma_core_bound --metric-only true
> core:cpus:% tma_core_bound:
> S0-D0-C0:2:10.5:
> S0-D0-C1:2:15.0:
> S0-D0-C2:2:16.5:
> S0-D0-C3:2:12.5:
>
> Signed-off-by: Namhyung Kim <namhyung@...nel.org>
> ---
> tools/perf/util/stat-display.c | 37 ++++++++++++++++++++++++++--------
> 1 file changed, 29 insertions(+), 8 deletions(-)
>
> diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c
> index 91d2f7f65df7..e8673c9f6b49 100644
> --- a/tools/perf/util/stat-display.c
> +++ b/tools/perf/util/stat-display.c
> @@ -47,16 +47,27 @@ static int aggr_header_lens[] = {
> };
>
> static const char *aggr_header_csv[] = {
> - [AGGR_CORE] = "core,cpus,",
> - [AGGR_CACHE] = "cache,cpus,",
> - [AGGR_DIE] = "die,cpus,",
> - [AGGR_SOCKET] = "socket,cpus,",
> - [AGGR_NONE] = "cpu,",
> - [AGGR_THREAD] = "comm-pid,",
> - [AGGR_NODE] = "node,",
> + [AGGR_CORE] = "core%scpus%s",
> + [AGGR_CACHE] = "cache%scpus%s",
> + [AGGR_DIE] = "die%scpus%s",
> + [AGGR_SOCKET] = "socket%scpus%s",
> + [AGGR_NONE] = "cpu%s",
> + [AGGR_THREAD] = "comm-pid%s",
> + [AGGR_NODE] = "node%s",
> [AGGR_GLOBAL] = ""
> };
>
> +static int aggr_header_num[] = {
> + [AGGR_CORE] = 2,
> + [AGGR_CACHE] = 2,
> + [AGGR_DIE] = 2,
> + [AGGR_SOCKET] = 2,
> + [AGGR_NONE] = 1,
> + [AGGR_THREAD] = 1,
> + [AGGR_NODE] = 1,
> + [AGGR_GLOBAL] = 0,
> +};
> +
> static const char *aggr_header_std[] = {
> [AGGR_CORE] = "core",
> [AGGR_CACHE] = "cache",
> @@ -1185,8 +1196,18 @@ static void print_metric_headers_csv(struct perf_stat_config *config,
> {
> if (config->interval)
> fputs("time,", config->output);
> - if (!config->iostat_run)
> + if (config->iostat_run)
> + return;
> +
Having a static count of commas seems somewhat error prone, perhaps:
```
const char *header = aggr_header_csv[config->aggr_mode];
if (config->csv_sep == ',' || !strchr(header, ',')) {
fputs(config->output, header);
} else {
char *tmp = strdup(header);
char *p = tmp;
while (p && *p) {
if (p == ',')
*p = config->csv_sep;
p++;
}
fputs(config->output, tmp);
free(tmp);
}
```
I'm somewhat surprised that we have no metric tests in the stat output
tests like tools/perf/tests/shell/stat+csv_output.sh. Perhaps we can
add the following:
```
diff --git a/tools/perf/tests/shell/lib/stat_output.sh
b/tools/perf/tests/shell/lib/stat_output.sh
index 9a176ceae4a3..a920b2d78abb 100644
--- a/tools/perf/tests/shell/lib/stat_output.sh
+++ b/tools/perf/tests/shell/lib/stat_output.sh
@@ -148,6 +148,14 @@ check_per_socket()
echo "[Success]"
}
+check_metric_only()
+{
+ echo -n "Checking $1 output: metric only "
+ perf stat --metric-only $2 -e instructions,cycles true
+ commachecker --metric-only
+ echo "[Success]"
+}
+
# The perf stat options for per-socket, per-core, per-die
# and -A ( no_aggr mode ) uses the info fetched from this
# directory: "/sys/devices/system/cpu/cpu*/topology". For
diff --git a/tools/perf/tests/shell/stat+csv_output.sh
b/tools/perf/tests/shell/stat+csv_output.sh
index fc2d8cc6e5e0..d6807dbab931 100755
--- a/tools/perf/tests/shell/stat+csv_output.sh
+++ b/tools/perf/tests/shell/stat+csv_output.sh
@@ -44,6 +44,7 @@ function commachecker()
;; "--per-die") exp=8
;; "--per-cluster") exp=8
;; "--per-cache") exp=8
+ ;; "--metric-only") exp=2
esac
while read line
@@ -83,6 +84,7 @@ then
check_per_cluster "CSV" "$perf_cmd"
check_per_die "CSV" "$perf_cmd"
check_per_socket "CSV" "$perf_cmd"
+ check_metric_only "CSV" "$perf_cmd"
else
echo "[Skip] Skipping tests for system_wide_no_aggr, per_core,
per_die and per_socket since
socket id exposed via topology is invalid"
fi
```
It is using the hard coded metrics and it looks like the header
printing for that is broken, but this is so often the case for stat
output :-(
Thanks,
Ian
> + if (aggr_header_num[config->aggr_mode] == 1) {
> + fprintf(config->output, aggr_header_csv[config->aggr_mode],
> + config->csv_sep);
> + } else if (aggr_header_num[config->aggr_mode] == 2) {
> + fprintf(config->output, aggr_header_csv[config->aggr_mode],
> + config->csv_sep, config->csv_sep);
> + } else {
> fputs(aggr_header_csv[config->aggr_mode], config->output);
> + }
> }
>
> static void print_metric_headers_json(struct perf_stat_config *config __maybe_unused,
> --
> 2.45.2.803.g4e1b14247a-goog
>
Powered by blists - more mailing lists