Re: [PATCH 3/4] perf stat: Add --metrics-file option

2020-05-13 Thread Ian Rogers 
On Wed, May 13, 2020 at 4:33 AM Jiri Olsa  wrote:
>
> On Wed, May 13, 2020 at 12:04:55AM -0700, Ian Rogers wrote:
>
> SNIP
>
> > > +METRICS FILE
> > > +
> > > +The file with metrics has following syntax:
> > > +
> > > +  NAME = EXPRESSION ;
> > > +  NAME = EXPRESSION ;
> > > +  ...
> > > +
> > > +where NAME is unique identifier of the metric, which is later used in
> > > +perf stat as -M option argument (see below).
> > > +
> > > +The EXPRESSION is the metric's formula with following grammar:
> > > +
> > > +  EXPR: EVENT
> > > +  EXPR: EXPR if EXPR else EXPR
> >
> > Not introduced by this patch, but this patch is exposing it as an API.
>
> yea, I was thinking about this and I think we will put a disclaimer in
> here that this is not an API and the interface can change.. it's really
> mostly intended to help out with running a custom metric which is not
> compiled in ... I don't want to be commited to support old API
>
> > This notion of if-else is really weird. For one thing there are no
> > comparison operators. The unit test doesn't really help:
> > ret |= test(, "1+1 if 3*4 else 0", 2);
> > What kind of comparison is "3*4"? If 0.0 causes the else clause then will 
> > -0.0?
> > A typical expression I see written in C is to give a ratio such:
> >   value = denom == 0 ? 0 : nom / denom;
> > I've worked around encoding this by extending expr.y locally.
>
> AFAICS it's used only with #SMT_on in the condition, aybe we could limit
> the condition only for #SMT_on term?
>
>
> >
> > > +  EXPR: NUMBER
> > > +  EXPR: EXPR | EXPR
> > > +  EXPR: EXPR & EXPR
> > > +  EXPR: EXPR ^ EXPR
> >
> > Again, it's odd that these cast the double to a long and then assign
> > the result back to a double.
>
> is this even used anywhere? perhaps it was added just to be complete

I don't believe they are used and checked with the pmu parsing test
that removing them doesn't cause any x86 expressions to break. I'd
prefer it if we could remove the unused operators and avoid
advertising here.

Thanks,
Ian

> SNIP
>
> > > +   2.002460174 0.8623.37 
> > > 0.86
> > > +   3.003969795 1.0323.93 
> > > 1.03
> > > +  ...
> >
> > A feature request would be to allow metrics in terms of other metrics,
> > not just events :-) For example, it is common to sum all cache
> > hit/miss events. It is laborious to copy that expression for hit rate,
> > miss rate, etc.
> >
> > Perhaps the expression parsing code should be folded into the event
> > parsing code.
>
> nice idea, but let's finish straighten up what we have first ;-)
>
> I'll try to go through all the fixes/tests you posted and let's
> get it in first
>
> thanks,
> jirka
>

Re: [PATCH 3/4] perf stat: Add --metrics-file option

2020-05-13 Thread Jiri Olsa 
On Wed, May 13, 2020 at 12:04:55AM -0700, Ian Rogers wrote:

SNIP

> > +METRICS FILE
> > +
> > +The file with metrics has following syntax:
> > +
> > +  NAME = EXPRESSION ;
> > +  NAME = EXPRESSION ;
> > +  ...
> > +
> > +where NAME is unique identifier of the metric, which is later used in
> > +perf stat as -M option argument (see below).
> > +
> > +The EXPRESSION is the metric's formula with following grammar:
> > +
> > +  EXPR: EVENT
> > +  EXPR: EXPR if EXPR else EXPR
> 
> Not introduced by this patch, but this patch is exposing it as an API.

yea, I was thinking about this and I think we will put a disclaimer in
here that this is not an API and the interface can change.. it's really
mostly intended to help out with running a custom metric which is not
compiled in ... I don't want to be commited to support old API

