2022-11-21 10:50:59

by Naveen N. Rao

Subject: [PATCH] perf test: Skip watchpoint tests if no watchpoints available

On IBM Power9, perf watchpoint tests fail since no hardware breakpoints
are available. Detect this by checking the error returned by
perf_event_open() and skip the tests in that case.

Reported-by: Disha Goel <[email protected]>
Signed-off-by: Naveen N. Rao <[email protected]>
---
tools/perf/tests/wp.c | 12 +++++++-----
1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/tools/perf/tests/wp.c b/tools/perf/tests/wp.c
index 56455da30341b4..cc8719609b19ea 100644
--- a/tools/perf/tests/wp.c
+++ b/tools/perf/tests/wp.c
@@ -59,8 +59,10 @@ static int __event(int wp_type, void *wp_addr, unsigned long wp_len)
get__perf_event_attr(&attr, wp_type, wp_addr, wp_len);
fd = sys_perf_event_open(&attr, 0, -1, -1,
perf_event_open_cloexec_flag());
- if (fd < 0)
+ if (fd < 0) {
+ fd = -errno;
pr_debug("failed opening event %x\n", attr.bp_type);
+ }

return fd;
}
@@ -77,7 +79,7 @@ static int test__wp_ro(struct test_suite *test __maybe_unused,

fd = __event(HW_BREAKPOINT_R, (void *)&data1, sizeof(data1));
if (fd < 0)
- return -1;
+ return fd == -ENODEV ? TEST_SKIP : -1;

tmp = data1;
WP_TEST_ASSERT_VAL(fd, "RO watchpoint", 1);
@@ -101,7 +103,7 @@ static int test__wp_wo(struct test_suite *test __maybe_unused,

fd = __event(HW_BREAKPOINT_W, (void *)&data1, sizeof(data1));
if (fd < 0)
- return -1;
+ return fd == -ENODEV ? TEST_SKIP : -1;

tmp = data1;
WP_TEST_ASSERT_VAL(fd, "WO watchpoint", 0);
@@ -126,7 +128,7 @@ static int test__wp_rw(struct test_suite *test __maybe_unused,
fd = __event(HW_BREAKPOINT_R | HW_BREAKPOINT_W, (void *)&data1,
sizeof(data1));
if (fd < 0)
- return -1;
+ return fd == -ENODEV ? TEST_SKIP : -1;

tmp = data1;
WP_TEST_ASSERT_VAL(fd, "RW watchpoint", 1);
@@ -150,7 +152,7 @@ static int test__wp_modify(struct test_suite *test __maybe_unused, int subtest _

fd = __event(HW_BREAKPOINT_W, (void *)&data1, sizeof(data1));
if (fd < 0)
- return -1;
+ return fd == -ENODEV ? TEST_SKIP : -1;

data1 = tmp;
WP_TEST_ASSERT_VAL(fd, "Modify watchpoint", 1);

base-commit: 63a3bf5e8d9e79ce456c8f73d4395a5a51d841b1
--
2.38.1

2022-11-22 19:40:16

by Christophe Leroy

Subject: Re: [PATCH] perf test: Skip watchpoint tests if no watchpoints available

Le 21/11/2022 à 11:27, Naveen N. Rao a écrit :
> On IBM Power9, perf watchpoint tests fail since no hardware breakpoints
> are available. Detect this by checking the error returned by
> perf_event_open() and skip the tests in that case.
>
> Reported-by: Disha Goel <[email protected]>
> Signed-off-by: Naveen N. Rao <[email protected]>
> ---
> tools/perf/tests/wp.c | 12 +++++++-----
> 1 file changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/tools/perf/tests/wp.c b/tools/perf/tests/wp.c
> index 56455da30341b4..cc8719609b19ea 100644
> --- a/tools/perf/tests/wp.c
> +++ b/tools/perf/tests/wp.c
> @@ -59,8 +59,10 @@ static int __event(int wp_type, void *wp_addr, unsigned long wp_len)
> get__perf_event_attr(&attr, wp_type, wp_addr, wp_len);
> fd = sys_perf_event_open(&attr, 0, -1, -1,
> perf_event_open_cloexec_flag());
> - if (fd < 0)
> + if (fd < 0) {
> + fd = -errno;
> pr_debug("failed opening event %x\n", attr.bp_type);
> + }

Do you really need that ?

Can't you directly check errno in the caller ?

>
> return fd;
> }
> @@ -77,7 +79,7 @@ static int test__wp_ro(struct test_suite *test __maybe_unused,
>
> fd = __event(HW_BREAKPOINT_R, (void *)&data1, sizeof(data1));
> if (fd < 0)
> - return -1;
> + return fd == -ENODEV ? TEST_SKIP : -1;
>
> tmp = data1;
> WP_TEST_ASSERT_VAL(fd, "RO watchpoint", 1);
> @@ -101,7 +103,7 @@ static int test__wp_wo(struct test_suite *test __maybe_unused,
>
> fd = __event(HW_BREAKPOINT_W, (void *)&data1, sizeof(data1));
> if (fd < 0)
> - return -1;
> + return fd == -ENODEV ? TEST_SKIP : -1;
>
> tmp = data1;
> WP_TEST_ASSERT_VAL(fd, "WO watchpoint", 0);
> @@ -126,7 +128,7 @@ static int test__wp_rw(struct test_suite *test __maybe_unused,
> fd = __event(HW_BREAKPOINT_R | HW_BREAKPOINT_W, (void *)&data1,
> sizeof(data1));
> if (fd < 0)
> - return -1;
> + return fd == -ENODEV ? TEST_SKIP : -1;
>
> tmp = data1;
> WP_TEST_ASSERT_VAL(fd, "RW watchpoint", 1);
> @@ -150,7 +152,7 @@ static int test__wp_modify(struct test_suite *test __maybe_unused, int subtest _
>
> fd = __event(HW_BREAKPOINT_W, (void *)&data1, sizeof(data1));
> if (fd < 0)
> - return -1;
> + return fd == -ENODEV ? TEST_SKIP : -1;
>
> data1 = tmp;
> WP_TEST_ASSERT_VAL(fd, "Modify watchpoint", 1);
>
> base-commit: 63a3bf5e8d9e79ce456c8f73d4395a5a51d841b1

2022-11-22 21:28:23

by Ian Rogers

Subject: Re: [PATCH] perf test: Skip watchpoint tests if no watchpoints available

On Tue, Nov 22, 2022 at 11:19 AM Christophe Leroy
<[email protected]> wrote:
>
>
>
> Le 21/11/2022 à 11:27, Naveen N. Rao a écrit :
> > On IBM Power9, perf watchpoint tests fail since no hardware breakpoints
> > are available. Detect this by checking the error returned by
> > perf_event_open() and skip the tests in that case.
> >
> > Reported-by: Disha Goel <[email protected]>
> > Signed-off-by: Naveen N. Rao <[email protected]>
> > ---
> > tools/perf/tests/wp.c | 12 +++++++-----
> > 1 file changed, 7 insertions(+), 5 deletions(-)
> >
> > diff --git a/tools/perf/tests/wp.c b/tools/perf/tests/wp.c
> > index 56455da30341b4..cc8719609b19ea 100644
> > --- a/tools/perf/tests/wp.c
> > +++ b/tools/perf/tests/wp.c
> > @@ -59,8 +59,10 @@ static int __event(int wp_type, void *wp_addr, unsigned long wp_len)
> > get__perf_event_attr(&attr, wp_type, wp_addr, wp_len);
> > fd = sys_perf_event_open(&attr, 0, -1, -1,
> > perf_event_open_cloexec_flag());
> > - if (fd < 0)
> > + if (fd < 0) {
> > + fd = -errno;
> > pr_debug("failed opening event %x\n", attr.bp_type);
> > + }
>
> Do you really need that ?
>
> Can't you directly check errno in the caller ?

errno is very easily clobbered and not clearly set on success - ie,
it'd be better not to do that.

Acked-by: Ian Rogers <[email protected]>

Thanks,
Ian

> >
> > return fd;
> > }
> > @@ -77,7 +79,7 @@ static int test__wp_ro(struct test_suite *test __maybe_unused,
> >
> > fd = __event(HW_BREAKPOINT_R, (void *)&data1, sizeof(data1));
> > if (fd < 0)
> > - return -1;
> > + return fd == -ENODEV ? TEST_SKIP : -1;
> >
> > tmp = data1;
> > WP_TEST_ASSERT_VAL(fd, "RO watchpoint", 1);
> > @@ -101,7 +103,7 @@ static int test__wp_wo(struct test_suite *test __maybe_unused,
> >
> > fd = __event(HW_BREAKPOINT_W, (void *)&data1, sizeof(data1));
> > if (fd < 0)
> > - return -1;
> > + return fd == -ENODEV ? TEST_SKIP : -1;
> >
> > tmp = data1;
> > WP_TEST_ASSERT_VAL(fd, "WO watchpoint", 0);
> > @@ -126,7 +128,7 @@ static int test__wp_rw(struct test_suite *test __maybe_unused,
> > fd = __event(HW_BREAKPOINT_R | HW_BREAKPOINT_W, (void *)&data1,
> > sizeof(data1));
> > if (fd < 0)
> > - return -1;
> > + return fd == -ENODEV ? TEST_SKIP : -1;
> >
> > tmp = data1;
> > WP_TEST_ASSERT_VAL(fd, "RW watchpoint", 1);
> > @@ -150,7 +152,7 @@ static int test__wp_modify(struct test_suite *test __maybe_unused, int subtest _
> >
> > fd = __event(HW_BREAKPOINT_W, (void *)&data1, sizeof(data1));
> > if (fd < 0)
> > - return -1;
> > + return fd == -ENODEV ? TEST_SKIP : -1;
> >
> > data1 = tmp;
> > WP_TEST_ASSERT_VAL(fd, "Modify watchpoint", 1);
> >
> > base-commit: 63a3bf5e8d9e79ce456c8f73d4395a5a51d841b1

2022-11-23 08:18:03

by kajoljain

Subject: Re: [PATCH] perf test: Skip watchpoint tests if no watchpoints available

On 11/21/22 15:57, Naveen N. Rao wrote:
> On IBM Power9, perf watchpoint tests fail since no hardware breakpoints
> are available. Detect this by checking the error returned by
> perf_event_open() and skip the tests in that case.
>
> Reported-by: Disha Goel <[email protected]>
> Signed-off-by: Naveen N. Rao <[email protected]>
> ---

Patch looks fine to me. Also tested it in power9 box

Test result without this patch changes :
23: Watchpoint :
23.1: Read Only Watchpoint : FAILED!
23.2: Write Only Watchpoint : FAILED!
23.3: Read / Write Watchpoint : FAILED!
23.4: Modify Watchpoint : FAILED!


Test result with patch changes:
23: Watchpoint :
23.1: Read Only Watchpoint : Skip (missing hardware support)
23.2: Write Only Watchpoint : Skip (missing hardware support)
23.3: Read / Write Watchpoint : Skip (missing hardware support)
23.4: Modify Watchpoint : Skip (missing hardware support)


Reviewed-by: Kajol Jain<[email protected]>
Tested-by: Kajol Jain<[email protected]>

Thanks,
Kajol Jain

> tools/perf/tests/wp.c | 12 +++++++-----
> 1 file changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/tools/perf/tests/wp.c b/tools/perf/tests/wp.c
> index 56455da30341b4..cc8719609b19ea 100644
> --- a/tools/perf/tests/wp.c
> +++ b/tools/perf/tests/wp.c
> @@ -59,8 +59,10 @@ static int __event(int wp_type, void *wp_addr, unsigned long wp_len)
> get__perf_event_attr(&attr, wp_type, wp_addr, wp_len);
> fd = sys_perf_event_open(&attr, 0, -1, -1,
> perf_event_open_cloexec_flag());
> - if (fd < 0)
> + if (fd < 0) {
> + fd = -errno;
> pr_debug("failed opening event %x\n", attr.bp_type);
> + }
>
> return fd;
> }
> @@ -77,7 +79,7 @@ static int test__wp_ro(struct test_suite *test __maybe_unused,
>
> fd = __event(HW_BREAKPOINT_R, (void *)&data1, sizeof(data1));
> if (fd < 0)
> - return -1;
> + return fd == -ENODEV ? TEST_SKIP : -1;
>
> tmp = data1;
> WP_TEST_ASSERT_VAL(fd, "RO watchpoint", 1);
> @@ -101,7 +103,7 @@ static int test__wp_wo(struct test_suite *test __maybe_unused,
>
> fd = __event(HW_BREAKPOINT_W, (void *)&data1, sizeof(data1));
> if (fd < 0)
> - return -1;
> + return fd == -ENODEV ? TEST_SKIP : -1;
>
> tmp = data1;
> WP_TEST_ASSERT_VAL(fd, "WO watchpoint", 0);
> @@ -126,7 +128,7 @@ static int test__wp_rw(struct test_suite *test __maybe_unused,
> fd = __event(HW_BREAKPOINT_R | HW_BREAKPOINT_W, (void *)&data1,
> sizeof(data1));
> if (fd < 0)
> - return -1;
> + return fd == -ENODEV ? TEST_SKIP : -1;
>
> tmp = data1;
> WP_TEST_ASSERT_VAL(fd, "RW watchpoint", 1);
> @@ -150,7 +152,7 @@ static int test__wp_modify(struct test_suite *test __maybe_unused, int subtest _
>
> fd = __event(HW_BREAKPOINT_W, (void *)&data1, sizeof(data1));
> if (fd < 0)
> - return -1;
> + return fd == -ENODEV ? TEST_SKIP : -1;
>
> data1 = tmp;
> WP_TEST_ASSERT_VAL(fd, "Modify watchpoint", 1);
>
> base-commit: 63a3bf5e8d9e79ce456c8f73d4395a5a51d841b1

2022-11-23 14:22:19

by Arnaldo Carvalho de Melo

Subject: Re: [PATCH] perf test: Skip watchpoint tests if no watchpoints available

Em Tue, Nov 22, 2022 at 12:57:05PM -0800, Ian Rogers escreveu:
> On Tue, Nov 22, 2022 at 11:19 AM Christophe Leroy
> <[email protected]> wrote:
> >
> >
> >
> > Le 21/11/2022 ? 11:27, Naveen N. Rao a ?crit :
> > > On IBM Power9, perf watchpoint tests fail since no hardware breakpoints
> > > are available. Detect this by checking the error returned by
> > > perf_event_open() and skip the tests in that case.
> > >
> > > Reported-by: Disha Goel <[email protected]>
> > > Signed-off-by: Naveen N. Rao <[email protected]>
> > > ---
> > > tools/perf/tests/wp.c | 12 +++++++-----
> > > 1 file changed, 7 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/tools/perf/tests/wp.c b/tools/perf/tests/wp.c
> > > index 56455da30341b4..cc8719609b19ea 100644
> > > --- a/tools/perf/tests/wp.c
> > > +++ b/tools/perf/tests/wp.c
> > > @@ -59,8 +59,10 @@ static int __event(int wp_type, void *wp_addr, unsigned long wp_len)
> > > get__perf_event_attr(&attr, wp_type, wp_addr, wp_len);
> > > fd = sys_perf_event_open(&attr, 0, -1, -1,
> > > perf_event_open_cloexec_flag());
> > > - if (fd < 0)
> > > + if (fd < 0) {
> > > + fd = -errno;
> > > pr_debug("failed opening event %x\n", attr.bp_type);
> > > + }
> >
> > Do you really need that ?
> >
> > Can't you directly check errno in the caller ?
>
> errno is very easily clobbered and not clearly set on success - ie,
> it'd be better not to do that.
>
> Acked-by: Ian Rogers <[email protected]>

Thanks, applied.

- Arnaldo