[PATCH 1/2] perf, tools, json: Fix ILP metrics

[PATCH 1/2] perf, tools, json: Fix ILP metrics

[Andi Kleen]

From: Andi Kleen <ak@linux.intel.com>

- Add missing brackets in the ILP metric definitions that lead
to impossible high ILP when Hyper Threading was disabled.

- Always spell events in upper case, needed for the next patch.

Signed-off-by: Andi Kleen <ak@linux.intel.com>
---
 tools/perf/pmu-events/arch/x86/broadwell/bdw-metrics.json     | 2 +-
 tools/perf/pmu-events/arch/x86/broadwellde/bdwde-metrics.json | 2 +-
 tools/perf/pmu-events/arch/x86/broadwellx/bdx-metrics.json    | 2 +-
 tools/perf/pmu-events/arch/x86/ivybridge/ivb-metrics.json     | 2 +-
 tools/perf/pmu-events/arch/x86/ivytown/ivt-metrics.json       | 2 +-
 tools/perf/pmu-events/arch/x86/jaketown/jkt-metrics.json      | 2 +-
 tools/perf/pmu-events/arch/x86/sandybridge/snb-metrics.json   | 2 +-
 7 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/tools/perf/pmu-events/arch/x86/broadwell/bdw-metrics.json b/tools/perf/pmu-events/arch/x86/broadwell/bdw-metrics.json
index 49c5f123d811..d2c78c302a71 100644
--- a/tools/perf/pmu-events/arch/x86/broadwell/bdw-metrics.json
+++ b/tools/perf/pmu-events/arch/x86/broadwell/bdw-metrics.json
@@ -55,7 +55,7 @@
     },
     {
         "BriefDescription": "Instruction-Level-Parallelism (average number of uops executed when there is at least 1 uop executed)",
-        "MetricExpr": "UOPS_EXECUTED.THREAD / ( cpu@uops_executed.core\\,cmask\\=1@ / 2) if #SMT_on else UOPS_EXECUTED.CYCLES_GE_1_UOP_EXEC",
+        "MetricExpr": "UOPS_EXECUTED.THREAD / ( ( cpu@uops_executed.core\\,cmask\\=1@ / 2) if #SMT_on else UOPS_EXECUTED.CYCLES_GE_1_UOP_EXEC)",
         "MetricGroup": "Pipeline;Ports_Utilization",
         "MetricName": "ILP"
     },
diff --git a/tools/perf/pmu-events/arch/x86/broadwellde/bdwde-metrics.json b/tools/perf/pmu-events/arch/x86/broadwellde/bdwde-metrics.json
index 49c5f123d811..804abe02b7ef 100644
--- a/tools/perf/pmu-events/arch/x86/broadwellde/bdwde-metrics.json
+++ b/tools/perf/pmu-events/arch/x86/broadwellde/bdwde-metrics.json
@@ -55,7 +55,7 @@
     },
     {
         "BriefDescription": "Instruction-Level-Parallelism (average number of uops executed when there is at least 1 uop executed)",
-        "MetricExpr": "UOPS_EXECUTED.THREAD / ( cpu@uops_executed.core\\,cmask\\=1@ / 2) if #SMT_on else UOPS_EXECUTED.CYCLES_GE_1_UOP_EXEC",
+        "MetricExpr": "UOPS_EXECUTED.THREAD / ( ( cpu@uops_executed.core\\,cmask\\=1@ / 2) if #SMT_on else UOPS_EXECUTED.CYCLES_GE_1_UOP_EXEC )",
         "MetricGroup": "Pipeline;Ports_Utilization",
         "MetricName": "ILP"
     },
diff --git a/tools/perf/pmu-events/arch/x86/broadwellx/bdx-metrics.json b/tools/perf/pmu-events/arch/x86/broadwellx/bdx-metrics.json
index 4248b029d199..f831673ddc4c 100644
--- a/tools/perf/pmu-events/arch/x86/broadwellx/bdx-metrics.json
+++ b/tools/perf/pmu-events/arch/x86/broadwellx/bdx-metrics.json
@@ -55,7 +55,7 @@
     },
     {
         "BriefDescription": "Instruction-Level-Parallelism (average number of uops executed when there is at least 1 uop executed)",
-        "MetricExpr": "UOPS_EXECUTED.THREAD / ( cpu@uops_executed.core\\,cmask\\=1@ / 2) if #SMT_on else UOPS_EXECUTED.CYCLES_GE_1_UOP_EXEC",
+        "MetricExpr": "UOPS_EXECUTED.THREAD / (( cpu@uops_executed.core\\,cmask\\=1@ / 2) if #SMT_on else UOPS_EXECUTED.CYCLES_GE_1_UOP_EXEC)",
         "MetricGroup": "Pipeline;Ports_Utilization",
         "MetricName": "ILP"
     },
diff --git a/tools/perf/pmu-events/arch/x86/ivybridge/ivb-metrics.json b/tools/perf/pmu-events/arch/x86/ivybridge/ivb-metrics.json
index 057a832f1a6a..19cf34686355 100644
--- a/tools/perf/pmu-events/arch/x86/ivybridge/ivb-metrics.json
+++ b/tools/perf/pmu-events/arch/x86/ivybridge/ivb-metrics.json
@@ -55,7 +55,7 @@
     },
     {
         "BriefDescription": "Instruction-Level-Parallelism (average number of uops executed when there is at least 1 uop executed)",
-        "MetricExpr": "UOPS_EXECUTED.THREAD / ( cpu@uops_executed.core\\,cmask\\=1@ / 2) if #SMT_on else UOPS_EXECUTED.CYCLES_GE_1_UOP_EXEC",
+        "MetricExpr": "UOPS_EXECUTED.THREAD / (( cpu@UOPS_EXECUTED.CORE\\,cmask\\=1@ / 2) if #SMT_on else UOPS_EXECUTED.CYCLES_GE_1_UOP_EXEC)",
         "MetricGroup": "Pipeline;Ports_Utilization",
         "MetricName": "ILP"
     },
diff --git a/tools/perf/pmu-events/arch/x86/ivytown/ivt-metrics.json b/tools/perf/pmu-events/arch/x86/ivytown/ivt-metrics.json
index 057a832f1a6a..1d8a8fef90f2 100644
--- a/tools/perf/pmu-events/arch/x86/ivytown/ivt-metrics.json
+++ b/tools/perf/pmu-events/arch/x86/ivytown/ivt-metrics.json
@@ -55,7 +55,7 @@
     },
     {
         "BriefDescription": "Instruction-Level-Parallelism (average number of uops executed when there is at least 1 uop executed)",
-        "MetricExpr": "UOPS_EXECUTED.THREAD / ( cpu@uops_executed.core\\,cmask\\=1@ / 2) if #SMT_on else UOPS_EXECUTED.CYCLES_GE_1_UOP_EXEC",
+        "MetricExpr": "UOPS_EXECUTED.THREAD / ( ( cpu@UOPS_EXECUTED.CORE\\,cmask\\=1@ / 2) if #SMT_on else UOPS_EXECUTED.CYCLES_GE_1_UOP_EXEC )",
         "MetricGroup": "Pipeline;Ports_Utilization",
         "MetricName": "ILP"
     },
