Re: [PATCH] perf stat: fix cvs output format
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
Re: [PATCH] perf stat: fix cvs output format
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 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
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
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 wrote: > On Tue, Mar 6, 2018 at 9:53 AM, Andi Kleen wrote: >>> Here is the output from your own commit: >>> >>> 423470,,stalled-cycles-frontend,509102,100.00,65.69,frontend cycles >>> idle >>> ,,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
Re: [PATCH] perf stat: fix cvs output format
On Tue, Mar 6, 2018 at 9:53 AM, Andi Kleen wrote: >> Here is the output from your own commit: >> >> 423470,,stalled-cycles-frontend,509102,100.00,65.69,frontend cycles >> idle >> ,,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. 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
> 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
> Here is the output from your own commit: > > 423470,,stalled-cycles-frontend,509102,100.00,65.69,frontend cycles idle > ,,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
On Tue, Mar 6, 2018 at 9:00 AM, Andi Kleen wrote: > On Mon, Mar 05, 2018 at 10:43:53PM -0800, Cong Wang wrote: >> From: Ilya Pronin >> >> When printing stats in CSV mode, perf stat appends extra CSV >> separators when counter is not supported: >> >> > 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 ,,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
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 > > > > When printing stats in CSV mode, perf stat appends extra CSV > > separators when counter is not supported: > > > > > 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
On Mon, Mar 05, 2018 at 10:43:53PM -0800, Cong Wang wrote: > From: Ilya Pronin > > When printing stats in CSV mode, perf stat appends extra CSV > separators when counter is not supported: > > 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
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 > > > > When printing stats in CSV mode, perf stat appends extra CSV > > separators when counter is not supported: > > > > > 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 > > Cc: Arnaldo Carvalho de Melo > > Cc: Jiri Olsa > > Signed-off-by: Ilya Pronin > > Signed-off-by: Cong Wang > > --- > > 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 > Thanks, applied to perf/urgent. - Arnaldo
Re: [PATCH] perf stat: fix cvs output format
On Mon, Mar 05, 2018 at 10:43:53PM -0800, Cong Wang wrote: > From: Ilya Pronin > > When printing stats in CSV mode, perf stat appends extra CSV > separators when counter is not supported: > > 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 > Cc: Arnaldo Carvalho de Melo > Cc: Jiri Olsa > Signed-off-by: Ilya Pronin > Signed-off-by: Cong Wang > --- > 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 thanks, jirka