* [PATCH v2 2/2] perf stat: Use field separator in the metric header
2024-06-28 0:06 [PATCH v2 1/2] perf stat: Fix a segfault with --per-cluster --metric-only Namhyung Kim
@ 2024-06-28 0:06 ` Namhyung Kim
2024-06-28 2:33 ` [PATCH v2 1/2] perf stat: Fix a segfault with --per-cluster --metric-only Yicong Yang
2024-07-02 18:44 ` Namhyung Kim
2 siblings, 0 replies; 4+ messages in thread
From: Namhyung Kim @ 2024-06-28 0:06 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 | 17 ++++++++++++++---
1 file changed, 14 insertions(+), 3 deletions(-)
diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c
index 186305fd2d0e..c38bcb6f4c78 100644
--- a/tools/perf/util/stat-display.c
+++ b/tools/perf/util/stat-display.c
@@ -1186,10 +1186,21 @@ static void print_metric_headers_std(struct perf_stat_config *config,
static void print_metric_headers_csv(struct perf_stat_config *config,
bool no_indent __maybe_unused)
{
+ const char *p;
+
if (config->interval)
- fputs("time,", config->output);
- if (!config->iostat_run)
- fputs(aggr_header_csv[config->aggr_mode], config->output);
+ fprintf(config->output, "time%s", config->csv_sep);
+ if (config->iostat_run)
+ return;
+
+ p = aggr_header_csv[config->aggr_mode];
+ while (*p) {
+ if (*p == ',')
+ fputs(config->csv_sep, config->output);
+ else
+ fputc(*p, config->output);
+ p++;
+ }
}
static void print_metric_headers_json(struct perf_stat_config *config __maybe_unused,
--
2.45.2.803.g4e1b14247a-goog
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH v2 1/2] perf stat: Fix a segfault with --per-cluster --metric-only
2024-06-28 0:06 [PATCH v2 1/2] perf stat: Fix a segfault with --per-cluster --metric-only Namhyung Kim
2024-06-28 0:06 ` [PATCH v2 2/2] perf stat: Use field separator in the metric header Namhyung Kim
@ 2024-06-28 2:33 ` Yicong Yang
2024-07-02 18:44 ` Namhyung Kim
2 siblings, 0 replies; 4+ messages in thread
From: Yicong Yang @ 2024-06-28 2:33 UTC (permalink / raw)
To: Namhyung Kim, Arnaldo Carvalho de Melo, Ian Rogers, Kan Liang
Cc: yangyicong, Jiri Olsa, Adrian Hunter, Peter Zijlstra,
Ingo Molnar, LKML, linux-perf-users
Hi Namhyung,
On 2024/6/28 8:06, 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().
>
> Fixes: cbc917a1b03b ("perf stat: Support per-cluster aggregation")
> Cc: Yicong Yang <yangyicong@hisilicon.com>
Thanks for fixing this.
Reviewed-by: Yicong Yang <yangyicong@hisilicon.com>
Tested on my cluster machine and this patch fixes the issue:
[root@localhost perf]# ./perf-before stat -M TopDownL1 --per-cluster --metric-only --timeout 100 -x ,
Segmentation fault (core dumped)
[root@localhost perf]# ./perf-after stat -M TopDownL1 --per-cluster --metric-only --timeout 100 -x ,
cluster,cpus,retiring,bad_speculation,backend_bound,frontend_bound,
S56-D0-CLS158,4,0.03,0.00,0.96,0.00,
[...]
So,
Tested-by: Yicong Yang <yangyicong@hisilicon.com>
Thanks.
> 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 91d2f7f65df7..186305fd2d0e 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,cpus,",
> [AGGR_CACHE] = "cache,cpus,",
> + [AGGR_CLUSTER] = "cluster,cpus,",
> [AGGR_DIE] = "die,cpus,",
> [AGGR_SOCKET] = "socket,cpus,",
> [AGGR_NONE] = "cpu,",
> @@ -60,6 +62,7 @@ static const char *aggr_header_csv[] = {
> static const char *aggr_header_std[] = {
> [AGGR_CORE] = "core",
> [AGGR_CACHE] = "cache",
> + [AGGR_CLUSTER] = "cluster",
> [AGGR_DIE] = "die",
> [AGGR_SOCKET] = "socket",
> [AGGR_NONE] = "cpu",
>
^ permalink raw reply [flat|nested] 4+ messages in thread