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

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

[Athira Rajeev]

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@linux.vnet.ibm.com>
Signed-off-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
---
 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

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

[Athira Rajeev]

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@linux.vnet.ibm.com>
Signed-off-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
---
 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

[PATCH 2/2] 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,1.89,msec,cpu-clock,1887692,100.00,1.013,CPUs utilized
	S0,1,2,,context-switches,1885842,100.00,1.060,K/sec
	S0,1,0,,cpu-migrations,1885374,100.00,0.000,/sec
	S0,1,2,,page-faults,1884880,100.00,1.060,K/sec
	S0,1,189544,,cycles,1263158,67.00,0.100,GHz
	S0,1,64602,,stalled-cycles-frontend,1876146,100.00,34.08,frontend cycles idle
	S0,1,128241,,stalled-cycles-backend,1875512,100.00,67.66,backend cycles idle
	S0,1,95578,,instructions,1874676,100.00,0.50,insn per cycle
===>	S0,1,,,,,,,1.34,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
separatorsi (for the 9 fields), where as last line
has 10 "," 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"
Example from line for instructions:
"1.89,msec,cpu-clock,1887692,100.00"

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,1.92,msec,cpu-clock,1917648,100.00,1.010,CPUs utilized
	S0,1,54,,context-switches,1916762,100.00,28.176,K/sec
	-------
	S0,1,528693,,instructions,1908854,100.00,0.36,insn per cycle
	S0,1,,,,,,1.81,stalled cycles per insn

Fixes: 92a61f6412d3 ("perf stat: Implement CSV metrics output")
Reported-by: Disha Goel <disgoel@linux.vnet.ibm.com>
Signed-off-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
---
 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 879874a4bc07..5ca151adf826 100644
--- a/tools/perf/util/stat-display.c
+++ b/tools/perf/util/stat-display.c
@@ -551,20 +551,9 @@ static void printout(struct perf_stat_config *config, struct aggr_cpu_id id, int
 	new_line_t nl;
 
 	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] = 0,
-		};
-
 		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

[PATCH 2/2] 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,1.89,msec,cpu-clock,1887692,100.00,1.013,CPUs utilized
	S0,1,2,,context-switches,1885842,100.00,1.060,K/sec
	S0,1,0,,cpu-migrations,1885374,100.00,0.000,/sec
	S0,1,2,,page-faults,1884880,100.00,1.060,K/sec
	S0,1,189544,,cycles,1263158,67.00,0.100,GHz
	S0,1,64602,,stalled-cycles-frontend,1876146,100.00,34.08,frontend cycles idle
	S0,1,128241,,stalled-cycles-backend,1875512,100.00,67.66,backend cycles idle
	S0,1,95578,,instructions,1874676,100.00,0.50,insn per cycle
===>	S0,1,,,,,,,1.34,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
separatorsi (for the 9 fields), where as last line
has 10 "," 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"
Example from line for instructions:
"1.89,msec,cpu-clock,1887692,100.00"

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,1.92,msec,cpu-clock,1917648,100.00,1.010,CPUs utilized
	S0,1,54,,context-switches,1916762,100.00,28.176,K/sec
	-------
	S0,1,528693,,instructions,1908854,100.00,0.36,insn per cycle
	S0,1,,,,,,1.81,stalled cycles per insn

Fixes: 92a61f6412d3 ("perf stat: Implement CSV metrics output")
Reported-by: Disha Goel <disgoel@linux.vnet.ibm.com>
Signed-off-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
---
 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 879874a4bc07..5ca151adf826 100644
--- a/tools/perf/util/stat-display.c
+++ b/tools/perf/util/stat-display.c
@@ -551,20 +551,9 @@ static void printout(struct perf_stat_config *config, struct aggr_cpu_id id, int
 	new_line_t nl;
 
 	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] = 0,