diff --git a/tools/perf/pmu-events/arch/x86/jaketown/jkt-metrics.json b/tools/perf/pmu-events/arch/x86/jaketown/jkt-metrics.json
index b35b1c153c8a..4ba75525852d 100644
--- a/tools/perf/pmu-events/arch/x86/jaketown/jkt-metrics.json
+++ b/tools/perf/pmu-events/arch/x86/jaketown/jkt-metrics.json
@@ -55,7 +55,7 @@
     },
     {
         "BriefDescription": "Instruction-Level-Parallelism (average number of uops executed when there is at least 1 uop executed)",
-	"MetricExpr": "UOPS_DISPATCHED.THREAD / ( cpu@UOPS_DISPATCHED.CORE\\,cmask\\=1@ / 2) if #SMT_on else cpu@UOPS_DISPATCHED.CORE\\,cmask\\=1@",
+	"MetricExpr": "UOPS_DISPATCHED.THREAD / (( cpu@UOPS_DISPATCHED.CORE\\,cmask\\=1@ / 2) if #SMT_on else cpu@UOPS_DISPATCHED.CORE\\,cmask\\=1@)",
         "MetricGroup": "Pipeline;Ports_Utilization",
         "MetricName": "ILP"
     },
diff --git a/tools/perf/pmu-events/arch/x86/sandybridge/snb-metrics.json b/tools/perf/pmu-events/arch/x86/sandybridge/snb-metrics.json
index b35b1c153c8a..4ba75525852d 100644
--- a/tools/perf/pmu-events/arch/x86/sandybridge/snb-metrics.json
+++ b/tools/perf/pmu-events/arch/x86/sandybridge/snb-metrics.json
@@ -55,7 +55,7 @@
     },
     {
         "BriefDescription": "Instruction-Level-Parallelism (average number of uops executed when there is at least 1 uop executed)",
-	"MetricExpr": "UOPS_DISPATCHED.THREAD / ( cpu@UOPS_DISPATCHED.CORE\\,cmask\\=1@ / 2) if #SMT_on else cpu@UOPS_DISPATCHED.CORE\\,cmask\\=1@",
+	"MetricExpr": "UOPS_DISPATCHED.THREAD / (( cpu@UOPS_DISPATCHED.CORE\\,cmask\\=1@ / 2) if #SMT_on else cpu@UOPS_DISPATCHED.CORE\\,cmask\\=1@)",
         "MetricGroup": "Pipeline;Ports_Utilization",
         "MetricName": "ILP"
     },
-- 
2.9.5

[PATCH 2/2] perf, tools: Don't force MetricExprs to lower case

[Andi Kleen]

From: Andi Kleen <ak@linux.intel.com>

There are still problems with BPF misinterpreting some events
that include .c. An earlier fix made it work for stand alone
aliases, but it still fails for more complex constructs.

REJECT keeps trying and trying a shorter string until
.c is matched and it appears like a valid BPF path.

% perf stat -e cpu/uops_executed.core,cmask=1/ true
bpf: builtin compilation failed: -95, try external compiler
ERROR: problems with path cpu/uops_executed.c: No such file or directory
event syntax error: 'cpu/uops_executed.core,cmask=1/'
                     \___ Failed to load cpu/uops_executed.c from source: Error when compiling BPF scriptlet

I tried to fix it, but it exceeds my flex knowledge, because
REJECT does not interact well with BEGIN states.

The BPF syntax in its current form really causes an ambigious
grammar.

A workaround for this is to spell the event upper case,
because upper case .C does not match the BPF rules.

This currently affects some of the ILP metrics. The previous
patch changed the ILP metrics to use upper case. Now if we
don't force everything to lower case the matching works.

This makes ILP work correctly on my IvyBridge:

%  ./perf stat -M ILP true

 Performance counter stats for 'true':

           711,067      UOPS_EXECUTED.THREAD      #      2.3 ILP
           311,680      cpu/UOPS_EXECUTED.CORE,cmask=1/
           311,680      UOPS_EXECUTED.CYCLES_GE_1_UOP_EXEC

       0.000371554 seconds time elapsed

Signed-off-by: Andi Kleen <ak@linux.intel.com>
---
 tools/perf/pmu-events/jevents.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/tools/perf/pmu-events/jevents.c b/tools/perf/pmu-events/jevents.c
index 9eb7047bafe4..07c0f9a46180 100644
--- a/tools/perf/pmu-events/jevents.c
+++ b/tools/perf/pmu-events/jevents.c
@@ -488,8 +488,6 @@ int json_events(const char *fn,
 				addfield(map, &metric_group, "", "", val);
 			} else if (json_streq(map, field, "MetricExpr")) {
 				addfield(map, &metric_expr, "", "", val);
-				for (s = metric_expr; *s; s++)
-					*s = tolower(*s);
 			}
 			/* ignore unknown fields */
 		}
-- 
2.9.5

Re: [PATCH 2/2] perf, tools: Don't force MetricExprs to lower case

[Arnaldo Carvalho de Melo]

Em Tue, Sep 12, 2017 at 12:56:43PM -0700, Andi Kleen escreveu:
> From: Andi Kleen <ak@linux.intel.com>
> 
> There are still problems with BPF misinterpreting some events
> that include .c. An earlier fix made it work for stand alone
> aliases, but it still fails for more complex constructs.

Hi Wang, Jiri,

	Can you please take a look at this and see if there is something
we can do to help Andi?

- Arnaldo
 
> REJECT keeps trying and trying a shorter string until
> .c is matched and it appears like a valid BPF path.
> 
> % perf stat -e cpu/uops_executed.core,cmask=1/ true
> bpf: builtin compilation failed: -95, try external compiler
> ERROR: problems with path cpu/uops_executed.c: No such file or directory
> event syntax error: 'cpu/uops_executed.core,cmask=1/'
>                      \___ Failed to load cpu/uops_executed.c from source: Error when compiling BPF scriptlet
> 
> I tried to fix it, but it exceeds my flex knowledge, because
> REJECT does not interact well with BEGIN states.
> 
> The BPF syntax in its current form really causes an ambigious
> grammar.
> 
> A workaround for this is to spell the event upper case,
> because upper case .C does not match the BPF rules.
> 
> This currently affects some of the ILP metrics. The previous
> patch changed the ILP metrics to use upper case. Now if we
> don't force everything to lower case the matching works.
> 
> This makes ILP work correctly on my IvyBridge:
> 
> %  ./perf stat -M ILP true
> 
>  Performance counter stats for 'true':
> 
>            711,067      UOPS_EXECUTED.THREAD      #      2.3 ILP
>            311,680      cpu/UOPS_EXECUTED.CORE,cmask=1/
>            311,680      UOPS_EXECUTED.CYCLES_GE_1_UOP_EXEC
> 
>        0.000371554 seconds time elapsed
> 
> Signed-off-by: Andi Kleen <ak@linux.intel.com>
> ---
>  tools/perf/pmu-events/jevents.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/tools/perf/pmu-events/jevents.c b/tools/perf/pmu-events/jevents.c
> index 9eb7047bafe4..07c0f9a46180 100644
> --- a/tools/perf/pmu-events/jevents.c
> +++ b/tools/perf/pmu-events/jevents.c
> @@ -488,8 +488,6 @@ int json_events(const char *fn,
>  				addfield(map, &metric_group, "", "", val);
>  			} else if (json_streq(map, field, "MetricExpr")) {
>  				addfield(map, &metric_expr, "", "", val);
> -				for (s = metric_expr; *s; s++)
> -					*s = tolower(*s);
>  			}
>  			/* ignore unknown fields */
>  		}
> -- 
> 2.9.5

