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

[Sarthak Sharma <sarthak.sharma@arm.com>]

Hi David!

On 9/7/26 8:45 PM, David Hildenbrand (Arm) wrote:
> On 9/4/26 14:36, Sarthak Sharma 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 5 test cases for every variant: get_user_pages,
>> get_user_pages_fast, pin_user_pages, pin_user_pages_fast and
>> pin_user_pages_longterm.
>>
>> Choose the number of default hugeTLB pages using a 256 MiB target,
>> with a minimum of 1 page and derive the mapping size from that
>> number. This avoids reserving excess memory when the hugeTLB page
>> size is too large and retains 128 pages for the most common case
>> of 2MiB hugeTLB pages.
>>
>> 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. In total the selftest reports 60 TAP cases and issues
>> 240 ioctls.

[...]

> 
> 
> BTW, I was wondering what it would take to:
> 
> 1) Turn mm/gup_test.o into an OOT module (would we need more EXPORT_SYMBOL_GPL?
> EXPORT_SYMBOL_FOOR_MODULE ?)
> 
> 2) Move it to tools/mm/modules or sth like that.
> 
> 3) Build it with the selftests etc
> 
> 4) Remove GUP_TEST
> 
> 5) Try insmod'ing it from the tools+selftests that need it.

This is an interesting change. We can keep this open for discussion
here. If required, I can work on this in the future.

> 
> [...]
> 
>> +int main(int argc, char **argv)
>> +{
>> +	char *file = "/dev/zero";
>> +	int fd;
>> +
>> +	fd = open(file, O_RDWR);
>> +	if (fd < 0) {
>> +		ksft_print_header();
>> +		ksft_exit_fail_msg("Unable to open %s: %s\n", file, strerror(errno));
>> +	}
>> +	close(fd);
> 
> 
> I'm confused. Why do we have to open+close /dev/zero?

This is a pre requisite check. Every test opens and closes /dev/zero and
/sys/kernel/debug/gup_test of its own. So I wanted to check before
running the harness if these two are available, so that we don't have
setup failures for 60 test cases.

> 
>> +
>> +	fd = open(GUP_TEST_FILE, O_RDWR);
>> +	if (fd == -1) {
>> +		ksft_print_header();
>> +		if (errno == EACCES)
>> +			ksft_exit_skip("Please run this test as root\n");
> 
> Wouldn't we want to fail here?

mm selftests normally skip if the test is not run as root. So I tried
keeping the same thing here. Do you think I should change it to fail?

> 
>> +		if (errno == ENOENT) {
>> +			DIR *debugfs = opendir("/sys/kernel/debug");
>> +
>> +			if (!debugfs) {
>> +				ksft_exit_skip("Mount debugfs at /sys/kernel/debug\n");
>> +			} else {
>> +				closedir(debugfs);
>> +				ksft_exit_skip("Check CONFIG_GUP_TEST in kernel config\n");
>> +			}
> 
> You can remove the } else { part as you skip on !debugfs.

Agreed

> 
>> +		}
>> +		ksft_exit_fail_msg("Failed to open %s: %s\n", GUP_TEST_FILE, strerror(errno));
>> +	}
>> +	close(fd);
>> +
>> +	hp_size = default_huge_page_size();
>> +	if (hp_size) {
>> +		nr_huge_pages = HUGETLB_TARGET_SIZE / hp_size;
>> +		if (!nr_huge_pages)
>> +			nr_huge_pages = 1;
>> +
>> +		hugetlb_setup_succeeded = hugetlb_setup_default(nr_huge_pages);
>> +	}
> 
> BTW, why are we using HUGETLB_TARGET_SIZE instead of just using the
> default_huge_page_size()?

HUGETLB_TARGET_SIZE is the target mapping size and
default_huge_page_size() gives the size of a single hugetlb page.

Using default_huge_page_size() would reduce coverage for 2MB hugetlb
pages. The old test set self->size to be 256 MB for the hugetlb case.

Now it was discussed in a previous version of this patchset that we can
derive the self->size for hugetlb case by fixing the nr_hugepages and
multiplying by hugetlb size, and thought 128 would be a good number for
nr_hugepages [1].

But in case the hugetlb pages are very large, eg we can have 1 GB
hugepages as well, reserving 128 GB is not a good idea. So I tried to
keep the target size of the mapping as 256 MB, as it was before in the
old gup test. If the hugetlb pages are larger than this, we'll reserve
only one of them. Else, we'll reserve (256 MB /
default_huge_page_size()) hugetlb pages, which comes out to be 128 for
the case of 2MB hugetlb pages.

[1] https://lore.kernel.org/all/anGyXt_PDImUglIN@kernel.org/

> 
>> +
>> +	return test_harness_run(argc, argv);
>> +}
>> diff --git a/tools/testing/selftests/mm/run_vmtests.sh b/tools/testing/selftests/mm/run_vmtests.sh
>> index 8f1e828e4f39..ae0ab5efabae 100755
>> --- a/tools/testing/selftests/mm/run_vmtests.sh
>> +++ b/tools/testing/selftests/mm/run_vmtests.sh
>> @@ -251,6 +251,7 @@ fi
>>  
>>  CATEGORY="mmap" run_test ./map_fixed_noreplace
>>  
>> +CATEGORY="gup_test" run_test ./gup
>>  CATEGORY="gup_test" run_test ./gup_longterm
> 
> Nice

:)

>