[PATCHSET 00/11] perf stat: Cleanup perf stat output display (v2)

[PATCHSET 00/11] perf stat: Cleanup perf stat output display (v2)

[Namhyung Kim]

Hello,

I'm working on cleaning up the perf stat code and found a number of issues.
Before moving on to the real changes, I'd like to fix those first.  I'll
think about how to test this properly.

changes in v2)
 * move applied patches to the beginning  (Arnaldo)
 * make it error to use --interval-clear with other output  (Ian)
 * add two more patches for CSV output
 * add Acked-by from Ian

You can get it from 'perf/stat-cleanup-v2' branch in

  git://git.kernel.org/pub/scm/linux/kernel/git/namhyung/linux-perf.git

Thanks,
Namhyung


Namhyung Kim (11):
  perf stat: Fix crash with --per-node --metric-only in CSV mode
  perf stat: Move common code in print_metric_headers()
  perf stat: Fix --metric-only --json output
  perf stat: Increase metric length to align outputs
  perf stat: Clear screen only if output file is a tty
  perf stat: Do not indent headers for JSON
  perf stat: Add header for interval in JSON output
  perf stat: Fix condition in print_interval()
  perf stat: Consolidate condition to print metrics
  perf stat: Fix summary output in CSV with --metric-only
  perf stat: Add missing separator in the CSV header

 tools/perf/builtin-stat.c      |  8 ++++
 tools/perf/util/stat-display.c | 68 ++++++++++++++++------------------
 2 files changed, 40 insertions(+), 36 deletions(-)


base-commit: 816815b852216f3aa3a43e2ce91c5510927cd61b
-- 
2.38.1.493.g58b659f92b-goog

[PATCH 01/11] perf stat: Fix crash with --per-node --metric-only in CSV mode

[Namhyung Kim]

The following command will get segfault due to missing aggr_header_csv
for AGGR_NODE:

  $ sudo perf stat -a --per-node -x, --metric-only true

Fixes: 86895b480a2f ("perf stat: Add --per-node agregation support")
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/stat-display.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c
index 657434cd29ee..ea41e6308c50 100644
--- a/tools/perf/util/stat-display.c
+++ b/tools/perf/util/stat-display.c
@@ -534,7 +534,7 @@ static void printout(struct perf_stat_config *config, struct aggr_cpu_id id, int
 			[AGGR_CORE] = 2,
 			[AGGR_THREAD] = 1,
 			[AGGR_UNSET] = 0,
-			[AGGR_NODE] = 0,
+			[AGGR_NODE] = 1,
 		};
 
 		pm = config->metric_only ? print_metric_only_csv : print_metric_csv;
@@ -819,6 +819,7 @@ static int aggr_header_lens[] = {
 	[AGGR_SOCKET] = 12,
 	[AGGR_NONE] = 6,
 	[AGGR_THREAD] = 24,
+	[AGGR_NODE] = 6,
 	[AGGR_GLOBAL] = 0,
 };
 
@@ -828,6 +829,7 @@ static const char *aggr_header_csv[] = {
 	[AGGR_SOCKET] 	= 	"socket,cpus",
 	[AGGR_NONE] 	= 	"cpu,",
 	[AGGR_THREAD] 	= 	"comm-pid,",
+	[AGGR_NODE] 	= 	"node,",
 	[AGGR_GLOBAL] 	=	""
 };
 
-- 
2.38.1.493.g58b659f92b-goog

[PATCH 02/11] perf stat: Move common code in print_metric_headers()

[Namhyung Kim]

The struct perf_stat_output_ctx is set in a loop with the same values.
Move the code out of the loop and keep the loop minimal.

