perf stat: Cleanup perf stat output display (v1)
Namhyung Kim
maintainercover perf stat: Cleanup perf stat output display (v1)
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
Signature · 1 line
2.38.1.431.g37b22c650d-goog
Namhyung Kim
maintainerPATCH 1/9 perf stat: Fix crash with --per-node --metric-only in CSV mode
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
--- tools/perf/util/stat-display.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
tools/perf/util/stat-display.c +3 -1
@@ -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] = "" };
Signature · 1 line
2.38.1.431.g37b22c650d-goog
Re: [PATCH 1/9] perf stat: Fix crash with --per-node --metric-only in CSV mode
Em Mon, Nov 07, 2022 at 01:33:06PM -0800, Namhyung Kim escreveu:
Quoted 4 lines
The following command will get segfault due to missing
aggr_header_csvforAGGR_NODE:$ sudo perf stat -a --per-node -x, --metric-only true
Thanks, applied to perf/urgent.
- Arnaldo
Quoted 37 lines
Fixes 86895b480a2f ("perf stat: Add --per-node agregation support")Signed-off-by Namhyung Kim <[email protected]>--- tools/perf/util/stat-display.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)tools/perf/util/stat-display.c +3
-1@@ -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] = "" };Signature · 1 line
2.38.1.431.g37b22c650d-goog
Signature · 1 line
- Arnaldo
Namhyung Kim
maintainerPATCH 2/9 perf stat: Increase metric length to align outputs
In reply to Namhyung KimWhen 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-misses1.002947355 seconds time elapsed
Increase the METRIC_LEN by 3 so that it can align properly.
--- tools/perf/util/stat-display.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
tools/perf/util/stat-display.c +1 -1
@@ -218,7 +218,7 @@ struct outstate {struct evsel *evsel; }; -#define METRIC_LEN 35 +#define METRIC_LEN 38static void new_line_std (struct perf_stat_config *config__maybe_unused ,void *ctx)
Signature · 1 line
2.38.1.431.g37b22c650d-goog
Re: [PATCH 2/9] perf stat: Increase metric length to align outputs
Quoted 31 lines · 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 1Performance 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
Quoted 3 lines
Increase the
METRIC_LENby 3 so that it can align properly.Signed-off-by Namhyung Kim <[email protected]>
Thanks,
Ian
Quoted 20 lines
--- tools/perf/util/stat-display.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)tools/perf/util/stat-display.c +1
-2@@ -218,7 +218,7 @@ struct outstate {struct evsel *evsel; }; -#define METRIC_LEN 35 +#define METRIC_LEN 38static void new_line_std (struct perf_stat_config *config__maybe_unused ,void *ctx) --2.38.1.431.g37b22c650d-goog
Namhyung Kim
maintainerPATCH 3/9 perf stat: Clear screen only if output file is a tty
In reply to Namhyung KimThe --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.
--- tools/perf/util/stat-display.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
tools/perf/util/stat-display.c +1 -1
@@ -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)
Signature · 1 line
2.38.1.431.g37b22c650d-goog
Ian Rogers
reviewerRe: [PATCH 3/9] perf stat: Clear screen only if output file is a tty
Quoted 4 lines · 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.
Would it be more intuitive to warn if interval-clear is specified with a file?
Thanks,
Ian
Quoted 21 lines
Signed-off-by Namhyung Kim <[email protected]>--- tools/perf/util/stat-display.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)tools/perf/util/stat-display.c +1
-2@@ -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
Namhyung Kim
maintainerRe: [PATCH 3/9] perf stat: Clear screen only if output file is a tty
Hi Ian,
Quoted 8 lines · Ian Rogers
Quoted 4 lines · 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.
Would it be more intuitive to warn if interval-clear is specified with a file?
Makes sense, will change.
Thanks,
Namhyung
Namhyung Kim
maintainerPATCH 4/9 perf stat: Move common code in print_metric_headers()
In reply to Namhyung KimThe 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.
--- tools/perf/util/stat-display.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-)
tools/perf/util/stat-display.c +8 -5
@@ -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,
Signature · 1 line
2.38.1.431.g37b22c650d-goog
Re: [PATCH 4/9] perf stat: Move common code in print_metric_headers()
Quoted 5 lines · Namhyung Kim
The struct
perf_stat_output_ctxis 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 <[email protected]>
Could also potentially make it const, but functions it is passed to would also need changing.
Thanks,
Ian
Quoted 45 lines
--- tools/perf/util/stat-display.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-)tools/perf/util/stat-display.c +8
-6@@ -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
Namhyung Kim
maintainerRe: [PATCH 4/9] perf stat: Move common code in print_metric_headers()
Quoted 11 lines · Ian Rogers
Quoted 5 lines · Namhyung Kim
The struct
perf_stat_output_ctxis 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 <[email protected]>Acked-by Ian Rogers <[email protected]>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
Namhyung Kim
maintainerPATCH 5/9 perf stat: Fix --metric-only --json output
In reply to Namhyung KimCurrently 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"}--- tools/perf/util/stat-display.c | 22 +++------------------- 1 file changed, 3 insertions(+), 19 deletions(-)
tools/perf/util/stat-display.c +3 -19
@@ -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 ; }
Signature · 1 line
2.38.1.431.g37b22c650d-goog
Arnaldo Carvalho de Melo
maintainerRe: [PATCH 5/9] perf stat: Fix --metric-only --json output
Em Mon, Nov 07, 2022 at 01:33:10PM -0800, Namhyung Kim escreveu:
Quoted 11 lines
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.
Quoted 5 lines
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
Quoted 72 lines
Signed-off-by Namhyung Kim <[email protected]>--- tools/perf/util/stat-display.c | 22 +++------------------- 1 file changed, 3 insertions(+), 19 deletions(-)tools/perf/util/stat-display.c +3
-19@@ -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 ; }Signature · 1 line
2.38.1.431.g37b22c650d-goog
Signature · 1 line
- Arnaldo
Namhyung Kim
maintainerRe: [PATCH 5/9] perf stat: Fix --metric-only --json output
Hi Arnaldo,
Quoted 13 lines · Arnaldo Carvalho de Melo
Em Mon, Nov 07, 2022 at 01:33:10PM -0800, Namhyung Kim escreveu:
Quoted 11 lines
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.
Quoted 7 lines
Quoted 5 lines
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
Quoted 77 lines
Quoted 72 lines
Signed-off-by Namhyung Kim <[email protected]>--- tools/perf/util/stat-display.c | 22 +++------------------- 1 file changed, 3 insertions(+), 19 deletions(-)tools/perf/util/stat-display.c +3
-20@@ -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 ; } --Quoted 1 line
2.38.1.431.g37b22c650d-goog
--
- Arnaldo
Arnaldo Carvalho de Melo
maintainerRe: [PATCH 5/9] perf stat: Fix --metric-only --json output
Em Tue, Nov 08, 2022 at 02:07:39PM -0800, Namhyung Kim escreveu:
Quoted 23 lines
Hi Arnaldo,
Quoted 19 lines · Arnaldo Carvalho de Melo
Em Mon, Nov 07, 2022 at 01:33:10PM -0800, Namhyung Kim escreveu:
Quoted 16 lines
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"}
Quoted 5 lines
Quoted 1 line
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
Quoted 2 lines
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
Quoted 80 lines
Thanks,
NamhyungQuoted 77 lines
Quoted 72 lines
Signed-off-by Namhyung Kim <[email protected]>--- tools/perf/util/stat-display.c | 22 +++------------------- 1 file changed, 3 insertions(+), 19 deletions(-)tools/perf/util/stat-display.c +3
-20@@ -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 ; } --Quoted 1 line
2.38.1.431.g37b22c650d-goog
--
- Arnaldo
Signature · 1 line
- Arnaldo
Ian Rogers
reviewerRe: [PATCH 5/9] perf stat: Fix --metric-only --json output
In reply to Namhyung KimQuoted 12 lines · 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.
Quoted 5 lines
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
Quoted 74 lines
Signed-off-by Namhyung Kim <[email protected]>--- tools/perf/util/stat-display.c | 22 +++------------------- 1 file changed, 3 insertions(+), 19 deletions(-)tools/perf/util/stat-display.c +3
-20@@ -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
Namhyung Kim
maintainerPATCH 6/9 perf stat: Do not indent headers for JSON
In reply to Namhyung KimCurrently --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.
--- tools/perf/util/stat-display.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
tools/perf/util/stat-display.c +1 -1
@@ -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) {
Signature · 1 line
2.38.1.431.g37b22c650d-goog
Re: [PATCH 6/9] perf stat: Do not indent headers for JSON
Quoted 11 lines · 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"} ...
Quoted 3 lines
Note that the other lines are broken JSON, but it will be handled later.
Signed-off-by Namhyung Kim <[email protected]>
Thanks,
Ian
Quoted 20 lines
--- tools/perf/util/stat-display.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)tools/perf/util/stat-display.c +1
-2@@ -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
Namhyung Kim
maintainerPATCH 7/9 perf stat: Add header for interval in JSON output
In reply to Namhyung KimIt 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"}
^CAfter:
# 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--- tools/perf/util/stat-display.c | 4 ++++ 1 file changed, 4 insertions(+)
tools/perf/util/stat-display.c +4 -0
@@ -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);
Signature · 1 line
2.38.1.431.g37b22c650d-goog
Re: [PATCH 7/9] perf stat: Add header for interval in JSON output
Quoted 9 lines · 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
Quoted 7 lines
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"} ^CSigned-off-by Namhyung Kim <[email protected]>
Thanks,
Ian
Quoted 22 lines
--- tools/perf/util/stat-display.c | 4 ++++ 1 file changed, 4 insertions(+)tools/perf/util/stat-display.c +4
-1@@ -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
Namhyung Kim
maintainerPATCH 8/9 perf stat: Fix condition in print_interval()
In reply to Namhyung KimThe 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.
--- tools/perf/util/stat-display.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
tools/perf/util/stat-display.c +2 -2
@@ -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" );
Signature · 1 line
2.38.1.431.g37b22c650d-goog
Re: [PATCH 8/9] perf stat: Fix condition in print_interval()
Quoted 7 lines · Namhyung Kim
The
num_print_intervalandconfig->interval_clearshould 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 <[email protected]>
Thanks,
Ian
Quoted 22 lines
--- tools/perf/util/stat-display.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)tools/perf/util/stat-display.c +2
-3@@ -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
Namhyung Kim
maintainerPATCH 9/9 perf stat: Consolidate condition to print metrics
In reply to Namhyung KimThe 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.
--- tools/perf/util/stat-display.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)
tools/perf/util/stat-display.c +1 -3
@@ -600,9 +600,7 @@ static void printout(struct perf_stat_config *config, struct aggr_cpu_id id, intpm (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 ; }
Signature · 1 line
2.38.1.431.g37b22c650d-goog
Re: [PATCH 9/9] perf stat: Consolidate condition to print metrics
Quoted 7 lines · 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 <[email protected]>
Thanks,
Ian
Quoted 22 lines
--- tools/perf/util/stat-display.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)tools/perf/util/stat-display.c +1
-4@@ -600,9 +600,7 @@ static void printout(struct perf_stat_config *config, struct aggr_cpu_id id, intpm (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