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

[Arnaldo Carvalho de Melo <acme@kernel.org>]

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