> This notion of if-else is really weird. For one thing there are no
> comparison operators. The unit test doesn't really help:
> ret |= test(, "1+1 if 3*4 else 0", 2);
> What kind of comparison is "3*4"? If 0.0 causes the else clause then will 
> -0.0?
> A typical expression I see written in C is to give a ratio such:
>   value = denom == 0 ? 0 : nom / denom;
> I've worked around encoding this by extending expr.y locally.

AFAICS it's used only with #SMT_on in the condition, aybe we could limit
the condition only for #SMT_on term?


> 
> > +  EXPR: NUMBER
> > +  EXPR: EXPR | EXPR
> > +  EXPR: EXPR & EXPR
> > +  EXPR: EXPR ^ EXPR
> 
> Again, it's odd that these cast the double to a long and then assign
> the result back to a double.

is this even used anywhere? perhaps it was added just to be complete

SNIP

> > +   2.002460174 0.8623.37   
> >   0.86
> > +   3.003969795 1.0323.93   
> >   1.03
> > +  ...
> 
> A feature request would be to allow metrics in terms of other metrics,
> not just events :-) For example, it is common to sum all cache
> hit/miss events. It is laborious to copy that expression for hit rate,
> miss rate, etc.
> 
> Perhaps the expression parsing code should be folded into the event
> parsing code.

nice idea, but let's finish straighten up what we have first ;-)

I'll try to go through all the fixes/tests you posted and let's
get it in first

thanks,
jirka

Re: [PATCH 3/4] perf stat: Add --metrics-file option

2020-05-13 Thread Ian Rogers 
On Mon, May 11, 2020 at 1:54 PM Jiri Olsa  wrote:
>
> Adding --metrics-file option that allows to specify metrics
> in the file.
>
> It's now possible to define metrics in file and use them like:
>
>   $ cat metrics
>   // IPC
>   mine1 = inst_retired.any / cpu_clk_unhalted.thread;
>
>   /* DECODED_ICACHE_UOPS% */
>   mine2 = 100 * (idq.dsb_uops / (idq.ms_uops + idq.mite_uops + idq.dsb_uops + 
> lsd.uops));
>
>   $ sudo perf stat --metrics-file ./metrics -M mine1,mine2 -a -I 1000 
> --metric-only
>   #   timemine1mine2
>1.000997184 0.4118.47
>2.002479737 0.5722.46
>3.003932935 0.4017.52
>   ...
>
> Signed-off-by: Jiri Olsa 
> ---
>  tools/perf/Documentation/perf-stat.txt | 77 ++
>  tools/perf/builtin-stat.c  |  7 ++-
>  tools/perf/util/metricgroup.c  | 69 ---
>  tools/perf/util/metricgroup.h  |  3 +-
>  tools/perf/util/stat.h |  1 +
>  5 files changed, 147 insertions(+), 10 deletions(-)
>
> diff --git a/tools/perf/Documentation/perf-stat.txt 
> b/tools/perf/Documentation/perf-stat.txt
> index 3fb5028aef08..9f0a646d719c 100644
> --- a/tools/perf/Documentation/perf-stat.txt
> +++ b/tools/perf/Documentation/perf-stat.txt
> @@ -266,6 +266,10 @@ For a group all metrics from the group are added.
>  The events from the metrics are automatically measured.
>  See perf list output for the possble metrics and metricgroups.
>
> +--metrics-file file::
> +Read metrics definitions from file in addition to compiled in metrics.
> +Please check the file's syntax details in METRICS FILE section below.
> +
>  -A::
>  --no-aggr::
>  Do not aggregate counts across all monitored CPUs.
> @@ -404,6 +408,79 @@ The fields are in this order:
>
>  Additional metrics may be printed with all earlier fields being empty.
>
> +METRICS FILE
> +
> +The file with metrics has following syntax:
> +
> +  NAME = EXPRESSION ;
> +  NAME = EXPRESSION ;
> +  ...
> +
> +where NAME is unique identifier of the metric, which is later used in
> +perf stat as -M option argument (see below).
> +
> +The EXPRESSION is the metric's formula with following grammar:
> +
> +  EXPR: EVENT
> +  EXPR: EXPR if EXPR else EXPR

