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.wangc...@gmail.com> wrote: > On Tue, Mar 6, 2018 at 9:53 AM, Andi Kleen <a...@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