2017-12-11 15:43:20

by Arnaldo Carvalho de Melo

[permalink] [raw]
Subject: Re: [PATCH v2] perf test shell: Fix check open filename arg using 'perf trace

Em Fri, Dec 08, 2017 at 06:48:17PM +0100, Michael Petlan escreveu:
> Hi Arnaldo, so I have tried what you've suggested and looks good.
> Patch is attached. Sorry for not posting it in-text, but I need to
> fix my mail client first, since it screwes some patches up due to
> flowed-text...
> Cheers,
> Michael

Thanks, applying.

- Arnaldo

> commit 92281a9ff73f98d8aca7595504340a25c92b9f1a
> Author: Michael Petlan <[email protected]>
> Date: Fri Dec 8 18:43:18 2017 +0100
>
> perf test shell: Fix check open filename arg using 'perf trace'
>
> The following commit added an exception for s390x to use openat()
> instead of open() in the test:
>
> commit f231af789b11a2f1a3795acc3228a3e178a80c21
> Author: Thomas Richter <[email protected]>
> Date: Tue Nov 14 08:18:46 2017 +0100
>
> Since the problem is not s390x-specific, this patch makes it more
> generic, so the test handles both open() and openat() no matter
> which architecture it is running on.
>
> Signed-off-by: Michael Petlan <[email protected]>
>
> diff --git a/tools/perf/tests/shell/trace+probe_vfs_getname.sh b/tools/perf/tests/shell/trace+probe_vfs_getname.sh
> index 2a9ef08..edd1073 100755
> --- a/tools/perf/tests/shell/trace+probe_vfs_getname.sh
> +++ b/tools/perf/tests/shell/trace+probe_vfs_getname.sh
> @@ -17,10 +17,9 @@ skip_if_no_perf_probe || exit 2
> file=$(mktemp /tmp/temporary_file.XXXXX)
>
> trace_open_vfs_getname() {
> - test "$(uname -m)" = s390x && { svc="openat"; txt="dfd: +CWD, +"; }
> -
> - perf trace -e ${svc:-open} touch $file 2>&1 | \
> - egrep " +[0-9]+\.[0-9]+ +\( +[0-9]+\.[0-9]+ ms\): +touch\/[0-9]+ ${svc:-open}\(${txt}filename: +${file}, +flags: CREAT\|NOCTTY\|NONBLOCK\|WRONLY, +mode: +IRUGO\|IWUGO\) += +[0-9]+$"
> + evts=`perf list syscalls:sys_enter_open* |& egrep 'open(at)? ' | sed -r 's/.*sys_enter_(.*) +\[.*/-e \1/'`
> + perf trace $evts touch $file 2>&1 | \
> + egrep " +[0-9]+\.[0-9]+ +\( +[0-9]+\.[0-9]+ ms\): +touch\/[0-9]+ open(at)?\((dfd: +CWD, +)?filename: +${file}, +flags: CREAT\|NOCTTY\|NONBLOCK\|WRONLY, +mode: +IRUGO\|IWUGO\) += +[0-9]+$"
> }
>
>


2017-12-11 17:07:56

by Arnaldo Carvalho de Melo

[permalink] [raw]
Subject: Re: [PATCH v2] perf test shell: Fix check open filename arg using 'perf trace

Em Mon, Dec 11, 2017 at 01:43:12PM -0200, Arnaldo de Melo escreveu:
> Em Fri, Dec 08, 2017 at 06:48:17PM +0100, Michael Petlan escreveu:
> > Hi Arnaldo, so I have tried what you've suggested and looks good.
> > Patch is attached. Sorry for not posting it in-text, but I need to
> > fix my mail client first, since it screwes some patches up due to
> > flowed-text...
> > Cheers,
> > Michael
>
> Thanks, applying.

It is not working here:

[root@jouet mnt]# perf test -v 62
62: Check open filename arg using perf trace + vfs_getname:
--- start ---
test child forked, pid 4919
Added new event:
probe:vfs_getname (on getname_flags:72 with pathname=result->name:string)

You can now use it in all perf tools, such as:

perf record -e probe:vfs_getname -aR sleep 1

