[PATCHSET 0/9] perf stat: Cleanup perf stat output display (v1)

[PATCHSET 0/9] perf stat: Cleanup perf stat output display (v1)

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

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

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

Thanks,
Namhyung

Namhyung Kim (9):
  perf stat: Fix crash with --per-node --metric-only in CSV mode
  perf stat: Increase metric length to align outputs
  perf stat: Clear screen only if output file is a tty
  perf stat: Move common code in print_metric_headers()
  perf stat: Fix --metric-only --json output
  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

 tools/perf/util/stat-display.c | 55 ++++++++++++++--------------------
 1 file changed, 23 insertions(+), 32 deletions(-)


base-commit: 96e6d929a6c3368ad6327a52f870294747888c77
-- 
2.38.1.431.g37b22c650d-goog

[PATCH 1/9] 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.431.g37b22c650d-goog

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

[Arnaldo Carvalho de Melo]

Em Mon, Nov 07, 2022 at 01:33:06PM -0800, Namhyung Kim escreveu:
> 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

Thanks, applied to perf/urgent.

- Arnaldo
 
> 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.431.g37b22c650d-goog

-- 

- Arnaldo

[PATCH 2/9] 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.

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 ea41e6308c50..17d656566cd9 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.431.g37b22c650d-goog

Re: [PATCH 2/9] perf stat: Increase metric length to align outputs

[Ian Rogers]

On Mon, Nov 7, 2022 at 1:33 PM Namhyung Kim <namhyung@kernel.org> wrote:
>
> 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.
>
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>

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

Thanks,
Ian

> ---
>  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 ea41e6308c50..17d656566cd9 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.431.g37b22c650d-goog
>