Not introduced by this patch, but this patch is exposing it as an API.
This notion of if-else is really weird. For one thing there are no
comparison operators. The unit test doesn't really help:
ret |= test(, "1+1 if 3*4 else 0", 2);
What kind of comparison is "3*4"? If 0.0 causes the else clause then will -0.0?
A typical expression I see written in C is to give a ratio such:
  value = denom == 0 ? 0 : nom / denom;
I've worked around encoding this by extending expr.y locally.

> +  EXPR: NUMBER
> +  EXPR: EXPR | EXPR
> +  EXPR: EXPR & EXPR
> +  EXPR: EXPR ^ EXPR

Again, it's odd that these cast the double to a long and then assign
the result back to a double.

> +  EXPR: EXPR + EXPR
> +  EXPR: EXPR - EXPR
> +  EXPR: EXPR * EXPR
> +  EXPR: EXPR / EXPR
> +  EXPR: EXPR % EXPR
> +  EXPR: - EXPR
> +  EXPR: ( EXPR )
> +  EXPR: min( EXPR, EXPR )
> +  EXPR: max( EXPR, EXPR )
> +  EXPR: #smt_on
> +
> +where:
> +
> +  EVENT is monitored event name
> +  min returns smaller of 2 expressions
> +  max returns bigger of 2 expressions
> +  #smt_on returns true if SMT is enabled
> +
> +The metric's definition can be spread across multiple lines and it's finished
> +with ';' character.
> +
> +The syntax allows for C style comments:
> +
> +  // single line
> +  /* multiple
> +  lines */
> +
> +Metrics file example with 2 custom metrics mine1 and mine2:
> +
> +  $ cat metrics
> +  // IPC
> +  mine1 = inst_retired.any / cpu_clk_unhalted.thread;
> +
> +   /* DECODED_ICACHE_UOPS% */
> +  mine2 = 100 * (idq.dsb_uops / (idq.ms_uops + idq.mite_uops + idq.dsb_uops 
> + lsd.uops));
> +
> +  $ sudo perf stat --metrics-file ./metrics -M mine1,mine2 -a -I 1000 
> --metric-only
> +  #   timemine1mine2
> +   1.000997184 0.4118.47
> +   2.002479737 0.5722.46
> +   3.003932935 0.4017.52
> +  ...
> +
> +
> +It's possible to mix custom metrics with compiled-in metrics in one -M 
> argument:
> +
> +  $ sudo perf stat --metrics-file ./metrics -M mine1,mine2,IPC -a -I 1000 
> --metric-only
> +  #   timemine1mine2 
>  IPC
> +   1.001142007 0.8522.33 
> 0.85
> +   2.002460174 0.8623.37 
> 0.86
> +   3.003969795 1.0323.93 
> 1.03
> +  ...

A feature request would be to allow metrics in terms of other metrics,
not just events :-) For example, it is common to

[PATCH 3/4] perf stat: Add --metrics-file option

2020-05-11 Thread Jiri Olsa 
Adding --metrics-file option that allows to specify metrics
in the file.

It's now possible to define metrics in file and use them like:

  $ cat metrics
  // IPC
  mine1 = inst_retired.any / cpu_clk_unhalted.thread;

  /* DECODED_ICACHE_UOPS% */
  mine2 = 100 * (idq.dsb_uops / (idq.ms_uops + idq.mite_uops + idq.dsb_uops + 
lsd.uops));

  $ sudo perf stat --metrics-file ./metrics -M mine1,mine2 -a -I 1000 
--metric-only
  #   timemine1mine2
   1.000997184 0.4118.47
   2.002479737 0.5722.46
   3.003932935 0.4017.52
  ...

Signed-off-by: Jiri Olsa 
---
 tools/perf/Documentation/perf-stat.txt | 77 ++
 tools/perf/builtin-stat.c  |  7 ++-
 tools/perf/util/metricgroup.c  | 69 ---
 tools/perf/util/metricgroup.h  |  3 +-
 tools/perf/util/stat.h |  1 +
 5 files changed, 147 insertions(+), 10 deletions(-)