test child finished with -1
---- end ----
Check open filename arg using perf trace + vfs_getname: FAILED!
[root@jouet mnt]#

and when I run this with strace -e open to see if it is picking the
right script, it is:

open("/home/acme/libexec/perf-core/tests/shell/record+script_probe_vfs_getname.sh", O_RDONLY) = 4
open("/home/acme/libexec/perf-core/tests/shell/trace+probe_libc_inet_pton.sh", O_RDONLY) = 4
open("/home/acme/libexec/perf-core/tests/shell/trace+probe_vfs_getname.sh", O_RDONLY) = 4
62: Check open filename arg using perf trace + vfs_getname:
--- start ---
test child forked, pid 4947
Added new event:
probe:vfs_getname (on getname_flags:72 with pathname=result->name:string)

You can now use it in all perf tools, such as:

perf record -e probe:vfs_getname -aR sleep 1

--- SIGCHLD {si_signo=SIGCHLD, si_code=CLD_EXITED, si_pid=4947, si_uid=0, si_status=255, si_utime=0, si_stime=0} ---
test child finished with -1
---- end ----
Check open filename arg using perf trace + vfs_getname: FAILED!
open("/home/acme/libexec/perf-core/tests/shell/probe_vfs_getname.sh", O_RDONLY) = 4
+++ exited with 0 +++
[root@jouet mnt]#

[acme@jouet linux]$ diff -u /home/acme/libexec/perf-core/tests/shell/probe_vfs_getname.sh tools/perf/tests/shell/probe_vfs_getname.sh
[acme@jouet linux]$ git log --oneline -1
0553ba305e4e (HEAD -> perf/core) perf test shell: Fix check open filename arg using 'perf trace
[acme@jouet linux]$

So can you please check if the file you're using is the one in this
patch submission?

- Arnaldo

2017-12-12 03:05:30

by Michael Petlan

[permalink] [raw]
Subject: Re: [PATCH v2] perf test shell: Fix check open filename arg using 'perf trace

On Mon, 11 Dec 2017, Arnaldo Carvalho de Melo wrote:
> It is not working here:

Hmm, I think I got it.

The following construction:

evts=`perf list syscalls:sys_enter_open* |& egrep 'open(at)? ' | sed -r 's/.*sys_enter_(.*) +\[.*/-e \1/'`

... expands to:

"-e open -e openat"

... which is a format that perf-trace does not seem to accept.
It simply overrides the first event with the second, thus it
only traces "openat". It worked for me, since I tested it on
aarch64 where $evts expanded to only one "-e" parameter.

Attaching a patch for perf-trace that should address it. It
seems to work, so perf-trace accepts "-e open -e openat".
However, one weak point is there:

"-e open,openat -e stat" works, while
"-e open -e openat,stat" does not.

In case you don't like the patch for perf-trace, the test's
patch would need to expand events to something else (which is
of course possible).

Also, on my system (x86_64, 4.15.0-rc1), I needed another
patch for the test (perf_test_shell_fix_filename_arg.patch),
because the variable name changed...