-		};
-
 		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: [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@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@linux.vnet.ibm.com>
> Signed-off-by: Athira Rajeev <atrajeev@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
> 

Re: [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@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@linux.vnet.ibm.com>
> Signed-off-by: Athira Rajeev <atrajeev@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
> 

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

[Athira Rajeev]

> On 18-Oct-2022, at 2:26 PM, Athira Rajeev <atrajeev@linux.vnet.ibm.com> wrote:
> 
> 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,1.89,msec,cpu-clock,1887692,100.00,1.013,CPUs utilized
> 	S0,1,2,,context-switches,1885842,100.00,1.060,K/sec
> 	S0,1,0,,cpu-migrations,1885374,100.00,0.000,/sec
> 	S0,1,2,,page-faults,1884880,100.00,1.060,K/sec
> 	S0,1,189544,,cycles,1263158,67.00,0.100,GHz
> 	S0,1,64602,,stalled-cycles-frontend,1876146,100.00,34.08,frontend cycles idle
> 	S0,1,128241,,stalled-cycles-backend,1875512,100.00,67.66,backend cycles idle
> 	S0,1,95578,,instructions,1874676,100.00,0.50,insn per cycle
> ===>	S0,1,,,,,,,1.34,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
> separatorsi (for the 9 fields), where as last line
> has 10 "," 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"
> Example from line for instructions:
> "1.89,msec,cpu-clock,1887692,100.00"
> 
> 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,1.92,msec,cpu-clock,1917648,100.00,1.010,CPUs utilized
> 	S0,1,54,,context-switches,1916762,100.00,28.176,K/sec
> 	-------
> 	S0,1,528693,,instructions,1908854,100.00,0.36,insn per cycle
> 	S0,1,,,,,,1.81,stalled cycles per insn
> 
> Fixes: 92a61f6412d3 ("perf stat: Implement CSV metrics output")
> Reported-by: Disha Goel <disgoel@linux.vnet.ibm.com>
> Signed-off-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com>

Hi All,

Looking for review comments for this change.

Thanks
Athira

> ---
> 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 879874a4bc07..5ca151adf826 100644
> --- a/tools/perf/util/stat-display.c
> +++ b/tools/perf/util/stat-display.c
> @@ -551,20 +551,9 @@ static void printout(struct perf_stat_config *config, struct aggr_cpu_id id, int
> 	new_line_t nl;
> 
> 	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] = 0,
> -		};
> -
> 		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: [PATCH 2/2] tools/perf: Fix printing field separator in CSV metrics output

[Athira Rajeev]

> On 18-Oct-2022, at 2:26 PM, Athira Rajeev <atrajeev@linux.vnet.ibm.com> wrote:
> 
> 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,1.89,msec,cpu-clock,1887692,100.00,1.013,CPUs utilized
> 	S0,1,2,,context-switches,1885842,100.00,1.060,K/sec
> 	S0,1,0,,cpu-migrations,1885374,100.00,0.000,/sec
> 	S0,1,2,,page-faults,1884880,100.00,1.060,K/sec
> 	S0,1,189544,,cycles,1263158,67.00,0.100,GHz
> 	S0,1,64602,,stalled-cycles-frontend,1876146,100.00,34.08,frontend cycles idle
> 	S0,1,128241,,stalled-cycles-backend,1875512,100.00,67.66,backend cycles idle
> 	S0,1,95578,,instructions,1874676,100.00,0.50,insn per cycle
> ===>	S0,1,,,,,,,1.34,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
> separatorsi (for the 9 fields), where as last line
> has 10 "," 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"
> Example from line for instructions:
> "1.89,msec,cpu-clock,1887692,100.00"
> 
> 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,1.92,msec,cpu-clock,1917648,100.00,1.010,CPUs utilized
> 	S0,1,54,,context-switches,1916762,100.00,28.176,K/sec
> 	-------
> 	S0,1,528693,,instructions,1908854,100.00,0.36,insn per cycle
> 	S0,1,,,,,,1.81,stalled cycles per insn
> 
> Fixes: 92a61f6412d3 ("perf stat: Implement CSV metrics output")
> Reported-by: Disha Goel <disgoel@linux.vnet.ibm.com>
> Signed-off-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com>

Hi All,

Looking for review comments for this change.

Thanks
Athira

> ---
> 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 879874a4bc07..5ca151adf826 100644
> --- a/tools/perf/util/stat-display.c
> +++ b/tools/perf/util/stat-display.c
> @@ -551,20 +551,9 @@ static void printout(struct perf_stat_config *config, struct aggr_cpu_id id, int
> 	new_line_t nl;
> 
> 	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] = 0,
> -		};
> -
> 		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: [PATCH 1/2] tools/perf: Fix printing os->prefix in CSV metrics output

