Re: [PATCH] perf: fix perf test case 14 result reporting

Re: [PATCH] perf: fix perf test case 14 result reporting

[Thomas-Mich Richter]

On 06/01/2017 11:04 PM, Jiri Olsa wrote:
> On Thu, Jun 01, 2017 at 10:20:38AM -0300, Arnaldo Carvalho de Melo wrote:
>> Em Thu, Jun 01, 2017 at 02:34:41PM +0200, Thomas Richter escreveu:
>>> Command perf test -v 14 (Setup struct perf_event_attr test)
>>> always reports success even if the test case fails.
>>> It works correctly if you also specify -F (for don't fork).
>>
>> Thanks for working on this, adding Jiri Olsa, that wrote this test
>> harness, so that he can check and provide his Acked-by or Reviewed-by,
>> Jiri?
>>
>> - Arnaldo
>>  
>>>    root@s35lp76 perf]# ./perf test -v 14
>>>    14: Setup struct perf_event_attr               :
>>>    --- start ---
>>>    running './tests/attr/test-record-no-delay'
>>>    [ perf record: Woken up 1 times to write data ]
>>>    [ perf record: Captured and wrote 0.002 MB /tmp/tmp4E1h7R/perf.data
>>>      (1 samples) ]
>>>    expected task=0, got 1
>>>    expected precise_ip=0, got 3
>>>    expected wakeup_events=1, got 0
>>>    FAILED './tests/attr/test-record-no-delay' - match failure
>>>    test child finished with 0
>>>    ---- end ----
>>>    Setup struct perf_event_attr: Ok
>>>
>>> The reason for the wrong error reporting is the return value of the
>>> system() library call. It is called in run_dir() file tests/attr.c
>>> and returns the exit status, in above case 0xff00.
>>> This value is given as parameter to the exit() function which
>>> can only handle values 0-0xff.
>>> The child process terminates with exit value of 0 and the parent
>>> does not detect any error.
>>>
>>> This patch corrects the error reporting and prints the
>>> correct test result.
>>>
>>> Signed-off-by: Thomas Richter <tmricht@linux.vnet.ibm.com>
>>> Reviewed-by: Hendrik Brueckner <brueckner@linux.vnet.ibm.com>
>>> ---
>>>  tools/perf/tests/attr.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/tools/perf/tests/attr.c b/tools/perf/tests/attr.c
>>> index 88dc51f..131b510 100644
>>> --- a/tools/perf/tests/attr.c
>>> +++ b/tools/perf/tests/attr.c
>>> @@ -150,7 +150,7 @@ static int run_dir(const char *d, const char *perf)
>>>  	snprintf(cmd, 3*PATH_MAX, PYTHON " %s/attr.py -d %s/attr/ -p %s %.*s",
>>>  		 d, d, perf, vcnt, v);
>>>  
>>> -	return system(cmd);
>>> +	return system(cmd) ? TEST_FAIL : TEST_OK;
>>>  }
>>>  
>>>  int test__attr(int subtest __maybe_unused)
>>> -- 
>>> 2.9.3
> 
> seems ok, however "perf test attr" is broken ATM, since it wasn't updated
> for some time as it showed false 'Ok'
> 
> I started fixing it some time ago, but got distracted, if you are
> interested, you're welcome to pick up from my branch ;-)
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/jolsa/perf.git
>   perf/attr_test
> 
> thanks,
> jirka
> 

I have started work on perf tool very recently to get s390 support
working and up to date.

I downloaded your branch and discovered you have already fixed
another issue I run into this week. For example 
commit 070b9644981e ("perf tests attr: Do not store failed events")

I can certainly help you to get this test case operational again.
Maybe you need to pull some of your patches out of your backlog
and submit them the kernel to get to a common base to work on.

I suggest we should move the discussion to the linux-perf-users
mailing list.

Your thoughts?

-- 
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] perf: fix perf test case 14 result reporting

[Arnaldo Carvalho de Melo]

