Re: [PATCH v1 2/3] selftests/mm: gup_longterm: new functional test for FOLL_LONGTERM

[Lorenzo Stoakes]

On Thu, Jun 01, 2023 at 10:16:41AM +0200, David Hildenbrand wrote:
> On 28.05.23 17:03, Lorenzo Stoakes wrote:
> > On Fri, May 19, 2023 at 12:27:22PM +0200, David Hildenbrand wrote:
> > > Let's add a new test for checking whether GUP long-term page pinning
> > > works as expected (R/O vs. R/W, MAP_PRIVATE vs. MAP_SHARED, GUP vs.
> > > GUP-fast). Note that COW handling with long-term R/O pinning in private
> > > mappings, and pinning of anonymous memory in general, is tested by the
> > > COW selftest. This test, therefore, focuses on page pinning in
> > > file mappings.
> > >
> > > The most interesting case is probably the "local tmpfile" case, as that
> > > will likely end up on a "real" filesystem such as ext4 or xfs, not on a
> > > virtual one like tmpfs or hugetlb where any long-term page pinning is
> > > always expected to succeed.
> > >
> > > For now, only add tests that use the "/sys/kernel/debug/gup_test"
> > > interface. We'll add tests based on liburing separately next.
> > >
> > > Signed-off-by: David Hildenbrand <david@xxxxxxxxxx>
> > > ---
>
> [...]
>
> > > +static void do_test(int fd, size_t size, enum test_type type, bool shared)
> > > +{
> > > +	__fsword_t fs_type = get_fs_type(fd);
> > > +	bool should_work;
> > > +	char *mem;
> > > +	int ret;
> > > +
> > > +	if (ftruncate(fd, size)) {
> > > +		ksft_test_result_fail("ftruncate() failed\n");
> > > +		return;
> > > +	}
> > > +
> > > +	if (fallocate(fd, 0, 0, size)) {
> > > +		if (size == pagesize)
> > > +			ksft_test_result_fail("fallocate() failed\n");
> > > +		else
> > > +			ksft_test_result_skip("need more free huge pages\n");
> > > +		return;
> > > +	}
> > > +
> > > +	mem = mmap(NULL, size, PROT_READ | PROT_WRITE,
> > > +		   shared ? MAP_SHARED : MAP_PRIVATE, fd, 0);
> > > +	if (mem == MAP_FAILED) {
> > > +		if (size == pagesize || shared)
> > > +			ksft_test_result_fail("mmap() failed\n");
> > > +		else
> > > +			ksft_test_result_skip("need more free huge pages\n");
> > > +		return;
> > > +	}
> > > +
> > > +	/*
> > > +	 * Fault in the page writable such that GUP-fast can eventually pin
> > > +	 * it immediately.
> > > +	 */
> > > +	memset(mem, 0, size);
> >
>
> For shared mappings, MAP_POPULATE will not fault-in the pages writable. See
> mm/gup.c:populate_vma_page_range().

Ughhh yeah, I was aware but hadn't considered the shared case, here. Fair
enough.

>
> [There is also the case that mmap() doesn't fail if populate fails, but
> that's only a side note regarding weird semantics of MAP_POPULATE]

Yes this is... a thing. And mm_populate() explicitly (void)-casting
__mm_populate() is the cherry on that particular cake :)

>
> [...]
>
> > > +	int flags = MFD_HUGETLB;
> > > +	int fd;
> > > +
> > > +	ksft_print_msg("[RUN] %s ... with memfd hugetlb (%zu kB)\n", desc,
> > > +		       hugetlbsize / 1024);
> > > +
> > > +	flags |= __builtin_ctzll(hugetlbsize) << MFD_HUGE_SHIFT;
> >
> > Hm this feels a little cute :)
>
> It's a weird interfacing, having to specify the desired size via flags ...
> see the man page of memfd_create, which links to the man page of mmap: "the
> desired huge page size can be configured by encoding the base-2 logarithm of
> the desired page size in the six bits at the offset MAP_HUGE_SHIFT".
>
> FWIW, we're using the same approach in cow.c already [and other memfd users
> like QEMU do it just like that, using ctz].

Ack, yeah I had assumed so, just felt slightly odd. Thanks for the
explanation!

>
> [...]
>
> > > diff --git a/tools/testing/selftests/mm/run_vmtests.sh b/tools/testing/selftests/mm/run_vmtests.sh
> > > index 4893eb60d96d..b6b1eb6a8a6b 100644
> > > --- a/tools/testing/selftests/mm/run_vmtests.sh
> > > +++ b/tools/testing/selftests/mm/run_vmtests.sh
> > > @@ -24,7 +24,7 @@ separated by spaces:
> > >   - mmap
> > >   	tests for mmap(2)
> > >   - gup_test
> > > -	tests for gup using gup_test interface
> > > +	tests for gup
> >
> > Super nitty again, but I'm guessing this means the CONFIG_GUP_TEST
> > interface, perhaps worth keeping?
>
> With this patch, agreed. But not longer with the next patch -- guess I
> simplified when splitting it up. If there are no strong feelings I'll leave
> it in this patch.
>
> [...]
>
> > >
> >
> > OK this patch is really nice + well implemented, I can only point out a
> > couple EXTREMELY nitty comments :) Thanks very much for adding a test for
> > this, it's super useful!
> >
> > Therefore,
> >
> > Reviewed-by: Lorenzo Stoakes <lstoakes@xxxxxxxxx>
> >
>
> Thanks for the review! My selftest patches rarely get that much attention,
> so highly appreciated :)

No worries, this is very much in my wheelhouse (relating directly to my
recent GUP series) so this is actually very useful and relevant to me. Also
I am very much in favour of improved test coverage, is a bug bear of mine.

>
> --
> Thanks,
>
> David / dhildenb
>