* [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 an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.