Re: [PATCH 2/2] perf, tools: Don't force MetricExprs to lower case

[Jiri Olsa]

On Tue, Oct 03, 2017 at 01:06:05PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Tue, Sep 12, 2017 at 12:56:43PM -0700, Andi Kleen escreveu:
> > From: Andi Kleen <ak@linux.intel.com>
> > 
> > There are still problems with BPF misinterpreting some events
> > that include .c. An earlier fix made it work for stand alone
> > aliases, but it still fails for more complex constructs.
> 
> Hi Wang, Jiri,
> 
> 	Can you please take a look at this and see if there is something
> we can do to help Andi?
> 
> - Arnaldo
>  
> > REJECT keeps trying and trying a shorter string until
> > .c is matched and it appears like a valid BPF path.
> > 
> > % perf stat -e cpu/uops_executed.core,cmask=1/ true
> > bpf: builtin compilation failed: -95, try external compiler
> > ERROR: problems with path cpu/uops_executed.c: No such file or directory
> > event syntax error: 'cpu/uops_executed.core,cmask=1/'
> >                      \___ Failed to load cpu/uops_executed.c from source: Error when compiling BPF scriptlet
> > 
> > I tried to fix it, but it exceeds my flex knowledge, because
> > REJECT does not interact well with BEGIN states.
> > 
> > The BPF syntax in its current form really causes an ambigious
> > grammar.

right, it looks like we allow whole path (including / char)
for BPF file, which messes up with out pmu/.../ syntax

do we need that? (Cc-ed some bpf folks)

if not attached patch seems to fix things.. otherwise
we need to come up with another fix

thanks,
jirka


---
diff --git a/tools/perf/util/parse-events.l b/tools/perf/util/parse-events.l
index ea2426daf7e8..e3c602f4bbbf 100644
--- a/tools/perf/util/parse-events.l
+++ b/tools/perf/util/parse-events.l
@@ -151,8 +151,8 @@ do {							\
 group		[^,{}/]*[{][^}]*[}][^,{}/]*
 event_pmu	[^,{}/]+[/][^/]*[/][^,{}/]*
 event		[^,{}/]+
-bpf_object	[^,{}]+\.(o|bpf)[a-zA-Z0-9._]*
-bpf_source	[^,{}]+\.c[a-zA-Z0-9._]*
+bpf_object	[^,{}/]+\.(o|bpf)[a-zA-Z0-9._]*
+bpf_source	[^,{}/]+\.c[a-zA-Z0-9._]*
 
 num_dec		[0-9]+
 num_hex		0x[a-fA-F0-9]+

Re: [PATCH 2/2] perf, tools: Don't force MetricExprs to lower case

[Andi Kleen]

On Wed, Oct 04, 2017 at 12:30:52PM +0200, Jiri Olsa wrote:
> On Tue, Oct 03, 2017 at 01:06:05PM -0300, Arnaldo Carvalho de Melo wrote:
> > Em Tue, Sep 12, 2017 at 12:56:43PM -0700, Andi Kleen escreveu:
> > > From: Andi Kleen <ak@linux.intel.com>
> > > 
> > > There are still problems with BPF misinterpreting some events
> > > that include .c. An earlier fix made it work for stand alone
> > > aliases, but it still fails for more complex constructs.
> > 
> > Hi Wang, Jiri,
> > 
> > 	Can you please take a look at this and see if there is something
> > we can do to help Andi?
> > 
> > - Arnaldo
> >  
> > > REJECT keeps trying and trying a shorter string until
> > > .c is matched and it appears like a valid BPF path.
> > > 
> > > % perf stat -e cpu/uops_executed.core,cmask=1/ true
> > > bpf: builtin compilation failed: -95, try external compiler
> > > ERROR: problems with path cpu/uops_executed.c: No such file or directory
> > > event syntax error: 'cpu/uops_executed.core,cmask=1/'
> > >                      \___ Failed to load cpu/uops_executed.c from source: Error when compiling BPF scriptlet
> > > 
> > > I tried to fix it, but it exceeds my flex knowledge, because
> > > REJECT does not interact well with BEGIN states.
> > > 
> > > The BPF syntax in its current form really causes an ambigious
> > > grammar.
> 
> right, it looks like we allow whole path (including / char)
> for BPF file, which messes up with out pmu/.../ syntax
> 
> do we need that? (Cc-ed some bpf folks)
> 
> if not attached patch seems to fix things.. otherwise
> we need to come up with another fix

I tried similar patches, but I always ran into more complex
situations where it still matched incorrectly.

e.g. try it with cpu/uops_executed.core,... vs uops_executed.core

The only real fix would be probably to add some unique 
prefix for BPF, but that would break all existing users.

-Andi

Re: [PATCH 2/2] perf, tools: Don't force MetricExprs to lower case

[Jiri Olsa]

On Wed, Oct 04, 2017 at 09:27:11AM -0700, Andi Kleen wrote:
> On Wed, Oct 04, 2017 at 12:30:52PM +0200, Jiri Olsa wrote:
> > On Tue, Oct 03, 2017 at 01:06:05PM -0300, Arnaldo Carvalho de Melo wrote:
> > > Em Tue, Sep 12, 2017 at 12:56:43PM -0700, Andi Kleen escreveu:
> > > > From: Andi Kleen <ak@linux.intel.com>
> > > > 
> > > > There are still problems with BPF misinterpreting some events
> > > > that include .c. An earlier fix made it work for stand alone
> > > > aliases, but it still fails for more complex constructs.
> > > 
> > > Hi Wang, Jiri,
> > > 
> > > 	Can you please take a look at this and see if there is something
> > > we can do to help Andi?
> > > 
> > > - Arnaldo
> > >  
> > > > REJECT keeps trying and trying a shorter string until
> > > > .c is matched and it appears like a valid BPF path.
> > > > 
> > > > % perf stat -e cpu/uops_executed.core,cmask=1/ true
> > > > bpf: builtin compilation failed: -95, try external compiler
> > > > ERROR: problems with path cpu/uops_executed.c: No such file or directory
> > > > event syntax error: 'cpu/uops_executed.core,cmask=1/'
> > > >                      \___ Failed to load cpu/uops_executed.c from source: Error when compiling BPF scriptlet
> > > > 
> > > > I tried to fix it, but it exceeds my flex knowledge, because
> > > > REJECT does not interact well with BEGIN states.
> > > > 
> > > > The BPF syntax in its current form really causes an ambigious
> > > > grammar.
> > 
> > right, it looks like we allow whole path (including / char)
> > for BPF file, which messes up with out pmu/.../ syntax
> > 
> > do we need that? (Cc-ed some bpf folks)
> > 
> > if not attached patch seems to fix things.. otherwise
> > we need to come up with another fix
> 
> I tried similar patches, but I always ran into more complex
> situations where it still matched incorrectly.
> 
> e.g. try it with cpu/uops_executed.core,... vs uops_executed.core

