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

[Axel Rasmussen <axelrasmussen@google.com>]

On Thu, Jan 6, 2022 at 9:43 AM Mike Kravetz <mike.kravetz@oracle.com> wrote:
>
> On 1/5/22 15:56, Andrew Morton wrote:
> > On Tue, 4 Jan 2022 14:35:34 -0800 Axel Rasmussen <axelrasmussen@google.com> wrote:
> >
> >> 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.
> >>>
> >>>         *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.
> >
> > When you have that many arguments I think it's clearer to put one per
> > line, viz.
> >
> >       *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);
> >
> >
> > But whatever...
> I agree, and also agree with Axel's comment about keeping the ternary all on
> one line.  However, there are examples of breaking both these conventions throughout the file.

For what it's worth, I don't at all mind Andrew's way either, where
the two "outcomes" of the ternary are indented a bit.

Not a big deal though, whatever you'd prefer is fine. :)

>
> My intention here was just to clean up the mess I created with the previous
> patch.  As such, I would prefer to leave this patch as is.  If someone really
> wants this modified, I will.  However, IMO if we make this one call easier
> to read, we should use the same convention throughout the file.  I can do that
> as well, but would prefer to first try to enable using mremap with hugetlb
> within the test.

+1, sounds like a good plan.

> --
> Mike Kravetz