Re: [PATCH 3/3] userfaultfd/selftests: enable huegtlb remap and remove event testing

[Axel Rasmussen <axelrasmussen@google.com>]

Besides the help text, looks correct to me. I applied the patches and
ran the userfaultfd selftests, and everything seems to work properly.

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

On Fri, Jan 28, 2022 at 2:26 PM Mike Kravetz <mike.kravetz@oracle.com> wrote:
>
> With MADV_DONTNEED support added to hugetlb mappings, mremap testing
> can also be enabled for hugetlb.
>
> Modify the tests to use madvise MADV_DONTNEED and MADV_REMOVE instead of
> fallocate hole puch for releasing hugetlb pages.
>
> Signed-off-by: Mike Kravetz <mike.kravetz@oracle.com>
> ---
>  tools/testing/selftests/vm/userfaultfd.c | 67 ++++++++++++------------
>  1 file changed, 34 insertions(+), 33 deletions(-)
>
> diff --git a/tools/testing/selftests/vm/userfaultfd.c b/tools/testing/selftests/vm/userfaultfd.c
> index d3fd24f9fae8..f5578ef85560 100644
> --- a/tools/testing/selftests/vm/userfaultfd.c
> +++ b/tools/testing/selftests/vm/userfaultfd.c
> @@ -88,7 +88,6 @@ static bool test_uffdio_minor = false;
>  static bool map_shared;
>  static int shm_fd;
>  static int huge_fd;
> -static char *huge_fd_off0;
>  static unsigned long long *count_verify;
>  static int uffd = -1;
>  static int uffd_flags, finished, *pipefd;
> @@ -124,9 +123,9 @@ const char *examples =
>      "./userfaultfd anon 100 99999\n\n"
>      "# Run share memory test on 1GiB region with 99 bounces:\n"
>      "./userfaultfd shmem 1000 99\n\n"
> -    "# Run hugetlb memory test on 256MiB region with 50 bounces (using /dev/hugepages/hugefile):\n"
> +    "# Run hugetlb memory test on 256MiB region with 50 bounces:\n"
>      "./userfaultfd hugetlb 256 50 /dev/hugepages/hugefile\n\n"

We should remove the path from the line above here as well, right?
Since for the hugetlb test type, we now just MAP_ANONYMOUS |
MAP_HUGETLB, we don't open a file descriptor.

