[PATCH] perf test: Skip perf bench breakpoint run if no breakpoints available

[Ian Rogers]

On Wed, Aug 23, 2023 at 4:00 AM Naveen N Rao <naveen at 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 at linux.vnet.ibm.com>
> > Signed-off-by: Kajol Jain <kjain at 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 at 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)
>