[Ian Rogers]

On Wed, Nov 2, 2022 at 1:36 AM Athira Rajeev
<atrajeev@linux.vnet.ibm.com> wrote:
>
>
>
> > On 18-Oct-2022, at 2:26 PM, Athira Rajeev <atrajeev@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@linux.vnet.ibm.com>
> > Signed-off-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
>
> Hi All,
>
> Looking for review comments for this change.

Hi,

Thanks for addressing issues in this code. What is the status of the
CSV output test following these changes?

I think going forward we need to move away from CSV and columns, to
something with structure like json. We also need to refactor this
code, trying to add meaning to a newline character is a bad strategy
and creates some unnatural things. To some extent this overlaps with
Namhyung's aggregation cleanup. There are also weirdnesses in
jevents/pmu-events, like the same ScaleUnit applying to a metric and
an event - why are metrics even parts of events?

Given the current code is wac-a-mole, and this is another whack, if
the testing is okay I think we should move forward with this change.

Thanks,
Ian




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

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

[Ian Rogers]

On Wed, Nov 2, 2022 at 1:36 AM Athira Rajeev
<atrajeev@linux.vnet.ibm.com> wrote:
>
>
>
> > On 18-Oct-2022, at 2:26 PM, Athira Rajeev <atrajeev@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@linux.vnet.ibm.com>
> > Signed-off-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
>
> Hi All,
>
> Looking for review comments for this change.

Hi,

Thanks for addressing issues in this code. What is the status of the
CSV output test following these changes?

I think going forward we need to move away from CSV and columns, to
something with structure like json. We also need to refactor this
code, trying to add meaning to a newline character is a bad strategy
and creates some unnatural things. To some extent this overlaps with
Namhyung's aggregation cleanup. There are also weirdnesses in
jevents/pmu-events, like the same ScaleUnit applying to a metric and
an event - why are metrics even parts of events?

Given the current code is wac-a-mole, and this is another whack, if
the testing is okay I think we should move forward with this change.

Thanks,
Ian




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

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

[Athira Rajeev]

> On 03-Nov-2022, at 9:45 PM, Ian Rogers <irogers@google.com> wrote:
> 
> On Wed, Nov 2, 2022 at 1:36 AM Athira Rajeev
> <atrajeev@linux.vnet.ibm.com> wrote:
>> 
>> 
>> 
>>> On 18-Oct-2022, at 2:26 PM, Athira Rajeev <atrajeev@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@linux.vnet.ibm.com>
>>> Signed-off-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
>> 
>> Hi All,
>> 
>> Looking for review comments for this change.
> 
> Hi,
> 
> Thanks for addressing issues in this code. What is the status of the
> CSV output test following these changes?
> 
> I think going forward we need to move away from CSV and columns, to
> something with structure like json. We also need to refactor this
> code, trying to add meaning to a newline character is a bad strategy
> and creates some unnatural things. To some extent this overlaps with
> Namhyung's aggregation cleanup. There are also weirdnesses in
> jevents/pmu-events, like the same ScaleUnit applying to a metric and
> an event - why are metrics even parts of events?
> 
> Given the current code is wac-a-mole, and this is another whack, if
> the testing is okay I think we should move forward with this change.
> 
> Thanks,
> Ian

Hi Ian,

Thanks for checking the patch.
Yes, CSV output test passes with the change.

	perf stat CSV output linter                                     : Ok
	perf stat csv summary test                                      : Ok

Thanks
Athira

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

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

[Disha Goel]