Em Fri, Jun 02, 2017 at 11:58:46AM +0200, Thomas-Mich Richter escreveu:
> On 06/01/2017 11:04 PM, Jiri Olsa wrote:
> > On Thu, Jun 01, 2017 at 10:20:38AM -0300, Arnaldo Carvalho de Melo wrote:
> >> Em Thu, Jun 01, 2017 at 02:34:41PM +0200, Thomas Richter escreveu:
> >>> Command perf test -v 14 (Setup struct perf_event_attr test)
> >>> always reports success even if the test case fails.
> >>> It works correctly if you also specify -F (for don't fork).
> >>
> >> Thanks for working on this, adding Jiri Olsa, that wrote this test
> >> harness, so that he can check and provide his Acked-by or Reviewed-by,
> >> Jiri?
> >>
> >> - Arnaldo
> >>  
> >>>    root@s35lp76 perf]# ./perf test -v 14
> >>>    14: Setup struct perf_event_attr               :
> >>>    --- start ---
> >>>    running './tests/attr/test-record-no-delay'
> >>>    [ perf record: Woken up 1 times to write data ]
> >>>    [ perf record: Captured and wrote 0.002 MB /tmp/tmp4E1h7R/perf.data
> >>>      (1 samples) ]
> >>>    expected task=0, got 1
> >>>    expected precise_ip=0, got 3
> >>>    expected wakeup_events=1, got 0
> >>>    FAILED './tests/attr/test-record-no-delay' - match failure
> >>>    test child finished with 0
> >>>    ---- end ----
> >>>    Setup struct perf_event_attr: Ok
> >>>
> >>> The reason for the wrong error reporting is the return value of the
> >>> system() library call. It is called in run_dir() file tests/attr.c
> >>> and returns the exit status, in above case 0xff00.
> >>> This value is given as parameter to the exit() function which
> >>> can only handle values 0-0xff.
> >>> The child process terminates with exit value of 0 and the parent
> >>> does not detect any error.
> >>>
> >>> This patch corrects the error reporting and prints the
> >>> correct test result.
> >>>
> >>> Signed-off-by: Thomas Richter <tmricht@linux.vnet.ibm.com>
> >>> Reviewed-by: Hendrik Brueckner <brueckner@linux.vnet.ibm.com>
> >>> ---
> >>>  tools/perf/tests/attr.c | 2 +-
> >>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>> diff --git a/tools/perf/tests/attr.c b/tools/perf/tests/attr.c
> >>> index 88dc51f..131b510 100644
> >>> --- a/tools/perf/tests/attr.c
> >>> +++ b/tools/perf/tests/attr.c
> >>> @@ -150,7 +150,7 @@ static int run_dir(const char *d, const char *perf)
> >>>  	snprintf(cmd, 3*PATH_MAX, PYTHON " %s/attr.py -d %s/attr/ -p %s %.*s",
> >>>  		 d, d, perf, vcnt, v);
> >>>  
> >>> -	return system(cmd);
> >>> +	return system(cmd) ? TEST_FAIL : TEST_OK;
> >>>  }
> >>>  
> >>>  int test__attr(int subtest __maybe_unused)
> >>> -- 
> >>> 2.9.3
> > 
> > seems ok, however "perf test attr" is broken ATM, since it wasn't updated
> > for some time as it showed false 'Ok'
> > 
> > I started fixing it some time ago, but got distracted, if you are
> > interested, you're welcome to pick up from my branch ;-)
> > 
> >   git://git.kernel.org/pub/scm/linux/kernel/git/jolsa/perf.git
> >   perf/attr_test
> > 
> > thanks,
> > jirka
> > 
> 
> I have started work on perf tool very recently to get s390 support
> working and up to date.
> 
> I downloaded your branch and discovered you have already fixed
> another issue I run into this week. For example 
> commit 070b9644981e ("perf tests attr: Do not store failed events")
> 
> I can certainly help you to get this test case operational again.
> Maybe you need to pull some of your patches out of your backlog
> and submit them the kernel to get to a common base to work on.
> 
> I suggest we should move the discussion to the linux-perf-users
> mailing list.
> 
> Your thoughts?

If you have specific patches in Jiri's branch that you think are good to
go, just point me to them and I'll cherry-pick them.

I'm looking now at the one you pointed out above (070b9644981e).

Thanks,

- Arnaldo

Re: [PATCH] perf: fix perf test case 14 result reporting

[Arnaldo Carvalho de Melo]

