Re: [PATCH 1/6] perf, tools, stat: Abstract stat metrics printing

[Jiri Olsa]

On Tue, Dec 22, 2015 at 02:16:30AM +0100, Andi Kleen wrote:
> > > - fprintf(out, "   ");
> > > + print_metric(ctxp, NULL, NULL, "insn per cycle", 0);
> > >   }
> > >   total = 
> > > avg_stats(_stalled_cycles_front_stats[ctx][cpu]);
> > >   total = max(total, 
> > > avg_stats(_stalled_cycles_back_stats[ctx][cpu]));
> > >  
> > > + out->new_line(ctxp);
> > >   if (total && avg) {
> > >   ratio = total / avg;
> > > - fprintf(out, "\n");
> > 
> > you haven't address my first comment in here 
> > http://marc.info/?l=linux-kernel=144662610723134=2
> 
> The new_line is always needed because stalled cycles is always printed.
> 
> The reason it is always printed is that metric-only needs to see all the
> metrics for its column headers. That's why there are else cases
> everywhere.

please notice extra line below - between instructions and cycles lines

[jolsa@krava perf]$ ./perf stat -e instructions,cycles  kill
kill: not enough arguments

 Performance counter stats for 'kill':

   769,784  instructions  #0.78  insn per cycle 



   990,795  cycles  



jirka
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/6] perf, tools, stat: Abstract stat metrics printing

[Andi Kleen]

> > -   fprintf(out, "   ");
> > +   print_metric(ctxp, NULL, NULL, "insn per cycle", 0);
> > }
> > total = 
> > avg_stats(_stalled_cycles_front_stats[ctx][cpu]);
> > total = max(total, 
> > avg_stats(_stalled_cycles_back_stats[ctx][cpu]));
> >  
> > +   out->new_line(ctxp);
> > if (total && avg) {
> > ratio = total / avg;
> > -   fprintf(out, "\n");
> 
> you haven't address my first comment in here 
> http://marc.info/?l=linux-kernel=144662610723134=2

The new_line is always needed because stalled cycles is always printed.

The reason it is always printed is that metric-only needs to see all the
metrics for its column headers. That's why there are else cases
everywhere.


-Andi
-- 
a...@linux.intel.com -- Speaking for myself only.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/6] perf, tools, stat: Abstract stat metrics printing

[Jiri Olsa]

On Mon, Dec 14, 2015 at 06:04:14PM -0800, Andi Kleen wrote:

SNIP

> -double avg, int cpu, enum aggr_mode aggr)
> +void perf_stat__print_shadow_stats(struct perf_evsel *evsel,
> +double avg, int cpu,
> +struct perf_stat_output_ctx *out)
>  {
> + void *ctxp = out->ctx;
> + print_metric_t print_metric = out->print_metric;
>   double total, ratio = 0.0, total2;
>   int ctx = evsel_context(evsel);
>  
> @@ -307,119 +302,145 @@ void perf_stat__print_shadow_stats(FILE *out, struct 
> perf_evsel *evsel,
>   total = avg_stats(_cycles_stats[ctx][cpu]);
>   if (total) {
>   ratio = avg / total;
> - fprintf(out, " #   %5.2f  insns per cycle", 
> ratio);
> + print_metric(ctxp, NULL, "%7.2f ",
> + "insn per cycle", ratio);
>   } else {
> - fprintf(out, "   ");
> + print_metric(ctxp, NULL, NULL, "insn per cycle", 0);
>   }
>   total = 
> avg_stats(_stalled_cycles_front_stats[ctx][cpu]);
>   total = max(total, 
> avg_stats(_stalled_cycles_back_stats[ctx][cpu]));
>  
> + out->new_line(ctxp);
>   if (total && avg) {
>   ratio = total / avg;
> - fprintf(out, "\n");

you haven't address my first comment in here 
http://marc.info/?l=linux-kernel=144662610723134=2

also please rebase to latest acme/perf/core
FYI there's been some stat changes, but your branch cleanly rebased for me

thanks,
jirka
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/6] perf, tools, stat: Abstract stat metrics printing

[Andi Kleen]

> > -   fprintf(out, "   ");
> > +   print_metric(ctxp, NULL, NULL, "insn per cycle", 0);
> > }
> > total = 
> > avg_stats(_stalled_cycles_front_stats[ctx][cpu]);
> > total = max(total, 
> > avg_stats(_stalled_cycles_back_stats[ctx][cpu]));
> >  
> > +   out->new_line(ctxp);
> > if (total && avg) {
> > ratio = total / avg;
> > -   fprintf(out, "\n");
> 
> you haven't address my first comment in here 
> http://marc.info/?l=linux-kernel=144662610723134=2

The new_line is always needed because stalled cycles is always printed.

The reason it is always printed is that metric-only needs to see all the
metrics for its column headers. That's why there are else cases
everywhere.


-Andi
-- 
a...@linux.intel.com -- Speaking for myself only.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/6] perf, tools, stat: Abstract stat metrics printing

[Jiri Olsa]

On Tue, Dec 22, 2015 at 02:16:30AM +0100, Andi Kleen wrote:
> > > - fprintf(out, "   ");
> > > + print_metric(ctxp, NULL, NULL, "insn per cycle", 0);
> > >   }
> > >   total = 
> > > avg_stats(_stalled_cycles_front_stats[ctx][cpu]);
> > >   total = max(total, 
> > > avg_stats(_stalled_cycles_back_stats[ctx][cpu]));
> > >  
> > > + out->new_line(ctxp);
> > >   if (total && avg) {
> > >   ratio = total / avg;
> > > - fprintf(out, "\n");
> > 
> > you haven't address my first comment in here 
> > http://marc.info/?l=linux-kernel=144662610723134=2
> 
> The new_line is always needed because stalled cycles is always printed.
> 
> The reason it is always printed is that metric-only needs to see all the
> metrics for its column headers. That's why there are else cases
> everywhere.

please notice extra line below - between instructions and cycles lines

[jolsa@krava perf]$ ./perf stat -e instructions,cycles  kill
kill: not enough arguments

 Performance counter stats for 'kill':

   769,784  instructions  #0.78  insn per cycle 



   990,795  cycles  



jirka
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/6] perf, tools, stat: Abstract stat metrics printing

