perf tools: Use perf_evsel__match in perf_evsel__is_bpf_output

The warning message in check_w function uses wrongly the $file variable instead of $file1 and $file2.

Fixes 582472973593 ("perf check-headers.sh: Add support to check 2 independent files")

Link http://lkml.kernel.org/n/[email protected] open here

Signed-off-by Jiri Olsa <[email protected]>

---
 tools/perf/check-headers.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

tools/perf/check-headers.sh +1 -1

index 814aaf269949..73e723675c5f 100755
@@ -67,7 +67,7 @@ check_2 () {
   cmd="diff $* $file1 $file2 > /dev/null"
 
   test -f $file2 &&
-  eval $cmd || echo "Warning: Kernel ABI header at 'tools/$file' differs from latest version at '$file'" >&2
+  eval $cmd || echo "Warning: Kernel ABI header at '$file1' differs from latest version at '$file2'" >&2
 }
 
 check () {

Signature · 1 line

2.17.1

Em Fri, Jul 20, 2018 at 01:00:35PM +0200, Jiri Olsa escreveu:

Quoted 2 lines

The warning message in check_w function uses wrongly the $file variable instead of $file1 and $file2.

Humm,

Before:

Warning: Kernel ABI header at 'tools/arch/powerpc/include/uapi/asm/unistd.h' differs from latest version at 'arch/powerpc/include/uapi/asm/unistd.h'

After:

Warning: Kernel ABI header at '../arch/powerpc/include/uapi/asm/unistd.h' differs from latest version at '../../arch/powerpc/include/uapi/asm/unistd.h'

The previous version is better, I can then just use:

diff -u tools/arch/powerpc/include/uapi/asm/unistd.h arch/powerpc/include/uapi/asm/unistd.h

and get what changed, with your change I have to go to tools/perf before doing that diff, which is an unnecessary extra step in at least my workflow.

Quoted 19 lines · Arnaldo Carvalho de Melo

Em Fri, Jul 20, 2018 at 01:00:35PM +0200, Jiri Olsa escreveu:

Quoted 2 lines

The warning message in check_w function uses wrongly the $file variable instead of $file1 and $file2.

Humm,

Before:

Warning: Kernel ABI header at 'tools/arch/powerpc/include/uapi/asm/unistd.h' differs from latest version at 'arch/powerpc/include/uapi/asm/unistd.h'

After:

Warning: Kernel ABI header at '../arch/powerpc/include/uapi/asm/unistd.h' differs from latest version at '../../arch/powerpc/include/uapi/asm/unistd.h'

The previous version is better, I can then just use:

diff -u tools/arch/powerpc/include/uapi/asm/unistd.h arch/powerpc/include/uapi/asm/unistd.h

Quoted 3 lines

and get what changed, with your change I have to go to tools/perf before doing that diff, which is an unnecessary extra step in at least my workflow.

so all paths output based in kernel tree root then, will change

jirka

Quoted 21 lines · Jiri Olsa

Quoted 19 lines · Arnaldo Carvalho de Melo

Em Fri, Jul 20, 2018 at 01:00:35PM +0200, Jiri Olsa escreveu:

Quoted 2 lines

The warning message in check_w function uses wrongly the $file variable instead of $file1 and $file2.

Humm,

Before:

Warning: Kernel ABI header at 'tools/arch/powerpc/include/uapi/asm/unistd.h' differs from latest version at 'arch/powerpc/include/uapi/asm/unistd.h'

After:

Warning: Kernel ABI header at '../arch/powerpc/include/uapi/asm/unistd.h' differs from latest version at '../../arch/powerpc/include/uapi/asm/unistd.h'

The previous version is better, I can then just use:

diff -u tools/arch/powerpc/include/uapi/asm/unistd.h arch/powerpc/include/uapi/asm/unistd.h

Quoted 7 lines

Quoted 3 lines

and get what changed, with your change I have to go to tools/perf before doing that diff, which is an unnecessary extra step in at least my workflow.

so all paths output based in kernel tree root then, will change

jirka

I was going to ask about this in a separate email initially, but then thought I'd use this email exchange instead, as my question is about the code in question. Hope you don't mind.

If I'm reading this right, the intended behavoir of the block of code below is to test file2 for existance, and if it exists, to evaluate $cmd. If file1 and file2 are found to differ, print the warning.

test -f $file2 &&
eval $cmd || echo "Warning: Kernel ABI header at 'tools/$file'
differs from latest version at '$file'" >&2

The '||' path of execution is however also taken if file2 doesn't exist, which is probably very unlikely to happen. See below.

% file1=file1; file2=file2
% cmd="echo diff $file1 $file2"
% test -f $file2 &&
eval $cmd || echo "Warning: Kernel ABI header at 'tools/$file1' differs from latest version at '$file2'" >&2
Warning: Kernel ABI header at 'tools/file1' differs from latest version at 'file2'

Is this something you would rather leave as is, or perhaps use something along the lines of the code below instead:

test -f $file2 && {
eval $cmd ||
echo "Warning: Kernel ABI header at 'tools/$file' differs from latest version at '$file'" >&2
}

Quoted 21 lines

Quoted 8 lines

% file1=file1; file2=file2
% cmd="echo diff $file1 $file2"
% test -f $file2 &&
eval $cmd || echo "Warning: Kernel ABI header at 'tools/$file1' differs from latest version at '$file2'" >&2
Warning: Kernel ABI header at 'tools/file1' differs from latest version at 'file2'

Quoted 8 lines

Is this something you would rather leave as is, or perhaps use something along the lines of the code below instead:

test -f $file2 && {
eval $cmd ||
echo "Warning: Kernel ABI header at 'tools/$file' differs from latest version at '$file'" >&2
}

hi,
yea, probably.. please feel free to post a patch.. just make sure all the displayed files paths are based on the kernel root

Quoted 3 lines

thanks,
jirka

I'm away traveling till August 10th, and I may not be able to send the patch in until I get back. Is that OK?

tools/perf/check-headers.sh check () { · line 95

Quoted 13 lines · Jiri Olsa

tools/perf/check-headers.sh +2 -0

index ea48aa6f8d19..9d466e853aec 100755
@@ -88,6 +88,8 @@ check () {
 # differences.
 test -d ../../include || exit 0
 
+pushd ../.. > /dev/null
+
 # simple diff check
 for i in $HEADERS; do
   check $i -B

This breaks the build when sh is not bash:

./check-headers.sh: 91: ./check-headers.sh: pushd: not found ./check-headers.sh: 107: ./check-headers.sh: popd: not found Makefile.perf:205: recipe for target 'sub-make' failed

Quoted 17 lines

tools/perf/check-headers.sh check () { · line 95

Quoted 13 lines · Jiri Olsa

tools/perf/check-headers.sh +2 -0

index ea48aa6f8d19..9d466e853aec 100755
@@ -88,6 +88,8 @@ check () {
 # differences.
 test -d ../../include || exit 0
 
+pushd ../.. > /dev/null
+
 # simple diff check
 for i in $HEADERS; do
   check $i -B

This breaks the build when sh is not bash:

Quoted 3 lines

./check-headers.sh: 91: ./check-headers.sh: pushd: not found ./check-headers.sh: 107: ./check-headers.sh: popd: not found Makefile.perf:205: recipe for target 'sub-make' failed

sry.. Arnaldo, would you change it for simple cd (attached below) or should I send the fix?

thanks,
jirka

---

tools/perf/check-headers.sh +2 -2

index 80bf84803677..466540ee8ea7 100755
@@ -88,7 +88,7 @@ check () {
 # differences.
 test -d ../../include || exit 0
 
-pushd ../.. > /dev/null
+cd ../..
 
 # simple diff check
 for i in $HEADERS; do

@@ -104,4 +104,4 @@ check include/uapi/linux/mman.h       '-I "^#include <\(uapi/\)*asm/mman.h>"'
 # diff non-symmetric files
 check_2 tools/perf/arch/x86/entry/syscalls/syscall_64.tbl arch/x86/entry/syscalls/syscall_64.tbl
 
-popd > /dev/null
+cd tools/perf

Em Tue, Aug 14, 2018 at 09:27:26AM +0200, Jiri Olsa escreveu:

Quoted 18 lines · Michael Ellerman

Quoted 17 lines

tools/perf/check-headers.sh check () { · line 95

Quoted 13 lines · Jiri Olsa

tools/perf/check-headers.sh +2 -0

index ea48aa6f8d19..9d466e853aec 100755
@@ -88,6 +88,8 @@ check () {
 # differences.
 test -d ../../include || exit 0
 
+pushd ../.. > /dev/null
+
 # simple diff check
 for i in $HEADERS; do
   check $i -B

This breaks the build when sh is not bash:

Quoted 6 lines

Quoted 3 lines

./check-headers.sh: 91: ./check-headers.sh: pushd: not found ./check-headers.sh: 107: ./check-headers.sh: popd: not found Makefile.perf:205: recipe for target 'sub-make' failed

sry.. Arnaldo, would you change it for simple cd (attached below) or should I send the fix?

Nah, I'm folding this in, to keep it bisectable.

Quoted 23 lines · Arnaldo Carvalho de Melo

Em Tue, Aug 14, 2018 at 09:27:26AM +0200, Jiri Olsa escreveu:

Quoted 22 lines · Michael Ellerman

Quoted 20 lines

tools/perf/check-headers.sh check () { · line 95

Quoted 13 lines · Jiri Olsa

tools/perf/check-headers.sh +2 -0

index ea48aa6f8d19..9d466e853aec 100755
@@ -88,6 +88,8 @@ check () {
 # differences.
 test -d ../../include || exit 0
 
+pushd ../.. > /dev/null
+
 # simple diff check
 for i in $HEADERS; do
   check $i -B

This breaks the build when sh is not bash:

./check-headers.sh: 91: ./check-headers.sh: pushd: not found ./check-headers.sh: 107: ./check-headers.sh: popd: not found Makefile.perf:205: recipe for target 'sub-make' failed

Quoted 4 lines

Quoted 2 lines

sry.. Arnaldo, would you change it for simple cd (attached below) or should I send the fix?

Nah, I'm folding this in, to keep it bisectable.

any chance one of your docker tests could run build in sh/zsh? ;-)

Quoted 8 lines · Jiri Olsa

Quoted 5 lines · Arnaldo Carvalho de Melo

Em Tue, Aug 14, 2018 at 09:27:26AM +0200, Jiri Olsa escreveu:

Quoted 2 lines

sry.. Arnaldo, would you change it for simple cd (attached below) or should I send the fix?

Nah, I'm folding this in, to keep it bisectable.

any chance one of your docker tests could run build in sh/zsh? ;-)

Just using an Ubuntu image, where /bin/sh == dash should work, that's how I hit it.

Em Wed, Aug 15, 2018 at 08:02:48PM +1000, Michael Ellerman escreveu:

Quoted 9 lines

Quoted 8 lines · Jiri Olsa

Quoted 5 lines · Arnaldo Carvalho de Melo

Em Tue, Aug 14, 2018 at 09:27:26AM +0200, Jiri Olsa escreveu:

Quoted 2 lines

sry.. Arnaldo, would you change it for simple cd (attached below) or should I send the fix?

Nah, I'm folding this in, to keep it bisectable.

any chance one of your docker tests could run build in sh/zsh? ;-)

It does already, see below :-)

Quoted 2 lines

Just using an Ubuntu image, where /bin/sh == dash should work, that's how I hit it.

So, I do the tests only prior to pushing to Ingo, so didn't catch this, lemme check, put that change back on, start a ubuntu:18.04 perf build container, try to build, see if it would fail, yeah, I'd have detected this before pushing to Ingo, so probably I have to run the tests before pushing to my acme/perf/core branch, will try to operate like that from now on.

The '||' path of execution in the 'test' block of the check_2() function may also be taken if file2 does not exist, in which case the warning message about the ABI headers being different would still be printed where it should not be. See below.

% file1=file1; file2=file2
% cmd="echo diff $file1 $file2"
% test -f $file2 &&
eval $cmd || echo "Warning: Kernel ABI header at 'tools/$file1' differs from latest version at '$file2'" >&2
Warning: Kernel ABI header at 'tools/file1' differs from latest version at 'file2'

The proposed patch converts the code following the '&&' operator into a compound list to be executed in the current process environment only if file2 does exist. Should the files being compared differ, a diff command to compare the files concerned is printed on standard output. E.g.

diff -u tools/arch/x86/lib/memcpy_64.S arch/x86/lib/memcpy_64.S

Signed-off-by Alexander Kapshuk <[email protected]>

---
 tools/perf/check-headers.sh | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

tools/perf/check-headers.sh +6 -2

index de28466c0186..ea48aa6f8d19 100755
@@ -67,8 +67,12 @@ check_2 () {
 
   cmd="diff $* $file1 $file2 > /dev/null"
 
-  test -f $file2 &&
-  eval $cmd || echo "Warning: Kernel ABI header at 'tools/$file' differs from latest version at '$file'" >&2
+  test -f $file2 && {
+    eval $cmd || {
+      echo "Warning: Kernel ABI header at 'tools/$file' differs from latest version at '$file'" >&2
+      echo diff -u tools/$file $file
+    }
+  }
 }
 
 check () {

Signature · 1 line

2.18.0

Quoted 13 lines · Alexander Kapshuk

The '||' path of execution in the 'test' block of the check_2() function may also be taken if file2 does not exist, in which case the warning message about the ABI headers being different would still be printed where it should not be. See below.

% file1=file1; file2=file2
% cmd="echo diff $file1 $file2"
% test -f $file2 &&
eval $cmd || echo "Warning: Kernel ABI header at 'tools/$file1' differs from latest version at '$file2'" >&2
Warning: Kernel ABI header at 'tools/file1' differs from latest version at 'file2'

Quoted 8 lines

The proposed patch converts the code following the '&&' operator into a compound list to be executed in the current process environment only if file2 does exist. Should the files being compared differ, a diff command to compare the files concerned is printed on standard output. E.g.

diff -u tools/arch/x86/lib/memcpy_64.S arch/x86/lib/memcpy_64.S

Signed-off-by Alexander Kapshuk <[email protected]>

I posted follow up patches based on this one
as a reply on this patch

for this one:

Acked-by Jiri Olsa <[email protected]>

thanks,
jirka

Quoted 27 lines

---
 tools/perf/check-headers.sh | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

tools/perf/check-headers.sh +6 -3

index de28466c0186..ea48aa6f8d19 100755
@@ -67,8 +67,12 @@ check_2 () {
 
   cmd="diff $* $file1 $file2 > /dev/null"
 
-  test -f $file2 &&
-  eval $cmd || echo "Warning: Kernel ABI header at 'tools/$file' differs from latest version at '$file'" >&2
+  test -f $file2 && {
+    eval $cmd || {
+      echo "Warning: Kernel ABI header at 'tools/$file' differs from latest version at '$file'" >&2
+      echo diff -u tools/$file $file
+    }
+  }
 }
 
 check () {
--

2.18.0

Quoted 29 lines · Alexander Kapshuk

Quoted 13 lines

The '||' path of execution in the 'test' block of the check_2() function may also be taken if file2 does not exist, in which case the warning message about the ABI headers being different would still be printed where it should not be. See below.

% file1=file1; file2=file2
% cmd="echo diff $file1 $file2"
% test -f $file2 &&
eval $cmd || echo "Warning: Kernel ABI header at 'tools/$file1' differs from latest version at '$file2'" >&2
Warning: Kernel ABI header at 'tools/file1' differs from latest version at 'file2'

Quoted 8 lines

The proposed patch converts the code following the '&&' operator into a compound list to be executed in the current process environment only if file2 does exist. Should the files being compared differ, a diff command to compare the files concerned is printed on standard output. E.g.

diff -u tools/arch/x86/lib/memcpy_64.S arch/x86/lib/memcpy_64.S

Signed-off-by Alexander Kapshuk <[email protected]>

I posted follow up patches based on this one
as a reply on this patch

for this one:

Quoted 4 lines

Acked-by Jiri Olsa <[email protected]>

thanks,
jirka

Noted.
Thanks.

Thanks, applied the three patches to acme/perf/core.

- Arnaldo

Probably leftover from the time we introducd the check-headers.sh script.

Link http://lkml.kernel.org/n/[email protected] open here

Signed-off-by Jiri Olsa <[email protected]>

---
 tools/perf/arch/x86/Makefile | 3 ---
 tools/perf/check-headers.sh  | 3 +++
 2 files changed, 3 insertions(+), 3 deletions(-)

tools/perf/arch/x86/Makefile +0 -3

index 1a38e78117ce..8cc6642fce7a 100644
@@ -19,9 +19,6 @@ systbl := $(sys)/syscalltbl.sh
 _dummy := $(shell [ -d '$(out)' ] || mkdir -p '$(out)')
 
 $(header): $(sys)/syscall_64.tbl $(systbl)
-	@(test -d ../../kernel -a -d ../../tools -a -d ../perf && ( \
-        (diff -B arch/x86/entry/syscalls/syscall_64.tbl ../../arch/x86/entry/syscalls/syscall_64.tbl >/dev/null) \
-        || echo "Warning: Kernel ABI header at 'tools/perf/arch/x86/entry/syscalls/syscall_64.tbl' differs from latest version at 'arch/x86/entry/syscalls/syscall_64.tbl'" >&2 )) || true
 	$(Q)$(SHELL) '$(systbl)' $(sys)/syscall_64.tbl 'x86_64' > $@
 
 clean::

tools/perf/check-headers.sh +3 -0

index 73e723675c5f..2c10e46d98e9 100755
@@ -93,3 +93,6 @@ check arch/x86/lib/memcpy_64.S        '-I "^EXPORT_SYMBOL" -I "^#include <asm/ex
 check arch/x86/lib/memset_64.S        '-I "^EXPORT_SYMBOL" -I "^#include <asm/export.h>"'
 check include/uapi/asm-generic/mman.h '-I "^#include <\(uapi/\)*asm-generic/mman-common.h>"'
 check include/uapi/linux/mman.h       '-I "^#include <\(uapi/\)*asm/mman.h>"'
+
+# diff non-symmetric files
+check_2 arch/x86/entry/syscalls/syscall_64.tbl ../../arch/x86/entry/syscalls/syscall_64.tbl

Signature · 1 line

2.17.1

Em Fri, Jul 20, 2018 at 01:00:36PM +0200, Jiri Olsa escreveu:

Quoted 1 line

Probably leftover from the time we introducd the check-headers.sh script.

So, with your patch:

[acme@jouet perf]$ git diff

arch/x86/entry/syscalls/syscall_64.tbl +1 -0

index f0b1709a5ffb..cfc1b0949bb1 100644
@@ -343,6 +343,7 @@
 332    common  statx                   __x64_sys_statx
 333    common  io_pgetevents           __x64_sys_io_pgetevents
 334    common  rseq                    __x64_sys_rseq
+335    common  krava                   __x64_sys_krava
 
 #
 # x32-specific system call numbers start at 512 to avoid cache impact

[acme@jouet perf]$

It doesn't warn about that new cool syscall, without your patch:

Warning: Kernel ABI header at 'tools/perf/arch/x86/entry/syscalls/syscall_64.tbl' differs from latest version at 'arch/x86/entry/syscalls/syscall_64.tbl'

Can you check this?