[-- Attachment #1: Type: text/plain, Size: 6228 bytes --]


On 11/4/22 12:59 PM, Athira Rajeev wrote:
>
>> On 03-Nov-2022, at 9:45 PM, Ian Rogers<irogers@google.com>  wrote:
>>
>> On Wed, Nov 2, 2022 at 1:36 AM Athira Rajeev
>> <atrajeev@linux.vnet.ibm.com>  wrote:
>>>
>>>
>>>> On 18-Oct-2022, at 2:26 PM, Athira Rajeev<atrajeev@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@linux.vnet.ibm.com>
>>>> Signed-off-by: Athira Rajeev<atrajeev@linux.vnet.ibm.com>

perf stat csv test passed after applying the patch
Tested-by: Disha Goel<disgoel@linux.vnet.ibm.com>

>>> Hi All,
>>>
>>> Looking for review comments for this change.
>> Hi,
>>
>> Thanks for addressing issues in this code. What is the status of the
>> CSV output test following these changes?
>>
>> I think going forward we need to move away from CSV and columns, to
>> something with structure like json. We also need to refactor this
>> code, trying to add meaning to a newline character is a bad strategy
>> and creates some unnatural things. To some extent this overlaps with
>> Namhyung's aggregation cleanup. There are also weirdnesses in
>> jevents/pmu-events, like the same ScaleUnit applying to a metric and
>> an event - why are metrics even parts of events?
>>
>> Given the current code is wac-a-mole, and this is another whack, if
>> the testing is okay I think we should move forward with this change.
>>
>> Thanks,
>> Ian
> Hi Ian,
>
> Thanks for checking the patch.
> Yes, CSV output test passes with the change.
>
> 	perf stat CSV output linter                                     : Ok
> 	perf stat csv summary test                                      : Ok
>
> Thanks
> Athira
>
>>
>>
>>
>>> 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

[-- Attachment #2: Type: text/html, Size: 7726 bytes --]

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

[kajoljain]

On 11/4/22 13:07, Disha Goel wrote:
> On 11/4/22 12: 59 PM, Athira Rajeev wrote: On 03-Nov-2022, at 9: 45 PM,
> Ian Rogers <irogers@ google. com> wrote: On Wed, Nov 2, 2022 at 1: 36 AM
> Athira Rajeev ‍ ‍ ‍ ‍ ‍ ‍ ‍ ‍ ‍ ‍ ‍ ‍ ‍ ‍ ‍ ‍ ‍ ‍ ‍ ‍ ‍ ‍ ‍ ‍ ‍ ‍ ‍ ‍ ‍
> ‍ ‍ ‍ ‍ ‍ ‍ ‍ ‍ ‍ ‍ ‍ ‍ ‍ ‍ ‍ ‍ ‍ ‍ ‍ ‍ ‍ ‍ ‍ ‍ ‍ ‍ ‍ ‍ ‍ ‍ ‍ ‍ ‍ ‍ ‍ ‍
> ‍ ‍ ‍ ‍ ‍ ‍ ‍ ‍ ‍ ‍ ‍ ‍ ‍ ‍
> 
> 
> 
> On 11/4/22 12:59 PM, Athira Rajeev wrote:
>>> On 03-Nov-2022, at 9:45 PM, Ian Rogers <irogers@google.com> wrote:
>>>
>>> On Wed, Nov 2, 2022 at 1:36 AM Athira Rajeev
>>> <atrajeev@linux.vnet.ibm.com> wrote:
>>>>> On 18-Oct-2022, at 2:26 PM, Athira Rajeev <atrajeev@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@linux.vnet.ibm.com>
>>>>> Signed-off-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
> 
> perf stat csv test passed after applying the patch
> Tested-by: Disha Goel <disgoel@linux.vnet.ibm.com>


Hi,
  Patch looks fine to me.

Reviewed-By: Kajol Jain <kjain@linux.ibm.com>

Thanks,
Kajol Jain

> 
>>>> Hi All,
>>>>
>>>> Looking for review comments for this change.
>>> Hi,
>>>
>>> Thanks for addressing issues in this code. What is the status of the
>>> CSV output test following these changes?
>>>
>>> I think going forward we need to move away from CSV and columns, to
>>> something with structure like json. We also need to refactor this
>>> code, trying to add meaning to a newline character is a bad strategy
>>> and creates some unnatural things. To some extent this overlaps with
>>> Namhyung's aggregation cleanup. There are also weirdnesses in
>>> jevents/pmu-events, like the same ScaleUnit applying to a metric and
>>> an event - why are metrics even parts of events?
>>>
>>> Given the current code is wac-a-mole, and this is another whack, if
>>> the testing is okay I think we should move forward with this change.
>>>
>>> Thanks,
>>> Ian
>> Hi Ian,
>>
>> Thanks for checking the patch.
>> Yes, CSV output test passes with the change.
>>
>> 	perf stat CSV output linter                                     : Ok
>> 	perf stat csv summary test                                      : Ok
>>
>> Thanks
>> Athira
>>
>>>
>>>> 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

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

[Arnaldo Carvalho de Melo]

Em Wed, Nov 02, 2022 at 02:07:06PM +0530, Athira Rajeev escreveu:
> 
> 
> > On 18-Oct-2022, at 2:26 PM, Athira Rajeev <atrajeev@linux.vnet.ibm.com> wrote:
> > 
> > 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,1.89,msec,cpu-clock,1887692,100.00,1.013,CPUs utilized
> > 	S0,1,2,,context-switches,1885842,100.00,1.060,K/sec
> > 	S0,1,0,,cpu-migrations,1885374,100.00,0.000,/sec
> > 	S0,1,2,,page-faults,1884880,100.00,1.060,K/sec
> > 	S0,1,189544,,cycles,1263158,67.00,0.100,GHz
> > 	S0,1,64602,,stalled-cycles-frontend,1876146,100.00,34.08,frontend cycles idle
> > 	S0,1,128241,,stalled-cycles-backend,1875512,100.00,67.66,backend cycles idle
> > 	S0,1,95578,,instructions,1874676,100.00,0.50,insn per cycle
> > ===>	S0,1,,,,,,,1.34,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
> > separatorsi (for the 9 fields), where as last line
> > has 10 "," 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"
> > Example from line for instructions:
> > "1.89,msec,cpu-clock,1887692,100.00"
> > 
> > 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,1.92,msec,cpu-clock,1917648,100.00,1.010,CPUs utilized
> > 	S0,1,54,,context-switches,1916762,100.00,28.176,K/sec
> > 	-------
> > 	S0,1,528693,,instructions,1908854,100.00,0.36,insn per cycle
> > 	S0,1,,,,,,1.81,stalled cycles per insn
> > 
> > Fixes: 92a61f6412d3 ("perf stat: Implement CSV metrics output")
> > Reported-by: Disha Goel <disgoel@linux.vnet.ibm.com>
> > Signed-off-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
> 
> Hi All,
> 
> Looking for review comments for this change.

This clashed with a patch from Namhyung that I just applied:

http://lore.kernel.org/lkml/20221107213314.3239159-2-namhyung@kernel.org

Can you please check? I just applied the other patch in this series.

Thanks,

- Arnaldo
 
> Thanks
> Athira
> 
> > ---
> > 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 879874a4bc07..5ca151adf826 100644
> > --- a/tools/perf/util/stat-display.c
> > +++ b/tools/perf/util/stat-display.c
> > @@ -551,20 +551,9 @@ static void printout(struct perf_stat_config *config, struct aggr_cpu_id id, int
> > 	new_line_t nl;
> > 
> > 	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] = 0,
> > -		};
> > -
> > 		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

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

[Arnaldo Carvalho de Melo]

Em Wed, Nov 02, 2022 at 02:07:06PM +0530, Athira Rajeev escreveu:
> 
> 
> > On 18-Oct-2022, at 2:26 PM, Athira Rajeev <atrajeev@linux.vnet.ibm.com> wrote:
> > 
> > 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,1.89,msec,cpu-clock,1887692,100.00,1.013,CPUs utilized
> > 	S0,1,2,,context-switches,1885842,100.00,1.060,K/sec
> > 	S0,1,0,,cpu-migrations,1885374,100.00,0.000,/sec
> > 	S0,1,2,,page-faults,1884880,100.00,1.060,K/sec
> > 	S0,1,189544,,cycles,1263158,67.00,0.100,GHz
> > 	S0,1,64602,,stalled-cycles-frontend,1876146,100.00,34.08,frontend cycles idle
> > 	S0,1,128241,,stalled-cycles-backend,1875512,100.00,67.66,backend cycles idle
> > 	S0,1,95578,,instructions,1874676,100.00,0.50,insn per cycle
> > ===>	S0,1,,,,,,,1.34,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
> > separatorsi (for the 9 fields), where as last line
> > has 10 "," 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"
> > Example from line for instructions:
> > "1.89,msec,cpu-clock,1887692,100.00"
> > 
> > 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,1.92,msec,cpu-clock,1917648,100.00,1.010,CPUs utilized
> > 	S0,1,54,,context-switches,1916762,100.00,28.176,K/sec
> > 	-------
> > 	S0,1,528693,,instructions,1908854,100.00,0.36,insn per cycle
> > 	S0,1,,,,,,1.81,stalled cycles per insn
> > 
> > Fixes: 92a61f6412d3 ("perf stat: Implement CSV metrics output")
> > Reported-by: Disha Goel <disgoel@linux.vnet.ibm.com>
> > Signed-off-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
> 
> Hi All,
> 
> Looking for review comments for this change.

This clashed with a patch from Namhyung that I just applied:

http://lore.kernel.org/lkml/20221107213314.3239159-2-namhyung@kernel.org

Can you please check? I just applied the other patch in this series.

Thanks,

- Arnaldo
 
> Thanks
> Athira
> 
> > ---
> > 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 879874a4bc07..5ca151adf826 100644
> > --- a/tools/perf/util/stat-display.c
> > +++ b/tools/perf/util/stat-display.c
> > @@ -551,20 +551,9 @@ static void printout(struct perf_stat_config *config, struct aggr_cpu_id id, int
> > 	new_line_t nl;
> > 
> > 	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] = 0,
> > -		};
> > -
> > 		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

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

