Message-ID: <CAJHvVchBhRf3pP4fCwsrr2s3S5jQP8z4sct5-_0n=tRd4XB9jQ@mail.gmail.com>
Date:   Wed, 12 Apr 2023 11:20:35 -0700
From:   Axel Rasmussen <axelrasmussen@...gle.com>
To:     Peter Xu <peterx@...hat.com>
Cc:     linux-kernel@...r.kernel.org, linux-mm@...ck.org,
        Andrew Morton <akpm@...ux-foundation.org>,
        Andrea Arcangeli <aarcange@...hat.com>,
        Mike Kravetz <mike.kravetz@...cle.com>,
        Nadav Amit <nadav.amit@...il.com>,
        David Hildenbrand <david@...hat.com>,
        Mike Rapoport <rppt@...ux.vnet.ibm.com>,
        Leonardo Bras Soares Passos <lsoaresp@...hat.com>
Subject: Re: [PATCH v2 14/31] selftests/mm: uffd_[un]register()

On Wed, Apr 12, 2023 at 9:42 AM Peter Xu <peterx@...hat.com> wrote:
>
> Add two helpers to register/unregister to an uffd.  Use them to drop
> duplicate codes.
>
> This patch also drops assert_expected_ioctls_present() and
> get_expected_ioctls().  Reasons:
>
>   - It'll need a lot of effort to pass test_type==HUGETLB into it from the
>   upper, so it's the simplest way to get rid of another global var
>
>   - The ioctls returned in UFFDIO_REGISTER is hardly useful at all, because
>   any app can already detect kernel support on any ioctl via its
>   corresponding UFFD_FEATURE_*.  The check here is for sanity mostly but
>   it's probably destined no user app will even use it.
>
>   - It's not friendly to one future goal of uffd to run on old kernels, the
>   problem is get_expected_ioctls() compiles against UFFD_API_RANGE_IOCTLS,
>   which is a value that can change depending on where the test is compiled,
>   rather than reflecting what the kernel underneath has.  It means it'll
>   report false negatives on old kernels so it's against our will.

I do think there is some value in this test, but I see patch 31 adds a
similar test back so it's fine to remove it in the meantime to clean
up things like get_expected_ioctls().

>
> So let's make our live easier.
>
> Signed-off-by: Peter Xu <peterx@...hat.com>

Reviewed-by: Axel Rasmussen <axelrasmussen@...gle.com>