diff --git a/tools/perf/Documentation/perf-stat.txt 
b/tools/perf/Documentation/perf-stat.txt
index 3fb5028aef08..9f0a646d719c 100644
--- a/tools/perf/Documentation/perf-stat.txt
+++ b/tools/perf/Documentation/perf-stat.txt
@@ -266,6 +266,10 @@ For a group all metrics from the group are added.
 The events from the metrics are automatically measured.
 See perf list output for the possble metrics and metricgroups.
 
+--metrics-file file::
+Read metrics definitions from file in addition to compiled in metrics.
+Please check the file's syntax details in METRICS FILE section below.
+
 -A::
 --no-aggr::
 Do not aggregate counts across all monitored CPUs.
@@ -404,6 +408,79 @@ The fields are in this order:
 
 Additional metrics may be printed with all earlier fields being empty.
 
+METRICS FILE
+
+The file with metrics has following syntax:
+
+  NAME = EXPRESSION ;
+  NAME = EXPRESSION ;
+  ...
+
+where NAME is unique identifier of the metric, which is later used in
+perf stat as -M option argument (see below).
+
+The EXPRESSION is the metric's formula with following grammar:
+
+  EXPR: EVENT
+  EXPR: EXPR if EXPR else EXPR
+  EXPR: NUMBER
+  EXPR: EXPR | EXPR
+  EXPR: EXPR & EXPR
+  EXPR: EXPR ^ EXPR
+  EXPR: EXPR + EXPR
+  EXPR: EXPR - EXPR
+  EXPR: EXPR * EXPR
+  EXPR: EXPR / EXPR
+  EXPR: EXPR % EXPR
+  EXPR: - EXPR
+  EXPR: ( EXPR )
+  EXPR: min( EXPR, EXPR )
+  EXPR: max( EXPR, EXPR )
+  EXPR: #smt_on
+
+where:
+
+  EVENT is monitored event name
+  min returns smaller of 2 expressions
+  max returns bigger of 2 expressions
+  #smt_on returns true if SMT is enabled
+
+The metric's definition can be spread across multiple lines and it's finished
+with ';' character.
+
+The syntax allows for C style comments:
+
+  // single line
+  /* multiple
+  lines */
+
+Metrics file example with 2 custom metrics mine1 and mine2:
+
+  $ cat metrics
+  // IPC
+  mine1 = inst_retired.any / cpu_clk_unhalted.thread;
+
+   /* DECODED_ICACHE_UOPS% */
+  mine2 = 100 * (idq.dsb_uops / (idq.ms_uops + idq.mite_uops + idq.dsb_uops + 
lsd.uops));
+
+  $ sudo perf stat --metrics-file ./metrics -M mine1,mine2 -a -I 1000 
--metric-only
+  #   timemine1mine2
+   1.000997184 0.4118.47
+   2.002479737 0.5722.46
+   3.003932935 0.4017.52
+  ...
+
+
+It's possible to mix custom metrics with compiled-in metrics in one -M 
argument:
+
+  $ sudo perf stat --metrics-file ./metrics -M mine1,mine2,IPC -a -I 1000 
--metric-only
+  #   timemine1mine2  
IPC
+   1.001142007 0.8522.33 
0.85
+   2.002460174 0.8623.37 
0.86
+   3.003969795 1.0323.93 
1.03
+  ...
+
+
 SEE ALSO
 
 linkperf:perf-top[1], linkperf:perf-list[1]
diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index e0c1ad23c768..5eda298654a8 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -840,7 +840,8 @@ static int parse_metric_groups(const struct option *opt,
   const char *str,
   int unset __maybe_unused)
 {
-   return metricgroup__parse_groups(opt, str, _config.metric_events);
+   return metricgroup__parse_groups(opt, str, _config.metric_events,
+stat_config.metrics_file);
 }
 
 static struct option stat_options[] = {
@@ -925,6 +926,8 @@ static struct option stat_options[] = {
OPT_CALLBACK('M', "metrics", _list, "metric/metric group list",
 "monitor specified metrics or metric groups (separated by 
,)",
 parse_metric_groups),
+   OPT_STRING(0, "metrics-file", _config.metrics_file,