[Jiri Olsa]

On Mon, Dec 14, 2015 at 06:04:14PM -0800, Andi Kleen wrote:

SNIP

> -double avg, int cpu, enum aggr_mode aggr)
> +void perf_stat__print_shadow_stats(struct perf_evsel *evsel,
> +double avg, int cpu,
> +struct perf_stat_output_ctx *out)
>  {
> + void *ctxp = out->ctx;
> + print_metric_t print_metric = out->print_metric;
>   double total, ratio = 0.0, total2;
>   int ctx = evsel_context(evsel);
>  
> @@ -307,119 +302,145 @@ void perf_stat__print_shadow_stats(FILE *out, struct 
> perf_evsel *evsel,
>   total = avg_stats(_cycles_stats[ctx][cpu]);
>   if (total) {
>   ratio = avg / total;
> - fprintf(out, " #   %5.2f  insns per cycle", 
> ratio);
> + print_metric(ctxp, NULL, "%7.2f ",
> + "insn per cycle", ratio);
>   } else {
> - fprintf(out, "   ");
> + print_metric(ctxp, NULL, NULL, "insn per cycle", 0);
>   }
>   total = 
> avg_stats(_stalled_cycles_front_stats[ctx][cpu]);
>   total = max(total, 
> avg_stats(_stalled_cycles_back_stats[ctx][cpu]));
>  
> + out->new_line(ctxp);
>   if (total && avg) {
>   ratio = total / avg;
> - fprintf(out, "\n");

you haven't address my first comment in here 
http://marc.info/?l=linux-kernel=144662610723134=2

also please rebase to latest acme/perf/core
FYI there's been some stat changes, but your branch cleanly rebased for me

thanks,
jirka
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 1/6] perf, tools, stat: Abstract stat metrics printing

[Andi Kleen]

From: Andi Kleen 

Abstract the printing of shadow metrics. Instead of every
metric calling fprintf directly and taking care of indentation,
use two call backs: one to print metrics and another to
start a new line.

This will allow adding metrics to CSV mode and also
using them for other purposes.

The computation of padding is now done in the central
callback, instead of every metric doing it manually.
This makes it easier to add new metrics.

v2: Refactor functions, printout now does more. Move
shadow printing. Improve fallback callbacks. Don't
use void * callback data.
v3: Remove unnecessary hunk. Add typedef for new_line
v4: Remove unnecessary hunk. Don't print metrics for CSV/interval
mode yet.  Move printout change to separate patch.
v5: Fix bisect bugs. Avoid bogus frontend cycles printing.
Fix indentation in different aggregation modes.
Signed-off-by: Andi Kleen 
---
 tools/perf/builtin-stat.c |  59 ++--
 tools/perf/util/stat-shadow.c | 211 +++---
 tools/perf/util/stat.h|  15 ++-
 3 files changed, 182 insertions(+), 103 deletions(-)

diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index 25a95f4..569db6b 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -507,6 +507,46 @@ static void aggr_printout(struct perf_evsel *evsel, int 
id, int nr)
}
 }
 