Acked-by: Ian Rogers <irogers@google.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/stat-display.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c
index ea41e6308c50..c7b3a1e10263 100644
--- a/tools/perf/util/stat-display.c
+++ b/tools/perf/util/stat-display.c
@@ -837,11 +837,16 @@ static void print_metric_headers(struct perf_stat_config *config,
 				 struct evlist *evlist,
 				 const char *prefix, bool no_indent)
 {
-	struct perf_stat_output_ctx out;
 	struct evsel *counter;
 	struct outstate os = {
 		.fh = config->output
 	};
+	struct perf_stat_output_ctx out = {
+		.ctx = &os,
+		.print_metric = print_metric_header,
+		.new_line = new_line_metric,
+		.force_header = true,
+	};
 	bool first = true;
 
 	if (config->json_output && !config->interval)
@@ -865,13 +870,11 @@ static void print_metric_headers(struct perf_stat_config *config,
 	/* Print metrics headers only */
 	evlist__for_each_entry(evlist, counter) {
 		os.evsel = counter;
-		out.ctx = &os;
-		out.print_metric = print_metric_header;
+
 		if (!first && config->json_output)
 			fprintf(config->output, ", ");
 		first = false;
-		out.new_line = new_line_metric;
-		out.force_header = true;
+
 		perf_stat__print_shadow_stats(config, counter, 0,
 					      0,
 					      &out,
-- 
2.38.1.493.g58b659f92b-goog

[PATCH 03/11] perf stat: Fix --metric-only --json output

[Namhyung Kim]

Currently it prints all metric headers for JSON output.  But actually it
skips some metrics with valid_only_metric().  So the output looks like:

  $ perf stat --metric-only --json true
  {"unit" : "CPUs utilized", "unit" : "/sec", "unit" : "/sec", "unit" : "/sec", "unit" : "GHz", "unit" : "insn per cycle", "unit" : "/sec", "unit" : "branch-misses of all branches"}
  {"metric-value" : "3.861"}{"metric-value" : "0.79"}{"metric-value" : "3.04"}

As you can see there are 8 units in the header but only 3 metric-values
are there.  It should skip the unused headers as well.  Also each unit
should be printed as a separate object like metric values.

With this patch:

  $ perf stat --metric-only --json true
  {"unit" : "GHz"}{"unit" : "insn per cycle"}{"unit" : "branch-misses of all branches"}
  {"metric-value" : "4.166"}{"metric-value" : "0.73"}{"metric-value" : "2.96"}

Fixes: df936cadfb58 ("perf stat: Add JSON output option")
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/stat-display.c | 22 +++-------------------
 1 file changed, 3 insertions(+), 19 deletions(-)

diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c
index c7b3a1e10263..96ad0c71adc2 100644
--- a/tools/perf/util/stat-display.c
+++ b/tools/perf/util/stat-display.c
@@ -430,12 +430,12 @@ static void print_metric_header(struct perf_stat_config *config,
 	    os->evsel->priv != os->evsel->evlist->selected->priv)
 		return;
 
-	if (!valid_only_metric(unit) && !config->json_output)
+	if (!valid_only_metric(unit))
 		return;
 	unit = fixunit(tbuf, os->evsel, unit);
 
 	if (config->json_output)
-		fprintf(os->fh, "\"unit\" : \"%s\"", unit);
+		fprintf(os->fh, "{\"unit\" : \"%s\"}", unit);
 	else if (config->csv_output)
 		fprintf(os->fh, "%s%s", unit, config->csv_sep);
 	else
@@ -847,10 +847,6 @@ static void print_metric_headers(struct perf_stat_config *config,
 		.new_line = new_line_metric,
 		.force_header = true,
 	};
-	bool first = true;
-
-	if (config->json_output && !config->interval)
-		fprintf(config->output, "{");
 
 	if (prefix && !config->json_output)
 		fprintf(config->output, "%s", prefix);
@@ -871,18 +867,12 @@ static void print_metric_headers(struct perf_stat_config *config,
 	evlist__for_each_entry(evlist, counter) {
 		os.evsel = counter;
 
-		if (!first && config->json_output)
-			fprintf(config->output, ", ");
-		first = false;
-
 		perf_stat__print_shadow_stats(config, counter, 0,
 					      0,
 					      &out,
 					      &config->metric_events,
 					      &rt_stat);
 	}
-	if (config->json_output)
-		fprintf(config->output, "}");
 	fputc('\n', config->output);
 }
 
@@ -954,14 +944,8 @@ static void print_interval(struct perf_stat_config *config,
 		}
 	}
 
-	if ((num_print_interval == 0 || config->interval_clear)
-			 && metric_only && !config->json_output)
+	if ((num_print_interval == 0 || config->interval_clear) && metric_only)
 		print_metric_headers(config, evlist, " ", true);
-	if ((num_print_interval == 0 || config->interval_clear)
-			 && metric_only && config->json_output) {
-		fprintf(output, "{");
-		print_metric_headers(config, evlist, " ", true);
-	}
 	if (++num_print_interval == 25)
 		num_print_interval = 0;
 }
-- 
2.38.1.493.g58b659f92b-goog

[PATCH 04/11] perf stat: Increase metric length to align outputs

[Namhyung Kim]

When perf stat is called with very detailed events, the output doesn't
align well like below:

  $ sudo perf stat -a -ddd sleep 1

   Performance counter stats for 'system wide':

          8,020.23 msec cpu-clock                        #    7.997 CPUs utilized
             3,970      context-switches                 #  494.998 /sec
               169      cpu-migrations                   #   21.072 /sec
               586      page-faults                      #   73.065 /sec
       649,568,060      cycles                           #    0.081 GHz                      (30.42%)
       304,044,345      instructions                     #    0.47  insn per cycle           (38.40%)
        60,313,022      branches                         #    7.520 M/sec                    (38.89%)
         2,766,919      branch-misses                    #    4.59% of all branches          (39.26%)
        74,422,951      L1-dcache-loads                  #    9.279 M/sec                    (39.39%)
         8,025,568      L1-dcache-load-misses            #   10.78% of all L1-dcache accesses  (39.22%)
         3,314,995      LLC-loads                        #  413.329 K/sec                    (30.83%)
         1,225,619      LLC-load-misses                  #   36.97% of all LL-cache accesses  (30.45%)
   <not supported>      L1-icache-loads
        20,420,493      L1-icache-load-misses            #    0.00% of all L1-icache accesses  (30.29%)
        58,017,947      dTLB-loads                       #    7.234 M/sec                    (30.37%)
           704,677      dTLB-load-misses                 #    1.21% of all dTLB cache accesses  (30.27%)
           234,225      iTLB-loads                       #   29.204 K/sec                    (30.29%)
           417,166      iTLB-load-misses                 #  178.10% of all iTLB cache accesses  (30.32%)
   <not supported>      L1-dcache-prefetches
   <not supported>      L1-dcache-prefetch-misses

       1.002947355 seconds time elapsed

Increase the METRIC_LEN by 3 so that it can align properly.

  $ sudo ./perf stat -a -ddd true

   Performance counter stats for 'system wide':

              233.70 msec cpu-clock                        #   11.714 CPUs utilized
                 320      context-switches                 #    1.369 K/sec
                  31      cpu-migrations                   #  132.648 /sec
                 482      page-faults                      #    2.062 K/sec
          38,414,140      cycles                           #    0.164 GHz                         (6.65%)
          13,656,335      instructions                     #    0.36  insn per cycle              (20.31%)
           3,594,871      branches                         #   15.382 M/sec                       (34.06%)
             124,071      branch-misses                    #    3.45% of all branches             (47.74%)
          14,016,847      L1-dcache-loads                  #   59.977 M/sec                       (61.42%)
             789,534      L1-dcache-load-misses            #    5.63% of all L1-dcache accesses   (65.98%)
             315,353      LLC-loads                        #    1.349 M/sec                       (47.13%)
             171,244      LLC-load-misses                  #   54.30% of all LL-cache accesses    (41.98%)
     <not supported>      L1-icache-loads
           2,591,299      L1-icache-load-misses            #    0.00% of all L1-icache accesses   (36.10%)
          60,421,987      dTLB-loads                       #  258.543 M/sec                       (27.47%)
             123,843      dTLB-load-misses                 #    0.20% of all dTLB cache accesses  (18.88%)
              38,289      iTLB-loads                       #  163.837 K/sec                       (10.34%)
              21,631      iTLB-load-misses                 #   56.49% of all iTLB cache accesses  (2.53%)
     <not supported>      L1-dcache-prefetches
     <not supported>      L1-dcache-prefetch-misses

Acked-by: Ian Rogers <irogers@google.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/stat-display.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c
index 96ad0c71adc2..3ddc159df4b2 100644
--- a/tools/perf/util/stat-display.c
+++ b/tools/perf/util/stat-display.c
@@ -218,7 +218,7 @@ struct outstate {
 	struct evsel *evsel;
 };
 
-#define METRIC_LEN  35
+#define METRIC_LEN  38
 
 static void new_line_std(struct perf_stat_config *config __maybe_unused,
 			 void *ctx)
-- 
2.38.1.493.g58b659f92b-goog

[PATCH 05/11] perf stat: Clear screen only if output file is a tty

[Namhyung Kim]

The --interval-clear option makes perf stat to clear the terminal at
each interval.  But it doesn't need to clear the screen when it saves
to a file.  Make it fail when it's enabled with the output options.

  $ perf stat -I 1 --interval-clear -o myfile true
  --interval-clear does not work with output

   Usage: perf stat [<options>] [<command>]

      -o, --output <file>   output file name
          --log-fd <n>      log output to fd, instead of stderr
          --interval-clear  clear screen in between new interval

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/builtin-stat.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index d5e1670bca20..1d79801f4e84 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -2403,6 +2403,14 @@ int cmd_stat(int argc, const char **argv)
 		}
 	}
 