Em Fri, Jun 02, 2017 at 11:09:36AM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Fri, Jun 02, 2017 at 11:58:46AM +0200, Thomas-Mich Richter escreveu:
> > On 06/01/2017 11:04 PM, Jiri Olsa wrote:
> > > On Thu, Jun 01, 2017 at 10:20:38AM -0300, Arnaldo Carvalho de Melo wrote:
> > >> Em Thu, Jun 01, 2017 at 02:34:41PM +0200, Thomas Richter escreveu:
> > >>> Command perf test -v 14 (Setup struct perf_event_attr test)
> > >>> always reports success even if the test case fails.
> > >>> It works correctly if you also specify -F (for don't fork).
> > >>
> > >> Thanks for working on this, adding Jiri Olsa, that wrote this test
> > >> harness, so that he can check and provide his Acked-by or Reviewed-by,
> > >> Jiri?
> > >>
> > >> - Arnaldo
> > >>  
> > >>>    root@s35lp76 perf]# ./perf test -v 14
> > >>>    14: Setup struct perf_event_attr               :
> > >>>    --- start ---
> > >>>    running './tests/attr/test-record-no-delay'
> > >>>    [ perf record: Woken up 1 times to write data ]
> > >>>    [ perf record: Captured and wrote 0.002 MB /tmp/tmp4E1h7R/perf.data
> > >>>      (1 samples) ]
> > >>>    expected task=0, got 1
> > >>>    expected precise_ip=0, got 3
> > >>>    expected wakeup_events=1, got 0
> > >>>    FAILED './tests/attr/test-record-no-delay' - match failure
> > >>>    test child finished with 0
> > >>>    ---- end ----
> > >>>    Setup struct perf_event_attr: Ok
> > >>>
> > >>> The reason for the wrong error reporting is the return value of the
> > >>> system() library call. It is called in run_dir() file tests/attr.c
> > >>> and returns the exit status, in above case 0xff00.
> > >>> This value is given as parameter to the exit() function which
> > >>> can only handle values 0-0xff.
> > >>> The child process terminates with exit value of 0 and the parent
> > >>> does not detect any error.
> > >>>
> > >>> This patch corrects the error reporting and prints the
> > >>> correct test result.
> > >>>
> > >>> Signed-off-by: Thomas Richter <tmricht@linux.vnet.ibm.com>
> > >>> Reviewed-by: Hendrik Brueckner <brueckner@linux.vnet.ibm.com>
> > >>> ---
> > >>>  tools/perf/tests/attr.c | 2 +-
> > >>>  1 file changed, 1 insertion(+), 1 deletion(-)
> > >>>
> > >>> diff --git a/tools/perf/tests/attr.c b/tools/perf/tests/attr.c
> > >>> index 88dc51f..131b510 100644
> > >>> --- a/tools/perf/tests/attr.c
> > >>> +++ b/tools/perf/tests/attr.c
> > >>> @@ -150,7 +150,7 @@ static int run_dir(const char *d, const char *perf)
> > >>>  	snprintf(cmd, 3*PATH_MAX, PYTHON " %s/attr.py -d %s/attr/ -p %s %.*s",
> > >>>  		 d, d, perf, vcnt, v);
> > >>>  
> > >>> -	return system(cmd);
> > >>> +	return system(cmd) ? TEST_FAIL : TEST_OK;
> > >>>  }
> > >>>  
> > >>>  int test__attr(int subtest __maybe_unused)
> > >>> -- 
> > >>> 2.9.3
> > > 
> > > seems ok, however "perf test attr" is broken ATM, since it wasn't updated
> > > for some time as it showed false 'Ok'
> > > 
> > > I started fixing it some time ago, but got distracted, if you are
> > > interested, you're welcome to pick up from my branch ;-)
> > > 
> > >   git://git.kernel.org/pub/scm/linux/kernel/git/jolsa/perf.git
> > >   perf/attr_test
> > > 
> > > thanks,
> > > jirka
> > > 
> > 
> > I have started work on perf tool very recently to get s390 support
> > working and up to date.
> > 
> > I downloaded your branch and discovered you have already fixed
> > another issue I run into this week. For example 
> > commit 070b9644981e ("perf tests attr: Do not store failed events")
> > 
> > I can certainly help you to get this test case operational again.
> > Maybe you need to pull some of your patches out of your backlog
> > and submit them the kernel to get to a common base to work on.
> > 
> > I suggest we should move the discussion to the linux-perf-users
> > mailing list.
> > 
> > Your thoughts?
> 
> If you have specific patches in Jiri's branch that you think are good to
> go, just point me to them and I'll cherry-pick them.
> 
> I'm looking now at the one you pointed out above (070b9644981e).