+struct outstate {
+   FILE *fh;
+};
+
+#define METRIC_LEN  35
+
+static void new_line_std(void *ctx)
+{
+   struct outstate *os = ctx;
+
+   fputc('\n', os->fh);
+   if (stat_config.aggr_mode == AGGR_NONE)
+   fprintf(os->fh, "");
+   if (stat_config.aggr_mode == AGGR_CORE)
+   fprintf(os->fh, "  ");
+   if (stat_config.aggr_mode == AGGR_SOCKET)
+   fprintf(os->fh, "");
+   fprintf(os->fh, " ");
+}
+
+static void print_metric_std(void *ctx, const char *color, const char *fmt,
+const char *unit, double val)
+{
+   struct outstate *os = ctx;
+   FILE *out = os->fh;
+   int n;
+
+   if (unit == NULL || fmt == NULL) {
+   fprintf(out, "%-*s", METRIC_LEN, "");
+   return;
+   }
+
+   n = fprintf(out, " # ");
+   if (color)
+   n += color_fprintf(out, color, fmt, val);
+   else
+   n += fprintf(out, fmt, val);
+   fprintf(out, " %-*s", METRIC_LEN - n - 1, unit);
+}
+
 static void nsec_printout(int id, int nr, struct perf_evsel *evsel, double avg)
 {
FILE *output = stat_config.output;
@@ -567,20 +607,27 @@ static void abs_printout(int id, int nr, struct 
perf_evsel *evsel, double avg)
 
 static void printout(int id, int nr, struct perf_evsel *counter, double uval)
 {
-   int cpu = cpu_map__id_to_cpu(id);
+   struct outstate os = { .fh = stat_config.output };
+   struct perf_stat_output_ctx out;
+   print_metric_t pm = print_metric_std;
+   void (*nl)(void *);
 
-   if (stat_config.aggr_mode == AGGR_GLOBAL)
-   cpu = 0;
+   nl = new_line_std;
 
if (nsec_counter(counter))
nsec_printout(id, nr, counter, uval);
else
abs_printout(id, nr, counter, uval);
 
+   out.print_metric = pm;
+   out.new_line = nl;
+   out.ctx = 
+
if (!csv_output && !stat_config.interval)
-   perf_stat__print_shadow_stats(stat_config.output, counter,
- uval, cpu,
- stat_config.aggr_mode);
+   perf_stat__print_shadow_stats(counter, uval,
+   stat_config.aggr_mode == AGGR_GLOBAL ? 0 :
+   cpu_map__id_to_cpu(id),
+   );
 }
 
 static void print_aggr(char *prefix)
diff --git a/tools/perf/util/stat-shadow.c b/tools/perf/util/stat-shadow.c
index 6ac0314..4d8f185 100644
--- a/tools/perf/util/stat-shadow.c
+++ b/tools/perf/util/stat-shadow.c
@@ -137,9 +137,10 @@ static const char *get_ratio_color(enum grc_type type, 
double ratio)
return color;
 }
 
-static void print_stalled_cycles_frontend(FILE *out, int cpu,
+static void print_stalled_cycles_frontend(int cpu,
  struct perf_evsel *evsel
- __maybe_unused, double avg)
+ __maybe_unused, double avg,
+ struct perf_stat_output_ctx *out)
 {
double total, ratio = 0.0;
const char *color;
@@ -152,14 +153,17 @@ static void print_stalled_cycles_frontend(FILE *out, int 
cpu,
 
color = get_ratio_color(GRC_STALLED_CYCLES_FE, ratio);
 
-   fprintf(out, " #  ");
-   color_fprintf(out, color, "%6.2f%%", ratio);
-   fprintf(out, " frontend cycles idle   ");
+   if (ratio)
+   

[PATCH 1/6] perf, tools, stat: Abstract stat metrics printing

[Andi Kleen]

From: Andi Kleen 

Abstract the printing of shadow metrics. Instead of every
metric calling fprintf directly and taking care of indentation,
use two call backs: one to print metrics and another to
start a new line.

This will allow adding metrics to CSV mode and also
using them for other purposes.

The computation of padding is now done in the central
callback, instead of every metric doing it manually.
This makes it easier to add new metrics.

v2: Refactor functions, printout now does more. Move
shadow printing. Improve fallback callbacks. Don't
use void * callback data.
v3: Remove unnecessary hunk. Add typedef for new_line
v4: Remove unnecessary hunk. Don't print metrics for CSV/interval
mode yet.  Move printout change to separate patch.
v5: Fix bisect bugs. Avoid bogus frontend cycles printing.
Fix indentation in different aggregation modes.
Signed-off-by: Andi Kleen 
---
 tools/perf/builtin-stat.c |  59 ++--
 tools/perf/util/stat-shadow.c | 211 +++---
 tools/perf/util/stat.h|  15 ++-
 3 files changed, 182 insertions(+), 103 deletions(-)

diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index 25a95f4..569db6b 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -507,6 +507,46 @@ static void aggr_printout(struct perf_evsel *evsel, int 
id, int nr)
}
 }
 