[Athira Rajeev]

> On 09-Nov-2022, at 2:27 AM, Arnaldo Carvalho de Melo <acme@kernel.org> wrote:
> 
> Em Wed, Nov 02, 2022 at 02:07:06PM +0530, Athira Rajeev escreveu:
>> 
>> 
>>> On 18-Oct-2022, at 2:26 PM, Athira Rajeev <atrajeev@linux.vnet.ibm.com> wrote:
>>> 
>>> 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,1.89,msec,cpu-clock,1887692,100.00,1.013,CPUs utilized
>>> 	S0,1,2,,context-switches,1885842,100.00,1.060,K/sec
>>> 	S0,1,0,,cpu-migrations,1885374,100.00,0.000,/sec
>>> 	S0,1,2,,page-faults,1884880,100.00,1.060,K/sec
>>> 	S0,1,189544,,cycles,1263158,67.00,0.100,GHz
>>> 	S0,1,64602,,stalled-cycles-frontend,1876146,100.00,34.08,frontend cycles idle
>>> 	S0,1,128241,,stalled-cycles-backend,1875512,100.00,67.66,backend cycles idle
>>> 	S0,1,95578,,instructions,1874676,100.00,0.50,insn per cycle
>>> ===>	S0,1,,,,,,,1.34,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
>>> separatorsi (for the 9 fields), where as last line
>>> has 10 "," 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"
>>> Example from line for instructions:
>>> "1.89,msec,cpu-clock,1887692,100.00"
>>> 
>>> 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,1.92,msec,cpu-clock,1917648,100.00,1.010,CPUs utilized
>>> 	S0,1,54,,context-switches,1916762,100.00,28.176,K/sec
>>> 	-------
>>> 	S0,1,528693,,instructions,1908854,100.00,0.36,insn per cycle
>>> 	S0,1,,,,,,1.81,stalled cycles per insn
>>> 
>>> Fixes: 92a61f6412d3 ("perf stat: Implement CSV metrics output")
>>> Reported-by: Disha Goel <disgoel@linux.vnet.ibm.com>
>>> Signed-off-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
>> 
>> Hi All,
>> 
>> Looking for review comments for this change.
> 
> This clashed with a patch from Namhyung that I just applied:
> 
> http://lore.kernel.org/lkml/20221107213314.3239159-2-namhyung@kernel.org
> 
> Can you please check? I just applied the other patch in this series.
> 
> Thanks,
> 
> - Arnaldo

