[V3] tools/perf: Fix printing field separator in CSV metrics output

[V3] tools/perf: Fix printing field separator in CSV metrics output

[Athira Rajeev]

In perf stat with CSV output option, number of fields
in metrics output is not matching with number of fields
in other event output lines.

Sample output below after applying patch to fix
printing os->prefix.

	# ./perf stat -x, --per-socket -a -C 1 ls
	S0,1,82.11,msec,cpu-clock,82111626,100.00,1.000,CPUs utilized
	S0,1,2,,context-switches,82109314,100.00,24.358,/sec
	------
====>	S0,1,,,,,,,1.71,stalled cycles per insn

The above command line uses field separator as ","
via "-x," option and per-socket option displays
socket value as first field. But here the last line
for "stalled cycles per insn" has more separators.
Each csv output line is expected to have 8 field
separators (for the 9 fields), where as last line
has 9 "," in the result. Patch fixes this issue.

The counter stats are displayed by function
"perf_stat__print_shadow_stats" in code
"util/stat-shadow.c". While printing the stats info
for "stalled cycles per insn", function "new_line_csv"
is used as new_line callback.

The fields printed in each line contains:
"Socket_id,aggr nr,Avg,unit,event_name,run,enable_percent,ratio,unit"

The metric output prints Socket_id, aggr nr, ratio
and unit. It has to skip through remaining five fields
ie, Avg,unit,event_name,run,enable_percent. The csv
line callback uses "os->nfields" to know the number of
fields to skip to match with other lines.
Currently it is set as:
	os.nfields = 3 + aggr_fields[config->aggr_mode] + (counter->cgrp ? 1 : 0);

But in case of aggregation modes, csv_sep already
gets printed along with each field (Function "aggr_printout"
in util/stat-display.c). So aggr_fields can be
removed from nfields. And fixed number of fields to
skip has to be "4". This is to skip fields for:
"avg, unit, event name, run, enable_percent"

This needs 4 csv separators. Patch removes aggr_fields
and uses 4 as fixed number of os->nfields to skip.

After the patch:

	# ./perf stat -x, --per-socket -a -C 1 ls
	S0,1,79.08,msec,cpu-clock,79085956,100.00,1.000,CPUs utilized
	S0,1,7,,context-switches,79084176,100.00,88.514,/sec
	------
====>	S0,1,,,,,,0.81,stalled cycles per insn

Fixes: 92a61f6412d3 ("perf stat: Implement CSV metrics output")
Reported-by: Disha Goel <disgoel@linux.vnet.ibm.com>
Reviewed-by: Kajol Jain <kjain@linux.ibm.com>
Tested-by: Disha Goel<disgoel@linux.vnet.ibm.com>
Signed-off-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
---
Changelog:
v2 -> v3:
- Rebased on top of tmp.perf/core

v1 -> v2:
- Rebased on top of latest source.
- Added Reviewed-by and Tested-by from Kajol and
  Disha.

 tools/perf/util/stat-display.c | 13 +------------
 1 file changed, 1 insertion(+), 12 deletions(-)

diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c
index f1ee4b052198..4e9696f4096f 100644
--- a/tools/perf/util/stat-display.c
+++ b/tools/perf/util/stat-display.c
@@ -686,20 +686,9 @@ static void printout(struct perf_stat_config *config, struct outstate *os,
 	struct evsel *counter = os->evsel;
 
 	if (config->csv_output) {
-		static const int aggr_fields[AGGR_MAX] = {
-			[AGGR_NONE] = 1,
-			[AGGR_GLOBAL] = 0,
-			[AGGR_SOCKET] = 2,
-			[AGGR_DIE] = 2,
-			[AGGR_CORE] = 2,
-			[AGGR_THREAD] = 1,
-			[AGGR_UNSET] = 0,
-			[AGGR_NODE] = 1,
-		};
-
 		pm = config->metric_only ? print_metric_only_csv : print_metric_csv;
 		nl = config->metric_only ? new_line_metric : new_line_csv;
-		os->nfields = 3 + aggr_fields[config->aggr_mode] + (counter->cgrp ? 1 : 0);
+		os->nfields = 4 + (counter->cgrp ? 1 : 0);
 	} else if (config->json_output) {
 		pm = config->metric_only ? print_metric_only_json : print_metric_json;
 		nl = config->metric_only ? new_line_metric : new_line_json;
-- 
2.31.1

Re: [V3] tools/perf: Fix printing field separator in CSV metrics output

[Arnaldo Carvalho de Melo]

Em Mon, Dec 05, 2022 at 09:58:52AM +0530, Athira Rajeev escreveu:
> In perf stat with CSV output option, number of fields
> in metrics output is not matching with number of fields
> in other event output lines.
> 
> Sample output below after applying patch to fix
> printing os->prefix.
> 
> 	# ./perf stat -x, --per-socket -a -C 1 ls
> 	S0,1,82.11,msec,cpu-clock,82111626,100.00,1.000,CPUs utilized
> 	S0,1,2,,context-switches,82109314,100.00,24.358,/sec
> 	------
> ====>	S0,1,,,,,,,1.71,stalled cycles per insn

Thanks, tested and applied.

- Arnaldo
 
> The above command line uses field separator as ","
> via "-x," option and per-socket option displays
> socket value as first field. But here the last line
> for "stalled cycles per insn" has more separators.
> Each csv output line is expected to have 8 field
> separators (for the 9 fields), where as last line
> has 9 "," in the result. Patch fixes this issue.
> 
> The counter stats are displayed by function
> "perf_stat__print_shadow_stats" in code
> "util/stat-shadow.c". While printing the stats info
> for "stalled cycles per insn", function "new_line_csv"
> is used as new_line callback.
> 
> The fields printed in each line contains:
> "Socket_id,aggr nr,Avg,unit,event_name,run,enable_percent,ratio,unit"
> 
> The metric output prints Socket_id, aggr nr, ratio
> and unit. It has to skip through remaining five fields
> ie, Avg,unit,event_name,run,enable_percent. The csv
> line callback uses "os->nfields" to know the number of
> fields to skip to match with other lines.
> Currently it is set as:
> 	os.nfields = 3 + aggr_fields[config->aggr_mode] + (counter->cgrp ? 1 : 0);
> 
> But in case of aggregation modes, csv_sep already
> gets printed along with each field (Function "aggr_printout"
> in util/stat-display.c). So aggr_fields can be
> removed from nfields. And fixed number of fields to
> skip has to be "4". This is to skip fields for:
> "avg, unit, event name, run, enable_percent"
> 
> This needs 4 csv separators. Patch removes aggr_fields
> and uses 4 as fixed number of os->nfields to skip.
> 
> After the patch:
> 
> 	# ./perf stat -x, --per-socket -a -C 1 ls
> 	S0,1,79.08,msec,cpu-clock,79085956,100.00,1.000,CPUs utilized
> 	S0,1,7,,context-switches,79084176,100.00,88.514,/sec
> 	------
> ====>	S0,1,,,,,,0.81,stalled cycles per insn
> 
> Fixes: 92a61f6412d3 ("perf stat: Implement CSV metrics output")
> Reported-by: Disha Goel <disgoel@linux.vnet.ibm.com>
> Reviewed-by: Kajol Jain <kjain@linux.ibm.com>
> Tested-by: Disha Goel<disgoel@linux.vnet.ibm.com>
> Signed-off-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
> ---
> Changelog:
> v2 -> v3:
> - Rebased on top of tmp.perf/core
> 
> v1 -> v2:
> - Rebased on top of latest source.
> - Added Reviewed-by and Tested-by from Kajol and
>   Disha.
> 
>  tools/perf/util/stat-display.c | 13 +------------
>  1 file changed, 1 insertion(+), 12 deletions(-)
> 
> diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c
> index f1ee4b052198..4e9696f4096f 100644
> --- a/tools/perf/util/stat-display.c
> +++ b/tools/perf/util/stat-display.c
> @@ -686,20 +686,9 @@ static void printout(struct perf_stat_config *config, struct outstate *os,
>  	struct evsel *counter = os->evsel;
>  
>  	if (config->csv_output) {
> -		static const int aggr_fields[AGGR_MAX] = {
> -			[AGGR_NONE] = 1,
> -			[AGGR_GLOBAL] = 0,
> -			[AGGR_SOCKET] = 2,
> -			[AGGR_DIE] = 2,
> -			[AGGR_CORE] = 2,
> -			[AGGR_THREAD] = 1,
> -			[AGGR_UNSET] = 0,
> -			[AGGR_NODE] = 1,
> -		};
> -
>  		pm = config->metric_only ? print_metric_only_csv : print_metric_csv;
>  		nl = config->metric_only ? new_line_metric : new_line_csv;
> -		os->nfields = 3 + aggr_fields[config->aggr_mode] + (counter->cgrp ? 1 : 0);
> +		os->nfields = 4 + (counter->cgrp ? 1 : 0);
>  	} else if (config->json_output) {
>  		pm = config->metric_only ? print_metric_only_json : print_metric_json;
>  		nl = config->metric_only ? new_line_metric : new_line_json;
> -- 
> 2.31.1

-- 

- Arnaldo