> [root@jouet mnt]# perf test -v 62
> 62: Check open filename arg using perf trace + vfs_getname:
> --- start ---
> test child forked, pid 4919
> Added new event:
> probe:vfs_getname (on getname_flags:72 with pathname=result->name:string)
>
> You can now use it in all perf tools, such as:
>
> perf record -e probe:vfs_getname -aR sleep 1
>
> test child finished with -1
> ---- end ----
> Check open filename arg using perf trace + vfs_getname: FAILED!
> [root@jouet mnt]#
>
> and when I run this with strace -e open to see if it is picking the
> right script, it is:
>
> open("/home/acme/libexec/perf-core/tests/shell/record+script_probe_vfs_getname.sh", O_RDONLY) = 4
> open("/home/acme/libexec/perf-core/tests/shell/trace+probe_libc_inet_pton.sh", O_RDONLY) = 4
> open("/home/acme/libexec/perf-core/tests/shell/trace+probe_vfs_getname.sh", O_RDONLY) = 4
> 62: Check open filename arg using perf trace + vfs_getname:
> --- start ---
> test child forked, pid 4947
> Added new event:
> probe:vfs_getname (on getname_flags:72 with pathname=result->name:string)
>
> You can now use it in all perf tools, such as:
>
> perf record -e probe:vfs_getname -aR sleep 1
>
> --- SIGCHLD {si_signo=SIGCHLD, si_code=CLD_EXITED, si_pid=4947, si_uid=0, si_status=255, si_utime=0, si_stime=0} ---
> test child finished with -1
> ---- end ----
> Check open filename arg using perf trace + vfs_getname: FAILED!
> open("/home/acme/libexec/perf-core/tests/shell/probe_vfs_getname.sh", O_RDONLY) = 4
> +++ exited with 0 +++
> [root@jouet mnt]#
>
> [acme@jouet linux]$ diff -u /home/acme/libexec/perf-core/tests/shell/probe_vfs_getname.sh tools/perf/tests/shell/probe_vfs_getname.sh
> [acme@jouet linux]$ git log --oneline -1
> 0553ba305e4e (HEAD -> perf/core) perf test shell: Fix check open filename arg using 'perf trace
> [acme@jouet linux]$
>
> So can you please check if the file you're using is the one in this
> patch submission?

So attaching all three patches to be sure. I have tested them on
two environments:

* x86_64 with 4.15.0-rc1
* aarch64 with 4.14.0

Both work with all three patches applied, applying on perf/core's
head (commit 26ea2ece7802f8fdaaacf321dbfb22de3271ab82).

>
> - Arnaldo
>

Thanks for your patience :)

Cheers,
Michael


Attachments:
perf_trace_support_multiple_-e_arguments.patch (1.85 kB)
perf_test_shell_fix_filename_arg.patch (691.00 B)
perf_test_shell_fix_check_open_filename_arg__v2.patch (1.74 kB)
Download all attachments

2017-12-12 14:57:14

by Arnaldo Carvalho de Melo

[permalink] [raw]
Subject: Re: [PATCH v2] perf test shell: Fix check open filename arg using 'perf trace

Em Tue, Dec 12, 2017 at 04:05:23AM +0100, Michael Petlan escreveu:
> On Mon, 11 Dec 2017, Arnaldo Carvalho de Melo wrote:
> > It is not working here:
>
> Hmm, I think I got it.
>
> The following construction:
>
> evts=`perf list syscalls:sys_enter_open* |& egrep 'open(at)? ' | sed -r 's/.*sys_enter_(.*) +\[.*/-e \1/'`
>
> ... expands to:
>
> "-e open -e openat"
>
> ... which is a format that perf-trace does not seem to accept.
> It simply overrides the first event with the second, thus it
> only traces "openat". It worked for me, since I tested it on
> aarch64 where $evts expanded to only one "-e" parameter.

Right, in this case 'perf trace -e' doesn't behave like -e in other
tools, which is unfortunate and should be fixed at some point.

> Attaching a patch for perf-trace that should address it. It
> seems to work, so perf-trace accepts "-e open -e openat".
> However, one weak point is there:
>
> "-e open,openat -e stat" works, while
> "-e open -e openat,stat" does not.
>
> In case you don't like the patch for perf-trace, the test's
> patch would need to expand events to something else (which is
> of course possible).
>
> Also, on my system (x86_64, 4.15.0-rc1), I needed another
> patch for the test (perf_test_shell_fix_filename_arg.patch),
> because the variable name changed...

the point here is not to add any new patch for perf trace, etc, but find
a way to fix just this test, so I think this works:

# evts=$(echo $(perf list syscalls:sys_enter_open* |& egrep 'open(at)? ' | sed -r 's/.*sys_enter_([a-z]+) +\[.*$/\1/') | sed 's/ /,/')
# echo $evts
open,openat
[root@jouet ~]#

I'll work on a 'perf list -x,' that will print all events matching as a
CSV with the informed delimiter, seems handy :-)

So the patch below does the trick for me, can you please check if does
for you?

With regards to the other patches, please consider submitting them in
separate messages, stating their purpose in a commit log, with example
usage, etc. Then people will be able to review it on its own.

