[PATCH] perf stat: fix cvs output format

[PATCH] perf stat: fix cvs output format

[Cong Wang]

From: Ilya Pronin <ipronin@twitter.com>

When printing stats in CSV mode, perf stat appends extra CSV
separators when counter is not supported:

<not supported>,,L1-dcache-store-misses,mesos/bd442f34-2b4a-47df-b966-9b281f9f56fc,0,100.00,,,,

which causes a failure of parsing fields. The numbers of separators
is fixed for each line, no matter supported or not supported.

Fixes: 92a61f6412d3 ("perf stat: Implement CSV metrics output")
Cc: Andi Kleen <ak@linux.intel.com>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Signed-off-by: Ilya Pronin <ipronin@twitter.com>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
 tools/perf/builtin-stat.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index 98bf9d32f222..54a4c152edb3 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -917,7 +917,7 @@ static void print_metric_csv(void *ctx,
 	char buf[64], *vals, *ends;
 
 	if (unit == NULL || fmt == NULL) {
-		fprintf(out, "%s%s%s%s", csv_sep, csv_sep, csv_sep, csv_sep);
+		fprintf(out, "%s%s", csv_sep, csv_sep);
 		return;
 	}
 	snprintf(buf, sizeof(buf), fmt, val);
-- 
2.13.0

Re: [PATCH] perf stat: fix cvs output format

[Jiri Olsa]

On Mon, Mar 05, 2018 at 10:43:53PM -0800, Cong Wang wrote:
> From: Ilya Pronin <ipronin@twitter.com>
> 
> When printing stats in CSV mode, perf stat appends extra CSV
> separators when counter is not supported:
> 
> <not supported>,,L1-dcache-store-misses,mesos/bd442f34-2b4a-47df-b966-9b281f9f56fc,0,100.00,,,,
> 
> which causes a failure of parsing fields. The numbers of separators
> is fixed for each line, no matter supported or not supported.
> 
> Fixes: 92a61f6412d3 ("perf stat: Implement CSV metrics output")
> Cc: Andi Kleen <ak@linux.intel.com>
> Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
> Cc: Jiri Olsa <jolsa@redhat.com>
> Signed-off-by: Ilya Pronin <ipronin@twitter.com>
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
> ---
>  tools/perf/builtin-stat.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
> index 98bf9d32f222..54a4c152edb3 100644
> --- a/tools/perf/builtin-stat.c
> +++ b/tools/perf/builtin-stat.c
> @@ -917,7 +917,7 @@ static void print_metric_csv(void *ctx,
>  	char buf[64], *vals, *ends;
>  
>  	if (unit == NULL || fmt == NULL) {
> -		fprintf(out, "%s%s%s%s", csv_sep, csv_sep, csv_sep, csv_sep);
> +		fprintf(out, "%s%s", csv_sep, csv_sep);
>  		return;
>  	}

right, the non else legs prints just 2 values:
  fprintf(out, "%s%s%s%s", csv_sep, vals, csv_sep, unit);

Acked-by: Jiri Olsa <jolsa@kernel.org>

thanks,
jirka

Re: [PATCH] perf stat: fix cvs output format

[Arnaldo Carvalho de Melo]

Em Tue, Mar 06, 2018 at 08:58:46AM +0100, Jiri Olsa escreveu:
> On Mon, Mar 05, 2018 at 10:43:53PM -0800, Cong Wang wrote:
> > From: Ilya Pronin <ipronin@twitter.com>
> > 
> > When printing stats in CSV mode, perf stat appends extra CSV
> > separators when counter is not supported:
> > 
> > <not supported>,,L1-dcache-store-misses,mesos/bd442f34-2b4a-47df-b966-9b281f9f56fc,0,100.00,,,,
> > 
> > which causes a failure of parsing fields. The numbers of separators
> > is fixed for each line, no matter supported or not supported.
> > 
> > Fixes: 92a61f6412d3 ("perf stat: Implement CSV metrics output")
> > Cc: Andi Kleen <ak@linux.intel.com>
> > Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
> > Cc: Jiri Olsa <jolsa@redhat.com>
> > Signed-off-by: Ilya Pronin <ipronin@twitter.com>
> > Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
> > ---
> >  tools/perf/builtin-stat.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
> > index 98bf9d32f222..54a4c152edb3 100644
> > --- a/tools/perf/builtin-stat.c
> > +++ b/tools/perf/builtin-stat.c
> > @@ -917,7 +917,7 @@ static void print_metric_csv(void *ctx,
> >  	char buf[64], *vals, *ends;
> >  
> >  	if (unit == NULL || fmt == NULL) {
> > -		fprintf(out, "%s%s%s%s", csv_sep, csv_sep, csv_sep, csv_sep);
> > +		fprintf(out, "%s%s", csv_sep, csv_sep);
> >  		return;
> >  	}
> 
> right, the non else legs prints just 2 values:
>   fprintf(out, "%s%s%s%s", csv_sep, vals, csv_sep, unit);
> 
> Acked-by: Jiri Olsa <jolsa@kernel.org>
> 

Thanks, applied to perf/urgent.

- Arnaldo

Re: [PATCH] perf stat: fix cvs output format

[Andi Kleen]

On Mon, Mar 05, 2018 at 10:43:53PM -0800, Cong Wang wrote:
> From: Ilya Pronin <ipronin@twitter.com>
> 
> When printing stats in CSV mode, perf stat appends extra CSV
> separators when counter is not supported:
> 
> <not supported>,,L1-dcache-store-misses,mesos/bd442f34-2b4a-47df-b966-9b281f9f56fc,0,100.00,,,,
> 
> which causes a failure of parsing fields. The numbers of separators

Causes failure in what?

> is fixed for each line, no matter supported or not supported.

I don't think they're extra fields, there are cases where they can be filled out
for variance, metricvalue, unit. And other code in perf too uses empty
fields when something is not available.

        - optional usec time stamp in fractions of second (with -I xxx)
        - optional CPU, core, or socket identifier
        - optional number of logical CPUs aggregated
        - counter value
        - unit of the counter value or empty
        - event name
        - run time of counter
        - percentage of measurement time the counter was running
        - optional variance if multiple values are collected with -r  
        - optional metric value
        - optional unit of metric


-Andi

Re: [PATCH] perf stat: fix cvs output format

[Arnaldo Carvalho de Melo]

Em Tue, Mar 06, 2018 at 09:00:11AM -0800, Andi Kleen escreveu:
> On Mon, Mar 05, 2018 at 10:43:53PM -0800, Cong Wang wrote:
> > From: Ilya Pronin <ipronin@twitter.com>
> > 
> > When printing stats in CSV mode, perf stat appends extra CSV
> > separators when counter is not supported:
> > 
> > <not supported>,,L1-dcache-store-misses,mesos/bd442f34-2b4a-47df-b966-9b281f9f56fc,0,100.00,,,,
> > 
> > which causes a failure of parsing fields. The numbers of separators
> 
> Causes failure in what?
> 
> > is fixed for each line, no matter supported or not supported.
> 
> I don't think they're extra fields, there are cases where they can be filled out

My understanding was that at some place there is a if/else

	if (supported counters)
		fprintf_something with N fields, all filled in
        else
		fprintf_empty_fields with != N fields

So I think this is not about using things like 'a,b,,,,,,' but about
using different number of commas (fields) for supported/unsupported
counters, no?

- Arnaldo

> for variance, metricvalue, unit. And other code in perf too uses empty
> fields when something is not available.
> 
>         - optional usec time stamp in fractions of second (with -I xxx)
>         - optional CPU, core, or socket identifier
>         - optional number of logical CPUs aggregated
>         - counter value
>         - unit of the counter value or empty
>         - event name
>         - run time of counter
>         - percentage of measurement time the counter was running
>         - optional variance if multiple values are collected with -r  
>         - optional metric value
>         - optional unit of metric
> 
> 
> -Andi

Re: [PATCH] perf stat: fix cvs output format

[Cong Wang]

On Tue, Mar 6, 2018 at 9:00 AM, Andi Kleen <ak@linux.intel.com> wrote:
> On Mon, Mar 05, 2018 at 10:43:53PM -0800, Cong Wang wrote:
>> From: Ilya Pronin <ipronin@twitter.com>
>>
>> When printing stats in CSV mode, perf stat appends extra CSV
>> separators when counter is not supported:
>>
>> <not supported>,,L1-dcache-store-misses,mesos/bd442f34-2b4a-47df-b966-9b281f9f56fc,0,100.00,,,,
>>
>> which causes a failure of parsing fields. The numbers of separators
>
> Causes failure in what?

Failed to know how many fields in that line, clearly there are less
separators when it is supported.


>
>> is fixed for each line, no matter supported or not supported.
>
> I don't think they're extra fields, there are cases where they can be filled out
> for variance, metricvalue, unit. And other code in perf too uses empty
> fields when something is not available.

Are you saying there should be more fields when it is not supported?

Here is the output from your own commit:

      423470,,stalled-cycles-frontend,509102,100.00,65.69,frontend cycles idle
      <not supported>,,stalled-cycles-backend,0,100.00,,,,

so line 1 has 7 fields, line 2 has 9 fields, and this is expected?

Re: [PATCH] perf stat: fix cvs output format

[Andi Kleen]

> Here is the output from your own commit:
> 
>       423470,,stalled-cycles-frontend,509102,100.00,65.69,frontend cycles idle
>       <not supported>,,stalled-cycles-backend,0,100.00,,,,
> 
> so line 1 has 7 fields, line 2 has 9 fields, and this is expected?

If you had metrics on line 1 it would be correct.

So you just shifted it to break that case.

If you always want to have the same number of fields
you need to add two empty fields to the normal output
when there are no metrics.

-Andi

Re: [PATCH] perf stat: fix cvs output format

[Andi Kleen]

> My understanding was that at some place there is a if/else
> 
> 	if (supported counters)
> 		fprintf_something with N fields, all filled in
>         else
> 		fprintf_empty_fields with != N fields
> 
> So I think this is not about using things like 'a,b,,,,,,' but about
> using different number of commas (fields) for supported/unsupported
> counters, no?

I believe it's only about empty fields at the end. I don't think
we ever get the columns wrong.

The original patch just moved the problem because there are still
cases where we can output different number of columns. 

If a tool looks only at the first row to allocate the number of columns 
it might error out if there are lines with more columns later. 

All outputs have to be padded to the maximum number of columns,
so removing columns is never the right fix.

-Andi

Re: [PATCH] perf stat: fix cvs output format

[Cong Wang]

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

On Tue, Mar 6, 2018 at 9:53 AM, Andi Kleen <ak@linux.intel.com> wrote:
>> Here is the output from your own commit:
>>
>>       423470,,stalled-cycles-frontend,509102,100.00,65.69,frontend cycles idle
>>       <not supported>,,stalled-cycles-backend,0,100.00,,,,
>>
>> so line 1 has 7 fields, line 2 has 9 fields, and this is expected?
>
> If you had metrics on line 1 it would be correct.
>
> So you just shifted it to break that case.
>
> If you always want to have the same number of fields
> you need to add two empty fields to the normal output
> when there are no metrics.

The number of separators is the only way to learn the number
of fields, therefore it must be a fixed number.

Yeah, it could be the other way that supported ones have less
separators than it should. If we look at print_metric_csv() alone,
it should produce a same number of separators for all cases,
otherwise hard to count.

So I believe we need an additional patch, like the one attached,
to make it complete? Note, I only spot the cgroup field here.

[-- Attachment #2: perf-stat.diff --]
[-- Type: text/plain, Size: 1542 bytes --]

diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index 54a4c152edb3..6896e739ae4e 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -1044,8 +1044,7 @@ static void nsec_printout(int id, int nr, struct perf_evsel *evsel, double avg)
 
 	fprintf(output, fmt_n, name);
 
-	if (evsel->cgrp)
-		fprintf(output, "%s%s", csv_sep, evsel->cgrp->name);
+	fprintf(output, "%s%s", csv_sep, evsel->cgrp ? evsel->cgrp->name : "");
 }
 
 static int first_shadow_cpu(struct perf_evsel *evsel, int id)
@@ -1092,12 +1091,13 @@ static void abs_printout(int id, int nr, struct perf_evsel *evsel, double avg)
 	if (evsel->unit)
 		fprintf(output, "%-*s%s",
 			csv_output ? 0 : unit_width,
-			evsel->unit, csv_sep);
+			evsel->unit ? evsel->unit : "", csv_sep);
+	else
+		fprintf(output, "%s", csv_sep);
 
 	fprintf(output, "%-*s", csv_output ? 0 : 25, perf_evsel__name(evsel));
 
-	if (evsel->cgrp)
-		fprintf(output, "%s%s", csv_sep, evsel->cgrp->name);
+	fprintf(output, "%s%s", csv_sep, evsel->cgrp ? evsel->cgrp->name : "");
 }
 
 static void printout(int id, int nr, struct perf_evsel *counter, double uval,
@@ -1163,9 +1163,8 @@ static void printout(int id, int nr, struct perf_evsel *counter, double uval,
 			csv_output ? 0 : -25,
 			perf_evsel__name(counter));
 
-		if (counter->cgrp)
-			fprintf(stat_config.output, "%s%s",
-				csv_sep, counter->cgrp->name);
+		fprintf(stat_config.output, "%s%s", csv_sep,
+			counter->cgrp ? counter->cgrp->name : "");
 
 		if (!csv_output)
 			pm(&os, NULL, NULL, "", 0);

Re: [PATCH] perf stat: fix cvs output format

[Ilya Pronin]

Speaking from the user's seat. An optional (not just empty) cgroup
field is fine as long it consistently appears when requested with -G
option. The problem with print_metric_csv() was that in the case of
unsupported counters 2 additional empty fields in the output are
completely unexpected and not documented anywhere.

Andi, in the output example in your commit
92a61f6412d3a09d6462252a522fa79c9290f405 stalled-cycles-backend event
has counter run time field, counter run time percentage field, empty
metric value, empty metric unit, and then 2 other empty fields. Are
they expected? If yes, what are they and why other events, e.g.
stalled-cycles-frontend, don't have them? Am I missing something here?

On Tue, Mar 6, 2018 at 11:03 AM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> On Tue, Mar 6, 2018 at 9:53 AM, Andi Kleen <ak@linux.intel.com> wrote:
>>> Here is the output from your own commit:
>>>
>>>       423470,,stalled-cycles-frontend,509102,100.00,65.69,frontend cycles idle
>>>       <not supported>,,stalled-cycles-backend,0,100.00,,,,
>>>
>>> so line 1 has 7 fields, line 2 has 9 fields, and this is expected?
>>
>> If you had metrics on line 1 it would be correct.
>>
>> So you just shifted it to break that case.
>>
>> If you always want to have the same number of fields
>> you need to add two empty fields to the normal output
>> when there are no metrics.
>
> The number of separators is the only way to learn the number
> of fields, therefore it must be a fixed number.
>
> Yeah, it could be the other way that supported ones have less
> separators than it should. If we look at print_metric_csv() alone,
> it should produce a same number of separators for all cases,
> otherwise hard to count.
>
> So I believe we need an additional patch, like the one attached,
> to make it complete? Note, I only spot the cgroup field here.

-- 
Ilya Pronin

[tip:perf/urgent] perf stat: Fix CVS output format for non-supported counters

[tip-bot for Ilya Pronin]

Commit-ID:  40c21898ba5372c14ef71717040529794a91ccc2
Gitweb:     https://git.kernel.org/tip/40c21898ba5372c14ef71717040529794a91ccc2
Author:     Ilya Pronin <ipronin@twitter.com>
AuthorDate: Mon, 5 Mar 2018 22:43:53 -0800
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Tue, 6 Mar 2018 10:53:52 -0300

perf stat: Fix CVS output format for non-supported counters

When printing stats in CSV mode, 'perf stat' appends extra separators
when a counter is not supported:

<not supported>,,L1-dcache-store-misses,mesos/bd442f34-2b4a-47df-b966-9b281f9f56fc,0,100.00,,,,

Which causes a failure when parsing fields. The numbers of separators
should be the same for each line, no matter if the counter is or not
supported.

Signed-off-by: Ilya Pronin <ipronin@twitter.com>
Acked-by: Jiri Olsa <jolsa@redhat.com>
Cc: Andi Kleen <ak@linux.intel.com>
Link: http://lkml.kernel.org/r/20180306064353.31930-1-xiyou.wangcong@gmail.com
Fixes: 92a61f6412d3 ("perf stat: Implement CSV metrics output")
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/builtin-stat.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index 98bf9d32f222..54a4c152edb3 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -917,7 +917,7 @@ static void print_metric_csv(void *ctx,
 	char buf[64], *vals, *ends;
 
 	if (unit == NULL || fmt == NULL) {
-		fprintf(out, "%s%s%s%s", csv_sep, csv_sep, csv_sep, csv_sep);
+		fprintf(out, "%s%s", csv_sep, csv_sep);
 		return;
 	}
 	snprintf(buf, sizeof(buf), fmt, val);

Re: [PATCH] perf stat: fix cvs output format

[Andi Kleen]

On Tue, Mar 06, 2018 at 12:31:03PM -0800, Ilya Pronin wrote:
> Speaking from the user's seat. An optional (not just empty) cgroup
> field is fine as long it consistently appears when requested with -G
> option. The problem with print_metric_csv() was that in the case of
> unsupported counters 2 additional empty fields in the output are
> completely unexpected and not documented anywhere.
> 
> Andi, in the output example in your commit
> 92a61f6412d3a09d6462252a522fa79c9290f405 stalled-cycles-backend event
> has counter run time field, counter run time percentage field, empty
> metric value, empty metric unit, and then 2 other empty fields. Are
> they expected? If yes, what are they and why other events, e.g.

No two extra empty fields are not expected. All lines should
have the same number of fields so that a tool that looks
at the first like can keep using the same number.

But I don't think that was it what the patch fixed, or did I 
misread it?

-Andi

Re: [PATCH] perf stat: fix cvs output format

[Ilya Pronin]

The patch merely eliminated those 2 unexpected fields. But from what I
saw they were violating the assumption that all lines have the same
number of fields.

On Wed, Mar 7, 2018 at 9:04 AM, Andi Kleen <ak@linux.intel.com> wrote:
> On Tue, Mar 06, 2018 at 12:31:03PM -0800, Ilya Pronin wrote:
>> Speaking from the user's seat. An optional (not just empty) cgroup
>> field is fine as long it consistently appears when requested with -G
>> option. The problem with print_metric_csv() was that in the case of
>> unsupported counters 2 additional empty fields in the output are
>> completely unexpected and not documented anywhere.
>>
>> Andi, in the output example in your commit
>> 92a61f6412d3a09d6462252a522fa79c9290f405 stalled-cycles-backend event
>> has counter run time field, counter run time percentage field, empty
>> metric value, empty metric unit, and then 2 other empty fields. Are
>> they expected? If yes, what are they and why other events, e.g.
>
> No two extra empty fields are not expected. All lines should
> have the same number of fields so that a tool that looks
> at the first like can keep using the same number.
>
> But I don't think that was it what the patch fixed, or did I
> misread it?
>
> -Andi



-- 
Ilya

Re: [PATCH] perf stat: fix cvs output format

[Andi Kleen]

On Thu, Mar 08, 2018 at 01:52:15PM -0800, Ilya Pronin wrote:
> The patch merely eliminated those 2 unexpected fields. But from what I
> saw they were violating the assumption that all lines have the same
> number of fields.

Ok then I must have misread the patch. Sorry about that.
It's fine for me then. I think Arnaldo has it already merged anyways.

-Andi