> -    "# Run the same hugetlb test but using shmem:\n"
> +    "# Run the same hugetlb test but using shared file:\n"
>      "./userfaultfd hugetlb_shared 256 50 /dev/hugepages/hugefile\n\n"
>      "# 10MiB-~6GiB 999 bounces anonymous test, "
>      "continue forever unless an error triggers\n"
> @@ -223,10 +222,13 @@ static void noop_alias_mapping(__u64 *start, size_t len, unsigned long offset)
>
>  static void hugetlb_release_pages(char *rel_area)
>  {
> -       if (fallocate(huge_fd, FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE,
> -                     rel_area == huge_fd_off0 ? 0 : nr_pages * page_size,
> -                     nr_pages * page_size))
> -               err("fallocate() failed");
> +       if (!map_shared) {
> +               if (madvise(rel_area, nr_pages * page_size, MADV_DONTNEED))
> +                       err("madvise(MADV_DONTNEED) failed");
> +       } else {
> +               if (madvise(rel_area, nr_pages * page_size, MADV_REMOVE))
> +                       err("madvise(MADV_REMOVE) failed");
> +       }
>  }
>
>  static void hugetlb_allocate_area(void **alloc_area)
> @@ -234,26 +236,37 @@ static void hugetlb_allocate_area(void **alloc_area)
>         void *area_alias = NULL;
>         char **alloc_area_alias;
>
> -       *alloc_area = mmap(NULL, nr_pages * page_size, PROT_READ | PROT_WRITE,
> -                          (map_shared ? MAP_SHARED : MAP_PRIVATE) |
> -                          MAP_HUGETLB |
> -                          (*alloc_area == area_src ? 0 : MAP_NORESERVE),
> -                          huge_fd, *alloc_area == area_src ? 0 :
> -                          nr_pages * page_size);
> +       if (!map_shared)
> +               *alloc_area = mmap(NULL,
> +                       nr_pages * page_size,
> +                       PROT_READ | PROT_WRITE,
> +                       MAP_PRIVATE | MAP_ANONYMOUS | MAP_HUGETLB |
> +                               (*alloc_area == area_src ? 0 : MAP_NORESERVE),
> +                       -1,
> +                       0);
> +       else
> +               *alloc_area = mmap(NULL,
> +                       nr_pages * page_size,
> +                       PROT_READ | PROT_WRITE,
> +                       MAP_SHARED |
> +                               (*alloc_area == area_src ? 0 : MAP_NORESERVE),
> +                       huge_fd,
> +                       *alloc_area == area_src ? 0 : nr_pages * page_size);
>         if (*alloc_area == MAP_FAILED)
>                 err("mmap of hugetlbfs file failed");
>
>         if (map_shared) {
> -               area_alias = mmap(NULL, nr_pages * page_size, PROT_READ | PROT_WRITE,
> -                                 MAP_SHARED | MAP_HUGETLB,
> -                                 huge_fd, *alloc_area == area_src ? 0 :
> -                                 nr_pages * page_size);
> +               area_alias = mmap(NULL,
> +                       nr_pages * page_size,
> +                       PROT_READ | PROT_WRITE,
> +                       MAP_SHARED,
> +                       huge_fd,
> +                       *alloc_area == area_src ? 0 : nr_pages * page_size);
>                 if (area_alias == MAP_FAILED)
>                         err("mmap of hugetlb file alias failed");
>         }
>
>         if (*alloc_area == area_src) {
> -               huge_fd_off0 = *alloc_area;
>                 alloc_area_alias = &area_src_alias;
>         } else {
>                 alloc_area_alias = &area_dst_alias;
> @@ -266,12 +279,7 @@ static void hugetlb_alias_mapping(__u64 *start, size_t len, unsigned long offset
>  {
>         if (!map_shared)
>                 return;
> -       /*
> -        * We can't zap just the pagetable with hugetlbfs because
> -        * MADV_DONTEED won't work. So exercise -EEXIST on a alias
> -        * mapping where the pagetables are not established initially,
> -        * this way we'll exercise the -EEXEC at the fs level.
> -        */
> +
>         *start = (unsigned long) area_dst_alias + offset;
>  }
>
> @@ -424,7 +432,6 @@ static void uffd_test_ctx_clear(void)
>                 uffd = -1;
>         }
>
> -       huge_fd_off0 = NULL;
>         munmap_area((void **)&area_src);
>         munmap_area((void **)&area_src_alias);
>         munmap_area((void **)&area_dst);
> @@ -922,10 +929,7 @@ static int faulting_process(int signal_test)
>         struct sigaction act;
>         unsigned long signalled = 0;
>
> -       if (test_type != TEST_HUGETLB)
> -               split_nr_pages = (nr_pages + 1) / 2;
> -       else
> -               split_nr_pages = nr_pages;
> +       split_nr_pages = (nr_pages + 1) / 2;
>
>         if (signal_test) {
>                 sigbuf = &jbuf;
> @@ -982,9 +986,6 @@ static int faulting_process(int signal_test)
>         if (signal_test)
>                 return signalled != split_nr_pages;
>
> -       if (test_type == TEST_HUGETLB)
> -               return 0;
> -
>         area_dst = mremap(area_dst, nr_pages * page_size,  nr_pages * page_size,
>                           MREMAP_MAYMOVE | MREMAP_FIXED, area_src);
>         if (area_dst == MAP_FAILED)
> @@ -1667,7 +1668,7 @@ int main(int argc, char **argv)
>         }
>         nr_pages = nr_pages_per_cpu * nr_cpus;
>
> -       if (test_type == TEST_HUGETLB) {
> +       if (test_type == TEST_HUGETLB && map_shared) {
>                 if (argc < 5)
>                         usage();
>                 huge_fd = open(argv[4], O_CREAT | O_RDWR, 0755);
> --
> 2.34.1
>