Re: [PATCH] userfaultfd/selftests: clean up hugetlb allocation code

[Axel Rasmussen <axelrasmussen@google.com>]

On Mon, Jan 3, 2022 at 6:17 PM Mike Kravetz <mike.kravetz@oracle.com> wrote:
>
> The message for commit f5c73297181c ("userfaultfd/selftests: fix hugetlb
> area allocations") says there is no need to create a hugetlb file in the
> non-shared testing case.  However, the commit did not actually change
> the code to prevent creation of the file.
>
> While it is technically true that there is no need to create and use a
> hugetlb file in the case of non-shared-testing, it is useful.  This is
> because 'hole punching' of a hugetlb file has the potentially incorrect
> side effect of also removing pages from private mappings.  The
> userfaultfd test relies on this side effect for removing pages from the
> destination buffer during rounds of stress testing.
>
> Remove the incomplete code that was added to deal with no hugetlb file.
> Just keep the code that prevents reserves from being created for the
> destination area.
>
> Signed-off-by: Mike Kravetz <mike.kravetz@oracle.com>
> ---
>  tools/testing/selftests/vm/userfaultfd.c | 15 ++++++---------
>  1 file changed, 6 insertions(+), 9 deletions(-)
>
> diff --git a/tools/testing/selftests/vm/userfaultfd.c b/tools/testing/selftests/vm/userfaultfd.c
> index 9354a5e0321c..0b543a9a42b2 100644
> --- a/tools/testing/selftests/vm/userfaultfd.c
> +++ b/tools/testing/selftests/vm/userfaultfd.c
> @@ -87,7 +87,7 @@ static bool test_uffdio_minor = false;
>
>  static bool map_shared;
>  static int shm_fd;
> -static int huge_fd = -1;       /* only used for hugetlb_shared test */
> +static int huge_fd;
>  static char *huge_fd_off0;
>  static unsigned long long *count_verify;
>  static int uffd = -1;
> @@ -223,9 +223,6 @@ static void noop_alias_mapping(__u64 *start, size_t len, unsigned long offset)
>
>  static void hugetlb_release_pages(char *rel_area)
>  {
> -       if (huge_fd == -1)
> -               return;
> -
>         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))
> @@ -238,17 +235,17 @@ static void hugetlb_allocate_area(void **alloc_area)
>         char **alloc_area_alias;
>
>         *alloc_area = mmap(NULL, nr_pages * page_size, PROT_READ | PROT_WRITE,
> -                          map_shared ? MAP_SHARED :
> -                          MAP_PRIVATE | MAP_HUGETLB |
> +                          (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);
> +                          huge_fd, *alloc_area == area_src ? 0 :
> +                          nr_pages * page_size);

Sorry to nitpick, but I think it was slightly more readable when the
ternary was all on one line.

>         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_SHARED | MAP_HUGETLB,
>                                   huge_fd, *alloc_area == area_src ? 0 :
>                                   nr_pages * page_size);
>                 if (area_alias == MAP_FAILED)
> --
> 2.33.1
>

Looks functionally correct to me besides the one style gripe,

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