[PATCH 3/9] 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.

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 17d656566cd9..d4936a502560 100644
--- a/tools/perf/util/stat-display.c
+++ b/tools/perf/util/stat-display.c
@@ -892,7 +892,7 @@ static void print_interval(struct perf_stat_config *config,
 	FILE *output = config->output;
 	static int num_print_interval;
 
-	if (config->interval_clear)
+	if (config->interval_clear && isatty(fileno(output)))
 		puts(CONSOLE_CLEAR);
 
 	if (!config->iostat_run && !config->json_output)
-- 
2.38.1.431.g37b22c650d-goog

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

[Ian Rogers]

On Mon, Nov 7, 2022 at 1:33 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.

Would it be more intuitive to warn if interval-clear is specified with a file?

Thanks,
Ian

> 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 17d656566cd9..d4936a502560 100644
> --- a/tools/perf/util/stat-display.c
> +++ b/tools/perf/util/stat-display.c
> @@ -892,7 +892,7 @@ static void print_interval(struct perf_stat_config *config,
>         FILE *output = config->output;
>         static int num_print_interval;
>
> -       if (config->interval_clear)
> +       if (config->interval_clear && isatty(fileno(output)))
>                 puts(CONSOLE_CLEAR);
>
>         if (!config->iostat_run && !config->json_output)
> --
> 2.38.1.431.g37b22c650d-goog
>

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

[Namhyung Kim]

Hi Ian,

On Tue, Nov 8, 2022 at 3:16 PM Ian Rogers <irogers@google.com> wrote:
>
> On Mon, Nov 7, 2022 at 1:33 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.
>
> Would it be more intuitive to warn if interval-clear is specified with a file?

Makes sense, will change.

Thanks,
Namhyung

[PATCH 4/9] 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.

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 d4936a502560..115477461224 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.431.g37b22c650d-goog

Re: [PATCH 4/9] perf stat: Move common code in print_metric_headers()

[Ian Rogers]

On Mon, Nov 7, 2022 at 1:33 PM Namhyung Kim <namhyung@kernel.org> wrote:
>
> 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.
>
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>

Acked-by: Ian Rogers <irogers@google.com>
Could also potentially make it const, but functions it is passed to
would also need changing.

Thanks,
Ian

> ---
>  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 d4936a502560..115477461224 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.431.g37b22c650d-goog
>

Re: [PATCH 4/9] perf stat: Move common code in print_metric_headers()

[Namhyung Kim]

On Tue, Nov 8, 2022 at 3:19 PM Ian Rogers <irogers@google.com> wrote:
>
> On Mon, Nov 7, 2022 at 1:33 PM Namhyung Kim <namhyung@kernel.org> wrote:
> >
> > 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.
> >
> > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
>
> Acked-by: Ian Rogers <irogers@google.com>
> Could also potentially make it const, but functions it is passed to
> would also need changing.

I'll consider when I work on it later.

Thanks,
Namhyung

[PATCH 5/9] 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"}

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 115477461224..515fb6db3d67 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.431.g37b22c650d-goog

Re: [PATCH 5/9] perf stat: Fix --metric-only --json output

[Arnaldo Carvalho de Melo]

Em Mon, Nov 07, 2022 at 01:33:10PM -0800, Namhyung Kim escreveu:
> 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"}

Can we get a Fixes tag for this one?

- Arnaldo
 
> 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 115477461224..515fb6db3d67 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.431.g37b22c650d-goog

-- 

- Arnaldo

Re: [PATCH 5/9] perf stat: Fix --metric-only --json output

[Namhyung Kim]

Hi Arnaldo,

On Tue, Nov 8, 2022 at 12:45 PM Arnaldo Carvalho de Melo
<acme@kernel.org> wrote:
>
> Em Mon, Nov 07, 2022 at 01:33:10PM -0800, Namhyung Kim escreveu:
> > 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"}
>
> Can we get a Fixes tag for this one?

I think this is it:

df936cadfb58 ("perf stat: Add JSON output option")

But this also depends on patch 4.  Do you want me to rebase
this not to depend on it?

Thanks,
Namhyung

>
> > 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 115477461224..515fb6db3d67 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.431.g37b22c650d-goog
>
> --
>
> - Arnaldo

Re: [PATCH 5/9] perf stat: Fix --metric-only --json output

[Arnaldo Carvalho de Melo]

Em Tue, Nov 08, 2022 at 02:07:39PM -0800, Namhyung Kim escreveu:
> Hi Arnaldo,
> 
> On Tue, Nov 8, 2022 at 12:45 PM Arnaldo Carvalho de Melo
> <acme@kernel.org> wrote:
> >
> > Em Mon, Nov 07, 2022 at 01:33:10PM -0800, Namhyung Kim escreveu:
> > > 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"}
> >
> > Can we get a Fixes tag for this one?
> 
> I think this is it:
> 
> df936cadfb58 ("perf stat: Add JSON output option")

I'll add it
 
> But this also depends on patch 4.  Do you want me to rebase
> this not to depend on it?

Nope, I put the first one on perf/urgent, 2- on perf/core,

- Arnaldo
 
> Thanks,
> Namhyung
> 
> >
> > > 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 115477461224..515fb6db3d67 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.431.g37b22c650d-goog
> >
> > --
> >
> > - Arnaldo

-- 

- Arnaldo

Re: [PATCH 5/9] perf stat: Fix --metric-only --json output

[Ian Rogers]

On Mon, Nov 7, 2022 at 1:33 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"}

It looks like the correct output would be to place the unit with the
metric-value, but perhaps that's something best done by deleting the
output code and starting again - the new output is wrong but better.
We could really do with functions like emit counter, emit metric and
have appropriate output for CSV, JSON, etc. A lot of the problems in
JSON and CSV outputting come from the unnatural way it is driven and
placing a large significance on newline characters. Putting the CSV
and JSON output to stdout also is problematic, as it naturally gets
interwoven with the monitored commands output. The dictionaries in the
new output should really be in a list, this is fixed in the test here:
https://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git/tree/tools/perf/tests/shell/lib/perf_json_output_lint.py?h=perf/core#n74

Thanks,
Ian

>
> 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 115477461224..515fb6db3d67 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.431.g37b22c650d-goog
>

[PATCH 6/9] 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.

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 515fb6db3d67..25f67fb37f6d 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.431.g37b22c650d-goog

Re: [PATCH 6/9] perf stat: Do not indent headers for JSON

[Ian Rogers]

On Mon, Nov 7, 2022 at 1:33 PM Namhyung Kim <namhyung@kernel.org> wrote:
>
> 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.
>
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>

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

Thanks,
Ian

> ---
>  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 515fb6db3d67..25f67fb37f6d 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.431.g37b22c650d-goog
>

[PATCH 7/9] 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

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 25f67fb37f6d..aab2576bd40f 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.431.g37b22c650d-goog

Re: [PATCH 7/9] perf stat: Add header for interval in JSON output

[Ian Rogers]

On Mon, Nov 7, 2022 at 1:33 PM Namhyung Kim <namhyung@kernel.org> wrote:
>
> 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
>
> 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, 4 insertions(+)
>
> diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c
> index 25f67fb37f6d..aab2576bd40f 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.431.g37b22c650d-goog
>

[PATCH 8/9] 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.

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 aab2576bd40f..a10af26c26ab 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.431.g37b22c650d-goog

Re: [PATCH 8/9] perf stat: Fix condition in print_interval()

[Ian Rogers]

On Mon, Nov 7, 2022 at 1:33 PM Namhyung Kim <namhyung@kernel.org> wrote:
>
> 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.
>
> 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 aab2576bd40f..a10af26c26ab 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.431.g37b22c650d-goog
>

[PATCH 9/9] 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.

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 a10af26c26ab..4d3999673dde 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.431.g37b22c650d-goog

Re: [PATCH 9/9] perf stat: Consolidate condition to print metrics

[Ian Rogers]

On Mon, Nov 7, 2022 at 1:33 PM Namhyung Kim <namhyung@kernel.org> wrote:
>
> 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.
>
> 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, 1 insertion(+), 3 deletions(-)
>
> diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c
> index a10af26c26ab..4d3999673dde 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.431.g37b22c650d-goog
>