On Mon, Jan 3, 2022 at 6:17 PM Mike Kravetz <mike.kravetz@xxxxxxxxxx> 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@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxx>
