* [PATCH 1/2] tools/perf: Fix printing os->prefix in CSV metrics output @ 2022-10-18 8:56 ` Athira Rajeev 0 siblings, 0 replies; 17+ messages in thread From: Athira Rajeev @ 2022-10-18 8:56 UTC (permalink / raw) To: acme, jolsa, ak Cc: namhyung, irogers, james.clark, mpe, linux-perf-users, linuxppc-dev, maddy, rnsastry, kjain, disgoel Perf stat with CSV output option prints an extra empty string as first field in metrics output line. Sample output below: # ./perf stat -x, --per-socket -a -C 1 ls S0,1,1.78,msec,cpu-clock,1785146,100.00,0.973,CPUs utilized S0,1,26,,context-switches,1781750,100.00,0.015,M/sec S0,1,1,,cpu-migrations,1780526,100.00,0.561,K/sec S0,1,1,,page-faults,1779060,100.00,0.561,K/sec S0,1,875807,,cycles,1769826,100.00,0.491,GHz S0,1,85281,,stalled-cycles-frontend,1767512,100.00,9.74,frontend cycles idle S0,1,576839,,stalled-cycles-backend,1766260,100.00,65.86,backend cycles idle S0,1,288430,,instructions,1762246,100.00,0.33,insn per cycle ====> ,S0,1,,,,,,,2.00,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 "," in the beginning. Sample output using interval mode: # ./perf stat -I 1000 -x, --per-socket -a -C 1 ls 0.001813453,S0,1,1.87,msec,cpu-clock,1872052,100.00,0.002,CPUs utilized 0.001813453,S0,1,2,,context-switches,1868028,100.00,1.070,K/sec ------ 0.001813453,S0,1,85379,,instructions,1856754,100.00,0.32,insn per cycle ====> 0.001813453,,S0,1,,,,,,,1.34,stalled cycles per insn Above result also has an extra csv separator after the timestamp. Patch addresses extra field separator in the beginning of the metric output line. 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 new_line_csv function has check for "os->prefix" and if prefix is not null, it will be printed along with cvs separator. Snippet from "new_line_csv": if (os->prefix) fprintf(os->fh, "%s%s", os->prefix, config->csv_sep); Here os->prefix gets printed followed by "," which is the cvs separator. The os->prefix is used in interval mode option ( -I ), to print time stamp on every new line. But prefix is already set to contain csv separator when used in interval mode for csv option. Reference: Function "static void print_interval" Snippet: sprintf(prefix, "%6lu.%09lu%s", ts->tv_sec, ts->tv_nsec, config->csv_sep); Also if prefix is not assigned (if not used with -I option), it gets set to empty string. Reference: function printout() in util/stat-display.c Snippet: .prefix = prefix ? prefix : "", Since prefix already set to contain cvs_sep in interval option, patch removes printing config->csv_sep in new_line_csv function to avoid printing extra field. After the patch: # ./perf stat -x, --per-socket -a -C 1 ls S0,1,2.04,msec,cpu-clock,2045202,100.00,1.013,CPUs utilized S0,1,2,,context-switches,2041444,100.00,979.289,/sec S0,1,0,,cpu-migrations,2040820,100.00,0.000,/sec S0,1,2,,page-faults,2040288,100.00,979.289,/sec S0,1,254589,,cycles,2036066,100.00,0.125,GHz S0,1,82481,,stalled-cycles-frontend,2032420,100.00,32.40,frontend cycles idle S0,1,113170,,stalled-cycles-backend,2031722,100.00,44.45,backend cycles idle S0,1,88766,,instructions,2030942,100.00,0.35,insn per cycle S0,1,,,,,,,1.27,stalled cycles per insn Fixes: 92a61f6412d3 ("perf stat: Implement CSV metrics output") Reported-by: Disha Goel <disgoel@linux.vnet.ibm.com> Signed-off-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com> --- tools/perf/util/stat-display.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c index 5c47ee9963a7..879874a4bc07 100644 --- a/tools/perf/util/stat-display.c +++ b/tools/perf/util/stat-display.c @@ -273,7 +273,7 @@ static void new_line_csv(struct perf_stat_config *config, void *ctx) fputc('\n', os->fh); if (os->prefix) - fprintf(os->fh, "%s%s", os->prefix, config->csv_sep); + fprintf(os->fh, "%s", os->prefix); aggr_printout(config, os->evsel, os->id, os->nr); for (i = 0; i < os->nfields; i++) fputs(config->csv_sep, os->fh); -- 2.31.1 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH 1/2] tools/perf: Fix printing os->prefix in CSV metrics output @ 2022-10-18 8:56 ` Athira Rajeev 0 siblings, 0 replies; 17+ messages in thread From: Athira Rajeev @ 2022-10-18 8:56 UTC (permalink / raw) To: acme, jolsa, ak Cc: irogers, maddy, rnsastry, linux-perf-users, james.clark, kjain, namhyung, disgoel, linuxppc-dev Perf stat with CSV output option prints an extra empty string as first field in metrics output line. Sample output below: # ./perf stat -x, --per-socket -a -C 1 ls S0,1,1.78,msec,cpu-clock,1785146,100.00,0.973,CPUs utilized S0,1,26,,context-switches,1781750,100.00,0.015,M/sec S0,1,1,,cpu-migrations,1780526,100.00,0.561,K/sec S0,1,1,,page-faults,1779060,100.00,0.561,K/sec S0,1,875807,,cycles,1769826,100.00,0.491,GHz S0,1,85281,,stalled-cycles-frontend,1767512,100.00,9.74,frontend cycles idle S0,1,576839,,stalled-cycles-backend,1766260,100.00,65.86,backend cycles idle S0,1,288430,,instructions,1762246,100.00,0.33,insn per cycle ====> ,S0,1,,,,,,,2.00,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 "," in the beginning. Sample output using interval mode: # ./perf stat -I 1000 -x, --per-socket -a -C 1 ls 0.001813453,S0,1,1.87,msec,cpu-clock,1872052,100.00,0.002,CPUs utilized 0.001813453,S0,1,2,,context-switches,1868028,100.00,1.070,K/sec ------ 0.001813453,S0,1,85379,,instructions,1856754,100.00,0.32,insn per cycle ====> 0.001813453,,S0,1,,,,,,,1.34,stalled cycles per insn Above result also has an extra csv separator after the timestamp. Patch addresses extra field separator in the beginning of the metric output line. 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 new_line_csv function has check for "os->prefix" and if prefix is not null, it will be printed along with cvs separator. Snippet from "new_line_csv": if (os->prefix) fprintf(os->fh, "%s%s", os->prefix, config->csv_sep); Here os->prefix gets printed followed by "," which is the cvs separator. The os->prefix is used in interval mode option ( -I ), to print time stamp on every new line. But prefix is already set to contain csv separator when used in interval mode for csv option. Reference: Function "static void print_interval" Snippet: sprintf(prefix, "%6lu.%09lu%s", ts->tv_sec, ts->tv_nsec, config->csv_sep); Also if prefix is not assigned (if not used with -I option), it gets set to empty string. Reference: function printout() in util/stat-display.c Snippet: .prefix = prefix ? prefix : "", Since prefix already set to contain cvs_sep in interval option, patch removes printing config->csv_sep in new_line_csv function to avoid printing extra field. After the patch: # ./perf stat -x, --per-socket -a -C 1 ls S0,1,2.04,msec,cpu-clock,2045202,100.00,1.013,CPUs utilized S0,1,2,,context-switches,2041444,100.00,979.289,/sec S0,1,0,,cpu-migrations,2040820,100.00,0.000,/sec S0,1,2,,page-faults,2040288,100.00,979.289,/sec S0,1,254589,,cycles,2036066,100.00,0.125,GHz S0,1,82481,,stalled-cycles-frontend,2032420,100.00,32.40,frontend cycles idle S0,1,113170,,stalled-cycles-backend,2031722,100.00,44.45,backend cycles idle S0,1,88766,,instructions,2030942,100.00,0.35,insn per cycle S0,1,,,,,,,1.27,stalled cycles per insn Fixes: 92a61f6412d3 ("perf stat: Implement CSV metrics output") Reported-by: Disha Goel <disgoel@linux.vnet.ibm.com> Signed-off-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com> --- tools/perf/util/stat-display.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c index 5c47ee9963a7..879874a4bc07 100644 --- a/tools/perf/util/stat-display.c +++ b/tools/perf/util/stat-display.c @@ -273,7 +273,7 @@ static void new_line_csv(struct perf_stat_config *config, void *ctx) fputc('\n', os->fh); if (os->prefix) - fprintf(os->fh, "%s%s", os->prefix, config->csv_sep); + fprintf(os->fh, "%s", os->prefix); aggr_printout(config, os->evsel, os->id, os->nr); for (i = 0; i < os->nfields; i++) fputs(config->csv_sep, os->fh); -- 2.31.1 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH 2/2] tools/perf: Fix printing field separator in CSV metrics output 2022-10-18 8:56 ` Athira Rajeev @ 2022-10-18 8:56 ` Athira Rajeev -1 siblings, 0 replies; 17+ messages in thread From: Athira Rajeev @ 2022-10-18 8:56 UTC (permalink / raw) To: acme, jolsa, ak Cc: namhyung, irogers, james.clark, mpe, linux-perf-users, linuxppc-dev, maddy, rnsastry, kjain, disgoel 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 <disgoel@linux.vnet.ibm.com> Signed-off-by: Athira Rajeev <atrajeev@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 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 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH 2/2] tools/perf: Fix printing field separator in CSV metrics output @ 2022-10-18 8:56 ` Athira Rajeev 0 siblings, 0 replies; 17+ messages in thread From: Athira Rajeev @ 2022-10-18 8:56 UTC (permalink / raw) To: acme, jolsa, ak Cc: irogers, maddy, rnsastry, linux-perf-users, james.clark, kjain, namhyung, disgoel, linuxppc-dev 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 <disgoel@linux.vnet.ibm.com> Signed-off-by: Athira Rajeev <atrajeev@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 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 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] tools/perf: Fix printing field separator in CSV metrics output 2022-10-18 8:56 ` Athira Rajeev @ 2022-11-02 8:37 ` Athira Rajeev -1 siblings, 0 replies; 17+ messages in thread From: Athira Rajeev @ 2022-11-02 8:37 UTC (permalink / raw) To: Arnaldo Carvalho de Melo, Jiri Olsa, Andi Kleen Cc: Ian Rogers, maddy, Nageswara Sastry, linux-perf-users, james.clark, kjain, namhyung, disgoel, linuxppc-dev > On 18-Oct-2022, at 2:26 PM, Athira Rajeev <atrajeev@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 <disgoel@linux.vnet.ibm.com> > Signed-off-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com> Hi All, Looking for review comments for this change. 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 > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] tools/perf: Fix printing field separator in CSV metrics output @ 2022-11-02 8:37 ` Athira Rajeev 0 siblings, 0 replies; 17+ messages in thread From: Athira Rajeev @ 2022-11-02 8:37 UTC (permalink / raw) To: Arnaldo Carvalho de Melo, Jiri Olsa, Andi Kleen Cc: Ian Rogers, maddy, Nageswara Sastry, kjain, linux-perf-users, james.clark, namhyung, disgoel, linuxppc-dev > On 18-Oct-2022, at 2:26 PM, Athira Rajeev <atrajeev@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 <disgoel@linux.vnet.ibm.com> > Signed-off-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com> Hi All, Looking for review comments for this change. 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 > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] tools/perf: Fix printing field separator in CSV metrics output 2022-11-02 8:37 ` Athira Rajeev @ 2022-11-08 20:57 ` Arnaldo Carvalho de Melo -1 siblings, 0 replies; 17+ messages in thread From: Arnaldo Carvalho de Melo @ 2022-11-08 20:57 UTC (permalink / raw) To: Athira Rajeev Cc: Jiri Olsa, Andi Kleen, Ian Rogers, maddy, Nageswara Sastry, linux-perf-users, james.clark, kjain, namhyung, disgoel, linuxppc-dev Em Wed, Nov 02, 2022 at 02:07:06PM +0530, Athira Rajeev escreveu: > > > > On 18-Oct-2022, at 2:26 PM, Athira Rajeev <atrajeev@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 <disgoel@linux.vnet.ibm.com> > > Signed-off-by: Athira Rajeev <atrajeev@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-namhyung@kernel.org Can you please check? I just applied the other patch in this series. Thanks, - Arnaldo > 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] tools/perf: Fix printing field separator in CSV metrics output @ 2022-11-08 20:57 ` Arnaldo Carvalho de Melo 0 siblings, 0 replies; 17+ messages in thread From: Arnaldo Carvalho de Melo @ 2022-11-08 20:57 UTC (permalink / raw) To: Athira Rajeev Cc: Ian Rogers, Andi Kleen, Nageswara Sastry, kjain, linux-perf-users, maddy, james.clark, Jiri Olsa, namhyung, disgoel, linuxppc-dev Em Wed, Nov 02, 2022 at 02:07:06PM +0530, Athira Rajeev escreveu: > > > > On 18-Oct-2022, at 2:26 PM, Athira Rajeev <atrajeev@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 <disgoel@linux.vnet.ibm.com> > > Signed-off-by: Athira Rajeev <atrajeev@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-namhyung@kernel.org Can you please check? I just applied the other patch in this series. Thanks, - Arnaldo > 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] tools/perf: Fix printing field separator in CSV metrics output 2022-11-08 20:57 ` Arnaldo Carvalho de Melo (?) @ 2022-11-09 10:23 ` Athira Rajeev 2022-11-14 9:30 ` Athira Rajeev -1 siblings, 1 reply; 17+ messages in thread From: Athira Rajeev @ 2022-11-09 10:23 UTC (permalink / raw) To: Arnaldo Carvalho de Melo Cc: Ian Rogers, Andi Kleen, Nageswara Sastry, Kajol Jain, linux-perf-users, maddy, James Clark, Jiri Olsa, Namhyung Kim, disgoel, linuxppc-dev > On 09-Nov-2022, at 2:27 AM, Arnaldo Carvalho de Melo <acme@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 <atrajeev@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 <disgoel@linux.vnet.ibm.com> >>> Signed-off-by: Athira Rajeev <atrajeev@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-namhyung@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. From dde8f830ad318c9111c3fea5415fd8170b4c51bd Mon Sep 17 00:00:00 2001 From: Athira Rajeev <atrajeev@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 <disgoel@linux.vnet.ibm.com> Signed-off-by: Athira Rajeev <atrajeev@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 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] tools/perf: Fix printing field separator in CSV metrics output 2022-11-09 10:23 ` Athira Rajeev @ 2022-11-14 9:30 ` Athira Rajeev 0 siblings, 0 replies; 17+ messages in thread From: Athira Rajeev @ 2022-11-14 9:30 UTC (permalink / raw) To: Arnaldo Carvalho de Melo Cc: Ian Rogers, Andi Kleen, Nageswara Sastry, Kajol Jain, linux-perf-users, maddy, James Clark, Jiri Olsa, Namhyung Kim, disgoel, linuxppc-dev > On 09-Nov-2022, at 3:53 PM, Athira Rajeev <atrajeev@linux.vnet.ibm.com> wrote: > > > >> On 09-Nov-2022, at 2:27 AM, Arnaldo Carvalho de Melo <acme@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 <atrajeev@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 <disgoel@linux.vnet.ibm.com> >>>> Signed-off-by: Athira Rajeev <atrajeev@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-namhyung@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-atrajeev@linux.vnet.ibm.com/T/#m1ba8c773b53f198923101684c39b13da686c211d Thanks Athira > > From dde8f830ad318c9111c3fea5415fd8170b4c51bd Mon Sep 17 00:00:00 2001 > From: Athira Rajeev <atrajeev@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 <disgoel@linux.vnet.ibm.com> > Signed-off-by: Athira Rajeev <atrajeev@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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/2] tools/perf: Fix printing os->prefix in CSV metrics output 2022-10-18 8:56 ` Athira Rajeev @ 2022-11-02 8:36 ` Athira Rajeev -1 siblings, 0 replies; 17+ messages in thread From: Athira Rajeev @ 2022-11-02 8:36 UTC (permalink / raw) To: Arnaldo Carvalho de Melo, Jiri Olsa, Andi Kleen Cc: Ian Rogers, maddy, Nageswara Sastry, linux-perf-users, james.clark, kjain, namhyung, disgoel, linuxppc-dev > On 18-Oct-2022, at 2:26 PM, Athira Rajeev <atrajeev@linux.vnet.ibm.com> wrote: > > Perf stat with CSV output option prints an extra empty > string as first field in metrics output line. > Sample output below: > > # ./perf stat -x, --per-socket -a -C 1 ls > S0,1,1.78,msec,cpu-clock,1785146,100.00,0.973,CPUs utilized > S0,1,26,,context-switches,1781750,100.00,0.015,M/sec > S0,1,1,,cpu-migrations,1780526,100.00,0.561,K/sec > S0,1,1,,page-faults,1779060,100.00,0.561,K/sec > S0,1,875807,,cycles,1769826,100.00,0.491,GHz > S0,1,85281,,stalled-cycles-frontend,1767512,100.00,9.74,frontend cycles idle > S0,1,576839,,stalled-cycles-backend,1766260,100.00,65.86,backend cycles idle > S0,1,288430,,instructions,1762246,100.00,0.33,insn per cycle > ====> ,S0,1,,,,,,,2.00,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 "," in the > beginning. > > Sample output using interval mode: > # ./perf stat -I 1000 -x, --per-socket -a -C 1 ls > 0.001813453,S0,1,1.87,msec,cpu-clock,1872052,100.00,0.002,CPUs utilized > 0.001813453,S0,1,2,,context-switches,1868028,100.00,1.070,K/sec > ------ > 0.001813453,S0,1,85379,,instructions,1856754,100.00,0.32,insn per cycle > ====> 0.001813453,,S0,1,,,,,,,1.34,stalled cycles per insn > > Above result also has an extra csv separator after > the timestamp. Patch addresses extra field separator > in the beginning of the metric output line. > > 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 new_line_csv function has check for "os->prefix" > and if prefix is not null, it will be printed along > with cvs separator. > Snippet from "new_line_csv": > if (os->prefix) > fprintf(os->fh, "%s%s", os->prefix, config->csv_sep); > > Here os->prefix gets printed followed by "," > which is the cvs separator. The os->prefix is > used in interval mode option ( -I ), to print > time stamp on every new line. But prefix is > already set to contain csv separator when used > in interval mode for csv option. > > Reference: Function "static void print_interval" > Snippet: > sprintf(prefix, "%6lu.%09lu%s", ts->tv_sec, ts->tv_nsec, config->csv_sep); > > Also if prefix is not assigned (if not used with > -I option), it gets set to empty string. > Reference: function printout() in util/stat-display.c > Snippet: > .prefix = prefix ? prefix : "", > > Since prefix already set to contain cvs_sep in interval > option, patch removes printing config->csv_sep in > new_line_csv function to avoid printing extra field. > > After the patch: > > # ./perf stat -x, --per-socket -a -C 1 ls > S0,1,2.04,msec,cpu-clock,2045202,100.00,1.013,CPUs utilized > S0,1,2,,context-switches,2041444,100.00,979.289,/sec > S0,1,0,,cpu-migrations,2040820,100.00,0.000,/sec > S0,1,2,,page-faults,2040288,100.00,979.289,/sec > S0,1,254589,,cycles,2036066,100.00,0.125,GHz > S0,1,82481,,stalled-cycles-frontend,2032420,100.00,32.40,frontend cycles idle > S0,1,113170,,stalled-cycles-backend,2031722,100.00,44.45,backend cycles idle > S0,1,88766,,instructions,2030942,100.00,0.35,insn per cycle > S0,1,,,,,,,1.27,stalled cycles per insn > > Fixes: 92a61f6412d3 ("perf stat: Implement CSV metrics output") > Reported-by: Disha Goel <disgoel@linux.vnet.ibm.com> > Signed-off-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com> Hi All, Looking for review comments for this change. Thanks Athira > --- > tools/perf/util/stat-display.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c > index 5c47ee9963a7..879874a4bc07 100644 > --- a/tools/perf/util/stat-display.c > +++ b/tools/perf/util/stat-display.c > @@ -273,7 +273,7 @@ static void new_line_csv(struct perf_stat_config *config, void *ctx) > > fputc('\n', os->fh); > if (os->prefix) > - fprintf(os->fh, "%s%s", os->prefix, config->csv_sep); > + fprintf(os->fh, "%s", os->prefix); > aggr_printout(config, os->evsel, os->id, os->nr); > for (i = 0; i < os->nfields; i++) > fputs(config->csv_sep, os->fh); > -- > 2.31.1 > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/2] tools/perf: Fix printing os->prefix in CSV metrics output @ 2022-11-02 8:36 ` Athira Rajeev 0 siblings, 0 replies; 17+ messages in thread From: Athira Rajeev @ 2022-11-02 8:36 UTC (permalink / raw) To: Arnaldo Carvalho de Melo, Jiri Olsa, Andi Kleen Cc: Ian Rogers, maddy, Nageswara Sastry, kjain, linux-perf-users, james.clark, namhyung, disgoel, linuxppc-dev > On 18-Oct-2022, at 2:26 PM, Athira Rajeev <atrajeev@linux.vnet.ibm.com> wrote: > > Perf stat with CSV output option prints an extra empty > string as first field in metrics output line. > Sample output below: > > # ./perf stat -x, --per-socket -a -C 1 ls > S0,1,1.78,msec,cpu-clock,1785146,100.00,0.973,CPUs utilized > S0,1,26,,context-switches,1781750,100.00,0.015,M/sec > S0,1,1,,cpu-migrations,1780526,100.00,0.561,K/sec > S0,1,1,,page-faults,1779060,100.00,0.561,K/sec > S0,1,875807,,cycles,1769826,100.00,0.491,GHz > S0,1,85281,,stalled-cycles-frontend,1767512,100.00,9.74,frontend cycles idle > S0,1,576839,,stalled-cycles-backend,1766260,100.00,65.86,backend cycles idle > S0,1,288430,,instructions,1762246,100.00,0.33,insn per cycle > ====> ,S0,1,,,,,,,2.00,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 "," in the > beginning. > > Sample output using interval mode: > # ./perf stat -I 1000 -x, --per-socket -a -C 1 ls > 0.001813453,S0,1,1.87,msec,cpu-clock,1872052,100.00,0.002,CPUs utilized > 0.001813453,S0,1,2,,context-switches,1868028,100.00,1.070,K/sec > ------ > 0.001813453,S0,1,85379,,instructions,1856754,100.00,0.32,insn per cycle > ====> 0.001813453,,S0,1,,,,,,,1.34,stalled cycles per insn > > Above result also has an extra csv separator after > the timestamp. Patch addresses extra field separator > in the beginning of the metric output line. > > 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 new_line_csv function has check for "os->prefix" > and if prefix is not null, it will be printed along > with cvs separator. > Snippet from "new_line_csv": > if (os->prefix) > fprintf(os->fh, "%s%s", os->prefix, config->csv_sep); > > Here os->prefix gets printed followed by "," > which is the cvs separator. The os->prefix is > used in interval mode option ( -I ), to print > time stamp on every new line. But prefix is > already set to contain csv separator when used > in interval mode for csv option. > > Reference: Function "static void print_interval" > Snippet: > sprintf(prefix, "%6lu.%09lu%s", ts->tv_sec, ts->tv_nsec, config->csv_sep); > > Also if prefix is not assigned (if not used with > -I option), it gets set to empty string. > Reference: function printout() in util/stat-display.c > Snippet: > .prefix = prefix ? prefix : "", > > Since prefix already set to contain cvs_sep in interval > option, patch removes printing config->csv_sep in > new_line_csv function to avoid printing extra field. > > After the patch: > > # ./perf stat -x, --per-socket -a -C 1 ls > S0,1,2.04,msec,cpu-clock,2045202,100.00,1.013,CPUs utilized > S0,1,2,,context-switches,2041444,100.00,979.289,/sec > S0,1,0,,cpu-migrations,2040820,100.00,0.000,/sec > S0,1,2,,page-faults,2040288,100.00,979.289,/sec > S0,1,254589,,cycles,2036066,100.00,0.125,GHz > S0,1,82481,,stalled-cycles-frontend,2032420,100.00,32.40,frontend cycles idle > S0,1,113170,,stalled-cycles-backend,2031722,100.00,44.45,backend cycles idle > S0,1,88766,,instructions,2030942,100.00,0.35,insn per cycle > S0,1,,,,,,,1.27,stalled cycles per insn > > Fixes: 92a61f6412d3 ("perf stat: Implement CSV metrics output") > Reported-by: Disha Goel <disgoel@linux.vnet.ibm.com> > Signed-off-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com> Hi All, Looking for review comments for this change. Thanks Athira > --- > tools/perf/util/stat-display.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c > index 5c47ee9963a7..879874a4bc07 100644 > --- a/tools/perf/util/stat-display.c > +++ b/tools/perf/util/stat-display.c > @@ -273,7 +273,7 @@ static void new_line_csv(struct perf_stat_config *config, void *ctx) > > fputc('\n', os->fh); > if (os->prefix) > - fprintf(os->fh, "%s%s", os->prefix, config->csv_sep); > + fprintf(os->fh, "%s", os->prefix); > aggr_printout(config, os->evsel, os->id, os->nr); > for (i = 0; i < os->nfields; i++) > fputs(config->csv_sep, os->fh); > -- > 2.31.1 > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/2] tools/perf: Fix printing os->prefix in CSV metrics output 2022-11-02 8:36 ` Athira Rajeev @ 2022-11-03 16:15 ` Ian Rogers -1 siblings, 0 replies; 17+ messages in thread From: Ian Rogers @ 2022-11-03 16:15 UTC (permalink / raw) To: Athira Rajeev Cc: Arnaldo Carvalho de Melo, Jiri Olsa, Andi Kleen, maddy, Nageswara Sastry, linux-perf-users, james.clark, kjain, namhyung, disgoel, linuxppc-dev On Wed, Nov 2, 2022 at 1:36 AM Athira Rajeev <atrajeev@linux.vnet.ibm.com> wrote: > > > > > On 18-Oct-2022, at 2:26 PM, Athira Rajeev <atrajeev@linux.vnet.ibm.com> wrote: > > > > Perf stat with CSV output option prints an extra empty > > string as first field in metrics output line. > > Sample output below: > > > > # ./perf stat -x, --per-socket -a -C 1 ls > > S0,1,1.78,msec,cpu-clock,1785146,100.00,0.973,CPUs utilized > > S0,1,26,,context-switches,1781750,100.00,0.015,M/sec > > S0,1,1,,cpu-migrations,1780526,100.00,0.561,K/sec > > S0,1,1,,page-faults,1779060,100.00,0.561,K/sec > > S0,1,875807,,cycles,1769826,100.00,0.491,GHz > > S0,1,85281,,stalled-cycles-frontend,1767512,100.00,9.74,frontend cycles idle > > S0,1,576839,,stalled-cycles-backend,1766260,100.00,65.86,backend cycles idle > > S0,1,288430,,instructions,1762246,100.00,0.33,insn per cycle > > ====> ,S0,1,,,,,,,2.00,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 "," in the > > beginning. > > > > Sample output using interval mode: > > # ./perf stat -I 1000 -x, --per-socket -a -C 1 ls > > 0.001813453,S0,1,1.87,msec,cpu-clock,1872052,100.00,0.002,CPUs utilized > > 0.001813453,S0,1,2,,context-switches,1868028,100.00,1.070,K/sec > > ------ > > 0.001813453,S0,1,85379,,instructions,1856754,100.00,0.32,insn per cycle > > ====> 0.001813453,,S0,1,,,,,,,1.34,stalled cycles per insn > > > > Above result also has an extra csv separator after > > the timestamp. Patch addresses extra field separator > > in the beginning of the metric output line. > > > > 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 new_line_csv function has check for "os->prefix" > > and if prefix is not null, it will be printed along > > with cvs separator. > > Snippet from "new_line_csv": > > if (os->prefix) > > fprintf(os->fh, "%s%s", os->prefix, config->csv_sep); > > > > Here os->prefix gets printed followed by "," > > which is the cvs separator. The os->prefix is > > used in interval mode option ( -I ), to print > > time stamp on every new line. But prefix is > > already set to contain csv separator when used > > in interval mode for csv option. > > > > Reference: Function "static void print_interval" > > Snippet: > > sprintf(prefix, "%6lu.%09lu%s", ts->tv_sec, ts->tv_nsec, config->csv_sep); > > > > Also if prefix is not assigned (if not used with > > -I option), it gets set to empty string. > > Reference: function printout() in util/stat-display.c > > Snippet: > > .prefix = prefix ? prefix : "", > > > > Since prefix already set to contain cvs_sep in interval > > option, patch removes printing config->csv_sep in > > new_line_csv function to avoid printing extra field. > > > > After the patch: > > > > # ./perf stat -x, --per-socket -a -C 1 ls > > S0,1,2.04,msec,cpu-clock,2045202,100.00,1.013,CPUs utilized > > S0,1,2,,context-switches,2041444,100.00,979.289,/sec > > S0,1,0,,cpu-migrations,2040820,100.00,0.000,/sec > > S0,1,2,,page-faults,2040288,100.00,979.289,/sec > > S0,1,254589,,cycles,2036066,100.00,0.125,GHz > > S0,1,82481,,stalled-cycles-frontend,2032420,100.00,32.40,frontend cycles idle > > S0,1,113170,,stalled-cycles-backend,2031722,100.00,44.45,backend cycles idle > > S0,1,88766,,instructions,2030942,100.00,0.35,insn per cycle > > S0,1,,,,,,,1.27,stalled cycles per insn > > > > Fixes: 92a61f6412d3 ("perf stat: Implement CSV metrics output") > > Reported-by: Disha Goel <disgoel@linux.vnet.ibm.com> > > Signed-off-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com> > > Hi All, > > Looking for review comments for this change. Hi, Thanks for addressing issues in this code. What is the status of the CSV output test following these changes? I think going forward we need to move away from CSV and columns, to something with structure like json. We also need to refactor this code, trying to add meaning to a newline character is a bad strategy and creates some unnatural things. To some extent this overlaps with Namhyung's aggregation cleanup. There are also weirdnesses in jevents/pmu-events, like the same ScaleUnit applying to a metric and an event - why are metrics even parts of events? Given the current code is wac-a-mole, and this is another whack, if the testing is okay I think we should move forward with this change. Thanks, Ian > Thanks > Athira > > > --- > > tools/perf/util/stat-display.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c > > index 5c47ee9963a7..879874a4bc07 100644 > > --- a/tools/perf/util/stat-display.c > > +++ b/tools/perf/util/stat-display.c > > @@ -273,7 +273,7 @@ static void new_line_csv(struct perf_stat_config *config, void *ctx) > > > > fputc('\n', os->fh); > > if (os->prefix) > > - fprintf(os->fh, "%s%s", os->prefix, config->csv_sep); > > + fprintf(os->fh, "%s", os->prefix); > > aggr_printout(config, os->evsel, os->id, os->nr); > > for (i = 0; i < os->nfields; i++) > > fputs(config->csv_sep, os->fh); > > -- > > 2.31.1 > > > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/2] tools/perf: Fix printing os->prefix in CSV metrics output @ 2022-11-03 16:15 ` Ian Rogers 0 siblings, 0 replies; 17+ messages in thread From: Ian Rogers @ 2022-11-03 16:15 UTC (permalink / raw) To: Athira Rajeev Cc: Andi Kleen, Nageswara Sastry, kjain, Arnaldo Carvalho de Melo, linux-perf-users, maddy, james.clark, Jiri Olsa, namhyung, disgoel, linuxppc-dev On Wed, Nov 2, 2022 at 1:36 AM Athira Rajeev <atrajeev@linux.vnet.ibm.com> wrote: > > > > > On 18-Oct-2022, at 2:26 PM, Athira Rajeev <atrajeev@linux.vnet.ibm.com> wrote: > > > > Perf stat with CSV output option prints an extra empty > > string as first field in metrics output line. > > Sample output below: > > > > # ./perf stat -x, --per-socket -a -C 1 ls > > S0,1,1.78,msec,cpu-clock,1785146,100.00,0.973,CPUs utilized > > S0,1,26,,context-switches,1781750,100.00,0.015,M/sec > > S0,1,1,,cpu-migrations,1780526,100.00,0.561,K/sec > > S0,1,1,,page-faults,1779060,100.00,0.561,K/sec > > S0,1,875807,,cycles,1769826,100.00,0.491,GHz > > S0,1,85281,,stalled-cycles-frontend,1767512,100.00,9.74,frontend cycles idle > > S0,1,576839,,stalled-cycles-backend,1766260,100.00,65.86,backend cycles idle > > S0,1,288430,,instructions,1762246,100.00,0.33,insn per cycle > > ====> ,S0,1,,,,,,,2.00,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 "," in the > > beginning. > > > > Sample output using interval mode: > > # ./perf stat -I 1000 -x, --per-socket -a -C 1 ls > > 0.001813453,S0,1,1.87,msec,cpu-clock,1872052,100.00,0.002,CPUs utilized > > 0.001813453,S0,1,2,,context-switches,1868028,100.00,1.070,K/sec > > ------ > > 0.001813453,S0,1,85379,,instructions,1856754,100.00,0.32,insn per cycle > > ====> 0.001813453,,S0,1,,,,,,,1.34,stalled cycles per insn > > > > Above result also has an extra csv separator after > > the timestamp. Patch addresses extra field separator > > in the beginning of the metric output line. > > > > 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 new_line_csv function has check for "os->prefix" > > and if prefix is not null, it will be printed along > > with cvs separator. > > Snippet from "new_line_csv": > > if (os->prefix) > > fprintf(os->fh, "%s%s", os->prefix, config->csv_sep); > > > > Here os->prefix gets printed followed by "," > > which is the cvs separator. The os->prefix is > > used in interval mode option ( -I ), to print > > time stamp on every new line. But prefix is > > already set to contain csv separator when used > > in interval mode for csv option. > > > > Reference: Function "static void print_interval" > > Snippet: > > sprintf(prefix, "%6lu.%09lu%s", ts->tv_sec, ts->tv_nsec, config->csv_sep); > > > > Also if prefix is not assigned (if not used with > > -I option), it gets set to empty string. > > Reference: function printout() in util/stat-display.c > > Snippet: > > .prefix = prefix ? prefix : "", > > > > Since prefix already set to contain cvs_sep in interval > > option, patch removes printing config->csv_sep in > > new_line_csv function to avoid printing extra field. > > > > After the patch: > > > > # ./perf stat -x, --per-socket -a -C 1 ls > > S0,1,2.04,msec,cpu-clock,2045202,100.00,1.013,CPUs utilized > > S0,1,2,,context-switches,2041444,100.00,979.289,/sec > > S0,1,0,,cpu-migrations,2040820,100.00,0.000,/sec > > S0,1,2,,page-faults,2040288,100.00,979.289,/sec > > S0,1,254589,,cycles,2036066,100.00,0.125,GHz > > S0,1,82481,,stalled-cycles-frontend,2032420,100.00,32.40,frontend cycles idle > > S0,1,113170,,stalled-cycles-backend,2031722,100.00,44.45,backend cycles idle > > S0,1,88766,,instructions,2030942,100.00,0.35,insn per cycle > > S0,1,,,,,,,1.27,stalled cycles per insn > > > > Fixes: 92a61f6412d3 ("perf stat: Implement CSV metrics output") > > Reported-by: Disha Goel <disgoel@linux.vnet.ibm.com> > > Signed-off-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com> > > Hi All, > > Looking for review comments for this change. Hi, Thanks for addressing issues in this code. What is the status of the CSV output test following these changes? I think going forward we need to move away from CSV and columns, to something with structure like json. We also need to refactor this code, trying to add meaning to a newline character is a bad strategy and creates some unnatural things. To some extent this overlaps with Namhyung's aggregation cleanup. There are also weirdnesses in jevents/pmu-events, like the same ScaleUnit applying to a metric and an event - why are metrics even parts of events? Given the current code is wac-a-mole, and this is another whack, if the testing is okay I think we should move forward with this change. Thanks, Ian > Thanks > Athira > > > --- > > tools/perf/util/stat-display.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c > > index 5c47ee9963a7..879874a4bc07 100644 > > --- a/tools/perf/util/stat-display.c > > +++ b/tools/perf/util/stat-display.c > > @@ -273,7 +273,7 @@ static void new_line_csv(struct perf_stat_config *config, void *ctx) > > > > fputc('\n', os->fh); > > if (os->prefix) > > - fprintf(os->fh, "%s%s", os->prefix, config->csv_sep); > > + fprintf(os->fh, "%s", os->prefix); > > aggr_printout(config, os->evsel, os->id, os->nr); > > for (i = 0; i < os->nfields; i++) > > fputs(config->csv_sep, os->fh); > > -- > > 2.31.1 > > > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/2] tools/perf: Fix printing os->prefix in CSV metrics output 2022-11-03 16:15 ` Ian Rogers (?) @ 2022-11-04 7:29 ` Athira Rajeev 2022-11-04 7:37 ` Disha Goel -1 siblings, 1 reply; 17+ messages in thread From: Athira Rajeev @ 2022-11-04 7:29 UTC (permalink / raw) To: Ian Rogers Cc: Andi Kleen, Nageswara Sastry, Kajol Jain, Arnaldo Carvalho de Melo, linux-perf-users, maddy, James Clark, Jiri Olsa, Namhyung Kim, disgoel, linuxppc-dev > On 03-Nov-2022, at 9:45 PM, Ian Rogers <irogers@google.com> wrote: > > On Wed, Nov 2, 2022 at 1:36 AM Athira Rajeev > <atrajeev@linux.vnet.ibm.com> wrote: >> >> >> >>> On 18-Oct-2022, at 2:26 PM, Athira Rajeev <atrajeev@linux.vnet.ibm.com> wrote: >>> >>> Perf stat with CSV output option prints an extra empty >>> string as first field in metrics output line. >>> Sample output below: >>> >>> # ./perf stat -x, --per-socket -a -C 1 ls >>> S0,1,1.78,msec,cpu-clock,1785146,100.00,0.973,CPUs utilized >>> S0,1,26,,context-switches,1781750,100.00,0.015,M/sec >>> S0,1,1,,cpu-migrations,1780526,100.00,0.561,K/sec >>> S0,1,1,,page-faults,1779060,100.00,0.561,K/sec >>> S0,1,875807,,cycles,1769826,100.00,0.491,GHz >>> S0,1,85281,,stalled-cycles-frontend,1767512,100.00,9.74,frontend cycles idle >>> S0,1,576839,,stalled-cycles-backend,1766260,100.00,65.86,backend cycles idle >>> S0,1,288430,,instructions,1762246,100.00,0.33,insn per cycle >>> ====> ,S0,1,,,,,,,2.00,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 "," in the >>> beginning. >>> >>> Sample output using interval mode: >>> # ./perf stat -I 1000 -x, --per-socket -a -C 1 ls >>> 0.001813453,S0,1,1.87,msec,cpu-clock,1872052,100.00,0.002,CPUs utilized >>> 0.001813453,S0,1,2,,context-switches,1868028,100.00,1.070,K/sec >>> ------ >>> 0.001813453,S0,1,85379,,instructions,1856754,100.00,0.32,insn per cycle >>> ====> 0.001813453,,S0,1,,,,,,,1.34,stalled cycles per insn >>> >>> Above result also has an extra csv separator after >>> the timestamp. Patch addresses extra field separator >>> in the beginning of the metric output line. >>> >>> 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 new_line_csv function has check for "os->prefix" >>> and if prefix is not null, it will be printed along >>> with cvs separator. >>> Snippet from "new_line_csv": >>> if (os->prefix) >>> fprintf(os->fh, "%s%s", os->prefix, config->csv_sep); >>> >>> Here os->prefix gets printed followed by "," >>> which is the cvs separator. The os->prefix is >>> used in interval mode option ( -I ), to print >>> time stamp on every new line. But prefix is >>> already set to contain csv separator when used >>> in interval mode for csv option. >>> >>> Reference: Function "static void print_interval" >>> Snippet: >>> sprintf(prefix, "%6lu.%09lu%s", ts->tv_sec, ts->tv_nsec, config->csv_sep); >>> >>> Also if prefix is not assigned (if not used with >>> -I option), it gets set to empty string. >>> Reference: function printout() in util/stat-display.c >>> Snippet: >>> .prefix = prefix ? prefix : "", >>> >>> Since prefix already set to contain cvs_sep in interval >>> option, patch removes printing config->csv_sep in >>> new_line_csv function to avoid printing extra field. >>> >>> After the patch: >>> >>> # ./perf stat -x, --per-socket -a -C 1 ls >>> S0,1,2.04,msec,cpu-clock,2045202,100.00,1.013,CPUs utilized >>> S0,1,2,,context-switches,2041444,100.00,979.289,/sec >>> S0,1,0,,cpu-migrations,2040820,100.00,0.000,/sec >>> S0,1,2,,page-faults,2040288,100.00,979.289,/sec >>> S0,1,254589,,cycles,2036066,100.00,0.125,GHz >>> S0,1,82481,,stalled-cycles-frontend,2032420,100.00,32.40,frontend cycles idle >>> S0,1,113170,,stalled-cycles-backend,2031722,100.00,44.45,backend cycles idle >>> S0,1,88766,,instructions,2030942,100.00,0.35,insn per cycle >>> S0,1,,,,,,,1.27,stalled cycles per insn >>> >>> Fixes: 92a61f6412d3 ("perf stat: Implement CSV metrics output") >>> Reported-by: Disha Goel <disgoel@linux.vnet.ibm.com> >>> Signed-off-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com> >> >> Hi All, >> >> Looking for review comments for this change. > > Hi, > > Thanks for addressing issues in this code. What is the status of the > CSV output test following these changes? > > I think going forward we need to move away from CSV and columns, to > something with structure like json. We also need to refactor this > code, trying to add meaning to a newline character is a bad strategy > and creates some unnatural things. To some extent this overlaps with > Namhyung's aggregation cleanup. There are also weirdnesses in > jevents/pmu-events, like the same ScaleUnit applying to a metric and > an event - why are metrics even parts of events? > > Given the current code is wac-a-mole, and this is another whack, if > the testing is okay I think we should move forward with this change. > > Thanks, > Ian Hi Ian, Thanks for checking the patch. Yes, CSV output test passes with the change. perf stat CSV output linter : Ok perf stat csv summary test : Ok Thanks Athira > > > > >> Thanks >> Athira >> >>> --- >>> tools/perf/util/stat-display.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c >>> index 5c47ee9963a7..879874a4bc07 100644 >>> --- a/tools/perf/util/stat-display.c >>> +++ b/tools/perf/util/stat-display.c >>> @@ -273,7 +273,7 @@ static void new_line_csv(struct perf_stat_config *config, void *ctx) >>> >>> fputc('\n', os->fh); >>> if (os->prefix) >>> - fprintf(os->fh, "%s%s", os->prefix, config->csv_sep); >>> + fprintf(os->fh, "%s", os->prefix); >>> aggr_printout(config, os->evsel, os->id, os->nr); >>> for (i = 0; i < os->nfields; i++) >>> fputs(config->csv_sep, os->fh); >>> -- >>> 2.31.1 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/2] tools/perf: Fix printing os->prefix in CSV metrics output 2022-11-04 7:29 ` Athira Rajeev @ 2022-11-04 7:37 ` Disha Goel 2022-11-07 11:26 ` kajoljain 0 siblings, 1 reply; 17+ messages in thread From: Disha Goel @ 2022-11-04 7:37 UTC (permalink / raw) To: Athira Rajeev, Ian Rogers Cc: Andi Kleen, Nageswara Sastry, Kajol Jain, Arnaldo Carvalho de Melo, linux-perf-users, maddy, James Clark, Jiri Olsa, Namhyung Kim, linuxppc-dev [-- Attachment #1: Type: text/plain, Size: 6228 bytes --] On 11/4/22 12:59 PM, Athira Rajeev wrote: > >> On 03-Nov-2022, at 9:45 PM, Ian Rogers<irogers@google.com> wrote: >> >> On Wed, Nov 2, 2022 at 1:36 AM Athira Rajeev >> <atrajeev@linux.vnet.ibm.com> wrote: >>> >>> >>>> On 18-Oct-2022, at 2:26 PM, Athira Rajeev<atrajeev@linux.vnet.ibm.com> wrote: >>>> >>>> Perf stat with CSV output option prints an extra empty >>>> string as first field in metrics output line. >>>> Sample output below: >>>> >>>> # ./perf stat -x, --per-socket -a -C 1 ls >>>> S0,1,1.78,msec,cpu-clock,1785146,100.00,0.973,CPUs utilized >>>> S0,1,26,,context-switches,1781750,100.00,0.015,M/sec >>>> S0,1,1,,cpu-migrations,1780526,100.00,0.561,K/sec >>>> S0,1,1,,page-faults,1779060,100.00,0.561,K/sec >>>> S0,1,875807,,cycles,1769826,100.00,0.491,GHz >>>> S0,1,85281,,stalled-cycles-frontend,1767512,100.00,9.74,frontend cycles idle >>>> S0,1,576839,,stalled-cycles-backend,1766260,100.00,65.86,backend cycles idle >>>> S0,1,288430,,instructions,1762246,100.00,0.33,insn per cycle >>>> ====> ,S0,1,,,,,,,2.00,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 "," in the >>>> beginning. >>>> >>>> Sample output using interval mode: >>>> # ./perf stat -I 1000 -x, --per-socket -a -C 1 ls >>>> 0.001813453,S0,1,1.87,msec,cpu-clock,1872052,100.00,0.002,CPUs utilized >>>> 0.001813453,S0,1,2,,context-switches,1868028,100.00,1.070,K/sec >>>> ------ >>>> 0.001813453,S0,1,85379,,instructions,1856754,100.00,0.32,insn per cycle >>>> ====> 0.001813453,,S0,1,,,,,,,1.34,stalled cycles per insn >>>> >>>> Above result also has an extra csv separator after >>>> the timestamp. Patch addresses extra field separator >>>> in the beginning of the metric output line. >>>> >>>> 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 new_line_csv function has check for "os->prefix" >>>> and if prefix is not null, it will be printed along >>>> with cvs separator. >>>> Snippet from "new_line_csv": >>>> if (os->prefix) >>>> fprintf(os->fh, "%s%s", os->prefix, config->csv_sep); >>>> >>>> Here os->prefix gets printed followed by "," >>>> which is the cvs separator. The os->prefix is >>>> used in interval mode option ( -I ), to print >>>> time stamp on every new line. But prefix is >>>> already set to contain csv separator when used >>>> in interval mode for csv option. >>>> >>>> Reference: Function "static void print_interval" >>>> Snippet: >>>> sprintf(prefix, "%6lu.%09lu%s", ts->tv_sec, ts->tv_nsec, config->csv_sep); >>>> >>>> Also if prefix is not assigned (if not used with >>>> -I option), it gets set to empty string. >>>> Reference: function printout() in util/stat-display.c >>>> Snippet: >>>> .prefix = prefix ? prefix : "", >>>> >>>> Since prefix already set to contain cvs_sep in interval >>>> option, patch removes printing config->csv_sep in >>>> new_line_csv function to avoid printing extra field. >>>> >>>> After the patch: >>>> >>>> # ./perf stat -x, --per-socket -a -C 1 ls >>>> S0,1,2.04,msec,cpu-clock,2045202,100.00,1.013,CPUs utilized >>>> S0,1,2,,context-switches,2041444,100.00,979.289,/sec >>>> S0,1,0,,cpu-migrations,2040820,100.00,0.000,/sec >>>> S0,1,2,,page-faults,2040288,100.00,979.289,/sec >>>> S0,1,254589,,cycles,2036066,100.00,0.125,GHz >>>> S0,1,82481,,stalled-cycles-frontend,2032420,100.00,32.40,frontend cycles idle >>>> S0,1,113170,,stalled-cycles-backend,2031722,100.00,44.45,backend cycles idle >>>> S0,1,88766,,instructions,2030942,100.00,0.35,insn per cycle >>>> S0,1,,,,,,,1.27,stalled cycles per insn >>>> >>>> Fixes: 92a61f6412d3 ("perf stat: Implement CSV metrics output") >>>> Reported-by: Disha Goel<disgoel@linux.vnet.ibm.com> >>>> Signed-off-by: Athira Rajeev<atrajeev@linux.vnet.ibm.com> perf stat csv test passed after applying the patch Tested-by: Disha Goel<disgoel@linux.vnet.ibm.com> >>> Hi All, >>> >>> Looking for review comments for this change. >> Hi, >> >> Thanks for addressing issues in this code. What is the status of the >> CSV output test following these changes? >> >> I think going forward we need to move away from CSV and columns, to >> something with structure like json. We also need to refactor this >> code, trying to add meaning to a newline character is a bad strategy >> and creates some unnatural things. To some extent this overlaps with >> Namhyung's aggregation cleanup. There are also weirdnesses in >> jevents/pmu-events, like the same ScaleUnit applying to a metric and >> an event - why are metrics even parts of events? >> >> Given the current code is wac-a-mole, and this is another whack, if >> the testing is okay I think we should move forward with this change. >> >> Thanks, >> Ian > Hi Ian, > > Thanks for checking the patch. > Yes, CSV output test passes with the change. > > perf stat CSV output linter : Ok > perf stat csv summary test : Ok > > Thanks > Athira > >> >> >> >>> Thanks >>> Athira >>> >>>> --- >>>> tools/perf/util/stat-display.c | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c >>>> index 5c47ee9963a7..879874a4bc07 100644 >>>> --- a/tools/perf/util/stat-display.c >>>> +++ b/tools/perf/util/stat-display.c >>>> @@ -273,7 +273,7 @@ static void new_line_csv(struct perf_stat_config *config, void *ctx) >>>> >>>> fputc('\n', os->fh); >>>> if (os->prefix) >>>> - fprintf(os->fh, "%s%s", os->prefix, config->csv_sep); >>>> + fprintf(os->fh, "%s", os->prefix); >>>> aggr_printout(config, os->evsel, os->id, os->nr); >>>> for (i = 0; i < os->nfields; i++) >>>> fputs(config->csv_sep, os->fh); >>>> -- >>>> 2.31.1 [-- Attachment #2: Type: text/html, Size: 7726 bytes --] ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/2] tools/perf: Fix printing os->prefix in CSV metrics output 2022-11-04 7:37 ` Disha Goel @ 2022-11-07 11:26 ` kajoljain 0 siblings, 0 replies; 17+ messages in thread From: kajoljain @ 2022-11-07 11:26 UTC (permalink / raw) To: Disha Goel, Athira Rajeev, Ian Rogers Cc: Andi Kleen, Nageswara Sastry, Arnaldo Carvalho de Melo, linux-perf-users, maddy, James Clark, Jiri Olsa, Namhyung Kim, linuxppc-dev On 11/4/22 13:07, Disha Goel wrote: > On 11/4/22 12: 59 PM, Athira Rajeev wrote: On 03-Nov-2022, at 9: 45 PM, > Ian Rogers <irogers@ google. com> wrote: On Wed, Nov 2, 2022 at 1: 36 AM > Athira Rajeev > > > > > > On 11/4/22 12:59 PM, Athira Rajeev wrote: >>> On 03-Nov-2022, at 9:45 PM, Ian Rogers <irogers@google.com> wrote: >>> >>> On Wed, Nov 2, 2022 at 1:36 AM Athira Rajeev >>> <atrajeev@linux.vnet.ibm.com> wrote: >>>>> On 18-Oct-2022, at 2:26 PM, Athira Rajeev <atrajeev@linux.vnet.ibm.com> wrote: >>>>> >>>>> Perf stat with CSV output option prints an extra empty >>>>> string as first field in metrics output line. >>>>> Sample output below: >>>>> >>>>> # ./perf stat -x, --per-socket -a -C 1 ls >>>>> S0,1,1.78,msec,cpu-clock,1785146,100.00,0.973,CPUs utilized >>>>> S0,1,26,,context-switches,1781750,100.00,0.015,M/sec >>>>> S0,1,1,,cpu-migrations,1780526,100.00,0.561,K/sec >>>>> S0,1,1,,page-faults,1779060,100.00,0.561,K/sec >>>>> S0,1,875807,,cycles,1769826,100.00,0.491,GHz >>>>> S0,1,85281,,stalled-cycles-frontend,1767512,100.00,9.74,frontend cycles idle >>>>> S0,1,576839,,stalled-cycles-backend,1766260,100.00,65.86,backend cycles idle >>>>> S0,1,288430,,instructions,1762246,100.00,0.33,insn per cycle >>>>> ====> ,S0,1,,,,,,,2.00,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 "," in the >>>>> beginning. >>>>> >>>>> Sample output using interval mode: >>>>> # ./perf stat -I 1000 -x, --per-socket -a -C 1 ls >>>>> 0.001813453,S0,1,1.87,msec,cpu-clock,1872052,100.00,0.002,CPUs utilized >>>>> 0.001813453,S0,1,2,,context-switches,1868028,100.00,1.070,K/sec >>>>> ------ >>>>> 0.001813453,S0,1,85379,,instructions,1856754,100.00,0.32,insn per cycle >>>>> ====> 0.001813453,,S0,1,,,,,,,1.34,stalled cycles per insn >>>>> >>>>> Above result also has an extra csv separator after >>>>> the timestamp. Patch addresses extra field separator >>>>> in the beginning of the metric output line. >>>>> >>>>> 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 new_line_csv function has check for "os->prefix" >>>>> and if prefix is not null, it will be printed along >>>>> with cvs separator. >>>>> Snippet from "new_line_csv": >>>>> if (os->prefix) >>>>> fprintf(os->fh, "%s%s", os->prefix, config->csv_sep); >>>>> >>>>> Here os->prefix gets printed followed by "," >>>>> which is the cvs separator. The os->prefix is >>>>> used in interval mode option ( -I ), to print >>>>> time stamp on every new line. But prefix is >>>>> already set to contain csv separator when used >>>>> in interval mode for csv option. >>>>> >>>>> Reference: Function "static void print_interval" >>>>> Snippet: >>>>> sprintf(prefix, "%6lu.%09lu%s", ts->tv_sec, ts->tv_nsec, config->csv_sep); >>>>> >>>>> Also if prefix is not assigned (if not used with >>>>> -I option), it gets set to empty string. >>>>> Reference: function printout() in util/stat-display.c >>>>> Snippet: >>>>> .prefix = prefix ? prefix : "", >>>>> >>>>> Since prefix already set to contain cvs_sep in interval >>>>> option, patch removes printing config->csv_sep in >>>>> new_line_csv function to avoid printing extra field. >>>>> >>>>> After the patch: >>>>> >>>>> # ./perf stat -x, --per-socket -a -C 1 ls >>>>> S0,1,2.04,msec,cpu-clock,2045202,100.00,1.013,CPUs utilized >>>>> S0,1,2,,context-switches,2041444,100.00,979.289,/sec >>>>> S0,1,0,,cpu-migrations,2040820,100.00,0.000,/sec >>>>> S0,1,2,,page-faults,2040288,100.00,979.289,/sec >>>>> S0,1,254589,,cycles,2036066,100.00,0.125,GHz >>>>> S0,1,82481,,stalled-cycles-frontend,2032420,100.00,32.40,frontend cycles idle >>>>> S0,1,113170,,stalled-cycles-backend,2031722,100.00,44.45,backend cycles idle >>>>> S0,1,88766,,instructions,2030942,100.00,0.35,insn per cycle >>>>> S0,1,,,,,,,1.27,stalled cycles per insn >>>>> >>>>> Fixes: 92a61f6412d3 ("perf stat: Implement CSV metrics output") >>>>> Reported-by: Disha Goel <disgoel@linux.vnet.ibm.com> >>>>> Signed-off-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com> > > perf stat csv test passed after applying the patch > Tested-by: Disha Goel <disgoel@linux.vnet.ibm.com> Hi, Patch looks fine to me. Reviewed-By: Kajol Jain <kjain@linux.ibm.com> Thanks, Kajol Jain > >>>> Hi All, >>>> >>>> Looking for review comments for this change. >>> Hi, >>> >>> Thanks for addressing issues in this code. What is the status of the >>> CSV output test following these changes? >>> >>> I think going forward we need to move away from CSV and columns, to >>> something with structure like json. We also need to refactor this >>> code, trying to add meaning to a newline character is a bad strategy >>> and creates some unnatural things. To some extent this overlaps with >>> Namhyung's aggregation cleanup. There are also weirdnesses in >>> jevents/pmu-events, like the same ScaleUnit applying to a metric and >>> an event - why are metrics even parts of events? >>> >>> Given the current code is wac-a-mole, and this is another whack, if >>> the testing is okay I think we should move forward with this change. >>> >>> Thanks, >>> Ian >> Hi Ian, >> >> Thanks for checking the patch. >> Yes, CSV output test passes with the change. >> >> perf stat CSV output linter : Ok >> perf stat csv summary test : Ok >> >> Thanks >> Athira >> >>> >>>> Thanks >>>> Athira >>>> >>>>> --- >>>>> tools/perf/util/stat-display.c | 2 +- >>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>> >>>>> diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c >>>>> index 5c47ee9963a7..879874a4bc07 100644 >>>>> --- a/tools/perf/util/stat-display.c >>>>> +++ b/tools/perf/util/stat-display.c >>>>> @@ -273,7 +273,7 @@ static void new_line_csv(struct perf_stat_config *config, void *ctx) >>>>> >>>>> fputc('\n', os->fh); >>>>> if (os->prefix) >>>>> - fprintf(os->fh, "%s%s", os->prefix, config->csv_sep); >>>>> + fprintf(os->fh, "%s", os->prefix); >>>>> aggr_printout(config, os->evsel, os->id, os->nr); >>>>> for (i = 0; i < os->nfields; i++) >>>>> fputs(config->csv_sep, os->fh); >>>>> -- >>>>> 2.31.1 ^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2022-11-14 9:31 UTC | newest] Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-10-18 8:56 [PATCH 1/2] tools/perf: Fix printing os->prefix in CSV metrics output Athira Rajeev 2022-10-18 8:56 ` Athira Rajeev 2022-10-18 8:56 ` [PATCH 2/2] tools/perf: Fix printing field separator " Athira Rajeev 2022-10-18 8:56 ` Athira Rajeev 2022-11-02 8:37 ` Athira Rajeev 2022-11-02 8:37 ` Athira Rajeev 2022-11-08 20:57 ` Arnaldo Carvalho de Melo 2022-11-08 20:57 ` Arnaldo Carvalho de Melo 2022-11-09 10:23 ` Athira Rajeev 2022-11-14 9:30 ` Athira Rajeev 2022-11-02 8:36 ` [PATCH 1/2] tools/perf: Fix printing os->prefix " Athira Rajeev 2022-11-02 8:36 ` Athira Rajeev 2022-11-03 16:15 ` Ian Rogers 2022-11-03 16:15 ` Ian Rogers 2022-11-04 7:29 ` Athira Rajeev 2022-11-04 7:37 ` Disha Goel 2022-11-07 11:26 ` kajoljain
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.