From: Ilya Pronin <[email protected]>
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 <[email protected]>
Cc: Arnaldo Carvalho de Melo <[email protected]>
Cc: Jiri Olsa <[email protected]>
Signed-off-by: Ilya Pronin <[email protected]>
Signed-off-by: Cong Wang <[email protected]>
---
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
On Mon, Mar 05, 2018 at 10:43:53PM -0800, Cong Wang wrote:
> From: Ilya Pronin <[email protected]>
>
> 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 <[email protected]>
> Cc: Arnaldo Carvalho de Melo <[email protected]>
> Cc: Jiri Olsa <[email protected]>
> Signed-off-by: Ilya Pronin <[email protected]>
> Signed-off-by: Cong Wang <[email protected]>
> ---
> 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 <[email protected]>
thanks,
jirka
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 <[email protected]>
> >
> > 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 <[email protected]>
> > Cc: Arnaldo Carvalho de Melo <[email protected]>
> > Cc: Jiri Olsa <[email protected]>
> > Signed-off-by: Ilya Pronin <[email protected]>
> > Signed-off-by: Cong Wang <[email protected]>
> > ---
> > 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 <[email protected]>
>
Thanks, applied to perf/urgent.
- Arnaldo
On Mon, Mar 05, 2018 at 10:43:53PM -0800, Cong Wang wrote:
> From: Ilya Pronin <[email protected]>
>
> 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
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 <[email protected]>
> >
> > 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
On Tue, Mar 6, 2018 at 9:00 AM, Andi Kleen <[email protected]> wrote:
> On Mon, Mar 05, 2018 at 10:43:53PM -0800, Cong Wang wrote:
>> From: Ilya Pronin <[email protected]>
>>
>> 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?
> 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
> 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
On Tue, Mar 6, 2018 at 9:53 AM, Andi Kleen <[email protected]> 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.
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 <[email protected]> wrote:
> On Tue, Mar 6, 2018 at 9:53 AM, Andi Kleen <[email protected]> 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
Commit-ID: 40c21898ba5372c14ef71717040529794a91ccc2
Gitweb: https://git.kernel.org/tip/40c21898ba5372c14ef71717040529794a91ccc2
Author: Ilya Pronin <[email protected]>
AuthorDate: Mon, 5 Mar 2018 22:43:53 -0800
Committer: Arnaldo Carvalho de Melo <[email protected]>
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 <[email protected]>
Acked-by: Jiri Olsa <[email protected]>
Cc: Andi Kleen <[email protected]>
Link: http://lkml.kernel.org/r/[email protected]
Fixes: 92a61f6412d3 ("perf stat: Implement CSV metrics output")
Signed-off-by: Cong Wang <[email protected]>
Signed-off-by: Arnaldo Carvalho de Melo <[email protected]>
---
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);
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
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 <[email protected]> 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
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