Running it here I get:

[root@jouet perf]# sh -x tools/perf/tests/shell/trace+probe_vfs_getname.sh
++ dirname tools/perf/tests/shell/trace+probe_vfs_getname.sh
+ . tools/perf/tests/shell/lib/probe.sh
+ skip_if_no_perf_probe
+ perf probe
+ grep -q 'is not a perf-command'
+ return 0
++ dirname tools/perf/tests/shell/trace+probe_vfs_getname.sh
+ . tools/perf/tests/shell/lib/probe_vfs_getname.sh
++ perf probe -l
++ grep -q probe:vfs_getname
++ had_vfs_getname=1
++ mktemp /tmp/temporary_file.XXXXX
+ file=/tmp/temporary_file.pSzKC
+ add_probe_vfs_getname
+ local verbose=
+ '[' 1 -eq 1 ']'
++ perf probe -L getname_flags
++ egrep 'result.*=.*filename;'
++ sed -r 's/[[:space:]]+([[:digit:]]+)[[:space:]]+result->uptr.*/\1/'
+ line=72
+ perf probe 'vfs_getname=getname_flags:72 pathname=result->name:string'
Added new event:
probe:vfs_getname (on getname_flags:72 with pathname=result->name:string)

You can now use it in all perf tools, such as:

perf record -e probe:vfs_getname -aR sleep 1

+ err=0
+ '[' 0 -ne 0 ']'
+ trace_open_vfs_getname
++ uname -m
+ test x86_64 = s390x
++ sed 's/ /,/'
+++ egrep 'open(at)? '
+++ perf list 'syscalls:sys_enter_open*'
+++ sed -r 's/.*sys_enter_([a-z]+) +\[.*$/\1/'
++ echo open openat
+ svc=open,openat
+ perf trace -e open,openat touch /tmp/temporary_file.pSzKC
+ egrep ' +[0-9]+\.[0-9]+ +\( +[0-9]+\.[0-9]+ ms\): +touch\/[0-9]+ open(at)?\(filename: +/tmp/temporary_file.pSzKC, +flags: CREAT\|NOCTTY\|NONBLOCK\|WRONLY, +mode: +IRUGO\|IWUGO\) += +[0-9]+$'
1.479 ( 0.073 ms): touch/27884 open(filename: /tmp/temporary_file.pSzKC, flags: CREAT|NOCTTY|NONBLOCK|WRONLY, mode: IRUGO|IWUGO) = 3
+ err=0
+ rm -f /tmp/temporary_file.pSzKC
+ cleanup_probe_vfs_getname
+ '[' 1 -eq 1 ']'
+ perf probe -q -d probe:vfs_getname
+ exit 0
[root@jouet perf]#


- Arnaldo

diff --git a/tools/perf/tests/shell/trace+probe_vfs_getname.sh b/tools/perf/tests/shell/trace+probe_vfs_getname.sh
index 2a9ef080efd0..d22f08568226 100755
--- a/tools/perf/tests/shell/trace+probe_vfs_getname.sh
+++ b/tools/perf/tests/shell/trace+probe_vfs_getname.sh
@@ -17,10 +17,10 @@ skip_if_no_perf_probe || exit 2
file=$(mktemp /tmp/temporary_file.XXXXX)

trace_open_vfs_getname() {
- test "$(uname -m)" = s390x && { svc="openat"; txt="dfd: +CWD, +"; }
-
- perf trace -e ${svc:-open} touch $file 2>&1 | \
- egrep " +[0-9]+\.[0-9]+ +\( +[0-9]+\.[0-9]+ ms\): +touch\/[0-9]+ ${svc:-open}\(${txt}filename: +${file}, +flags: CREAT\|NOCTTY\|NONBLOCK\|WRONLY, +mode: +IRUGO\|IWUGO\) += +[0-9]+$"
+ test "$(uname -m)" = s390x && { txt="dfd: +CWD, +"; }
+ svc=$(echo $(perf list syscalls:sys_enter_open* |& egrep 'open(at)? ' | sed -r 's/.*sys_enter_([a-z]+) +\[.*$/\1/') | sed 's/ /,/')
+ perf trace -e ${svc} touch $file 2>&1 | \
+ egrep " +[0-9]+\.[0-9]+ +\( +[0-9]+\.[0-9]+ ms\): +touch\/[0-9]+ open(at)?\(${txt}filename: +${file}, +flags: CREAT\|NOCTTY\|NONBLOCK\|WRONLY, +mode: +IRUGO\|IWUGO\) += +[0-9]+$"
}



