[PATCH v2 1/2] perf stat: Fix a segfault with --per-cluster --metric-only

[PATCH v2 1/2] perf stat: Fix a segfault with --per-cluster --metric-only

[Namhyung Kim]

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 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",
-- 
2.45.2.803.g4e1b14247a-goog

[PATCH v2 2/2] perf stat: Use field separator in the metric header

[Namhyung Kim]

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

Re: [PATCH v2 1/2] perf stat: Fix a segfault with --per-cluster --metric-only

[Yicong Yang]

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",
> 

Re: [PATCH v2 1/2] perf stat: Fix a segfault with --per-cluster --metric-only

[Namhyung Kim]

On Thu, 27 Jun 2024 17:06:03 -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().
> 
> 

Applied both to perf-tools-next, thanks!

Best regards,
Namhyung