On Thu, Jan 6, 2022 at 9:43 AM Mike Kravetz <mike.kravetz@xxxxxxxxxx> wrote:
>
> On 1/5/22 15:56, Andrew Morton wrote:
> > On Tue, 4 Jan 2022 14:35:34 -0800 Axel Rasmussen <axelrasmussen@xxxxxxxxxx> wrote:
> >
> >> 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.
> >>>
> >>> *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
