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

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

[Arnaldo de Melo]

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 <mpetlan@redhat.com>
> 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 <tmricht@linux.vnet.ibm.com>
>       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 <mpetlan@redhat.com>
> 
> 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]+$"
>  }
>  
>  

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

[Arnaldo Carvalho de Melo]

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

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

[Michael Petlan]

[-- Attachment #1: Type: text/plain, Size: 3314 bytes --]

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

[-- Attachment #2: Type: text/plain, Size: 1899 bytes --]

commit b8abd0f49d680c9f943ac460a5eba291424bb6d0
Author: Michael Petlan <mpetlan@redhat.com>
Date:   Tue Dec 12 03:43:11 2017 +0100

    perf trace: support multiple -e arguments
    
    Other perf subcommands, such as perf-stat allow multiple event specification
    arguments, like "perf stat -e evt1 -e evt2 ...".
    
    This patch enables perf-trace to allow that too.
    
    Before:
         0.030 ( 0.004 ms): true/19173 close(fd: 3) = 0
         0.089 ( 0.007 ms): true/19173 close(fd: 3) = 0
    
    After:
         0.018 ( 0.013 ms): true/19175 open(filename: /etc/ld.so.cache, flags: CLOEXEC) = 3
         0.040 ( 0.004 ms): true/19175 close(fd: 3</etc/ld.so.cache>) = 0
         0.053 ( 0.014 ms): true/19175 open(filename: /lib64/libc.so.6, flags: CLOEXEC) = 3
         0.107 ( 0.006 ms): true/19175 close(fd: 3</lib64/libc.so.6>) = 0
    
    Signed-off-by: Michael Petlan <mpetlan@redhat.com>

diff --git a/tools/perf/builtin-trace.c b/tools/perf/builtin-trace.c
index 7c57898..abcfdcc 100644
--- a/tools/perf/builtin-trace.c
+++ b/tools/perf/builtin-trace.c
@@ -2935,10 +2935,18 @@ static int trace__parse_events_option(const struct option *opt, const char *str,
 			.dirname = strace_groups_dir,
 		};
 
-		trace->ev_qualifier = strlist__new(lists[1], &slist_config);
 		if (trace->ev_qualifier == NULL) {
-			fputs("Not enough memory to parse event qualifier", trace->output);
-			goto out;
+			trace->ev_qualifier = strlist__new(lists[1], &slist_config);
+			if (trace->ev_qualifier == NULL) {
+				fputs("Not enough memory to parse event qualifier", trace->output);
+				goto out;
+			}
+		} else {
+			int rv = strlist__add(trace->ev_qualifier, lists[1]);
+			if (rv != 0) {
+				fputs("A problem occurred when parsing event qualifier", trace->output);
+				goto out;
+			}
 		}
 
 		if (trace__validate_ev_qualifier(trace))

[-- Attachment #3: Type: text/plain, Size: 691 bytes --]

diff --git a/tools/perf/tests/shell/lib/probe_vfs_getname.sh b/tools/perf/tests/shell/lib/probe_vfs_getname.sh
index 30a950c..3759582 100644
--- a/tools/perf/tests/shell/lib/probe_vfs_getname.sh
+++ b/tools/perf/tests/shell/lib/probe_vfs_getname.sh
@@ -13,7 +13,7 @@ add_probe_vfs_getname() {
 	local verbose=$1
 	if [ $had_vfs_getname -eq 1 ] ; then
 		line=$(perf probe -L getname_flags 2>&1 | egrep 'result.*=.*filename;' | sed -r 's/[[:space:]]+([[:digit:]]+)[[:space:]]+result->uptr.*/\1/')
-		perf probe $verbose "vfs_getname=getname_flags:${line} pathname=result->name:string"
+		perf probe $verbose "vfs_getname=getname_flags:${line} pathname=filename:string"
 	fi
 }
 

[-- Attachment #4: Type: text/plain, Size: 1785 bytes --]

commit 92281a9ff73f98d8aca7595504340a25c92b9f1a
Author: Michael Petlan <mpetlan@redhat.com>
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 <tmricht@linux.vnet.ibm.com>
      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 <mpetlan@redhat.com>

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]+$"
 }
 
 

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

[Arnaldo Carvalho de Melo]

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]+$"
 }
 
 

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

[Arnaldo Carvalho de Melo]

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

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

[Michael Petlan]

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
> 

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

[Arnaldo Carvalho de Melo]

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

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

[Thomas-Mich Richter]

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

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

[Arnaldo Carvalho de Melo]

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

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

[tip-bot for Michael Petlan]

Commit-ID:  69b5c953400897a978f8a7d212c53aa90ff5027d
Gitweb:     https://git.kernel.org/tip/69b5c953400897a978f8a7d212c53aa90ff5027d
Author:     Michael Petlan <mpetlan@redhat.com>
AuthorDate: Tue, 12 Dec 2017 11:22:03 -0500
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
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 <mpetlan@redhat.com>
Tested-by: Arnaldo Carvalho de Melo <acme@redhat.com>
Tested-by: Thomas Richter <tmricht@linux.vnet.ibm.com>
Link: https://lkml.kernel.org/r/ab23fe42-1080-a46b-503e-744e097f414f@linux.vnet.ibm.com
Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: David Ahern <dsahern@gmail.com>
Cc: Hendrik Brueckner <brueckner@linux.vnet.ibm.com>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Wang Nan <wangnan0@huawei.com>
LPU-Reference: 1275675985.12835754.1513095723265.JavaMail.zimbra@redhat.com
Link: https://lkml.kernel.org/n/tip-j2wbz9av1rw3thr3t0g4dtuk@git.kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 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]+$"
 }
 
 

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

[Michael Petlan]

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]+$"
>  }
>  
>  
>