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

Hi Mike!

On 8/3/26 2:30 PM, Mike Rapoport wrote:
>> 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.
> 

I was following the existing logic that run_gup_matrix() had. I can
implement this. Any suggestions what number of huge pages we can fix?

>> +
>> +	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();
> 	}

Got it, will change.

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

Ack

> 
> 
>> +			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?

Will move both which_pages and gup_flags to the initialization.