hm, both works for me with the change:

  perf stat -e cpu/uops_executed.core/ ls
  perf stat -e uops_executed.core ls

> The only real fix would be probably to add some unique 
> prefix for BPF, but that would break all existing users.


yea, there was no response from bpf folks, but it's  probably not an option

how about checking if the file exist like below..

jirka


---
diff --git a/tools/perf/util/parse-events.l b/tools/perf/util/parse-events.l
index ea2426daf7e8..38a42bdf1492 100644
--- a/tools/perf/util/parse-events.l
+++ b/tools/perf/util/parse-events.l
@@ -8,6 +8,9 @@
 
 %{
 #include <errno.h>
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <unistd.h>
 #include "../perf.h"
 #include "parse-events.h"
 #include "parse-events-bison.h"
@@ -53,9 +56,8 @@ static int str(yyscan_t scanner, int token)
 	return token;
 }
 
-static bool isbpf(yyscan_t scanner)
+static bool isbpf_suffix(char *text)
 {
-	char *text = parse_events_get_text(scanner);
 	int len = strlen(text);
 
 	if (len < 2)
@@ -68,6 +70,17 @@ static bool isbpf(yyscan_t scanner)
 	return false;
 }
 
+static bool isbpf(yyscan_t scanner)
+{
+	char *text = parse_events_get_text(scanner);
+	struct stat st;
+
+	if (!isbpf_suffix(text))
+		return false;
+
+	return stat(text, &st) == 0;
+}
+
 /*
  * This function is called when the parser gets two kind of input:
  *

Re: [PATCH 2/2] perf, tools: Don't force MetricExprs to lower case

[Andi Kleen]

On Mon, Oct 09, 2017 at 03:41:51PM +0200, Jiri Olsa wrote:
> On Wed, Oct 04, 2017 at 09:27:11AM -0700, Andi Kleen wrote:
> > On Wed, Oct 04, 2017 at 12:30:52PM +0200, Jiri Olsa wrote:
> > > On Tue, Oct 03, 2017 at 01:06:05PM -0300, Arnaldo Carvalho de Melo wrote:
> > > > Em Tue, Sep 12, 2017 at 12:56:43PM -0700, Andi Kleen escreveu:
> > > > > From: Andi Kleen <ak@linux.intel.com>
> > > > > 
> > > > > There are still problems with BPF misinterpreting some events
> > > > > that include .c. An earlier fix made it work for stand alone
> > > > > aliases, but it still fails for more complex constructs.
> > > > 
> > > > Hi Wang, Jiri,
> > > > 
> > > > 	Can you please take a look at this and see if there is something
> > > > we can do to help Andi?
> > > > 
> > > > - Arnaldo
> > > >  
> > > > > REJECT keeps trying and trying a shorter string until
> > > > > .c is matched and it appears like a valid BPF path.
> > > > > 
> > > > > % perf stat -e cpu/uops_executed.core,cmask=1/ true
> > > > > bpf: builtin compilation failed: -95, try external compiler
> > > > > ERROR: problems with path cpu/uops_executed.c: No such file or directory
> > > > > event syntax error: 'cpu/uops_executed.core,cmask=1/'
> > > > >                      \___ Failed to load cpu/uops_executed.c from source: Error when compiling BPF scriptlet
> > > > > 
> > > > > I tried to fix it, but it exceeds my flex knowledge, because
> > > > > REJECT does not interact well with BEGIN states.
> > > > > 
> > > > > The BPF syntax in its current form really causes an ambigious
> > > > > grammar.
> > > 
> > > right, it looks like we allow whole path (including / char)
> > > for BPF file, which messes up with out pmu/.../ syntax
> > > 
> > > do we need that? (Cc-ed some bpf folks)
> > > 
> > > if not attached patch seems to fix things.. otherwise
> > > we need to come up with another fix
> > 
> > I tried similar patches, but I always ran into more complex
> > situations where it still matched incorrectly.
> > 
> > e.g. try it with cpu/uops_executed.core,... vs uops_executed.core
> 
> hm, both works for me with the change:
> 
>   perf stat -e cpu/uops_executed.core/ ls
>   perf stat -e uops_executed.core ls

Ok. If it works it's fine for me.

> 
> > The only real fix would be probably to add some unique 
> > prefix for BPF, but that would break all existing users.
> 
> 
> yea, there was no response from bpf folks, but it's  probably not an optio
> 
> how about checking if the file exist like below..

I presume that would interact badly with good error messages for typos
for file names.

-Andi

Re: [PATCH 2/2] perf, tools: Don't force MetricExprs to lower case

[Arnaldo Carvalho de Melo]

Em Mon, Oct 09, 2017 at 03:41:51PM +0200, Jiri Olsa escreveu:
> On Wed, Oct 04, 2017 at 09:27:11AM -0700, Andi Kleen wrote:
> > On Wed, Oct 04, 2017 at 12:30:52PM +0200, Jiri Olsa wrote:
> > > right, it looks like we allow whole path (including / char)
> > > for BPF file, which messes up with out pmu/.../ syntax

> > > do we need that? (Cc-ed some bpf folks)

> > > if not attached patch seems to fix things.. otherwise
> > > we need to come up with another fix

> > I tried similar patches, but I always ran into more complex
> > situations where it still matched incorrectly.

> > e.g. try it with cpu/uops_executed.core,... vs uops_executed.core

> hm, both works for me with the change:

>   perf stat -e cpu/uops_executed.core/ ls
>   perf stat -e uops_executed.core ls

> > The only real fix would be probably to add some unique 
> > prefix for BPF, but that would break all existing users.

> yea, there was no response from bpf folks, but it's  probably not an option
 
> how about checking if the file exist like below..

Works well for me, Jiri, I found a problem in the last case, can you take a look?

[root@jouet bpf]# perf stat -e cpu/uops_executed.core/,uops_executed.core sleep 1

 Performance counter stats for 'sleep 1':

         1,094,963      cpu/uops_executed.core/                                     
         1,094,963      uops_executed.core                                          

       1.001925575 seconds time elapsed

[root@jouet bpf]# perf stat -e uops_executed.core sleep 1

 Performance counter stats for 'sleep 1':

         2,309,245      uops_executed.core                                          

       1.003314897 seconds time elapsed

[root@jouet bpf]# perf stat -e cpu/uops_executed.core/ sleep 1

 Performance counter stats for 'sleep 1':

         1,664,618      cpu/uops_executed.core/                                     

       1.002284214 seconds time elapsed

[root@jouet bpf]# cat sys_read.c 
#define SEC(NAME) __attribute__((section(NAME), used))
SEC("func=sys_read")
int bpf_func__sys_read(void *ctx)
{
	return 1;
}
char _license[] SEC("license") = "GPL";
int _version SEC("version") = LINUX_VERSION_CODE;
[root@jouet bpf]# perf trace --no-syscalls -e sys_read.c/max-stack=5/ sleep 1
bpf: builtin compilation failed: -95, try external compiler
     0.000 perf_bpf_probe:func:(ffffffffb7263190))
                                       sys_read ([kernel.kallsyms])
                                       entry_SYSCALL_64_fastpath ([kernel.kallsyms])
                                       __read (/usr/lib64/ld-2.25.so)
                                       _dl_map_object (/usr/lib64/ld-2.25.so)
[root@jouet bpf]# perf trace --no-syscalls -e sys_read.c sleep 1
bpf: builtin compilation failed: -95, try external compiler
     0.000 perf_bpf_probe:func:(ffffffffb7263190))
[root@jouet bpf]#

[root@jouet bpf]# perf stat -e UOPS_EXECUTED.CORE sleep 1

 Performance counter stats for 'sleep 1':

         1,205,347      UOPS_EXECUTED.CORE                                          

       1.001694225 seconds time elapsed

But I noticed this problem:

[root@jouet bpf]# perf stat -e cpu/uops_executed.core/,uops_executed.core,cpu/UOPS_EXECUTED.CORE sleep 1
event syntax error: '..d=48;5;232;38;5;3:or=48;5;232;38;5;9:mi=01;05;37;41:su=48;5;196;38;5;15:sg=48;5;11;38;5;16:ca=48;5;196;38;5;226:IY�'
                                  \___ parser error
Run 'perf list' for a list of valid events

 Usage: perf stat [<options>] [<command>]

    -e, --event <event>   event selector. use 'perf list' to list available events
[root@jouet bpf]# 

Re: [PATCH 2/2] perf, tools: Don't force MetricExprs to lower case

[Arnaldo Carvalho de Melo]

Em Mon, Oct 09, 2017 at 07:07:29AM -0700, Andi Kleen escreveu:
> On Mon, Oct 09, 2017 at 03:41:51PM +0200, Jiri Olsa wrote:
> > On Wed, Oct 04, 2017 at 09:27:11AM -0700, Andi Kleen wrote:
> > > On Wed, Oct 04, 2017 at 12:30:52PM +0200, Jiri Olsa wrote:
> > > > On Tue, Oct 03, 2017 at 01:06:05PM -0300, Arnaldo Carvalho de Melo wrote:
> > > > > Em Tue, Sep 12, 2017 at 12:56:43PM -0700, Andi Kleen escreveu:
> > > > > > From: Andi Kleen <ak@linux.intel.com>
> > > > > > 
> > > > > > There are still problems with BPF misinterpreting some events
> > > > > > that include .c. An earlier fix made it work for stand alone
> > > > > > aliases, but it still fails for more complex constructs.
> > > > > 
> > > > > Hi Wang, Jiri,
> > > > > 
> > > > > 	Can you please take a look at this and see if there is something
> > > > > we can do to help Andi?
> > > > > 
> > > > > - Arnaldo
> > > > >  
> > > > > > REJECT keeps trying and trying a shorter string until
> > > > > > .c is matched and it appears like a valid BPF path.
> > > > > > 
> > > > > > % perf stat -e cpu/uops_executed.core,cmask=1/ true
> > > > > > bpf: builtin compilation failed: -95, try external compiler
> > > > > > ERROR: problems with path cpu/uops_executed.c: No such file or directory
> > > > > > event syntax error: 'cpu/uops_executed.core,cmask=1/'
> > > > > >                      \___ Failed to load cpu/uops_executed.c from source: Error when compiling BPF scriptlet
> > > > > > 
> > > > > > I tried to fix it, but it exceeds my flex knowledge, because
> > > > > > REJECT does not interact well with BEGIN states.
> > > > > > 
> > > > > > The BPF syntax in its current form really causes an ambigious
> > > > > > grammar.
> > > > 
> > > > right, it looks like we allow whole path (including / char)
> > > > for BPF file, which messes up with out pmu/.../ syntax
> > > > 
> > > > do we need that? (Cc-ed some bpf folks)
> > > > 
> > > > if not attached patch seems to fix things.. otherwise
> > > > we need to come up with another fix
> > > 
> > > I tried similar patches, but I always ran into more complex
> > > situations where it still matched incorrectly.
> > > 
> > > e.g. try it with cpu/uops_executed.core,... vs uops_executed.core
> > 
> > hm, both works for me with the change:
> > 
> >   perf stat -e cpu/uops_executed.core/ ls
> >   perf stat -e uops_executed.core ls
> 
> Ok. If it works it's fine for me.
> 
> > 
> > > The only real fix would be probably to add some unique 
> > > prefix for BPF, but that would break all existing users.
> > 
> > 
> > yea, there was no response from bpf folks, but it's  probably not an optio
> > 
> > how about checking if the file exist like below..
> 
> I presume that would interact badly with good error messages for typos
> for file names.

Right :-\

Perhaps we can be clever and use something like:

[root@jouet bpf]# git pussh
git: 'pussh' is not a git command. See 'git --help'.

The most similar command is
	push
[root@jouet bpf]# 

I.e. try to detect typos instead of plain using stat(), or perhaps
reverting to consider it a file when no match for other event takes
place?

- Arnaldo

Re: [PATCH 2/2] perf, tools: Don't force MetricExprs to lower case

[Jiri Olsa]

On Mon, Oct 09, 2017 at 11:12:58AM -0300, Arnaldo Carvalho de Melo wrote:
> Em Mon, Oct 09, 2017 at 07:07:29AM -0700, Andi Kleen escreveu:
> > On Mon, Oct 09, 2017 at 03:41:51PM +0200, Jiri Olsa wrote:
> > > On Wed, Oct 04, 2017 at 09:27:11AM -0700, Andi Kleen wrote:
> > > > On Wed, Oct 04, 2017 at 12:30:52PM +0200, Jiri Olsa wrote:
> > > > > On Tue, Oct 03, 2017 at 01:06:05PM -0300, Arnaldo Carvalho de Melo wrote:
> > > > > > Em Tue, Sep 12, 2017 at 12:56:43PM -0700, Andi Kleen escreveu:
> > > > > > > From: Andi Kleen <ak@linux.intel.com>
> > > > > > > 
> > > > > > > There are still problems with BPF misinterpreting some events
> > > > > > > that include .c. An earlier fix made it work for stand alone
> > > > > > > aliases, but it still fails for more complex constructs.
> > > > > > 
> > > > > > Hi Wang, Jiri,
> > > > > > 
> > > > > > 	Can you please take a look at this and see if there is something
> > > > > > we can do to help Andi?
> > > > > > 
> > > > > > - Arnaldo
> > > > > >  
> > > > > > > REJECT keeps trying and trying a shorter string until
> > > > > > > .c is matched and it appears like a valid BPF path.
> > > > > > > 
> > > > > > > % perf stat -e cpu/uops_executed.core,cmask=1/ true
> > > > > > > bpf: builtin compilation failed: -95, try external compiler
> > > > > > > ERROR: problems with path cpu/uops_executed.c: No such file or directory
> > > > > > > event syntax error: 'cpu/uops_executed.core,cmask=1/'
> > > > > > >                      \___ Failed to load cpu/uops_executed.c from source: Error when compiling BPF scriptlet
> > > > > > > 
> > > > > > > I tried to fix it, but it exceeds my flex knowledge, because
> > > > > > > REJECT does not interact well with BEGIN states.
> > > > > > > 
> > > > > > > The BPF syntax in its current form really causes an ambigious
> > > > > > > grammar.
> > > > > 
> > > > > right, it looks like we allow whole path (including / char)
> > > > > for BPF file, which messes up with out pmu/.../ syntax
> > > > > 
> > > > > do we need that? (Cc-ed some bpf folks)
> > > > > 
> > > > > if not attached patch seems to fix things.. otherwise
> > > > > we need to come up with another fix
> > > > 
> > > > I tried similar patches, but I always ran into more complex
> > > > situations where it still matched incorrectly.
> > > > 
> > > > e.g. try it with cpu/uops_executed.core,... vs uops_executed.core
> > > 
> > > hm, both works for me with the change:
> > > 
> > >   perf stat -e cpu/uops_executed.core/ ls
> > >   perf stat -e uops_executed.core ls
> > 
> > Ok. If it works it's fine for me.

well it works, but it means that bpf file cannot contains any directory
part.. which im not sure is ok with bpf folks ;-) anyone?

jirka

Re: [PATCH 2/2] perf, tools: Don't force MetricExprs to lower case

[Jiri Olsa]

On Mon, Oct 09, 2017 at 11:09:44AM -0300, Arnaldo Carvalho de Melo wrote:

SNIP

> [root@jouet bpf]# cat sys_read.c 
> #define SEC(NAME) __attribute__((section(NAME), used))
> SEC("func=sys_read")
> int bpf_func__sys_read(void *ctx)
> {
> 	return 1;
> }
> char _license[] SEC("license") = "GPL";
> int _version SEC("version") = LINUX_VERSION_CODE;
> [root@jouet bpf]# perf trace --no-syscalls -e sys_read.c/max-stack=5/ sleep 1
> bpf: builtin compilation failed: -95, try external compiler
>      0.000 perf_bpf_probe:func:(ffffffffb7263190))
>                                        sys_read ([kernel.kallsyms])
>                                        entry_SYSCALL_64_fastpath ([kernel.kallsyms])
>                                        __read (/usr/lib64/ld-2.25.so)
>                                        _dl_map_object (/usr/lib64/ld-2.25.so)
> [root@jouet bpf]# perf trace --no-syscalls -e sys_read.c sleep 1
> bpf: builtin compilation failed: -95, try external compiler
>      0.000 perf_bpf_probe:func:(ffffffffb7263190))
> [root@jouet bpf]#

is this ok?

> 
> [root@jouet bpf]# perf stat -e UOPS_EXECUTED.CORE sleep 1
> 
>  Performance counter stats for 'sleep 1':
> 
>          1,205,347      UOPS_EXECUTED.CORE                                          
> 
>        1.001694225 seconds time elapsed
> 
> But I noticed this problem:
> 
> [root@jouet bpf]# perf stat -e cpu/uops_executed.core/,uops_executed.core,cpu/UOPS_EXECUTED.CORE sleep 1
> event syntax error: '..d=48;5;232;38;5;3:or=48;5;232;38;5;9:mi=01;05;37;41:su=48;5;196;38;5;15:sg=48;5;11;38;5;16:ca=48;5;196;38;5;226:IY�'
>                                   \___ parser error
> Run 'perf list' for a list of valid events
> 

mama mia, this looks like unhandled case of error printing.. I'll check

jirka

Re: [PATCH 2/2] perf, tools: Don't force MetricExprs to lower case

[Jiri Olsa]

On Mon, Oct 09, 2017 at 07:07:29AM -0700, Andi Kleen wrote:

SNIP

> > 
> > > The only real fix would be probably to add some unique 
> > > prefix for BPF, but that would break all existing users.
> > 
> > 
> > yea, there was no response from bpf folks, but it's  probably not an optio
> > 
> > how about checking if the file exist like below..
> 
> I presume that would interact badly with good error messages for typos
> for file names.

it'd misplaced them for event names.. maybe not big deal

jirka

Re: [PATCH 2/2] perf, tools: Don't force MetricExprs to lower case

[Arnaldo Carvalho de Melo]

Em Mon, Oct 09, 2017 at 04:39:53PM +0200, Jiri Olsa escreveu:
> On Mon, Oct 09, 2017 at 11:12:58AM -0300, Arnaldo Carvalho de Melo wrote:
> > Em Mon, Oct 09, 2017 at 07:07:29AM -0700, Andi Kleen escreveu:
> > > On Mon, Oct 09, 2017 at 03:41:51PM +0200, Jiri Olsa wrote:
> > > > On Wed, Oct 04, 2017 at 09:27:11AM -0700, Andi Kleen wrote:
> > > > > I tried similar patches, but I always ran into more complex
> > > > > situations where it still matched incorrectly.
> > > > > 
> > > > > e.g. try it with cpu/uops_executed.core,... vs uops_executed.core
> > > > 
> > > > hm, both works for me with the change:
> > > > 
> > > >   perf stat -e cpu/uops_executed.core/ ls
> > > >   perf stat -e uops_executed.core ls
> > > 
> > > Ok. If it works it's fine for me.
> 
> well it works, but it means that bpf file cannot contains any directory
> part.. which im not sure is ok with bpf folks ;-) anyone?

That is a big limitation :-\

- Arnaldo

Re: [PATCH 2/2] perf, tools: Don't force MetricExprs to lower case

[Andi Kleen]

> > > >   perf stat -e cpu/uops_executed.core/ ls
> > > >   perf stat -e uops_executed.core ls
> > > 
> > > Ok. If it works it's fine for me.
> 
> well it works, but it means that bpf file cannot contains any directory
> part.. which im not sure is ok with bpf folks ;-) anyone?

One way that may work is to 

- switch to a new proper syntax for bpf
(like bpf("filename")) that can be parsed properly

- handle simple cases like just that file name in the old syntax
in a pre processing pass of the scanner that converts it.

- users who specify multiple scripts or combine with other events
in the same -e line would need to change their syntax, but I assume that's
relatively rare.

-Andi

Re: [PATCH 2/2] perf, tools: Don't force MetricExprs to lower case

[Jiri Olsa]

On Mon, Oct 09, 2017 at 04:41:55PM +0200, Jiri Olsa wrote:
> On Mon, Oct 09, 2017 at 11:09:44AM -0300, Arnaldo Carvalho de Melo wrote:
> 
> SNIP
> 
> > [root@jouet bpf]# cat sys_read.c 
> > #define SEC(NAME) __attribute__((section(NAME), used))
> > SEC("func=sys_read")
> > int bpf_func__sys_read(void *ctx)
> > {
> > 	return 1;
> > }
> > char _license[] SEC("license") = "GPL";
> > int _version SEC("version") = LINUX_VERSION_CODE;
> > [root@jouet bpf]# perf trace --no-syscalls -e sys_read.c/max-stack=5/ sleep 1
> > bpf: builtin compilation failed: -95, try external compiler
> >      0.000 perf_bpf_probe:func:(ffffffffb7263190))
> >                                        sys_read ([kernel.kallsyms])
> >                                        entry_SYSCALL_64_fastpath ([kernel.kallsyms])
> >                                        __read (/usr/lib64/ld-2.25.so)
> >                                        _dl_map_object (/usr/lib64/ld-2.25.so)
> > [root@jouet bpf]# perf trace --no-syscalls -e sys_read.c sleep 1
> > bpf: builtin compilation failed: -95, try external compiler
> >      0.000 perf_bpf_probe:func:(ffffffffb7263190))
> > [root@jouet bpf]#
> 
> is this ok?
> 
> > 
> > [root@jouet bpf]# perf stat -e UOPS_EXECUTED.CORE sleep 1
> > 
> >  Performance counter stats for 'sleep 1':
> > 
> >          1,205,347      UOPS_EXECUTED.CORE                                          
> > 
> >        1.001694225 seconds time elapsed
> > 
> > But I noticed this problem:
> > 
> > [root@jouet bpf]# perf stat -e cpu/uops_executed.core/,uops_executed.core,cpu/UOPS_EXECUTED.CORE sleep 1
> > event syntax error: '..d=48;5;232;38;5;3:or=48;5;232;38;5;9:mi=01;05;37;41:su=48;5;196;38;5;15:sg=48;5;11;38;5;16:ca=48;5;196;38;5;226:IY�'
> >                                   \___ parser error
> > Run 'perf list' for a list of valid events
> > 
> 
> mama mia, this looks like unhandled case of error printing.. I'll check

I think I found the issue, could you please check following patch?

and wrt Andi's concern about wrong error message,
now if you make typo in your bpf file it says:

	[jolsa@krava perf]$ ./perf stat -e krava.c// sleep 1 
	event syntax error: 'krava.c//'
			     \___ Cannot find PMU `krava.c'. Missing kernel support?
	Run 'perf list' for a list of valid events
	...

looks sane enough.. ;-)

thanks,
jirka


---
diff --git a/tools/perf/util/parse-events.l b/tools/perf/util/parse-events.l
index ea2426daf7e8..241396cd059d 100644
--- a/tools/perf/util/parse-events.l
+++ b/tools/perf/util/parse-events.l
@@ -8,6 +8,9 @@
 
 %{
 #include <errno.h>
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <unistd.h>
 #include "../perf.h"
 #include "parse-events.h"
 #include "parse-events-bison.h"
@@ -53,9 +56,8 @@ static int str(yyscan_t scanner, int token)
 	return token;
 }
 
-static bool isbpf(yyscan_t scanner)
+static bool isbpf_suffix(char *text)
 {
-	char *text = parse_events_get_text(scanner);
 	int len = strlen(text);
 
 	if (len < 2)
@@ -68,6 +70,17 @@ static bool isbpf(yyscan_t scanner)
 	return false;
 }
 
+static bool isbpf(yyscan_t scanner)
+{
+	char *text = parse_events_get_text(scanner);
+	struct stat st;
+
+	if (!isbpf_suffix(text))
+		return false;
+
+	return stat(text, &st) == 0;
+}
+
 /*
  * This function is called when the parser gets two kind of input:
  *
@@ -141,6 +154,10 @@ do {							\
 	yycolumn += yyleng;				\
 } while (0);
 
+#define USER_REJECT		\
+	yycolumn -= yyleng;	\
+	REJECT
+
 %}
 
 %x mem
@@ -323,8 +340,8 @@ r{num_raw_hex}		{ return raw(yyscanner); }
 {num_hex}		{ return value(yyscanner, 16); }
 
 {modifier_event}	{ return str(yyscanner, PE_MODIFIER_EVENT); }
-{bpf_object}		{ if (!isbpf(yyscanner)) REJECT; return str(yyscanner, PE_BPF_OBJECT); }
-{bpf_source}		{ if (!isbpf(yyscanner)) REJECT; return str(yyscanner, PE_BPF_SOURCE); }
+{bpf_object}		{ if (!isbpf(yyscanner)) USER_REJECT; return str(yyscanner, PE_BPF_OBJECT); }
+{bpf_source}		{ if (!isbpf(yyscanner)) USER_REJECT; return str(yyscanner, PE_BPF_SOURCE); }
 {name}			{ return pmu_str_check(yyscanner); }
 "/"			{ BEGIN(config); return '/'; }
 -			{ return '-'; }

Re: [PATCH 2/2] perf, tools: Don't force MetricExprs to lower case

[Wangnan (F)]

On 2017/10/9 22:39, Jiri Olsa wrote:
> On Mon, Oct 09, 2017 at 11:12:58AM -0300, Arnaldo Carvalho de Melo wrote:
>> Em Mon, Oct 09, 2017 at 07:07:29AM -0700, Andi Kleen escreveu:
>>> On Mon, Oct 09, 2017 at 03:41:51PM +0200, Jiri Olsa wrote:
>>>> On Wed, Oct 04, 2017 at 09:27:11AM -0700, Andi Kleen wrote:
>>>>> On Wed, Oct 04, 2017 at 12:30:52PM +0200, Jiri Olsa wrote:
>>>>>> On Tue, Oct 03, 2017 at 01:06:05PM -0300, Arnaldo Carvalho de Melo wrote:
>>>>>>> Em Tue, Sep 12, 2017 at 12:56:43PM -0700, Andi Kleen escreveu:
>>>>>>>> From: Andi Kleen <ak@linux.intel.com>
>>>>>>>>
>>>>>>>> There are still problems with BPF misinterpreting some events
>>>>>>>> that include .c. An earlier fix made it work for stand alone
>>>>>>>> aliases, but it still fails for more complex constructs.
>>>>>>> Hi Wang, Jiri,
>>>>>>>
>>>>>>> 	Can you please take a look at this and see if there is something
>>>>>>> we can do to help Andi?
>>>>>>>
>>>>>>> - Arnaldo
>>>>>>>   
>>>>>>>> REJECT keeps trying and trying a shorter string until
>>>>>>>> .c is matched and it appears like a valid BPF path.
>>>>>>>>
>>>>>>>> % perf stat -e cpu/uops_executed.core,cmask=1/ true
>>>>>>>> bpf: builtin compilation failed: -95, try external compiler
>>>>>>>> ERROR: problems with path cpu/uops_executed.c: No such file or directory
>>>>>>>> event syntax error: 'cpu/uops_executed.core,cmask=1/'
>>>>>>>>                       \___ Failed to load cpu/uops_executed.c from source: Error when compiling BPF scriptlet
>>>>>>>>
>>>>>>>> I tried to fix it, but it exceeds my flex knowledge, because
>>>>>>>> REJECT does not interact well with BEGIN states.
>>>>>>>>
>>>>>>>> The BPF syntax in its current form really causes an ambigious
>>>>>>>> grammar.
>>>>>> right, it looks like we allow whole path (including / char)
>>>>>> for BPF file, which messes up with out pmu/.../ syntax
>>>>>>
>>>>>> do we need that? (Cc-ed some bpf folks)
>>>>>>
>>>>>> if not attached patch seems to fix things.. otherwise
>>>>>> we need to come up with another fix
>>>>> I tried similar patches, but I always ran into more complex
>>>>> situations where it still matched incorrectly.
>>>>>
>>>>> e.g. try it with cpu/uops_executed.core,... vs uops_executed.core
>>>> hm, both works for me with the change:
>>>>
>>>>    perf stat -e cpu/uops_executed.core/ ls
>>>>    perf stat -e uops_executed.core ls
>>> Ok. If it works it's fine for me.
> well it works, but it means that bpf file cannot contains any directory
> part.. which im not sure is ok with bpf folks ;-) anyone?

Sorry I didn't see this thread these days.

Do you think adding a special escape character to suppress BPF
name parsing in a event is a good idea? for example:

% perf stat -e cpu/uops_executed.core,cmask=1/ true
bpf: builtin compilation failed: -95, try external compiler
ERROR: problems with path cpu/uops_executed.c: No such file or directory
event syntax error: 'cpu/uops_executed.core,cmask=1/'
                      \___ Failed to load cpu/uops_executed.c from 
source: Error when compiling BPF scriptlet. Add a leading '@' to avoid 
BPF syntax

% perf stat -e @cpu/uops_executed.core,cmask=1/ true
...

Re: [PATCH 2/2] perf, tools: Don't force MetricExprs to lower case

[Jiri Olsa]

On Thu, Oct 12, 2017 at 11:31:51PM +0800, Wangnan (F) wrote:

SNIP

> > > > Ok. If it works it's fine for me.
> > well it works, but it means that bpf file cannot contains any directory
> > part.. which im not sure is ok with bpf folks ;-) anyone?
> 
> Sorry I didn't see this thread these days.
> 
> Do you think adding a special escape character to suppress BPF
> name parsing in a event is a good idea? for example:
> 
> % perf stat -e cpu/uops_executed.core,cmask=1/ true
> bpf: builtin compilation failed: -95, try external compiler
> ERROR: problems with path cpu/uops_executed.c: No such file or directory
> event syntax error: 'cpu/uops_executed.core,cmask=1/'
>                      \___ Failed to load cpu/uops_executed.c from source:
> Error when compiling BPF scriptlet. Add a leading '@' to avoid BPF syntax
> 
> % perf stat -e @cpu/uops_executed.core,cmask=1/ true
> ...
> 

if we go this way, I'd rather mark the bpf syntax
instead of changing the generic event format,
like Andi suggested in some earlier email

but maybe we can workaround this with patch
I sent in my last email

thanks,
jirka

Re: [PATCH 2/2] perf, tools: Don't force MetricExprs to lower case

[Wangnan (F)]

On 2017/10/12 23:59, Jiri Olsa wrote:
> On Thu, Oct 12, 2017 at 11:31:51PM +0800, Wangnan (F) wrote:
>
> SNIP
>
>>>>> Ok. If it works it's fine for me.
>>> well it works, but it means that bpf file cannot contains any directory
>>> part.. which im not sure is ok with bpf folks ;-) anyone?
>> Sorry I didn't see this thread these days.
>>
>> Do you think adding a special escape character to suppress BPF
>> name parsing in a event is a good idea? for example:
>>
>> % perf stat -e cpu/uops_executed.core,cmask=1/ true
>> bpf: builtin compilation failed: -95, try external compiler
>> ERROR: problems with path cpu/uops_executed.c: No such file or directory
>> event syntax error: 'cpu/uops_executed.core,cmask=1/'
>>                       \___ Failed to load cpu/uops_executed.c from source:
>> Error when compiling BPF scriptlet. Add a leading '@' to avoid BPF syntax
>>
>> % perf stat -e @cpu/uops_executed.core,cmask=1/ true
>> ...
>>
> if we go this way, I'd rather mark the bpf syntax
> instead of changing the generic event format,
> like Andi suggested in some earlier email
>
> but maybe we can workaround this with patch
> I sent in my last email

Great. I'm also working on a patch which check the existence of
BPF file and have it rejected if the file is not exist. The only
inconvenient is we need to call stat() many times because bpf_object
matchs c[a-zA-Z0-9._]*. I think you have solved the problem in your
patch.

Thank you.

Re: [PATCH 2/2] perf, tools: Don't force MetricExprs to lower case

[Andi Kleen]

> 
> I think I found the issue, could you please check following patch?
> 
I did some testing and it appears to work. Thanks! Please submit it.

Tested-by: Andi Kleen <ak@linux.intel.com>

-Andi

[tip:perf/urgent] perf tools: Unwind properly location after REJECT

[tip-bot for Jiri Olsa]

Commit-ID:  9445464bb8318e42e5232b37fc7218ed028517f6
Gitweb:     https://git.kernel.org/tip/9445464bb8318e42e5232b37fc7218ed028517f6
Author:     Jiri Olsa <jolsa@kernel.org>
AuthorDate: Thu, 12 Oct 2017 17:03:38 +0200
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Fri, 27 Oct 2017 11:42:51 -0300

perf tools: Unwind properly location after REJECT

We have defined YY_USER_ACTION to keep trace of the column location
during events parsing, but we need to clean it up when we call REJECT.

When REJECT is called, the lexer shrinks the text and re-runs the
matching, so we need to address it in resuming the previous location
value to keep it correct for error display, like:

Before:
  $ perf stat -e 'cpu/uops_executed.core,krava/'  true
  event syntax error: '..38;5;9:mi=01;05;37;41:su=48;5;196;38;5;15:sg=48;5;1\
1;38;5;16:ca=48;5;196;38;5;226:tw=48;5;10;38;5;16:ow=48;5;10;38;5;21:st=48;5;\
21;38;50
�'
                                  \___ unknown term

After:
  $ ./perf stat -e 'cpu/uops_executed.core,krava/'  true
  event syntax error: '..cuted.core,krava/'
                                    \___ unknown term

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
Reported-by: Arnaldo Carvalho de Melo <acme@redhat.com>
Tested-by: Arnaldo Carvalho de Melo <acme@redhat.com>
Tested-by: Andi Kleen <ak@linux.intel.com>
Cc: Changbin Du <changbin.du@intel.com>
Cc: David Ahern <dsahern@gmail.com>
Cc: Jin Yao <yao.jin@linux.intel.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Wang Nan <wangnan0@huawei.com>
Link: http://lkml.kernel.org/n/tip-vug2hchlny30jfsfrumbym26@git.kernel.org
Link: http://lkml.kernel.org/r/20171009140944.GD28623@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/parse-events.l | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/tools/perf/util/parse-events.l b/tools/perf/util/parse-events.l
index dcfdafd..6680e4f 100644
--- a/tools/perf/util/parse-events.l
+++ b/tools/perf/util/parse-events.l
@@ -154,6 +154,10 @@ do {							\
 	yycolumn += yyleng;				\
 } while (0);
 
+#define USER_REJECT		\
+	yycolumn -= yyleng;	\
+	REJECT
+
 %}
 
 %x mem
@@ -335,8 +339,8 @@ r{num_raw_hex}		{ return raw(yyscanner); }
 {num_hex}		{ return value(yyscanner, 16); }
 
 {modifier_event}	{ return str(yyscanner, PE_MODIFIER_EVENT); }
-{bpf_object}		{ if (!isbpf(yyscanner)) REJECT; return str(yyscanner, PE_BPF_OBJECT); }
-{bpf_source}		{ if (!isbpf(yyscanner)) REJECT; return str(yyscanner, PE_BPF_SOURCE); }
+{bpf_object}		{ if (!isbpf(yyscanner)) USER_REJECT; return str(yyscanner, PE_BPF_OBJECT); }
+{bpf_source}		{ if (!isbpf(yyscanner)) USER_REJECT; return str(yyscanner, PE_BPF_SOURCE); }
 {name}			{ return pmu_str_check(yyscanner); }
 "/"			{ BEGIN(config); return '/'; }
 -			{ return '-'; }