Hi Arnaldo,

Thanks for checking the patch series.
Please find the updated patch below which is created on top of perf/urgent.

From dde8f830ad318c9111c3fea5415fd8170b4c51bd Mon Sep 17 00:00:00 2001
From: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
Date: Tue, 18 Oct 2022 14:26:05 +0530
Subject: [PATCH] tools/perf: Fix printing field separator in CSV metrics
 output

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,1.89,msec,cpu-clock,1887692,100.00,1.013,CPUs utilized
	S0,1,2,,context-switches,1885842,100.00,1.060,K/sec
	S0,1,0,,cpu-migrations,1885374,100.00,0.000,/sec
	S0,1,2,,page-faults,1884880,100.00,1.060,K/sec
	S0,1,189544,,cycles,1263158,67.00,0.100,GHz
	S0,1,64602,,stalled-cycles-frontend,1876146,100.00,34.08,frontend cycles idle
	S0,1,128241,,stalled-cycles-backend,1875512,100.00,67.66,backend cycles idle
	S0,1,95578,,instructions,1874676,100.00,0.50,insn per cycle
===>	S0,1,,,,,,,1.34,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
separatorsi (for the 9 fields), where as last line
has 10 "," 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"
Example from line for instructions:
"1.89,msec,cpu-clock,1887692,100.00"

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,1.92,msec,cpu-clock,1917648,100.00,1.010,CPUs utilized
	S0,1,54,,context-switches,1916762,100.00,28.176,K/sec
	-------
	S0,1,528693,,instructions,1908854,100.00,0.36,insn per cycle
	S0,1,,,,,,1.81,stalled cycles per insn

