> On 09-Nov-2022, at 3:53 PM, Athira Rajeev <atraj...@linux.vnet.ibm.com> wrote:
>
>
>
>> On 09-Nov-2022, at 2:27 AM, Arnaldo Carvalho de Melo <a...@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 <atraj...@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 <disg...@linux.vnet.ibm.com>
>>>> Signed-off-by: Athira Rajeev <atraj...@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-namhy...@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-atraj...@linux.vnet.ibm.com/T/#m1ba8c773b53f198923101684c39b13da686c211d
Thanks
Athira
>
> From dde8f830ad318c9111c3fea5415fd8170b4c51bd Mon Sep 17 00:00:00 2001
> From: Athira Rajeev <atraj...@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 <disg...@linux.vnet.ibm.com>
> Signed-off-by: Athira Rajeev <atraj...@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
