[PATCH 1/2] tools/perf: Fix printing os->prefix in CSV metrics output

[Athira Rajeev]

> On 18-Oct-2022, at 2:26 PM, Athira Rajeev <atrajeev at linux.vnet.ibm.com> wrote:
> 
> Perf stat with CSV output option prints an extra empty
> string as first field in metrics output line.
> Sample output below:
> 
> 	# ./perf stat -x, --per-socket -a -C 1 ls
> 	S0,1,1.78,msec,cpu-clock,1785146,100.00,0.973,CPUs utilized
> 	S0,1,26,,context-switches,1781750,100.00,0.015,M/sec
> 	S0,1,1,,cpu-migrations,1780526,100.00,0.561,K/sec
> 	S0,1,1,,page-faults,1779060,100.00,0.561,K/sec
> 	S0,1,875807,,cycles,1769826,100.00,0.491,GHz
> 	S0,1,85281,,stalled-cycles-frontend,1767512,100.00,9.74,frontend cycles idle
> 	S0,1,576839,,stalled-cycles-backend,1766260,100.00,65.86,backend cycles idle
> 	S0,1,288430,,instructions,1762246,100.00,0.33,insn per cycle
> ====>	,S0,1,,,,,,,2.00,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 "," in the
> beginning.
> 
> Sample output using interval mode:
> 	# ./perf stat -I 1000 -x, --per-socket -a -C 1 ls
> 	0.001813453,S0,1,1.87,msec,cpu-clock,1872052,100.00,0.002,CPUs utilized
> 	0.001813453,S0,1,2,,context-switches,1868028,100.00,1.070,K/sec
> 	------
> 	0.001813453,S0,1,85379,,instructions,1856754,100.00,0.32,insn per cycle
> ====>	0.001813453,,S0,1,,,,,,,1.34,stalled cycles per insn
> 
> Above result also has an extra csv separator after
> the timestamp. Patch addresses extra field separator
> in the beginning of the metric output line.
> 
> 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 new_line_csv function has check for "os->prefix"
> and if prefix is not null, it will be printed along
> with cvs separator.
> Snippet from "new_line_csv":
> 	if (os->prefix)
>               fprintf(os->fh, "%s%s", os->prefix, config->csv_sep);
> 
> Here os->prefix gets printed followed by ","
> which is the cvs separator. The os->prefix is
> used in interval mode option ( -I ), to print
> time stamp on every new line. But prefix is
> already set to contain csv separator when used
> in interval mode for csv option.
> 
> Reference: Function "static void print_interval"
> Snippet:
> 	sprintf(prefix, "%6lu.%09lu%s", ts->tv_sec, ts->tv_nsec, config->csv_sep);
> 
> Also if prefix is not assigned (if not used with
> -I option), it gets set to empty string.
> Reference: function printout() in util/stat-display.c
> Snippet:
> 	.prefix = prefix ? prefix : "",
> 
> Since prefix already set to contain cvs_sep in interval
> option, patch removes printing config->csv_sep in
> new_line_csv function to avoid printing extra field.
> 
> After the patch:
> 
> 	# ./perf stat -x, --per-socket -a -C 1 ls
> 	S0,1,2.04,msec,cpu-clock,2045202,100.00,1.013,CPUs utilized
> 	S0,1,2,,context-switches,2041444,100.00,979.289,/sec
> 	S0,1,0,,cpu-migrations,2040820,100.00,0.000,/sec
> 	S0,1,2,,page-faults,2040288,100.00,979.289,/sec
> 	S0,1,254589,,cycles,2036066,100.00,0.125,GHz
> 	S0,1,82481,,stalled-cycles-frontend,2032420,100.00,32.40,frontend cycles idle
> 	S0,1,113170,,stalled-cycles-backend,2031722,100.00,44.45,backend cycles idle
> 	S0,1,88766,,instructions,2030942,100.00,0.35,insn per cycle
> 	S0,1,,,,,,,1.27,stalled cycles per insn
> 
> Fixes: 92a61f6412d3 ("perf stat: Implement CSV metrics output")
> Reported-by: Disha Goel <disgoel at linux.vnet.ibm.com>
> Signed-off-by: Athira Rajeev <atrajeev at linux.vnet.ibm.com>

Hi All,

Looking for review comments for this change.

Thanks
Athira

> ---
> 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 5c47ee9963a7..879874a4bc07 100644
> --- a/tools/perf/util/stat-display.c
> +++ b/tools/perf/util/stat-display.c
> @@ -273,7 +273,7 @@ static void new_line_csv(struct perf_stat_config *config, void *ctx)
> 
> 	fputc('\n', os->fh);
> 	if (os->prefix)
> -		fprintf(os->fh, "%s%s", os->prefix, config->csv_sep);
> +		fprintf(os->fh, "%s", os->prefix);
> 	aggr_printout(config, os->evsel, os->id, os->nr);
> 	for (i = 0; i < os->nfields; i++)
> 		fputs(config->csv_sep, os->fh);
> -- 
> 2.31.1
>