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

[Andrew Morton]

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...