Re: [PATCH v2 4/6] selftests/mm: refactor uffd_poll_thread to allow custom fault handlers

[Axel Rasmussen]

On Tue, Jul 4, 2023 at 2:03 PM Peter Xu <peterx@xxxxxxxxxx> wrote:
>
> On Thu, Jun 29, 2023 at 01:50:38PM -0700, Axel Rasmussen wrote:
> > Previously, we had "one fault handler to rule them all", which used
> > several branches to deal with all of the scenarios required by all of
> > the various tests.
> >
> > In upcoming patches, I plan to add a new test, which has its own
> > slightly different fault handling logic. Instead of continuing to add
> > cruft to the existing fault handler, let's allow tests to define custom
> > ones, separate from other tests.
> >
> > Signed-off-by: Axel Rasmussen <axelrasmussen@xxxxxxxxxx>
> > ---
> >  tools/testing/selftests/mm/uffd-common.c |  5 ++++-
> >  tools/testing/selftests/mm/uffd-common.h |  3 +++
> >  tools/testing/selftests/mm/uffd-stress.c | 12 +++++++-----
> >  3 files changed, 14 insertions(+), 6 deletions(-)
> >
> > diff --git a/tools/testing/selftests/mm/uffd-common.c b/tools/testing/selftests/mm/uffd-common.c
> > index ba20d7504022..02b89860e193 100644
> > --- a/tools/testing/selftests/mm/uffd-common.c
> > +++ b/tools/testing/selftests/mm/uffd-common.c
> > @@ -499,6 +499,9 @@ void *uffd_poll_thread(void *arg)
> >       int ret;
> >       char tmp_chr;
> >
> > +     if (!args->handle_fault)
> > +             args->handle_fault = uffd_handle_page_fault;
> > +
> >       pollfd[0].fd = uffd;
> >       pollfd[0].events = POLLIN;
> >       pollfd[1].fd = pipefd[cpu*2];
> > @@ -527,7 +530,7 @@ void *uffd_poll_thread(void *arg)
> >                       err("unexpected msg event %u\n", msg.event);
> >                       break;
> >               case UFFD_EVENT_PAGEFAULT:
> > -                     uffd_handle_page_fault(&msg, args);
> > +                     args->handle_fault(&msg, args);
> >                       break;
> >               case UFFD_EVENT_FORK:
> >                       close(uffd);
> > diff --git a/tools/testing/selftests/mm/uffd-common.h b/tools/testing/selftests/mm/uffd-common.h
> > index 197f5262fe0d..7c4fa964c3b0 100644
> > --- a/tools/testing/selftests/mm/uffd-common.h
> > +++ b/tools/testing/selftests/mm/uffd-common.h
> > @@ -77,6 +77,9 @@ struct uffd_args {
> >       unsigned long missing_faults;
> >       unsigned long wp_faults;
> >       unsigned long minor_faults;
> > +
> > +     /* A custom fault handler; defaults to uffd_handle_page_fault. */
> > +     void (*handle_fault)(struct uffd_msg *msg, struct uffd_args *args);
> >  };
> >
> >  struct uffd_test_ops {
> > diff --git a/tools/testing/selftests/mm/uffd-stress.c b/tools/testing/selftests/mm/uffd-stress.c
> > index 995ff13e74c7..50b1224d72c7 100644
> > --- a/tools/testing/selftests/mm/uffd-stress.c
> > +++ b/tools/testing/selftests/mm/uffd-stress.c
> > @@ -189,10 +189,8 @@ static int stress(struct uffd_args *args)
> >                                  locking_thread, (void *)cpu))
> >                       return 1;
> >               if (bounces & BOUNCE_POLL) {
> > -                     if (pthread_create(&uffd_threads[cpu], &attr,
> > -                                        uffd_poll_thread,
> > -                                        (void *)&args[cpu]))
> > -                             return 1;
> > +                     if (pthread_create(&uffd_threads[cpu], &attr, uffd_poll_thread, &args[cpu]))
> > +                             err("uffd_poll_thread create");
>
> irrelevant change?

Right, I'll revert this. In an earlier version I had a more
substantial change here, and just didn't fully revert it.

>
> >               } else {
> >                       if (pthread_create(&uffd_threads[cpu], &attr,
> >                                          uffd_read_thread,
> > @@ -247,9 +245,13 @@ static int userfaultfd_stress(void)
> >  {
> >       void *area;
> >       unsigned long nr;
> > -     struct uffd_args args[nr_cpus];
> > +     struct uffd_args *args;
> >       uint64_t mem_size = nr_pages * page_size;
> >
> > +     args = calloc(nr_cpus, sizeof(struct uffd_args));
> > +     if (!args)
> > +             err("allocating args array failed");
> > +
>
> It's leaked?
>
> Isn't "args[] = { 0 }" already working?

That works, but GCC can warn in this case (-Wmissing-braces) depending
on the definition of struct uffd_args. I liked switching to calloc
because it avoids any possibility of that even as we add/remove things
to struct uffd_args in the future.

Since it's a selftest and this function is only called exactly once,
it didn't seem worth the code making certain we free it, instead just
leaving it to be cleaned up when the process exits.

>
> Thanks,
>
> >       if (uffd_test_ctx_init(UFFD_FEATURE_WP_UNPOPULATED, NULL))
> >               err("context init failed");
> >
> > --
> > 2.41.0.255.g8b1d071c50-goog
> >
>
> --
> Peter Xu
>