| Message ID | 20230823075103.190565-1-kjain@linux.ibm.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | perf test: Skip perf bench breakpoint run if no breakpoints available | expand |
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)
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)
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 >
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) >
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)
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(-)