Fixes: 92a61f6412d3 ("perf stat: Implement CSV metrics output")
Reported-by: Disha Goel <disgoel@linux.vnet.ibm.com>
Signed-off-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
---
 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 ba66bb7fc1ca..5ce14bf18055 100644
--- a/tools/perf/util/stat-display.c
+++ b/tools/perf/util/stat-display.c
@@ -551,20 +551,9 @@ static void printout(struct perf_stat_config *config, struct aggr_cpu_id id, int
 	new_line_t nl;
 
 	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


Thanks
Athira

> 
>> Thanks
>> Athira
>> 
>>> ---
>>> 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 879874a4bc07..5ca151adf826 100644
>>> --- a/tools/perf/util/stat-display.c
>>> +++ b/tools/perf/util/stat-display.c
>>> @@ -551,20 +551,9 @@ static void printout(struct perf_stat_config *config, struct aggr_cpu_id id, int
>>> 	new_line_t nl;
>>> 
>>> 	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] = 0,
>>> -		};
>>> -
>>> 		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

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

[Athira Rajeev]

> On 09-Nov-2022, at 3:53 PM, Athira Rajeev <atrajeev@linux.vnet.ibm.com> wrote:
> 
> 
> 
>> On 09-Nov-2022, at 2:27 AM, Arnaldo Carvalho de Melo <acme@kernel.org> wrote:
>> 
>> Em Wed, Nov 02, 2022 at 02:07:06PM +0530, Athira Rajeev escreveu:
>>> 
>>> 
>>>> On 18-Oct-2022, at 2:26 PM, Athira Rajeev <atrajeev@linux.vnet.ibm.com> wrote:
>>>> 
>>>> 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,1.89,msec,cpu-clock,1887692,100.00,1.013,CPUs utilized
>>>> 	S0,1,2,,context-switches,1885842,100.00,1.060,K/sec
>>>> 	S0,1,0,,cpu-migrations,1885374,100.00,0.000,/sec
>>>> 	S0,1,2,,page-faults,1884880,100.00,1.060,K/sec
>>>> 	S0,1,189544,,cycles,1263158,67.00,0.100,GHz
>>>> 	S0,1,64602,,stalled-cycles-frontend,1876146,100.00,34.08,frontend cycles idle
>>>> 	S0,1,128241,,stalled-cycles-backend,1875512,100.00,67.66,backend cycles idle
>>>> 	S0,1,95578,,instructions,1874676,100.00,0.50,insn per cycle
>>>> ===>	S0,1,,,,,,,1.34,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
>>>> separatorsi (for the 9 fields), where as last line
>>>> has 10 "," 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"
>>>> Example from line for instructions:
>>>> "1.89,msec,cpu-clock,1887692,100.00"
>>>> 
>>>> 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,1.92,msec,cpu-clock,1917648,100.00,1.010,CPUs utilized
>>>> 	S0,1,54,,context-switches,1916762,100.00,28.176,K/sec
>>>> 	-------
>>>> 	S0,1,528693,,instructions,1908854,100.00,0.36,insn per cycle
>>>> 	S0,1,,,,,,1.81,stalled cycles per insn
>>>> 
>>>> Fixes: 92a61f6412d3 ("perf stat: Implement CSV metrics output")
>>>> Reported-by: Disha Goel <disgoel@linux.vnet.ibm.com>
>>>> Signed-off-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
>>> 
>>> Hi All,
>>> 
>>> Looking for review comments for this change.
>> 
>> This clashed with a patch from Namhyung that I just applied:
>> 
>> http://lore.kernel.org/lkml/20221107213314.3239159-2-namhyung@kernel.org
>> 
>> Can you please check? I just applied the other patch in this series.
>> 
>> Thanks,
>> 
>> - Arnaldo
> 
> Hi Arnaldo,
> 
> Thanks for checking the patch series.
> Please find the updated patch below which is created on top of perf/urgent.

