* [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 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 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
* 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 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
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).