> ---
>  tools/testing/selftests/mm/hugepage-mremap.c  |  7 +-
>  .../selftests/mm/ksm_functional_tests.c       |  6 +-
>  tools/testing/selftests/mm/uffd-common.c      | 27 ------
>  tools/testing/selftests/mm/uffd-common.h      |  2 -
>  tools/testing/selftests/mm/uffd-stress.c      | 83 +++++--------------
>  tools/testing/selftests/mm/vm_util.c          | 37 +++++++++
>  tools/testing/selftests/mm/vm_util.h          |  4 +
>  7 files changed, 62 insertions(+), 104 deletions(-)
>
> diff --git a/tools/testing/selftests/mm/hugepage-mremap.c b/tools/testing/selftests/mm/hugepage-mremap.c
> index e53b5eaa8fce..2084692fe1c4 100644
> --- a/tools/testing/selftests/mm/hugepage-mremap.c
> +++ b/tools/testing/selftests/mm/hugepage-mremap.c
> @@ -60,7 +60,6 @@ static void register_region_with_uffd(char *addr, size_t len)
>  {
>         long uffd; /* userfaultfd file descriptor */
>         struct uffdio_api uffdio_api;
> -       struct uffdio_register uffdio_register;
>
>         /* Create and enable userfaultfd object. */
>
> @@ -96,11 +95,7 @@ static void register_region_with_uffd(char *addr, size_t len)
>          * handling by the userfaultfd object. In mode, we request to track
>          * missing pages (i.e., pages that have not yet been faulted in).
>          */
> -
> -       uffdio_register.range.start = (unsigned long)addr;
> -       uffdio_register.range.len = len;
> -       uffdio_register.mode = UFFDIO_REGISTER_MODE_MISSING;
> -       if (ioctl(uffd, UFFDIO_REGISTER, &uffdio_register) == -1) {
> +       if (uffd_register(uffd, addr, len, true, false, false)) {

For what it's worth, I agree with Mike that the booleans here are hard
to read. It's not clear what "true, false, false" means without going
to look at the header file, whereas "uffd_register(uffd, addr, len,
MINOR)" would be immediately clear.

One solution I've seen outside the kernel is to comment the bools, like:

uffd_register(..., /*minor=*/true, /*wp=*/false, /*minor=*/false);

But, then I feel we lose most of the benefit we wanted from switching
to bools anyway (code length). :)

I do agree the macro names are unwieldy, and lots of tests already use
booleans so we'd have to convert from bool -> flag. If it were me, I
would resolve that by:

- Define much shorter aliases for these macros in uffd-common.h
- Consider refactoring callers to not use bools either.

Then again, I also agree with Mike that it's not a deal breaker, if
you like this way much more than that alternative we can leave it.

>                 perror("ioctl-UFFDIO_REGISTER");
>                 exit(1);
>         }
> diff --git a/tools/testing/selftests/mm/ksm_functional_tests.c b/tools/testing/selftests/mm/ksm_functional_tests.c
> index d8b5b4930412..d3f26050dfd7 100644
> --- a/tools/testing/selftests/mm/ksm_functional_tests.c
> +++ b/tools/testing/selftests/mm/ksm_functional_tests.c
> @@ -178,7 +178,6 @@ static void test_unmerge_discarded(void)
>  static void test_unmerge_uffd_wp(void)
>  {
>         struct uffdio_writeprotect uffd_writeprotect;
> -       struct uffdio_register uffdio_register;
>         const unsigned int size = 2 * MiB;
>         struct uffdio_api uffdio_api;
>         char *map;
> @@ -210,10 +209,7 @@ static void test_unmerge_uffd_wp(void)
>         }
>
>         /* Register UFFD-WP, no need for an actual handler. */
> -       uffdio_register.range.start = (unsigned long) map;
> -       uffdio_register.range.len = size;
> -       uffdio_register.mode = UFFDIO_REGISTER_MODE_WP;
> -       if (ioctl(uffd, UFFDIO_REGISTER, &uffdio_register) < 0) {
> +       if (uffd_register(uffd, map, size, false, true, false)) {
>                 ksft_test_result_fail("UFFDIO_REGISTER_MODE_WP failed\n");
>                 goto close_uffd;
>         }
> diff --git a/tools/testing/selftests/mm/uffd-common.c b/tools/testing/selftests/mm/uffd-common.c
> index c57757c2a36f..daa5b5781e7a 100644
> --- a/tools/testing/selftests/mm/uffd-common.c
> +++ b/tools/testing/selftests/mm/uffd-common.c
> @@ -330,33 +330,6 @@ void uffd_test_ctx_init(uint64_t features)
>                         err("pipe");
>  }
>
> -uint64_t get_expected_ioctls(uint64_t mode)
> -{
> -       uint64_t ioctls = UFFD_API_RANGE_IOCTLS;
> -
> -       if (test_type == TEST_HUGETLB)
> -               ioctls &= ~(1 << _UFFDIO_ZEROPAGE);
> -
> -       if (!((mode & UFFDIO_REGISTER_MODE_WP) && test_uffdio_wp))
> -               ioctls &= ~(1 << _UFFDIO_WRITEPROTECT);
> -
> -       if (!((mode & UFFDIO_REGISTER_MODE_MINOR) && test_uffdio_minor))
> -               ioctls &= ~(1 << _UFFDIO_CONTINUE);
> -
> -       return ioctls;
> -}
> -
> -void assert_expected_ioctls_present(uint64_t mode, uint64_t ioctls)
> -{
> -       uint64_t expected = get_expected_ioctls(mode);
> -       uint64_t actual = ioctls & expected;
> -
> -       if (actual != expected) {
> -               err("missing ioctl(s): expected %"PRIx64" actual: %"PRIx64,
> -                   expected, actual);
> -       }
> -}
> -
>  void wp_range(int ufd, __u64 start, __u64 len, bool wp)
>  {
>         struct uffdio_writeprotect prms;
> diff --git a/tools/testing/selftests/mm/uffd-common.h b/tools/testing/selftests/mm/uffd-common.h
> index d9430cfdcb19..11f770391bd9 100644
> --- a/tools/testing/selftests/mm/uffd-common.h
> +++ b/tools/testing/selftests/mm/uffd-common.h
> @@ -101,8 +101,6 @@ extern uffd_test_ops_t *uffd_test_ops;
>  void uffd_stats_report(struct uffd_stats *stats, int n_cpus);
>  void uffd_test_ctx_init(uint64_t features);
>  void userfaultfd_open(uint64_t *features);
> -uint64_t get_expected_ioctls(uint64_t mode);
> -void assert_expected_ioctls_present(uint64_t mode, uint64_t ioctls);
>  int uffd_read_msg(int ufd, struct uffd_msg *msg);
>  void wp_range(int ufd, __u64 start, __u64 len, bool wp);
>  void uffd_handle_page_fault(struct uffd_msg *msg, struct uffd_stats *stats);
> diff --git a/tools/testing/selftests/mm/uffd-stress.c b/tools/testing/selftests/mm/uffd-stress.c
> index c68a9aeefc41..e6d39a755082 100644
> --- a/tools/testing/selftests/mm/uffd-stress.c
> +++ b/tools/testing/selftests/mm/uffd-stress.c
> @@ -463,28 +463,19 @@ static int uffdio_zeropage(int ufd, unsigned long offset)
>  /* exercise UFFDIO_ZEROPAGE */
>  static int userfaultfd_zeropage_test(void)
>  {
> -       struct uffdio_register uffdio_register;
> -
>         printf("testing UFFDIO_ZEROPAGE: ");
>         fflush(stdout);
>
>         uffd_test_ctx_init(0);
>
> -       uffdio_register.range.start = (unsigned long) area_dst;
> -       uffdio_register.range.len = nr_pages * page_size;
> -       uffdio_register.mode = UFFDIO_REGISTER_MODE_MISSING;
> -       if (test_uffdio_wp)
> -               uffdio_register.mode |= UFFDIO_REGISTER_MODE_WP;
> -       if (ioctl(uffd, UFFDIO_REGISTER, &uffdio_register))
> +       if (uffd_register(uffd, area_dst, nr_pages * page_size,
> +                         true, test_uffdio_wp, false))
>                 err("register failure");
>
> -       assert_expected_ioctls_present(
> -               uffdio_register.mode, uffdio_register.ioctls);
> -
>         if (area_dst_alias) {
>                 /* Needed this to test zeropage-retry on shared memory */
> -               uffdio_register.range.start = (unsigned long) area_dst_alias;
> -               if (ioctl(uffd, UFFDIO_REGISTER, &uffdio_register))
> +               if (uffd_register(uffd, area_dst_alias, nr_pages * page_size,
> +                                 true, test_uffdio_wp, false))
>                         err("register failure");
>         }
>
> @@ -498,7 +489,6 @@ static int userfaultfd_zeropage_test(void)
>
>  static int userfaultfd_events_test(void)
>  {
> -       struct uffdio_register uffdio_register;
>         pthread_t uffd_mon;
>         int err, features;
>         pid_t pid;
> @@ -514,17 +504,10 @@ static int userfaultfd_events_test(void)
>
>         fcntl(uffd, F_SETFL, uffd_flags | O_NONBLOCK);
>
> -       uffdio_register.range.start = (unsigned long) area_dst;
> -       uffdio_register.range.len = nr_pages * page_size;
> -       uffdio_register.mode = UFFDIO_REGISTER_MODE_MISSING;
> -       if (test_uffdio_wp)
> -               uffdio_register.mode |= UFFDIO_REGISTER_MODE_WP;
> -       if (ioctl(uffd, UFFDIO_REGISTER, &uffdio_register))
> +       if (uffd_register(uffd, area_dst, nr_pages * page_size,
> +                         true, test_uffdio_wp, false))
>                 err("register failure");
>
> -       assert_expected_ioctls_present(
> -               uffdio_register.mode, uffdio_register.ioctls);
> -
>         if (pthread_create(&uffd_mon, &attr, uffd_poll_thread, &stats))
>                 err("uffd_poll_thread create");
>
> @@ -550,7 +533,6 @@ static int userfaultfd_events_test(void)
>
>  static int userfaultfd_sig_test(void)
>  {
> -       struct uffdio_register uffdio_register;
>         unsigned long userfaults;
>         pthread_t uffd_mon;
>         int err, features;
> @@ -566,17 +548,10 @@ static int userfaultfd_sig_test(void)
>
>         fcntl(uffd, F_SETFL, uffd_flags | O_NONBLOCK);
>
> -       uffdio_register.range.start = (unsigned long) area_dst;
> -       uffdio_register.range.len = nr_pages * page_size;
> -       uffdio_register.mode = UFFDIO_REGISTER_MODE_MISSING;
> -       if (test_uffdio_wp)
> -               uffdio_register.mode |= UFFDIO_REGISTER_MODE_WP;
> -       if (ioctl(uffd, UFFDIO_REGISTER, &uffdio_register))
> +       if (uffd_register(uffd, area_dst, nr_pages * page_size,
> +                         true, test_uffdio_wp, false))
>                 err("register failure");
>
> -       assert_expected_ioctls_present(
> -               uffdio_register.mode, uffdio_register.ioctls);
> -
>         if (faulting_process(1))
>                 err("faulting process failed");
>
> @@ -629,7 +604,6 @@ void check_memory_contents(char *p)
>  static int userfaultfd_minor_test(void)
>  {
>         unsigned long p;
> -       struct uffdio_register uffdio_register;
>         pthread_t uffd_mon;
>         char c;
>         struct uffd_stats stats = { 0 };
> @@ -642,17 +616,10 @@ static int userfaultfd_minor_test(void)
>
>         uffd_test_ctx_init(uffd_minor_feature());
>
> -       uffdio_register.range.start = (unsigned long)area_dst_alias;
> -       uffdio_register.range.len = nr_pages * page_size;
> -       uffdio_register.mode = UFFDIO_REGISTER_MODE_MINOR;
> -       if (test_uffdio_wp)
> -               uffdio_register.mode |= UFFDIO_REGISTER_MODE_WP;
> -       if (ioctl(uffd, UFFDIO_REGISTER, &uffdio_register))
> +       if (uffd_register(uffd, area_dst_alias, nr_pages * page_size,
> +                         false, test_uffdio_wp, true))
>                 err("register failure");
>
> -       assert_expected_ioctls_present(
> -               uffdio_register.mode, uffdio_register.ioctls);
> -
>         /*
>          * After registering with UFFD, populate the non-UFFD-registered side of
>          * the shared mapping. This should *not* trigger any UFFD minor faults.
> @@ -777,7 +744,6 @@ static void userfaultfd_wp_unpopulated_test(int pagemap_fd)
>
>  static void userfaultfd_pagemap_test(unsigned int test_pgsize)
>  {
> -       struct uffdio_register uffdio_register;
>         int pagemap_fd;
>         uint64_t value;
>
> @@ -805,10 +771,8 @@ static void userfaultfd_pagemap_test(unsigned int test_pgsize)
>                         err("madvise(MADV_NOHUGEPAGE) failed");
>         }
>
> -       uffdio_register.range.start = (unsigned long) area_dst;
> -       uffdio_register.range.len = nr_pages * page_size;
> -       uffdio_register.mode = UFFDIO_REGISTER_MODE_WP;
> -       if (ioctl(uffd, UFFDIO_REGISTER, &uffdio_register))
> +       if (uffd_register(uffd, area_dst, nr_pages * page_size,
> +                         false, true, false))
>                 err("register failed");
>
>         pagemap_fd = pagemap_open();
> @@ -858,8 +822,8 @@ static int userfaultfd_stress(void)
>  {
>         void *area;
>         unsigned long nr;
> -       struct uffdio_register uffdio_register;
>         struct uffd_stats uffd_stats[nr_cpus];
> +       uint64_t mem_size = nr_pages * page_size;
>
>         uffd_test_ctx_init(UFFD_FEATURE_WP_UNPOPULATED);
>
> @@ -894,20 +858,13 @@ static int userfaultfd_stress(void)
>                         fcntl(uffd, F_SETFL, uffd_flags & ~O_NONBLOCK);
>
>                 /* register */
> -               uffdio_register.range.start = (unsigned long) area_dst;
> -               uffdio_register.range.len = nr_pages * page_size;
> -               uffdio_register.mode = UFFDIO_REGISTER_MODE_MISSING;
> -               if (test_uffdio_wp)
> -                       uffdio_register.mode |= UFFDIO_REGISTER_MODE_WP;
> -               if (ioctl(uffd, UFFDIO_REGISTER, &uffdio_register))
> +               if (uffd_register(uffd, area_dst, mem_size,
> +                                 true, test_uffdio_wp, false))
>                         err("register failure");
> -               assert_expected_ioctls_present(
> -                       uffdio_register.mode, uffdio_register.ioctls);
>
>                 if (area_dst_alias) {
> -                       uffdio_register.range.start = (unsigned long)
> -                               area_dst_alias;
> -                       if (ioctl(uffd, UFFDIO_REGISTER, &uffdio_register))
> +                       if (uffd_register(uffd, area_dst_alias, mem_size,
> +                                         true, test_uffdio_wp, false))
>                                 err("register failure alias");
>                 }
>
> @@ -949,12 +906,10 @@ static int userfaultfd_stress(void)
>                                  nr_pages * page_size, false);
>
>                 /* unregister */
> -               if (ioctl(uffd, UFFDIO_UNREGISTER, &uffdio_register.range))
> +               if (uffd_unregister(uffd, area_dst, mem_size))
>                         err("unregister failure");
>                 if (area_dst_alias) {
> -                       uffdio_register.range.start = (unsigned long) area_dst;
> -                       if (ioctl(uffd, UFFDIO_UNREGISTER,
> -                                 &uffdio_register.range))
> +                       if (uffd_unregister(uffd, area_dst_alias, mem_size))
>                                 err("unregister failure alias");
>                 }
>
> diff --git a/tools/testing/selftests/mm/vm_util.c b/tools/testing/selftests/mm/vm_util.c
> index 54d227d6f70a..bb633d050d71 100644
> --- a/tools/testing/selftests/mm/vm_util.c
> +++ b/tools/testing/selftests/mm/vm_util.c
> @@ -1,6 +1,8 @@
>  // SPDX-License-Identifier: GPL-2.0
>  #include <string.h>
>  #include <fcntl.h>
> +#include <sys/ioctl.h>
> +#include <linux/userfaultfd.h>
>  #include "../kselftest.h"
>  #include "vm_util.h"
>
> @@ -193,3 +195,38 @@ unsigned long default_huge_page_size(void)
>         fclose(f);
>         return hps;
>  }
> +
> +int uffd_register(int uffd, void *addr, uint64_t len,
> +                 bool miss, bool wp, bool minor)
> +{
> +       struct uffdio_register uffdio_register = { 0 };
> +       uint64_t mode = 0;
> +       int ret = 0;
> +
> +       if (miss)
> +               mode |= UFFDIO_REGISTER_MODE_MISSING;
> +       if (wp)
> +               mode |= UFFDIO_REGISTER_MODE_WP;
> +       if (minor)
> +               mode |= UFFDIO_REGISTER_MODE_MINOR;
> +
> +       uffdio_register.range.start = (unsigned long)addr;
> +       uffdio_register.range.len = len;
> +       uffdio_register.mode = mode;
> +
> +       if (ioctl(uffd, UFFDIO_REGISTER, &uffdio_register) == -1)
> +               ret = -errno;
> +
> +       return ret;
> +}
> +
> +int uffd_unregister(int uffd, void *addr, uint64_t len)
> +{
> +       struct uffdio_range range = { .start = (uintptr_t)addr, .len = len };
> +       int ret = 0;
> +
> +       if (ioctl(uffd, UFFDIO_UNREGISTER, &range) == -1)
> +               ret = -errno;
> +
> +       return ret;
> +}
> diff --git a/tools/testing/selftests/mm/vm_util.h b/tools/testing/selftests/mm/vm_util.h
> index d9fadddb5c69..3a9762022efd 100644
> --- a/tools/testing/selftests/mm/vm_util.h
> +++ b/tools/testing/selftests/mm/vm_util.h
> @@ -45,6 +45,10 @@ bool check_huge_shmem(void *addr, int nr_hpages, uint64_t hpage_size);
>  int64_t allocate_transhuge(void *ptr, int pagemap_fd);
>  unsigned long default_huge_page_size(void);
>
> +int uffd_register(int uffd, void *addr, uint64_t len,
> +                 bool miss, bool wp, bool minor);
> +int uffd_unregister(int uffd, void *addr, uint64_t len);
> +
>  /*
>   * On ppc64 this will only work with radix 2M hugepage size
>   */
> --
> 2.39.1
>

Powered by blists - more mailing lists