Hi Arnaldo,

I posted this as a separate patch with version V2 here: 

https://lore.kernel.org/linux-perf-users/20221114085523.86570-1-atrajeev@linux.vnet.ibm.com/T/#m1ba8c773b53f198923101684c39b13da686c211d

Thanks
Athira

> 
> From dde8f830ad318c9111c3fea5415fd8170b4c51bd Mon Sep 17 00:00:00 2001
> From: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
> Date: Tue, 18 Oct 2022 14:26:05 +0530
> Subject: [PATCH] tools/perf: Fix printing field separator in CSV metrics
> output
> 
> 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,1.89,msec,cpu-clock,1887692,100.00,1.013,CPUs utilized
> 	S0,1,2,,context-switches,1885842,100.00,1.060,K/sec
> 	S0,1,0,,cpu-migrations,1885374,100.00,0.000,/sec
> 	S0,1,2,,page-faults,1884880,100.00,1.060,K/sec
> 	S0,1,189544,,cycles,1263158,67.00,0.100,GHz
> 	S0,1,64602,,stalled-cycles-frontend,1876146,100.00,34.08,frontend cycles idle
> 	S0,1,128241,,stalled-cycles-backend,1875512,100.00,67.66,backend cycles idle
> 	S0,1,95578,,instructions,1874676,100.00,0.50,insn per cycle
> ===>	S0,1,,,,,,,1.34,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
> separatorsi (for the 9 fields), where as last line
> has 10 "," 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"
> Example from line for instructions:
> "1.89,msec,cpu-clock,1887692,100.00"
> 
> 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,1.92,msec,cpu-clock,1917648,100.00,1.010,CPUs utilized
> 	S0,1,54,,context-switches,1916762,100.00,28.176,K/sec
> 	-------
> 	S0,1,528693,,instructions,1908854,100.00,0.36,insn per cycle
> 	S0,1,,,,,,1.81,stalled cycles per insn
> 
> Fixes: 92a61f6412d3 ("perf stat: Implement CSV metrics output")
> Reported-by: Disha Goel <disgoel@linux.vnet.ibm.com>
> Signed-off-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
> ---
> 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 ba66bb7fc1ca..5ce14bf18055 100644
> --- a/tools/perf/util/stat-display.c
> +++ b/tools/perf/util/stat-display.c
> @@ -551,20 +551,9 @@ static void printout(struct perf_stat_config *config, struct aggr_cpu_id id, int
> 	new_line_t nl;
> 
> 	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
> 
> 
> Thanks
> Athira
> 
>> 
>>> Thanks
>>> Athira
>>> 
>>>> ---
>>>> 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 879874a4bc07..5ca151adf826 100644
>>>> --- a/tools/perf/util/stat-display.c
>>>> +++ b/tools/perf/util/stat-display.c
>>>> @@ -551,20 +551,9 @@ static void printout(struct perf_stat_config *config, struct aggr_cpu_id id, int
>>>> 	new_line_t nl;
>>>> 
>>>> 	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] = 0,
>>>> -		};
>>>> -
>>>> 		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