2017-12-12 15:19:54

by Arnaldo Carvalho de Melo

[permalink] [raw]
Subject: Re: [PATCH v2] perf test shell: Fix check open filename arg using 'perf trace

Em Tue, Dec 12, 2017 at 11:57:07AM -0300, Arnaldo Carvalho de Melo escreveu:
> So the patch below does the trick for me, can you please check if does
> for you?

So I've put this with a long explanation at:

https://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git/commit/?h=tmp.perf/core&id=ba15be2acc9e35dc9f034fef3aa292406b518b04

- Arnaldo

2017-12-12 16:24:49

by Michael Petlan

[permalink] [raw]
Subject: Re: [PATCH v2] perf test shell: Fix check open filename arg using 'perf trace

Hey Arnaldo, this one did not remove the s390x hack and also, won't work
on arm. Please use the one I just have sent, few seconds ago...

On Tue, 12 Dec 2017, Arnaldo Carvalho de Melo wrote:
> Em Tue, Dec 12, 2017 at 11:57:07AM -0300, Arnaldo Carvalho de Melo escreveu:
> > So the patch below does the trick for me, can you please check if does
> > for you?
>
> So I've put this with a long explanation at:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git/commit/?h=tmp.perf/core&id=ba15be2acc9e35dc9f034fef3aa292406b518b04
>
> - Arnaldo
>

2017-12-12 16:30:15

by Michael Petlan

[permalink] [raw]
Subject: Re: [PATCH v2] perf test shell: Fix check open filename arg using 'perf trace

On Tue, 12 Dec 2017, Arnaldo Carvalho de Melo wrote:
[...]
> the point here is not to add any new patch for perf trace, etc, but find
> a way to fix just this test, so I think this works:
>
> # evts=$(echo $(perf list syscalls:sys_enter_open* |& egrep 'open(at)? ' | sed -r 's/.*sys_enter_([a-z]+) +\[.*$/\1/') | sed 's/ /,/')
> # echo $evts
> open,openat
> [root@jouet ~]#

Just sent you patches with this approach.

> I'll work on a 'perf list -x,' that will print all events matching as a
> CSV with the informed delimiter, seems handy :-)

Really looking forward to have that. It will make all my parsing much easier!

> So the patch below does the trick for me, can you please check if does
> for you?

As said, the patch below does not remove the s390x-hack:

test "$(uname -m)" = s390x && { txt="dfd: +CWD, +"; }

My patch gets rid of this and makes it work on non-s390x openat-only envs
(which was the original purpose :))

>
> With regards to the other patches, please consider submitting them in
> separate messages, stating their purpose in a commit log, with example
> usage, etc. Then people will be able to review it on its own.

I have just done it.

