perf, tools, json: Fix ILP metrics

From: Andi Kleen <[email protected]>

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 <[email protected]>

---
 tools/perf/pmu-events/jevents.c | 2 --
 1 file changed, 2 deletions(-)

tools/perf/pmu-events/jevents.c +0 -2

index 9eb7047bafe4..07c0f9a46180 100644
@@ -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 */
 		}

Signature · 1 line

2.9.5

Em Tue, Sep 12, 2017 at 12:56:43PM -0700, Andi Kleen escreveu:

Quoted 5 lines

From: Andi Kleen <[email protected]>

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

Quoted 14 lines

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.

Quoted 7 lines · Arnaldo Carvalho de Melo

Em Tue, Sep 12, 2017 at 12:56:43PM -0700, Andi Kleen escreveu:

Quoted 5 lines

From: Andi Kleen <[email protected]>

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.

Quoted 21 lines

Hi Wang, Jiri,

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

- Arnaldo

Quoted 14 lines

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

Quoted 35 lines · Jiri Olsa

Quoted 7 lines · Arnaldo Carvalho de Melo

Em Tue, Sep 12, 2017 at 12:56:43PM -0700, Andi Kleen escreveu:

Quoted 5 lines

From: Andi Kleen <[email protected]>

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.

Quoted 21 lines

Hi Wang, Jiri,

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

- Arnaldo

Quoted 14 lines

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)

Quoted 2 lines

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.

Quoted 42 lines · Andi Kleen

Quoted 30 lines · Jiri Olsa

Quoted 7 lines · Arnaldo Carvalho de Melo

Em Tue, Sep 12, 2017 at 12:56:43PM -0700, Andi Kleen escreveu:

Quoted 5 lines

From: Andi Kleen <[email protected]>

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.

Quoted 21 lines

Hi Wang, Jiri,

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

- Arnaldo

Quoted 14 lines

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.

Quoted 7 lines

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.

Quoted 1 line

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

Quoted 3 lines

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..

Quoted 43 lines · Andi Kleen

Quoted 42 lines · Jiri Olsa

Quoted 30 lines · Arnaldo Carvalho de Melo

Quoted 7 lines

Em Tue, Sep 12, 2017 at 12:56:43PM -0700, Andi Kleen escreveu:

Quoted 5 lines

From: Andi Kleen <[email protected]>

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.

Quoted 21 lines

Hi Wang, Jiri,

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

- Arnaldo

Quoted 14 lines

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.

Quoted 7 lines

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.

Quoted 6 lines

Quoted 1 line

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.

Quoted 5 lines

Quoted 3 lines

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

Quoted 3 lines

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.

Em Mon, Oct 09, 2017 at 07:07:29AM -0700, Andi Kleen escreveu:

Quoted 52 lines · Jiri Olsa

Quoted 43 lines · Andi Kleen

Quoted 42 lines · Jiri Olsa

Quoted 30 lines · Arnaldo Carvalho de Melo

Quoted 7 lines

Em Tue, Sep 12, 2017 at 12:56:43PM -0700, Andi Kleen escreveu:

Quoted 5 lines

From: Andi Kleen <[email protected]>

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.

Quoted 21 lines

Hi Wang, Jiri,

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

- Arnaldo

Quoted 14 lines

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.

Quoted 7 lines

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.

Quoted 6 lines

Quoted 1 line

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.

Quoted 10 lines

Em Mon, Oct 09, 2017 at 07:07:29AM -0700, Andi Kleen escreveu:

Quoted 9 lines · Jiri Olsa

Quoted 8 lines · Andi Kleen

Quoted 4 lines

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:

Quoted 4 lines

Quoted 4 lines

Quoted 2 lines

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?

Quoted 4 lines

Quoted 4 lines

Quoted 2 lines

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?

Quoted 42 lines · Arnaldo Carvalho de Melo

Em Mon, Oct 09, 2017 at 07:07:29AM -0700, Andi Kleen escreveu:

Quoted 41 lines · Jiri Olsa

Quoted 40 lines · Andi Kleen

Quoted 39 lines · Jiri Olsa

Quoted 28 lines · Arnaldo Carvalho de Melo

Quoted 6 lines

Em Tue, Sep 12, 2017 at 12:56:43PM -0700, Andi Kleen escreveu:

Quoted 5 lines

From: Andi Kleen <[email protected]>

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.

Quoted 21 lines

Hi Wang, Jiri,

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

- Arnaldo

Quoted 14 lines

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.

Quoted 7 lines

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.

Quoted 6 lines

Quoted 6 lines

Quoted 5 lines

Quoted 1 line

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?

Quoted 3 lines

Quoted 1 line

Quoted 1 line

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
...

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

SNIP

Quoted 15 lines

Quoted 3 lines

Quoted 1 line

Quoted 1 line

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

Quoted 3 lines

% 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

Quoted 8 lines

Quoted 3 lines

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.

Quoted 6 lines · Jiri Olsa

Quoted 4 lines

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)

Quoted 7 lines

Quoted 2 lines

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

Quoted 2 lines

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..

[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

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

SNIP

Quoted 19 lines

[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))

Quoted 1 line

[root@jouet bpf]#

is this ok?

Quoted 11 lines

[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:

Quoted 5 lines

[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?