+struct outstate {
+   FILE *fh;
+};
+
+#define METRIC_LEN  35
+
+static void new_line_std(void *ctx)
+{
+   struct outstate *os = ctx;
+
+   fputc('\n', os->fh);
+   if (stat_config.aggr_mode == AGGR_NONE)
+   fprintf(os->fh, "");
+   if (stat_config.aggr_mode == AGGR_CORE)
+   fprintf(os->fh, "  ");
+   if (stat_config.aggr_mode == AGGR_SOCKET)
+   fprintf(os->fh, "");
+   fprintf(os->fh, " ");
+}
+
+static void print_metric_std(void *ctx, const char *color, const char *fmt,
+const char *unit, double val)
+{
+   struct outstate *os = ctx;
+   FILE *out = os->fh;
+   int n;
+
+   if (unit == NULL || fmt == NULL) {
+   fprintf(out, "%-*s", METRIC_LEN, "");
+   return;
+   }
+
+   n = fprintf(out, " # ");
+   if (color)
+   n += color_fprintf(out, color, fmt, val);
+   else
+   n += fprintf(out, fmt, val);
+   fprintf(out, " %-*s", METRIC_LEN - n - 1, unit);
+}
+
 static void nsec_printout(int id, int nr, struct perf_evsel *evsel, double avg)
 {
FILE *output = stat_config.output;
@@ -567,20 +607,27 @@ static void abs_printout(int id, int nr, struct 
perf_evsel *evsel, double avg)
 
 static void printout(int id, int nr, struct perf_evsel *counter, double uval)
 {
-   int cpu = cpu_map__id_to_cpu(id);
+   struct outstate os = { .fh = stat_config.output };
+   struct perf_stat_output_ctx out;
+   print_metric_t pm = print_metric_std;
+   void (*nl)(void *);
 
-   if (stat_config.aggr_mode == AGGR_GLOBAL)
-   cpu = 0;
+   nl = new_line_std;
 
if (nsec_counter(counter))
nsec_printout(id, nr, counter, uval);
else
abs_printout(id, nr, counter, uval);
 
+   out.print_metric = pm;
+   out.new_line = nl;
+   out.ctx = 
+
if (!csv_output && !stat_config.interval)
-   perf_stat__print_shadow_stats(stat_config.output, counter,
- uval, cpu,
- stat_config.aggr_mode);
+   perf_stat__print_shadow_stats(counter, uval,
+   stat_config.aggr_mode == AGGR_GLOBAL ? 0 :
+   cpu_map__id_to_cpu(id),
+   );
 }
 
 static void print_aggr(char *prefix)
diff --git a/tools/perf/util/stat-shadow.c b/tools/perf/util/stat-shadow.c
index 6ac0314..4d8f185 100644
--- a/tools/perf/util/stat-shadow.c
+++ b/tools/perf/util/stat-shadow.c
@@ -137,9 +137,10 @@ static const char *get_ratio_color(enum grc_type type, 
double ratio)
return color;
 }
 
-static void print_stalled_cycles_frontend(FILE *out, int cpu,
+static void print_stalled_cycles_frontend(int cpu,
  struct perf_evsel *evsel
- __maybe_unused, double avg)
+ __maybe_unused, double avg,
+ struct perf_stat_output_ctx *out)
 {
double total, ratio = 0.0;
const char *color;
@@ -152,14 +153,17 @@ static void print_stalled_cycles_frontend(FILE *out, int 
cpu,
 
color = get_ratio_color(GRC_STALLED_CYCLES_FE, ratio);
 
-   fprintf(out, " #  ");
-   color_fprintf(out, color, "%6.2f%%", ratio);
-   fprintf(out, " frontend cycles idle   ");
+