* Re: [PATCH] perf test: Skip perf bench breakpoint run if no breakpoints available
2023-08-23 7:51 [PATCH] perf test: Skip perf bench breakpoint run if no breakpoints available Kajol Jain
@ 2023-08-23 8:45 ` Disha Goel
2023-08-23 10:58 ` Naveen N Rao
2023-08-23 11:40 ` Arnaldo Carvalho de Melo
2 siblings, 0 replies; 5+ messages in thread
From: Disha Goel @ 2023-08-23 8:45 UTC (permalink / raw)
To: Kajol Jain, acme
Cc: irogers, atrajeev, linux-perf-users, maddy, namhyung,
naveen.n.rao, Disha Goel, linuxppc-dev
[-- Attachment #1: Type: text/plain, Size: 3755 bytes --]
On 23/08/23 1:21 pm, Kajol Jain wrote:
> Based on commit 7d54a4acd8c1 ("perf test: Skip watchpoint
> tests if no watchpoints available"), hardware breakpoints
> are not available for power9 platform and because of that
> perf bench breakpoint run fails on power9 platform.
> Add code to check for the return value of perf_event_open()
> in breakpoint run and skip the perf bench breakpoint run,
> if hardware breakpoints are not available.
>
> Result on power9 system before patch changes:
> [command]# perf bench breakpoint thread
> perf_event_open: No such device
>
> Result on power9 system after patch changes:
> [command]# ./perf bench breakpoint thread
> Skipping perf bench breakpoint thread: No hardware support
>
> Reported-by: Disha Goel<disgoel@linux.vnet.ibm.com>
> Signed-off-by: Kajol Jain<kjain@linux.ibm.com>
With this patch applied perf bench breakpoint test works correctly on Power9 and Power10.
Result on Power9 system with patch changes:
[root@]# ./perf bench breakpoint all
# Running breakpoint/thread benchmark...
Skipping perf bench breakpoint thread: No hardware support
# Running breakpoint/enable benchmark...
Skipping perf bench breakpoint enable: No hardware support
Result on Power10 system with patch changes:
[root@]# ./perf bench breakpoint all
# Running breakpoint/thread benchmark...
# Created/joined 10 threads with 1 breakpoints and 1 parallelism
Total time: 0.001 [sec]
115.100000 usecs/op
115.100000 usecs/op/cpu
# Running breakpoint/enable benchmark...
# Enabled/disabled breakpoint 10 time with 0 passive and 0 active threads
Total time: 0.000 [sec]
2.800000 usecs/op
Tested-by: Disha Goel<disgoel@linux.ibm.com>
> ---
> tools/perf/bench/breakpoint.c | 24 +++++++++++++++++++++---
> 1 file changed, 21 insertions(+), 3 deletions(-)
>
> diff --git a/tools/perf/bench/breakpoint.c b/tools/perf/bench/breakpoint.c
> index 41385f89ffc7..dfd18f5db97d 100644
> --- a/tools/perf/bench/breakpoint.c
> +++ b/tools/perf/bench/breakpoint.c
> @@ -47,6 +47,7 @@ struct breakpoint {
> static int breakpoint_setup(void *addr)
> {
> struct perf_event_attr attr = { .size = 0, };
> + int fd;
>
> attr.type = PERF_TYPE_BREAKPOINT;
> attr.size = sizeof(attr);
> @@ -56,7 +57,12 @@ static int breakpoint_setup(void *addr)
> attr.bp_addr = (unsigned long)addr;
> attr.bp_type = HW_BREAKPOINT_RW;
> attr.bp_len = HW_BREAKPOINT_LEN_1;
> - return syscall(SYS_perf_event_open, &attr, 0, -1, -1, 0);
> + fd = syscall(SYS_perf_event_open, &attr, 0, -1, -1, 0);
> +
> + if (fd < 0)
> + fd = -errno;
> +
> + return fd;
> }
>
> static void *passive_thread(void *arg)
> @@ -122,8 +128,14 @@ int bench_breakpoint_thread(int argc, const char **argv)
>
> for (i = 0; i < thread_params.nbreakpoints; i++) {
> breakpoints[i].fd = breakpoint_setup(&breakpoints[i].watched);
> - if (breakpoints[i].fd == -1)
> +
> + if (breakpoints[i].fd < 0) {
> + if (breakpoints[i].fd == -ENODEV) {
> + printf("Skipping perf bench breakpoint thread: No hardware support\n");
> + return 0;
> + }
> exit((perror("perf_event_open"), EXIT_FAILURE));
> + }
> }
> gettimeofday(&start, NULL);
> for (i = 0; i < thread_params.nparallel; i++) {
> @@ -196,8 +208,14 @@ int bench_breakpoint_enable(int argc, const char **argv)
> exit(EXIT_FAILURE);
> }
> fd = breakpoint_setup(&watched);
> - if (fd == -1)
> +
> + if (fd < 0) {
> + if (fd == -ENODEV) {
> + printf("Skipping perf bench breakpoint enable: No hardware support\n");
> + return 0;
> + }
> exit((perror("perf_event_open"), EXIT_FAILURE));
> + }
> nthreads = enable_params.npassive + enable_params.nactive;
> threads = calloc(nthreads, sizeof(threads[0]));
> if (!threads)
[-- Attachment #2: Type: text/html, Size: 4310 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] perf test: Skip perf bench breakpoint run if no breakpoints available
2023-08-23 7:51 [PATCH] perf test: Skip perf bench breakpoint run if no breakpoints available Kajol Jain
2023-08-23 8:45 ` Disha Goel
@ 2023-08-23 10:58 ` Naveen N Rao
2023-08-29 19:03 ` Ian Rogers
2023-08-23 11:40 ` Arnaldo Carvalho de Melo
2 siblings, 1 reply; 5+ messages in thread
From: Naveen N Rao @ 2023-08-23 10:58 UTC (permalink / raw)
To: Kajol Jain, acme
Cc: irogers, atrajeev, Disha Goel, linux-perf-users, maddy, namhyung,
naveen.n.rao, disgoel, linuxppc-dev
Hi Kajol,
On Wed Aug 23, 2023 at 1:21 PM IST, Kajol Jain wrote:
> Based on commit 7d54a4acd8c1 ("perf test: Skip watchpoint
> tests if no watchpoints available"), hardware breakpoints
> are not available for power9 platform and because of that
> perf bench breakpoint run fails on power9 platform.
> Add code to check for the return value of perf_event_open()
> in breakpoint run and skip the perf bench breakpoint run,
> if hardware breakpoints are not available.
>
> Result on power9 system before patch changes:
> [command]# perf bench breakpoint thread
> perf_event_open: No such device
>
> Result on power9 system after patch changes:
> [command]# ./perf bench breakpoint thread
> Skipping perf bench breakpoint thread: No hardware support
>
> Reported-by: Disha Goel <disgoel@linux.vnet.ibm.com>
> Signed-off-by: Kajol Jain <kjain@linux.ibm.com>
> ---
> tools/perf/bench/breakpoint.c | 24 +++++++++++++++++++++---
> 1 file changed, 21 insertions(+), 3 deletions(-)
Thanks for fixing this to not report an error. A minor nit below, but
otherwise:
Acked-by: Naveen N Rao <naveen@kernel.org>
>
> diff --git a/tools/perf/bench/breakpoint.c b/tools/perf/bench/breakpoint.c
> index 41385f89ffc7..dfd18f5db97d 100644
> --- a/tools/perf/bench/breakpoint.c
> +++ b/tools/perf/bench/breakpoint.c
> @@ -47,6 +47,7 @@ struct breakpoint {
> static int breakpoint_setup(void *addr)
> {
> struct perf_event_attr attr = { .size = 0, };
> + int fd;
>
> attr.type = PERF_TYPE_BREAKPOINT;
> attr.size = sizeof(attr);
> @@ -56,7 +57,12 @@ static int breakpoint_setup(void *addr)
> attr.bp_addr = (unsigned long)addr;
> attr.bp_type = HW_BREAKPOINT_RW;
> attr.bp_len = HW_BREAKPOINT_LEN_1;
> - return syscall(SYS_perf_event_open, &attr, 0, -1, -1, 0);
> + fd = syscall(SYS_perf_event_open, &attr, 0, -1, -1, 0);
> +
> + if (fd < 0)
> + fd = -errno;
> +
> + return fd;
> }
>
> static void *passive_thread(void *arg)
> @@ -122,8 +128,14 @@ int bench_breakpoint_thread(int argc, const char **argv)
>
> for (i = 0; i < thread_params.nbreakpoints; i++) {
> breakpoints[i].fd = breakpoint_setup(&breakpoints[i].watched);
> - if (breakpoints[i].fd == -1)
> +
> + if (breakpoints[i].fd < 0) {
> + if (breakpoints[i].fd == -ENODEV) {
> + printf("Skipping perf bench breakpoint thread: No hardware support\n");
> + return 0;
Should we instead do 'exit(0)' here to stop further benchmarks? Perhaps:
err(EXIT_SUCCESS, "Skipping perf bench breakpoint thread: No hardware support");
EXIT_SUCCESS looks weird, but should help document that this is not an
error.
> + }
> exit((perror("perf_event_open"), EXIT_FAILURE));
> + }
> }
> gettimeofday(&start, NULL);
> for (i = 0; i < thread_params.nparallel; i++) {
> @@ -196,8 +208,14 @@ int bench_breakpoint_enable(int argc, const char **argv)
> exit(EXIT_FAILURE);
> }
> fd = breakpoint_setup(&watched);
> - if (fd == -1)
> +
> + if (fd < 0) {
> + if (fd == -ENODEV) {
> + printf("Skipping perf bench breakpoint enable: No hardware support\n");
> + return 0;
Here too.
- Naveen
> + }
> exit((perror("perf_event_open"), EXIT_FAILURE));
> + }
> nthreads = enable_params.npassive + enable_params.nactive;
> threads = calloc(nthreads, sizeof(threads[0]));
> if (!threads)
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] perf test: Skip perf bench breakpoint run if no breakpoints available
2023-08-23 10:58 ` Naveen N Rao
@ 2023-08-29 19:03 ` Ian Rogers
0 siblings, 0 replies; 5+ messages in thread
From: Ian Rogers @ 2023-08-29 19:03 UTC (permalink / raw)
To: Naveen N Rao
Cc: Disha Goel, atrajeev, Kajol Jain, acme, linux-perf-users, maddy,
namhyung, naveen.n.rao, disgoel, linuxppc-dev
On Wed, Aug 23, 2023 at 4:00 AM Naveen N Rao <naveen@kernel.org> wrote:
>
> Hi Kajol,
>
> On Wed Aug 23, 2023 at 1:21 PM IST, Kajol Jain wrote:
> > Based on commit 7d54a4acd8c1 ("perf test: Skip watchpoint
> > tests if no watchpoints available"), hardware breakpoints
> > are not available for power9 platform and because of that
> > perf bench breakpoint run fails on power9 platform.
> > Add code to check for the return value of perf_event_open()
> > in breakpoint run and skip the perf bench breakpoint run,
> > if hardware breakpoints are not available.
> >
> > Result on power9 system before patch changes:
> > [command]# perf bench breakpoint thread
> > perf_event_open: No such device
> >
> > Result on power9 system after patch changes:
> > [command]# ./perf bench breakpoint thread
> > Skipping perf bench breakpoint thread: No hardware support
> >
> > Reported-by: Disha Goel <disgoel@linux.vnet.ibm.com>
> > Signed-off-by: Kajol Jain <kjain@linux.ibm.com>
> > ---
> > tools/perf/bench/breakpoint.c | 24 +++++++++++++++++++++---
> > 1 file changed, 21 insertions(+), 3 deletions(-)
>
> Thanks for fixing this to not report an error. A minor nit below, but
> otherwise:
> Acked-by: Naveen N Rao <naveen@kernel.org>
>
> >
> > diff --git a/tools/perf/bench/breakpoint.c b/tools/perf/bench/breakpoint.c
> > index 41385f89ffc7..dfd18f5db97d 100644
> > --- a/tools/perf/bench/breakpoint.c
> > +++ b/tools/perf/bench/breakpoint.c
> > @@ -47,6 +47,7 @@ struct breakpoint {
> > static int breakpoint_setup(void *addr)
> > {
> > struct perf_event_attr attr = { .size = 0, };
> > + int fd;
> >
> > attr.type = PERF_TYPE_BREAKPOINT;
> > attr.size = sizeof(attr);
> > @@ -56,7 +57,12 @@ static int breakpoint_setup(void *addr)
> > attr.bp_addr = (unsigned long)addr;
> > attr.bp_type = HW_BREAKPOINT_RW;
> > attr.bp_len = HW_BREAKPOINT_LEN_1;
> > - return syscall(SYS_perf_event_open, &attr, 0, -1, -1, 0);
> > + fd = syscall(SYS_perf_event_open, &attr, 0, -1, -1, 0);
> > +
> > + if (fd < 0)
> > + fd = -errno;
> > +
> > + return fd;
> > }
> >
> > static void *passive_thread(void *arg)
> > @@ -122,8 +128,14 @@ int bench_breakpoint_thread(int argc, const char **argv)
> >
> > for (i = 0; i < thread_params.nbreakpoints; i++) {
> > breakpoints[i].fd = breakpoint_setup(&breakpoints[i].watched);
> > - if (breakpoints[i].fd == -1)
> > +
> > + if (breakpoints[i].fd < 0) {
> > + if (breakpoints[i].fd == -ENODEV) {
> > + printf("Skipping perf bench breakpoint thread: No hardware support\n");
> > + return 0;
>
> Should we instead do 'exit(0)' here to stop further benchmarks? Perhaps:
> err(EXIT_SUCCESS, "Skipping perf bench breakpoint thread: No hardware support");
>
> EXIT_SUCCESS looks weird, but should help document that this is not an
> error.
In tools/perf/tests/tests.h is:
enum {
TEST_OK = 0,
TEST_FAIL = -1,
TEST_SKIP = -2,
};
So I think the EXIT_SUCCESS/0 should really be TEST_OK, but I think it
would clearer if these cases were TEST_SKIP.
Thanks,
Ian
> > + }
> > exit((perror("perf_event_open"), EXIT_FAILURE));
> > + }
> > }
> > gettimeofday(&start, NULL);
> > for (i = 0; i < thread_params.nparallel; i++) {
> > @@ -196,8 +208,14 @@ int bench_breakpoint_enable(int argc, const char **argv)
> > exit(EXIT_FAILURE);
> > }
> > fd = breakpoint_setup(&watched);
> > - if (fd == -1)
> > +
> > + if (fd < 0) {
> > + if (fd == -ENODEV) {
> > + printf("Skipping perf bench breakpoint enable: No hardware support\n");
> > + return 0;
>
> Here too.
>
> - Naveen
>
> > + }
> > exit((perror("perf_event_open"), EXIT_FAILURE));
> > + }
> > nthreads = enable_params.npassive + enable_params.nactive;
> > threads = calloc(nthreads, sizeof(threads[0]));
> > if (!threads)
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] perf test: Skip perf bench breakpoint run if no breakpoints available
2023-08-23 7:51 [PATCH] perf test: Skip perf bench breakpoint run if no breakpoints available Kajol Jain
2023-08-23 8:45 ` Disha Goel
2023-08-23 10:58 ` Naveen N Rao
@ 2023-08-23 11:40 ` Arnaldo Carvalho de Melo
2 siblings, 0 replies; 5+ messages in thread
From: Arnaldo Carvalho de Melo @ 2023-08-23 11:40 UTC (permalink / raw)
To: Kajol Jain
Cc: irogers, atrajeev, Disha Goel, linux-perf-users, maddy, namhyung,
naveen.n.rao, disgoel, linuxppc-dev
Em Wed, Aug 23, 2023 at 01:21:03PM +0530, Kajol Jain escreveu:
> Based on commit 7d54a4acd8c1 ("perf test: Skip watchpoint
> tests if no watchpoints available"), hardware breakpoints
> are not available for power9 platform and because of that
> perf bench breakpoint run fails on power9 platform.
> Add code to check for the return value of perf_event_open()
> in breakpoint run and skip the perf bench breakpoint run,
> if hardware breakpoints are not available.
>
> Result on power9 system before patch changes:
> [command]# perf bench breakpoint thread
> perf_event_open: No such device
>
> Result on power9 system after patch changes:
> [command]# ./perf bench breakpoint thread
> Skipping perf bench breakpoint thread: No hardware support
>
> Reported-by: Disha Goel <disgoel@linux.vnet.ibm.com>
> Signed-off-by: Kajol Jain <kjain@linux.ibm.com>
Thanks, applied.
- Arnaldo
> ---
> tools/perf/bench/breakpoint.c | 24 +++++++++++++++++++++---
> 1 file changed, 21 insertions(+), 3 deletions(-)
>
> diff --git a/tools/perf/bench/breakpoint.c b/tools/perf/bench/breakpoint.c
> index 41385f89ffc7..dfd18f5db97d 100644
> --- a/tools/perf/bench/breakpoint.c
> +++ b/tools/perf/bench/breakpoint.c
> @@ -47,6 +47,7 @@ struct breakpoint {
> static int breakpoint_setup(void *addr)
> {
> struct perf_event_attr attr = { .size = 0, };
> + int fd;
>
> attr.type = PERF_TYPE_BREAKPOINT;
> attr.size = sizeof(attr);
> @@ -56,7 +57,12 @@ static int breakpoint_setup(void *addr)
> attr.bp_addr = (unsigned long)addr;
> attr.bp_type = HW_BREAKPOINT_RW;
> attr.bp_len = HW_BREAKPOINT_LEN_1;
> - return syscall(SYS_perf_event_open, &attr, 0, -1, -1, 0);
> + fd = syscall(SYS_perf_event_open, &attr, 0, -1, -1, 0);
> +
> + if (fd < 0)
> + fd = -errno;
> +
> + return fd;
> }
>
> static void *passive_thread(void *arg)
> @@ -122,8 +128,14 @@ int bench_breakpoint_thread(int argc, const char **argv)
>
> for (i = 0; i < thread_params.nbreakpoints; i++) {
> breakpoints[i].fd = breakpoint_setup(&breakpoints[i].watched);
> - if (breakpoints[i].fd == -1)
> +
> + if (breakpoints[i].fd < 0) {
> + if (breakpoints[i].fd == -ENODEV) {
> + printf("Skipping perf bench breakpoint thread: No hardware support\n");
> + return 0;
> + }
> exit((perror("perf_event_open"), EXIT_FAILURE));
> + }
> }
> gettimeofday(&start, NULL);
> for (i = 0; i < thread_params.nparallel; i++) {
> @@ -196,8 +208,14 @@ int bench_breakpoint_enable(int argc, const char **argv)
> exit(EXIT_FAILURE);
> }
> fd = breakpoint_setup(&watched);
> - if (fd == -1)
> +
> + if (fd < 0) {
> + if (fd == -ENODEV) {
> + printf("Skipping perf bench breakpoint enable: No hardware support\n");
> + return 0;
> + }
> exit((perror("perf_event_open"), EXIT_FAILURE));
> + }
> nthreads = enable_params.npassive + enable_params.nactive;
> threads = calloc(nthreads, sizeof(threads[0]));
> if (!threads)
> --
> 2.39.3
>
--
- Arnaldo
^ permalink raw reply [flat|nested] 5+ messages in thread