>
> Running it here I get:
>
> [root@jouet perf]# sh -x tools/perf/tests/shell/trace+probe_vfs_getname.sh
> ++ dirname tools/perf/tests/shell/trace+probe_vfs_getname.sh
> + . tools/perf/tests/shell/lib/probe.sh
> + skip_if_no_perf_probe
> + perf probe
> + grep -q 'is not a perf-command'
> + return 0
> ++ dirname tools/perf/tests/shell/trace+probe_vfs_getname.sh
> + . tools/perf/tests/shell/lib/probe_vfs_getname.sh
> ++ perf probe -l
> ++ grep -q probe:vfs_getname
> ++ had_vfs_getname=1
> ++ mktemp /tmp/temporary_file.XXXXX
> + file=/tmp/temporary_file.pSzKC
> + add_probe_vfs_getname
> + local verbose=
> + '[' 1 -eq 1 ']'
> ++ perf probe -L getname_flags
> ++ egrep 'result.*=.*filename;'
> ++ sed -r 's/[[:space:]]+([[:digit:]]+)[[:space:]]+result->uptr.*/\1/'
> + line=72
> + perf probe 'vfs_getname=getname_flags:72 pathname=result->name:string'
> Added new event:
> probe:vfs_getname (on getname_flags:72 with pathname=result->name:string)
>
> You can now use it in all perf tools, such as:
>
> perf record -e probe:vfs_getname -aR sleep 1
>
> + err=0
> + '[' 0 -ne 0 ']'
> + trace_open_vfs_getname
> ++ uname -m
> + test x86_64 = s390x
> ++ sed 's/ /,/'
> +++ egrep 'open(at)? '
> +++ perf list 'syscalls:sys_enter_open*'
> +++ sed -r 's/.*sys_enter_([a-z]+) +\[.*$/\1/'
> ++ echo open openat
> + svc=open,openat
> + perf trace -e open,openat touch /tmp/temporary_file.pSzKC
> + egrep ' +[0-9]+\.[0-9]+ +\( +[0-9]+\.[0-9]+ ms\): +touch\/[0-9]+ open(at)?\(filename: +/tmp/temporary_file.pSzKC, +flags: CREAT\|NOCTTY\|NONBLOCK\|WRONLY, +mode: +IRUGO\|IWUGO\) += +[0-9]+$'
> 1.479 ( 0.073 ms): touch/27884 open(filename: /tmp/temporary_file.pSzKC, flags: CREAT|NOCTTY|NONBLOCK|WRONLY, mode: IRUGO|IWUGO) = 3
> + err=0
> + rm -f /tmp/temporary_file.pSzKC
> + cleanup_probe_vfs_getname
> + '[' 1 -eq 1 ']'
> + perf probe -q -d probe:vfs_getname
> + exit 0
> [root@jouet perf]#
>
>
> - Arnaldo
>
> diff --git a/tools/perf/tests/shell/trace+probe_vfs_getname.sh b/tools/perf/tests/shell/trace+probe_vfs_getname.sh
> index 2a9ef080efd0..d22f08568226 100755
> --- a/tools/perf/tests/shell/trace+probe_vfs_getname.sh
> +++ b/tools/perf/tests/shell/trace+probe_vfs_getname.sh
> @@ -17,10 +17,10 @@ skip_if_no_perf_probe || exit 2
> file=$(mktemp /tmp/temporary_file.XXXXX)
>
> trace_open_vfs_getname() {
> - test "$(uname -m)" = s390x && { svc="openat"; txt="dfd: +CWD, +"; }
> -
> - perf trace -e ${svc:-open} touch $file 2>&1 | \
> - egrep " +[0-9]+\.[0-9]+ +\( +[0-9]+\.[0-9]+ ms\): +touch\/[0-9]+ ${svc:-open}\(${txt}filename: +${file}, +flags: CREAT\|NOCTTY\|NONBLOCK\|WRONLY, +mode: +IRUGO\|IWUGO\) += +[0-9]+$"
> + test "$(uname -m)" = s390x && { txt="dfd: +CWD, +"; }
> + svc=$(echo $(perf list syscalls:sys_enter_open* |& egrep 'open(at)? ' | sed -r 's/.*sys_enter_([a-z]+) +\[.*$/\1/') | sed 's/ /,/')
> + perf trace -e ${svc} touch $file 2>&1 | \
> + egrep " +[0-9]+\.[0-9]+ +\( +[0-9]+\.[0-9]+ ms\): +touch\/[0-9]+ open(at)?\(${txt}filename: +${file}, +flags: CREAT\|NOCTTY\|NONBLOCK\|WRONLY, +mode: +IRUGO\|IWUGO\) += +[0-9]+$"
> }
>
>
>

2017-12-12 17:13:11

by Arnaldo Carvalho de Melo

[permalink] [raw]
Subject: Re: [PATCH v2] perf test shell: Fix check open filename arg using 'perf trace

Em Tue, Dec 12, 2017 at 05:24:43PM +0100, Michael Petlan escreveu:
> Hey Arnaldo, this one did not remove the s390x hack and also, won't work
> on arm. Please use the one I just have sent, few seconds ago...

