Re: [PATCH v6 6/6] selftests/mm: add a GUP selftest

> Add a new GUP selftest which uses kselftest_harness.h. Cover
> 12 mapping configurations: THP enabled, THP disabled and
> HugeTLB, each across private/shared mappings and with/without
> FOLL_WRITE. Run 7 testcases for every variant: get_user_pages,
> get_user_pages_fast, pin_user_pages, pin_user_pages_fast,
> pin_user_pages_longterm, and DUMP_USER_PAGES_TEST using both
> get and pin.
> 
> Sweep four nr_pages_per_call values for each test: 1, 512, 123 and
> all pages. This preserves the coverage previously provided by
> run_gup_matrix(): 12 mapping combinations x 5 GUP/PUP operations x 4
> batch sizes, for 240 ioctl calls. The two dump modes add another 96
> calls.
> 
> Preserve the previous sparse dump coverage with a standalone test for
> pages 0, 19 and 0x1000. In total the selftest reports 85 TAP
> cases and issues 337 ioctls.
> 
> Add the new gup binary to the selftests/mm build, .gitignore,
> run_vmtests.sh and MAINTAINERS. Update
> Documentation/core-api/pin_user_pages.rst for the new test.
> 
> Suggested-by: David Hildenbrand (Arm) <[email protected]>
> Signed-off-by: Sarthak Sharma <[email protected]>
>
> +
> +FIXTURE_SETUP(gup_test)
> +{
> +	int mmap_flags = MAP_PRIVATE;
> +	int zero_fd;
> +	char *p;
> +
> +	/* zero_fd has to be >= 0. Already checked in main() */
> +	zero_fd = open("/dev/zero", O_RDWR);
> +	ASSERT_GE(zero_fd, 0);
> +
> +	/* gup_fd has to be >= 0. Already checked in main() */
> +	self->gup_fd = open(GUP_TEST_FILE, O_RDWR);
> +	ASSERT_GE(self->gup_fd, 0);
> +
> +	self->size = variant->hugetlb ? 256 * MB : 128 * MB;

I'd derive the hugetbl variant size from the size of a huge page and
predefined number of huge pages.

> +
> +	if (variant->hugetlb) {
> +		unsigned long hp_size = default_huge_page_size();
> +
> +		if (!hp_size) {
> +			close(zero_fd);
> +			close(self->gup_fd);

You can move hugetlb setup after opening those and save the headache of
closing them.

And in any rate prefer 

	goto err_do_cleanup

to
	if (something_failed) {
		cleanup1();
		cleanup2();
	}

	if (something_else_failed) {
		cleanup1();
		cleanup2();
		cleanup3();
	}

> +			SKIP(return, "HugeTLB not available\n");
> +		}
> +
> +		self->size = (self->size + hp_size - 1) & ~(hp_size - 1);
> +		if (!hugetlb_setup_default(self->size / hp_size)) {
> +			hugetlb_restore_settings();

No need to call restore() here.

Also, if you use a constant number of huge pages you can just add
HUGETLB_SETUP_DEFAULT_PAGES(NR_HUGE_PAGES) somewhere in the begining and
you won't need to setup and teardown hugetlb explicitly for every
hugetlb test.


> +			close(zero_fd);
> +			close(self->gup_fd);
> +			SKIP(return, "Not enough huge pages\n");
> +		}
> +
> +		mmap_flags |= (MAP_HUGETLB | MAP_ANONYMOUS);
> +	}
> +
> +	if (variant->shared)
> +		mmap_flags = (mmap_flags & ~MAP_PRIVATE) | MAP_SHARED;
> +
> +	self->addr = mmap(NULL, self->size, PROT_READ | PROT_WRITE,
> +			  mmap_flags, zero_fd, 0);
> +
> +	ASSERT_NE(self->addr, MAP_FAILED) {
> +		int err = errno;
> +
> +		close(zero_fd);
> +		close(self->gup_fd);
> +		if (variant->hugetlb)
> +			hugetlb_restore_settings();
> +		TH_LOG("mmap failed: %s", strerror(err));
> +	}
> +	close(zero_fd);
> +
> +	if (variant->thp)
> +		madvise(self->addr, self->size, MADV_HUGEPAGE);
> +	else if (!variant->hugetlb)
> +		madvise(self->addr, self->size, MADV_NOHUGEPAGE);
> +
> +	for (p = self->addr; (unsigned long)p < (unsigned long)self->addr
> +			+ self->size; p += psize())
> +		p[0] = 0;
> +}
> +
> +FIXTURE_TEARDOWN(gup_test)
> +{
> +	munmap(self->addr, self->size);
> +	close(self->gup_fd);
> +
> +	if (variant->hugetlb)
> +		hugetlb_restore_settings();
> +}
> +
> +static void run_gup_cmd(struct __test_metadata *_metadata,
> +			 FIXTURE_DATA(gup_test) *self,
> +			 const FIXTURE_VARIANT(gup_test) *variant,
> +			 unsigned long command,
> +			 unsigned int test_flags,
> +			 unsigned int which_page)
> +{
> +	int i;
> +
> +	for (i = 0; i < (int)ARRAY_SIZE(nr_pages_list); i++) {
> +		struct gup_test gup = {
> +			.addr = (unsigned long)self->addr,
> +			.size = self->size,
> +			.nr_pages_per_call = nr_pages_list[i] < 0 ?
> +				self->size / psize() : nr_pages_list[i],
> +			.test_flags = test_flags,
> +		};
> +
> +		if (variant->write)
> +			gup.gup_flags |= FOLL_WRITE;
> +
> +		gup.which_pages[0] = which_page;

Can't these to go to the static initialization?

-- 
Sincerely yours,
Mike.