Just looked, but the cset comment should state what is the problem and
how it is solved, right now it has just a one line summary :-\

- Arnaldo

Re: [PATCH] perf: fix perf test case 14 result reporting

[Jiri Olsa]

On Fri, Jun 02, 2017 at 11:11:05AM -0300, Arnaldo Carvalho de Melo wrote:

SNIP

> > > 
> > > I suggest we should move the discussion to the linux-perf-users
> > > mailing list.
> > > 
> > > Your thoughts?
> > 
> > If you have specific patches in Jiri's branch that you think are good to
> > go, just point me to them and I'll cherry-pick them.
> > 
> > I'm looking now at the one you pointed out above (070b9644981e).
> 
> Just looked, but the cset comment should state what is the problem and
> how it is solved, right now it has just a one line summary :-\

yep, sry ;-)

jirka

Re: [PATCH] perf: fix perf test case 14 result reporting

[Thomas-Mich Richter]

On 06/02/2017 04:11 PM, Arnaldo Carvalho de Melo wrote:

[....]

>>
>> If you have specific patches in Jiri's branch that you think are good to
>> go, just point me to them and I'll cherry-pick them.
>>
>> I'm looking now at the one you pointed out above (070b9644981e).
> 
> Just looked, but the cset comment should state what is the problem and
> how it is solved, right now it has just a one line summary :-\
> 
> - Arnaldo
> 

Looks like a misunderstanding. When I clone Jiri's tree and checkout
branch perf/attr_test:

url = git://git.kernel.org/pub/scm/linux/kernel/git/jolsa/perf.git

oc2666213455 12 $ git branch
  master
* perf/attr_test
  perf/stat
oc2666213455 13 $ git show 070b9644981e
commit 070b9644981e2dd160a6aae2723d0ec2d8b4c0b5
Author: Jiri Olsa <jolsa@kernel.org>
Date:   Fri Mar 3 15:59:45 2017 +0100

    perf tests attr: Do not store failed events

diff --git a/tools/perf/tests/attr.c b/tools/perf/tests/attr.c
index 0dd7749..0f2b619 100644
--- a/tools/perf/tests/attr.c
+++ b/tools/perf/tests/attr.c
@@ -138,7 +138,7 @@ void test_attr__open(struct perf_event_attr *attr, pid_t pid, int cpu,
 {
        int errno_saved = errno;
 
-       if (store_event(attr, pid, cpu, fd, group_fd, flags))
+       if ((fd != -1) && store_event(attr, pid, cpu, fd, group_fd, flags))
                die("test attr FAILED");
 
        errno = errno_saved;
oc2666213455 14 $ 

I get this commit which is another fix for an issue I discovered last week
while working on test case 14.

-- 
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] perf: fix perf test case 14 result reporting

[Jiri Olsa]

On Fri, Jun 02, 2017 at 11:58:46AM +0200, Thomas-Mich Richter wrote:

SNBIP

> > I started fixing it some time ago, but got distracted, if you are
> > interested, you're welcome to pick up from my branch ;-)
> > 
> >   git://git.kernel.org/pub/scm/linux/kernel/git/jolsa/perf.git
> >   perf/attr_test
> > 
> > thanks,
> > jirka
> > 
> 
> I have started work on perf tool very recently to get s390 support
> working and up to date.
> 
> I downloaded your branch and discovered you have already fixed
> another issue I run into this week. For example 
> commit 070b9644981e ("perf tests attr: Do not store failed events")
> 
> I can certainly help you to get this test case operational again.
> Maybe you need to pull some of your patches out of your backlog
> and submit them the kernel to get to a common base to work on.
> 
> I suggest we should move the discussion to the linux-perf-users
> mailing list.
> 
> Your thoughts?

good idea, for me it'd be best if you pull them and change them
as you see fit.. I dont think I'll get to it any time soon

thanks,
jirka