Ok, I have you latest in, with the cset log I wrote, which clarifies
that this is not a arch issue, but a glibc one, its only that some
arches have distros where glibc was already at 2.26, where open() calls
are mapped to openat() syscalls.

- Arnaldo

> On Tue, 12 Dec 2017, Arnaldo Carvalho de Melo wrote:
> > Em Tue, Dec 12, 2017 at 11:57:07AM -0300, Arnaldo Carvalho de Melo escreveu:
> > > So the patch below does the trick for me, can you please check if does
> > > for you?
> >
> > So I've put this with a long explanation at:
> >
> > https://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git/commit/?h=tmp.perf/core&id=ba15be2acc9e35dc9f034fef3aa292406b518b04
> >
> > - Arnaldo
> >

2017-12-14 12:08:53

by Thomas-Mich Richter

[permalink] [raw]
Subject: Re: [PATCH v2] perf test shell: Fix check open filename arg using 'perf trace

On 12/12/2017 06:12 PM, Arnaldo Carvalho de Melo wrote:
> Em Tue, Dec 12, 2017 at 05:24:43PM +0100, Michael Petlan escreveu:
>> Hey Arnaldo, this one did not remove the s390x hack and also, won't work
>> on arm. Please use the one I just have sent, few seconds ago...
>
> Ok, I have you latest in, with the cset log I wrote, which clarifies
> that this is not a arch issue, but a glibc one, its only that some
> arches have distros where glibc was already at 2.26, where open() calls
> are mapped to openat() syscalls.
>
> - Arnaldo
>
>> On Tue, 12 Dec 2017, Arnaldo Carvalho de Melo wrote:
>>> Em Tue, Dec 12, 2017 at 11:57:07AM -0300, Arnaldo Carvalho de Melo escreveu:
>>>> So the patch below does the trick for me, can you please check if does
>>>> for you?
>>>
>>> So I've put this with a long explanation at:
>>>
>>> https://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git/commit/?h=tmp.perf/core&id=ba15be2acc9e35dc9f034fef3aa292406b518b04
>>>
>>> - Arnaldo
>>>
>

I have extracted the latest patch with commit id 6c23b4f
("perf test shell: Fix check open filename arg using 'perf trace'") from the git
repository perf/core today and it works for me.


--
Thomas Richter, Dept 3303, IBM LTC Boeblingen Germany
--
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen / Registergericht: Amtsgericht Stuttgart, HRB 243294

2017-12-14 14:03:47

by Arnaldo Carvalho de Melo

[permalink] [raw]
Subject: Re: [PATCH v2] perf test shell: Fix check open filename arg using 'perf trace

Em Thu, Dec 14, 2017 at 01:08:45PM +0100, Thomas-Mich Richter escreveu:
> On 12/12/2017 06:12 PM, Arnaldo Carvalho de Melo wrote:
> > Em Tue, Dec 12, 2017 at 05:24:43PM +0100, Michael Petlan escreveu:
> >> Hey Arnaldo, this one did not remove the s390x hack and also, won't work
> >> on arm. Please use the one I just have sent, few seconds ago...
> >
> > Ok, I have you latest in, with the cset log I wrote, which clarifies
> > that this is not a arch issue, but a glibc one, its only that some
> > arches have distros where glibc was already at 2.26, where open() calls
> > are mapped to openat() syscalls.
> >
> > - Arnaldo
> >
> >> On Tue, 12 Dec 2017, Arnaldo Carvalho de Melo wrote:
> >>> Em Tue, Dec 12, 2017 at 11:57:07AM -0300, Arnaldo Carvalho de Melo escreveu:
> >>>> So the patch below does the trick for me, can you please check if does
> >>>> for you?
> >>>
> >>> So I've put this with a long explanation at:
> >>>
> >>> https://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git/commit/?h=tmp.perf/core&id=ba15be2acc9e35dc9f034fef3aa292406b518b04
> >>>
> >>> - Arnaldo
> >>>
> >
>
> I have extracted the latest patch with commit id 6c23b4f
> ("perf test shell: Fix check open filename arg using 'perf trace'") from the git
> repository perf/core today and it works for me.