+	if (stat_config.interval_clear && !isatty(fileno(output))) {
+		fprintf(stderr, "--interval-clear does not work with output\n");
+		parse_options_usage(stat_usage, stat_options, "o", 1);
+		parse_options_usage(NULL, stat_options, "log-fd", 0);
+		parse_options_usage(NULL, stat_options, "interval-clear", 0);
+		return -1;
+	}
+
 	stat_config.output = output;
 
 	/*
-- 
2.38.1.493.g58b659f92b-goog

[PATCH 06/11] perf stat: Do not indent headers for JSON

[Namhyung Kim]

Currently --metric-only with --json indents header lines.  This is not
needed for JSON.

  $ perf stat -aA --metric-only -j true
        {"unit" : "GHz"}{"unit" : "insn per cycle"}{"unit" : "branch-misses of all branches"}
  {"cpu" : "0", {"metric-value" : "0.101"}{"metric-value" : "0.86"}{"metric-value" : "1.91"}
  {"cpu" : "1", {"metric-value" : "0.102"}{"metric-value" : "0.87"}{"metric-value" : "2.02"}
  {"cpu" : "2", {"metric-value" : "0.085"}{"metric-value" : "1.02"}{"metric-value" : "1.69"}
  ...

Note that the other lines are broken JSON, but it will be handled later.

Acked-by: Ian Rogers <irogers@google.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/stat-display.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c
index 3ddc159df4b2..dbd3ba380c9f 100644
--- a/tools/perf/util/stat-display.c
+++ b/tools/perf/util/stat-display.c
@@ -851,7 +851,7 @@ static void print_metric_headers(struct perf_stat_config *config,
 	if (prefix && !config->json_output)
 		fprintf(config->output, "%s", prefix);
 
-	if (!config->csv_output && !no_indent)
+	if (!config->csv_output && !config->json_output && !no_indent)
 		fprintf(config->output, "%*s",
 			aggr_header_lens[config->aggr_mode], "");
 	if (config->csv_output) {
-- 
2.38.1.493.g58b659f92b-goog

[PATCH 07/11] perf stat: Add header for interval in JSON output

[Namhyung Kim]

It missed to print a matching header line for intervals.

Before:
  # perf stat -a -e cycles,instructions --metric-only -j -I 500
  {"unit" : "insn per cycle"}
  {"interval" : 0.500544283}{"metric-value" : "1.96"}
  ^C

After:
  # perf stat -a -e cycles,instructions --metric-only -j -I 500
  {"unit" : "sec"}{"unit" : "insn per cycle"}
  {"interval" : 0.500515681}{"metric-value" : "2.31"}
  ^C

Acked-by: Ian Rogers <irogers@google.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/stat-display.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c
index dbd3ba380c9f..03d58277e8d6 100644
--- a/tools/perf/util/stat-display.c
+++ b/tools/perf/util/stat-display.c
@@ -860,6 +860,10 @@ static void print_metric_headers(struct perf_stat_config *config,
 		if (!config->iostat_run)
 			fputs(aggr_header_csv[config->aggr_mode], config->output);
 	}
+	if (config->json_output) {
+		if (config->interval)
+			fputs("{\"unit\" : \"sec\"}", config->output);
+	}
 	if (config->iostat_run)
 		iostat_print_header_prefix(config);
 
-- 
2.38.1.493.g58b659f92b-goog

[PATCH 08/11] perf stat: Fix condition in print_interval()

[Namhyung Kim]

The num_print_interval and config->interval_clear should be checked
together like other places like later in the function.  Otherwise,
the --interval-clear option could print the headers for the CSV or
JSON output unnecessarily.

Acked-by: Ian Rogers <irogers@google.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/stat-display.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c
index 03d58277e8d6..5f4fb0bd4037 100644
--- a/tools/perf/util/stat-display.c
+++ b/tools/perf/util/stat-display.c
@@ -902,8 +902,8 @@ static void print_interval(struct perf_stat_config *config,
 		sprintf(prefix, "{\"interval\" : %lu.%09lu}", (unsigned long)
 				 ts->tv_sec, ts->tv_nsec);
 
-	if ((num_print_interval == 0 && !config->csv_output && !config->json_output)
-			 || config->interval_clear) {
+	if ((num_print_interval == 0 || config->interval_clear) &&
+			!config->csv_output && !config->json_output) {
 		switch (config->aggr_mode) {
 		case AGGR_NODE:
 			fprintf(output, "#           time node   cpus");
-- 
2.38.1.493.g58b659f92b-goog

[PATCH 09/11] perf stat: Consolidate condition to print metrics

[Namhyung Kim]

The pm variable holds an appropriate function to print metrics for CSV
anf JSON already.  So we can combine the if statement to simplify the
code a little bit.  This also matches to the above condition for non-CSV
and non-JSON case.

Acked-by: Ian Rogers <irogers@google.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/stat-display.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c
index 5f4fb0bd4037..ce81798b5864 100644
--- a/tools/perf/util/stat-display.c
+++ b/tools/perf/util/stat-display.c
@@ -600,9 +600,7 @@ static void printout(struct perf_stat_config *config, struct aggr_cpu_id id, int
 			pm(config, &os, NULL, NULL, "", 0);
 		print_noise(config, counter, noise);
 		print_running(config, run, ena);
-		if (config->csv_output)
-			pm(config, &os, NULL, NULL, "", 0);
-		else if (config->json_output)
+		if (config->csv_output || config->json_output)
 			pm(config, &os, NULL, NULL, "", 0);
 		return;
 	}
-- 
2.38.1.493.g58b659f92b-goog

[PATCH 10/11] perf stat: Fix summary output in CSV with --metric-only

[Namhyung Kim]

It should not print "summary" for each event when --metric-only is set.

Before:
  $ sudo perf stat -a --per-socket --summary -x, --metric-only true
   time,socket,cpusGhz,insn per cycle,branch-misses of all branches,
       0.000709079,S0,8,0.893,2.40,0.45,
  S0,8,         summary,         summary,         summary,         summary,         summary,0.893,         summary,2.40,         summary,         summary,0.45,

After:
  $ sudo perf stat -a --per-socket --summary -x, --metric-only true
   time,socket,cpusGHz,insn per cycle,branch-misses of all branches,
       0.000882297,S0,8,0.598,1.64,0.64,
           summary,S0,8,0.598,1.64,0.64,

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/stat-display.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c
index ce81798b5864..96bb7a42fd41 100644
--- a/tools/perf/util/stat-display.c
+++ b/tools/perf/util/stat-display.c
@@ -549,7 +549,7 @@ static void printout(struct perf_stat_config *config, struct aggr_cpu_id id, int
 	}
 
 	if (!config->no_csv_summary && config->csv_output &&
-	    config->summary && !config->interval) {
+	    config->summary && !config->interval && !config->metric_only) {
 		fprintf(config->output, "%16s%s", "summary", config->csv_sep);
 	}
 
@@ -732,8 +732,13 @@ static void print_aggr(struct perf_stat_config *config,
 	 * Without each counter has its own line.
 	 */
 	for (s = 0; s < config->aggr_map->nr; s++) {
-		if (prefix && metric_only)
-			fprintf(output, "%s", prefix);
+		if (metric_only) {
+			if (prefix)
+				fprintf(output, "%s", prefix);
+			else if (config->summary && !config->no_csv_summary &&
+				 config->csv_output && !config->interval)
+				fprintf(output, "%16s%s", "summary", config->csv_sep);
+		}
 
 		first = true;
 		evlist__for_each_entry(evlist, counter) {
-- 
2.38.1.493.g58b659f92b-goog

[PATCH 11/11] perf stat: Add missing separator in the CSV header

[Namhyung Kim]

It should have a comma after 'cpus' for socket and die aggregation mode.
The output of the following command shows the issue.

  $ sudo perf stat -a --per-socket -x, --metric-only -I1 true

Before:
                  +--- here
                  V
   time,socket,cpusGhz,insn per cycle,branch-misses of all branches,
       0.000908461,S0,8,0.950,1.65,1.21,

After:
   time,socket,cpus,GHz,insn per cycle,branch-misses of all branches,
       0.000683094,S0,8,0.593,2.00,0.60,

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/stat-display.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c
index 96bb7a42fd41..a316807255cd 100644
--- a/tools/perf/util/stat-display.c
+++ b/tools/perf/util/stat-display.c
@@ -828,8 +828,8 @@ static int aggr_header_lens[] = {
 
 static const char *aggr_header_csv[] = {
 	[AGGR_CORE] 	= 	"core,cpus,",
-	[AGGR_DIE] 	= 	"die,cpus",
-	[AGGR_SOCKET] 	= 	"socket,cpus",
+	[AGGR_DIE] 	= 	"die,cpus,",
+	[AGGR_SOCKET] 	= 	"socket,cpus,",
 	[AGGR_NONE] 	= 	"cpu,",
 	[AGGR_THREAD] 	= 	"comm-pid,",
 	[AGGR_NODE] 	= 	"node,",
-- 
2.38.1.493.g58b659f92b-goog

Re: [PATCH 01/11] perf stat: Fix crash with --per-node --metric-only in CSV mode

[Ian Rogers]

On Fri, Nov 11, 2022 at 7:22 PM Namhyung Kim <namhyung@kernel.org> wrote:
>
> The following command will get segfault due to missing aggr_header_csv
> for AGGR_NODE:
>
>   $ sudo perf stat -a --per-node -x, --metric-only true
>
> Fixes: 86895b480a2f ("perf stat: Add --per-node agregation support")
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>

Acked-by: Ian Rogers <irogers@google.com>

Thanks,
Ian

> ---
>  tools/perf/util/stat-display.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c
> index 657434cd29ee..ea41e6308c50 100644
> --- a/tools/perf/util/stat-display.c
> +++ b/tools/perf/util/stat-display.c
> @@ -534,7 +534,7 @@ static void printout(struct perf_stat_config *config, struct aggr_cpu_id id, int
>                         [AGGR_CORE] = 2,
>                         [AGGR_THREAD] = 1,
>                         [AGGR_UNSET] = 0,
> -                       [AGGR_NODE] = 0,
> +                       [AGGR_NODE] = 1,
>                 };
>
>                 pm = config->metric_only ? print_metric_only_csv : print_metric_csv;
> @@ -819,6 +819,7 @@ static int aggr_header_lens[] = {
>         [AGGR_SOCKET] = 12,
>         [AGGR_NONE] = 6,
>         [AGGR_THREAD] = 24,
> +       [AGGR_NODE] = 6,
>         [AGGR_GLOBAL] = 0,
>  };
>
> @@ -828,6 +829,7 @@ static const char *aggr_header_csv[] = {
>         [AGGR_SOCKET]   =       "socket,cpus",
>         [AGGR_NONE]     =       "cpu,",
>         [AGGR_THREAD]   =       "comm-pid,",
> +       [AGGR_NODE]     =       "node,",
>         [AGGR_GLOBAL]   =       ""
>  };
>
> --
> 2.38.1.493.g58b659f92b-goog
>

Re: [PATCH 03/11] perf stat: Fix --metric-only --json output

[Ian Rogers]

On Fri, Nov 11, 2022 at 7:22 PM Namhyung Kim <namhyung@kernel.org> wrote:
>
> Currently it prints all metric headers for JSON output.  But actually it
> skips some metrics with valid_only_metric().  So the output looks like:
>
>   $ perf stat --metric-only --json true
>   {"unit" : "CPUs utilized", "unit" : "/sec", "unit" : "/sec", "unit" : "/sec", "unit" : "GHz", "unit" : "insn per cycle", "unit" : "/sec", "unit" : "branch-misses of all branches"}
>   {"metric-value" : "3.861"}{"metric-value" : "0.79"}{"metric-value" : "3.04"}
>
> As you can see there are 8 units in the header but only 3 metric-values
> are there.  It should skip the unused headers as well.  Also each unit
> should be printed as a separate object like metric values.
>
> With this patch:
>
>   $ perf stat --metric-only --json true
>   {"unit" : "GHz"}{"unit" : "insn per cycle"}{"unit" : "branch-misses of all branches"}
>   {"metric-value" : "4.166"}{"metric-value" : "0.73"}{"metric-value" : "2.96"}
>
> Fixes: df936cadfb58 ("perf stat: Add JSON output option")
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>

Acked-by: Ian Rogers <irogers@google.com>

Thanks,
Ian

> ---
>  tools/perf/util/stat-display.c | 22 +++-------------------
>  1 file changed, 3 insertions(+), 19 deletions(-)
>
> diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c
> index c7b3a1e10263..96ad0c71adc2 100644
> --- a/tools/perf/util/stat-display.c
> +++ b/tools/perf/util/stat-display.c
> @@ -430,12 +430,12 @@ static void print_metric_header(struct perf_stat_config *config,
>             os->evsel->priv != os->evsel->evlist->selected->priv)
>                 return;
>
> -       if (!valid_only_metric(unit) && !config->json_output)
> +       if (!valid_only_metric(unit))
>                 return;
>         unit = fixunit(tbuf, os->evsel, unit);
>
>         if (config->json_output)
> -               fprintf(os->fh, "\"unit\" : \"%s\"", unit);
> +               fprintf(os->fh, "{\"unit\" : \"%s\"}", unit);
>         else if (config->csv_output)
>                 fprintf(os->fh, "%s%s", unit, config->csv_sep);
>         else
> @@ -847,10 +847,6 @@ static void print_metric_headers(struct perf_stat_config *config,
>                 .new_line = new_line_metric,
>                 .force_header = true,
>         };
> -       bool first = true;
> -
> -       if (config->json_output && !config->interval)
> -               fprintf(config->output, "{");
>
>         if (prefix && !config->json_output)
>                 fprintf(config->output, "%s", prefix);
> @@ -871,18 +867,12 @@ static void print_metric_headers(struct perf_stat_config *config,
>         evlist__for_each_entry(evlist, counter) {
>                 os.evsel = counter;
>
> -               if (!first && config->json_output)
> -                       fprintf(config->output, ", ");
> -               first = false;
> -
>                 perf_stat__print_shadow_stats(config, counter, 0,
>                                               0,
>                                               &out,
>                                               &config->metric_events,
>                                               &rt_stat);
>         }
> -       if (config->json_output)
> -               fprintf(config->output, "}");
>         fputc('\n', config->output);
>  }
>
> @@ -954,14 +944,8 @@ static void print_interval(struct perf_stat_config *config,
>                 }
>         }
>
> -       if ((num_print_interval == 0 || config->interval_clear)
> -                        && metric_only && !config->json_output)
> +       if ((num_print_interval == 0 || config->interval_clear) && metric_only)
>                 print_metric_headers(config, evlist, " ", true);
> -       if ((num_print_interval == 0 || config->interval_clear)
> -                        && metric_only && config->json_output) {
> -               fprintf(output, "{");
> -               print_metric_headers(config, evlist, " ", true);
> -       }
>         if (++num_print_interval == 25)
>                 num_print_interval = 0;
>  }
> --
> 2.38.1.493.g58b659f92b-goog
>

Re: [PATCH 05/11] perf stat: Clear screen only if output file is a tty

[Ian Rogers]

On Fri, Nov 11, 2022 at 7:22 PM Namhyung Kim <namhyung@kernel.org> wrote:
>
> The --interval-clear option makes perf stat to clear the terminal at
> each interval.  But it doesn't need to clear the screen when it saves
> to a file.  Make it fail when it's enabled with the output options.
>
>   $ perf stat -I 1 --interval-clear -o myfile true
>   --interval-clear does not work with output
>
>    Usage: perf stat [<options>] [<command>]
>
>       -o, --output <file>   output file name
>           --log-fd <n>      log output to fd, instead of stderr
>           --interval-clear  clear screen in between new interval
>
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>

Acked-by: Ian Rogers <irogers@google.com>

Thanks,
Ian

> ---
>  tools/perf/builtin-stat.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
> index d5e1670bca20..1d79801f4e84 100644
> --- a/tools/perf/builtin-stat.c
> +++ b/tools/perf/builtin-stat.c
> @@ -2403,6 +2403,14 @@ int cmd_stat(int argc, const char **argv)
>                 }
>         }
>
> +       if (stat_config.interval_clear && !isatty(fileno(output))) {
> +               fprintf(stderr, "--interval-clear does not work with output\n");
> +               parse_options_usage(stat_usage, stat_options, "o", 1);
> +               parse_options_usage(NULL, stat_options, "log-fd", 0);
> +               parse_options_usage(NULL, stat_options, "interval-clear", 0);
> +               return -1;
> +       }
> +
>         stat_config.output = output;
>
>         /*
> --
> 2.38.1.493.g58b659f92b-goog
>

Re: [PATCH 10/11] perf stat: Fix summary output in CSV with --metric-only

[Ian Rogers]

On Fri, Nov 11, 2022 at 7:23 PM Namhyung Kim <namhyung@kernel.org> wrote:
>
> It should not print "summary" for each event when --metric-only is set.
>
> Before:
>   $ sudo perf stat -a --per-socket --summary -x, --metric-only true
>    time,socket,cpusGhz,insn per cycle,branch-misses of all branches,
>        0.000709079,S0,8,0.893,2.40,0.45,
>   S0,8,         summary,         summary,         summary,         summary,         summary,0.893,         summary,2.40,         summary,         summary,0.45,
>
> After:
>   $ sudo perf stat -a --per-socket --summary -x, --metric-only true
>    time,socket,cpusGHz,insn per cycle,branch-misses of all branches,
>        0.000882297,S0,8,0.598,1.64,0.64,
>            summary,S0,8,0.598,1.64,0.64,
>
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>

Acked-by: Ian Rogers <irogers@google.com>

Thanks,
Ian

> ---
>  tools/perf/util/stat-display.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c
> index ce81798b5864..96bb7a42fd41 100644
> --- a/tools/perf/util/stat-display.c
> +++ b/tools/perf/util/stat-display.c
> @@ -549,7 +549,7 @@ static void printout(struct perf_stat_config *config, struct aggr_cpu_id id, int
>         }
>
>         if (!config->no_csv_summary && config->csv_output &&
> -           config->summary && !config->interval) {
> +           config->summary && !config->interval && !config->metric_only) {
>                 fprintf(config->output, "%16s%s", "summary", config->csv_sep);
>         }
>
> @@ -732,8 +732,13 @@ static void print_aggr(struct perf_stat_config *config,
>          * Without each counter has its own line.
>          */
>         for (s = 0; s < config->aggr_map->nr; s++) {
> -               if (prefix && metric_only)
> -                       fprintf(output, "%s", prefix);
> +               if (metric_only) {
> +                       if (prefix)
> +                               fprintf(output, "%s", prefix);
> +                       else if (config->summary && !config->no_csv_summary &&
> +                                config->csv_output && !config->interval)
> +                               fprintf(output, "%16s%s", "summary", config->csv_sep);
> +               }
>
>                 first = true;
>                 evlist__for_each_entry(evlist, counter) {
> --
> 2.38.1.493.g58b659f92b-goog
>

Re: [PATCH 11/11] perf stat: Add missing separator in the CSV header

[Ian Rogers]

On Fri, Nov 11, 2022 at 7:23 PM Namhyung Kim <namhyung@kernel.org> wrote:
>
> It should have a comma after 'cpus' for socket and die aggregation mode.
> The output of the following command shows the issue.
>
>   $ sudo perf stat -a --per-socket -x, --metric-only -I1 true
>
> Before:
>                   +--- here
>                   V
>    time,socket,cpusGhz,insn per cycle,branch-misses of all branches,
>        0.000908461,S0,8,0.950,1.65,1.21,
>
> After:
>    time,socket,cpus,GHz,insn per cycle,branch-misses of all branches,
>        0.000683094,S0,8,0.593,2.00,0.60,
>
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>

Acked-by: Ian Rogers <irogers@google.com>

Thanks,
Ian

> ---
>  tools/perf/util/stat-display.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c
> index 96bb7a42fd41..a316807255cd 100644
> --- a/tools/perf/util/stat-display.c
> +++ b/tools/perf/util/stat-display.c
> @@ -828,8 +828,8 @@ static int aggr_header_lens[] = {
>
>  static const char *aggr_header_csv[] = {
>         [AGGR_CORE]     =       "core,cpus,",
> -       [AGGR_DIE]      =       "die,cpus",
> -       [AGGR_SOCKET]   =       "socket,cpus",
> +       [AGGR_DIE]      =       "die,cpus,",
> +       [AGGR_SOCKET]   =       "socket,cpus,",
>         [AGGR_NONE]     =       "cpu,",
>         [AGGR_THREAD]   =       "comm-pid,",
>         [AGGR_NODE]     =       "node,",
> --
> 2.38.1.493.g58b659f92b-goog
>