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