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

[David Hildenbrand]

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

[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]

[...]

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

[...]

> > 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 :)

--
Thanks,

David / dhildenb