* [PATCH 1/2] perf stat: Use field separator in the metric header
@ 2024-06-27 20:03 Namhyung Kim
2024-06-27 20:03 ` [PATCH 2/2] perf stat: Fix a segfault with --per-cluster --metric-only Namhyung Kim
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: Namhyung Kim @ 2024-06-27 20:03 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo, Ian Rogers, Kan Liang
Cc: Jiri Olsa, Adrian Hunter, Peter Zijlstra, Ingo Molnar, LKML,
linux-perf-users
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@kernel.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;
+
+ 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
^ permalink raw reply related [flat|nested] 8+ messages in thread* [PATCH 2/2] perf stat: Fix a segfault with --per-cluster --metric-only 2024-06-27 20:03 [PATCH 1/2] perf stat: Use field separator in the metric header Namhyung Kim @ 2024-06-27 20:03 ` Namhyung Kim 2024-06-28 12:47 ` Arnaldo Carvalho de Melo 2024-06-27 20:48 ` [PATCH 1/2] perf stat: Use field separator in the metric header Ian Rogers 2024-06-28 12:44 ` Arnaldo Carvalho de Melo 2 siblings, 1 reply; 8+ messages in thread From: Namhyung Kim @ 2024-06-27 20:03 UTC (permalink / raw) To: Arnaldo Carvalho de Melo, Ian Rogers, Kan Liang Cc: Jiri Olsa, Adrian Hunter, Peter Zijlstra, Ingo Molnar, LKML, linux-perf-users, Yicong Yang The new --per-cluster option was added recently but it forgot to update the aggr_header fields which are used for --metric-only option. And it resulted in a segfault due to NULL string in fputs(). Fixes: cbc917a1b03b ("perf stat: Support per-cluster aggregation") Cc: Yicong Yang <yangyicong@hisilicon.com> Signed-off-by: Namhyung Kim <namhyung@kernel.org> --- tools/perf/util/stat-display.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c index e8673c9f6b49..462227f663cb 100644 --- a/tools/perf/util/stat-display.c +++ b/tools/perf/util/stat-display.c @@ -38,6 +38,7 @@ static int aggr_header_lens[] = { [AGGR_CORE] = 18, [AGGR_CACHE] = 22, + [AGGR_CLUSTER] = 20, [AGGR_DIE] = 12, [AGGR_SOCKET] = 6, [AGGR_NODE] = 6, @@ -49,6 +50,7 @@ static int aggr_header_lens[] = { static const char *aggr_header_csv[] = { [AGGR_CORE] = "core%scpus%s", [AGGR_CACHE] = "cache%scpus%s", + [AGGR_CLUSTER] = "cluster%scpus%s", [AGGR_DIE] = "die%scpus%s", [AGGR_SOCKET] = "socket%scpus%s", [AGGR_NONE] = "cpu%s", @@ -60,6 +62,7 @@ static const char *aggr_header_csv[] = { static int aggr_header_num[] = { [AGGR_CORE] = 2, [AGGR_CACHE] = 2, + [AGGR_CLUSTER] = 2, [AGGR_DIE] = 2, [AGGR_SOCKET] = 2, [AGGR_NONE] = 1, -- 2.45.2.803.g4e1b14247a-goog ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] perf stat: Fix a segfault with --per-cluster --metric-only 2024-06-27 20:03 ` [PATCH 2/2] perf stat: Fix a segfault with --per-cluster --metric-only Namhyung Kim @ 2024-06-28 12:47 ` Arnaldo Carvalho de Melo 0 siblings, 0 replies; 8+ messages in thread From: Arnaldo Carvalho de Melo @ 2024-06-28 12:47 UTC (permalink / raw) To: Namhyung Kim Cc: Ian Rogers, Kan Liang, Jiri Olsa, Adrian Hunter, Peter Zijlstra, Ingo Molnar, LKML, linux-perf-users, Yicong Yang On Thu, Jun 27, 2024 at 01:03:53PM -0700, Namhyung Kim wrote: > The new --per-cluster option was added recently but it forgot to update > the aggr_header fields which are used for --metric-only option. And it > resulted in a segfault due to NULL string in fputs(). Before: acme@number:~$ sudo ~acme/bin/perf stat -a -x : --per-cluster -M tma_core_bound --metric-only true Segmentation fault acme@number:~$ acme@number:~$ sudo su - root@number:~# gdb perf (gdb) run stat -a -x : --per-cluster -M tma_core_bound --metric-only true Starting program: /root/bin/perf stat -a -x : --per-cluster -M tma_core_bound --metric-only true Program received signal SIGSEGV, Segmentation fault. 0x00007ffff6f7f8dd in __strlen_avx2 () from /lib64/libc.so.6 (gdb) bt #0 0x00007ffff6f7f8dd in __strlen_avx2 () from /lib64/libc.so.6 #1 0x00007ffff6e97a3a in fputs () from /lib64/libc.so.6 #2 0x000000000056b805 in print_metric_headers () #3 0x000000000056e084 in evlist.print_counters () #4 0x0000000000432513 in cmd_stat () #5 0x00000000004c5fb9 in run_builtin () #6 0x00000000004c62c9 in handle_internal_command () #7 0x0000000000410e57 in main () (gdb) After: acme@number:~$ sudo ~acme/bin/perf stat -a -x : --per-cluster -M tma_core_bound --metric-only true cluster:cpus:% tma_core_bound:% tma_core_bound: S0-D0-CLS0:2:18.2:::: S0-D0-CLS8:2:26.7:::: S0-D0-CLS16:2:14.2:::: S0-D0-CLS24:2:10.6:::: S0-D0-CLS32:2:0.6:::: S0-D0-CLS40:2:42.5:::: S0-D0-CLS48:2:21.1:::: S0-D0-CLS56:2:36.8:::: S0-D0-CLS64:0:::::::1.0: S0-D0-CLS72:0:::::::0.8: S0-D0-CLS80:0:::::::1.0: acme@number:~$ Tested-by: Arnaldo Carvalho de Melo <acme@redhat.com> - Arnaldo > Fixes: cbc917a1b03b ("perf stat: Support per-cluster aggregation") > Cc: Yicong Yang <yangyicong@hisilicon.com> > Signed-off-by: Namhyung Kim <namhyung@kernel.org> > --- > tools/perf/util/stat-display.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c > index e8673c9f6b49..462227f663cb 100644 > --- a/tools/perf/util/stat-display.c > +++ b/tools/perf/util/stat-display.c > @@ -38,6 +38,7 @@ > static int aggr_header_lens[] = { > [AGGR_CORE] = 18, > [AGGR_CACHE] = 22, > + [AGGR_CLUSTER] = 20, > [AGGR_DIE] = 12, > [AGGR_SOCKET] = 6, > [AGGR_NODE] = 6, > @@ -49,6 +50,7 @@ static int aggr_header_lens[] = { > static const char *aggr_header_csv[] = { > [AGGR_CORE] = "core%scpus%s", > [AGGR_CACHE] = "cache%scpus%s", > + [AGGR_CLUSTER] = "cluster%scpus%s", > [AGGR_DIE] = "die%scpus%s", > [AGGR_SOCKET] = "socket%scpus%s", > [AGGR_NONE] = "cpu%s", > @@ -60,6 +62,7 @@ static const char *aggr_header_csv[] = { > static int aggr_header_num[] = { > [AGGR_CORE] = 2, > [AGGR_CACHE] = 2, > + [AGGR_CLUSTER] = 2, > [AGGR_DIE] = 2, > [AGGR_SOCKET] = 2, > [AGGR_NONE] = 1, > -- > 2.45.2.803.g4e1b14247a-goog ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] perf stat: Use field separator in the metric header 2024-06-27 20:03 [PATCH 1/2] perf stat: Use field separator in the metric header Namhyung Kim 2024-06-27 20:03 ` [PATCH 2/2] perf stat: Fix a segfault with --per-cluster --metric-only Namhyung Kim @ 2024-06-27 20:48 ` Ian Rogers 2024-06-27 22:23 ` Namhyung Kim 2024-06-28 12:44 ` Arnaldo Carvalho de Melo 2 siblings, 1 reply; 8+ messages in thread From: Ian Rogers @ 2024-06-27 20:48 UTC (permalink / raw) To: Namhyung Kim Cc: Arnaldo Carvalho de Melo, Kan Liang, Jiri Olsa, Adrian Hunter, Peter Zijlstra, Ingo Molnar, LKML, linux-perf-users On Thu, Jun 27, 2024 at 1:03 PM Namhyung Kim <namhyung@kernel.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@kernel.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 > ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] perf stat: Use field separator in the metric header 2024-06-27 20:48 ` [PATCH 1/2] perf stat: Use field separator in the metric header Ian Rogers @ 2024-06-27 22:23 ` Namhyung Kim 2025-02-24 18:50 ` Ian Rogers 0 siblings, 1 reply; 8+ messages in thread From: Namhyung Kim @ 2024-06-27 22:23 UTC (permalink / raw) To: Ian Rogers Cc: Arnaldo Carvalho de Melo, Kan Liang, Jiri Olsa, Adrian Hunter, Peter Zijlstra, Ingo Molnar, LKML, linux-perf-users Hi Ian, On Thu, Jun 27, 2024 at 1:48 PM Ian Rogers <irogers@google.com> wrote: > > On Thu, Jun 27, 2024 at 1:03 PM Namhyung Kim <namhyung@kernel.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@kernel.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); > } > ``` Looks good. But I think we should handle longer separators like -x ":::". Will do in v2. > 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 :-( Right, I also noticed something in the header. One more work item to the list. Anyway, I'll add it to the test case! Thanks, Namhyung > > > + 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 > > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] perf stat: Use field separator in the metric header 2024-06-27 22:23 ` Namhyung Kim @ 2025-02-24 18:50 ` Ian Rogers 2025-02-24 20:18 ` Namhyung Kim 0 siblings, 1 reply; 8+ messages in thread From: Ian Rogers @ 2025-02-24 18:50 UTC (permalink / raw) To: Namhyung Kim Cc: Arnaldo Carvalho de Melo, Kan Liang, Jiri Olsa, Adrian Hunter, Peter Zijlstra, Ingo Molnar, LKML, linux-perf-users On Thu, Jun 27, 2024 at 3:24 PM Namhyung Kim <namhyung@kernel.org> wrote: > > Hi Ian, > > On Thu, Jun 27, 2024 at 1:48 PM Ian Rogers <irogers@google.com> wrote: > > > > On Thu, Jun 27, 2024 at 1:03 PM Namhyung Kim <namhyung@kernel.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@kernel.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); > > } > > ``` > > Looks good. But I think we should handle longer separators like -x ":::". > Will do in v2. Hi Namhyung, It looks like this has been forgotten. Did you have a v2? Thanks, Ian > > 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 :-( > > Right, I also noticed something in the header. One more work > item to the list. > > Anyway, I'll add it to the test case! > > Thanks, > Namhyung > > > > > > + 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 > > > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] perf stat: Use field separator in the metric header 2025-02-24 18:50 ` Ian Rogers @ 2025-02-24 20:18 ` Namhyung Kim 0 siblings, 0 replies; 8+ messages in thread From: Namhyung Kim @ 2025-02-24 20:18 UTC (permalink / raw) To: Ian Rogers Cc: Arnaldo Carvalho de Melo, Kan Liang, Jiri Olsa, Adrian Hunter, Peter Zijlstra, Ingo Molnar, LKML, linux-perf-users On Mon, Feb 24, 2025 at 10:50:24AM -0800, Ian Rogers wrote: > On Thu, Jun 27, 2024 at 3:24 PM Namhyung Kim <namhyung@kernel.org> wrote: > > > > Hi Ian, > > > > On Thu, Jun 27, 2024 at 1:48 PM Ian Rogers <irogers@google.com> wrote: > > > > > > On Thu, Jun 27, 2024 at 1:03 PM Namhyung Kim <namhyung@kernel.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@kernel.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); > > > } > > > ``` > > > > Looks good. But I think we should handle longer separators like -x ":::". > > Will do in v2. > > Hi Namhyung, > > It looks like this has been forgotten. Did you have a v2? It's merged to v6.11, please see https://lore.kernel.org/linux-perf-users/171994580797.2905908.17252651084023923233.b4-ty@kernel.org/ Thanks, Namhyung ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] perf stat: Use field separator in the metric header 2024-06-27 20:03 [PATCH 1/2] perf stat: Use field separator in the metric header Namhyung Kim 2024-06-27 20:03 ` [PATCH 2/2] perf stat: Fix a segfault with --per-cluster --metric-only Namhyung Kim 2024-06-27 20:48 ` [PATCH 1/2] perf stat: Use field separator in the metric header Ian Rogers @ 2024-06-28 12:44 ` Arnaldo Carvalho de Melo 2 siblings, 0 replies; 8+ messages in thread From: Arnaldo Carvalho de Melo @ 2024-06-28 12:44 UTC (permalink / raw) To: Namhyung Kim Cc: Ian Rogers, Kan Liang, Jiri Olsa, Adrian Hunter, Peter Zijlstra, Ingo Molnar, LKML, linux-perf-users On Thu, Jun 27, 2024 at 01:03:52PM -0700, Namhyung Kim 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@kernel.org> Tested-by: Arnaldo Carvalho de Melo <acme@redhat.com> - Arnaldo > --- > 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; > + > + 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2025-02-24 20:18 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-06-27 20:03 [PATCH 1/2] perf stat: Use field separator in the metric header Namhyung Kim 2024-06-27 20:03 ` [PATCH 2/2] perf stat: Fix a segfault with --per-cluster --metric-only Namhyung Kim 2024-06-28 12:47 ` Arnaldo Carvalho de Melo 2024-06-27 20:48 ` [PATCH 1/2] perf stat: Use field separator in the metric header Ian Rogers 2024-06-27 22:23 ` Namhyung Kim 2025-02-24 18:50 ` Ian Rogers 2025-02-24 20:18 ` Namhyung Kim 2024-06-28 12:44 ` Arnaldo Carvalho de Melo
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).