Thanks for checking, I'll add a Tested-by: you then.

- Arnaldo

Subject: [tip:perf/core] perf test shell: Fix check open filename arg using 'perf trace'

Commit-ID: 69b5c953400897a978f8a7d212c53aa90ff5027d
Gitweb: https://git.kernel.org/tip/69b5c953400897a978f8a7d212c53aa90ff5027d
Author: Michael Petlan <[email protected]>
AuthorDate: Tue, 12 Dec 2017 11:22:03 -0500
Committer: Arnaldo Carvalho de Melo <[email protected]>
CommitDate: Wed, 27 Dec 2017 12:15:56 -0300

perf test shell: Fix check open filename arg using 'perf trace'

Commit f231af789b11 ("perf test shell: Fix check open filename arg using
'perf trace' on s390x") added an exception for s390x to use openat()
instead of open() in the test that intercepts a open syscall to look for
the filename argument as obtained by the vfs_getname 'perf probe' it
puts in place at the getname_flags kernel function.

Its not just s390x that uses openat() instead of open(), so use 'perf
list' to look for the syscall:sys_enter_open(at)? present in the system
being tested instead of checking if the system is s390x.

In fact Namhyung pointed out that glibc 2.26 changed this behaviour, as
described in https://lwn.net/Articles/738694/, so systems where glibc is
>= 2.26 will need this patch for this test to work, which already took
place in some distros for architectures such as s390x, while Fedora 26
x86_64 is at glibc 2.25, i.e. still uses open().

Signed-off-by: Michael Petlan <[email protected]>
Tested-by: Arnaldo Carvalho de Melo <[email protected]>
Tested-by: Thomas Richter <[email protected]>
Link: https://lkml.kernel.org/r/[email protected]
Cc: Adrian Hunter <[email protected]>
Cc: David Ahern <[email protected]>
Cc: Hendrik Brueckner <[email protected]>
Cc: Jiri Olsa <[email protected]>
Cc: Namhyung Kim <[email protected]>
Cc: Wang Nan <[email protected]>
LPU-Reference: [email protected]
Link: https://lkml.kernel.org/n/[email protected]
Signed-off-by: Arnaldo Carvalho de Melo <[email protected]>
---
tools/perf/tests/shell/trace+probe_vfs_getname.sh | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/tools/perf/tests/shell/trace+probe_vfs_getname.sh b/tools/perf/tests/shell/trace+probe_vfs_getname.sh
index 2a9ef08..55ad979 100755
--- a/tools/perf/tests/shell/trace+probe_vfs_getname.sh
+++ b/tools/perf/tests/shell/trace+probe_vfs_getname.sh
@@ -17,10 +17,9 @@ skip_if_no_perf_probe || exit 2
file=$(mktemp /tmp/temporary_file.XXXXX)

trace_open_vfs_getname() {
- test "$(uname -m)" = s390x && { svc="openat"; txt="dfd: +CWD, +"; }
-
- perf trace -e ${svc:-open} touch $file 2>&1 | \
- egrep " +[0-9]+\.[0-9]+ +\( +[0-9]+\.[0-9]+ ms\): +touch\/[0-9]+ ${svc:-open}\(${txt}filename: +${file}, +flags: CREAT\|NOCTTY\|NONBLOCK\|WRONLY, +mode: +IRUGO\|IWUGO\) += +[0-9]+$"
+ evts=$(echo $(perf list syscalls:sys_enter_open* |& egrep 'open(at)? ' | sed -r 's/.*sys_enter_([a-z]+) +\[.*$/\1/') | sed 's/ /,/')
+ perf trace -e $evts touch $file 2>&1 | \
+ egrep " +[0-9]+\.[0-9]+ +\( +[0-9]+\.[0-9]+ ms\): +touch\/[0-9]+ open(at)?\((dfd: +CWD, +)?filename: +${file}, +flags: CREAT\|NOCTTY\|NONBLOCK\|WRONLY, +mode: +IRUGO\|IWUGO\) += +[0-9]+$"
}