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

[Sarthak Sharma]

Hi David!

On 9/9/26 10:33 PM, David Hildenbrand (Arm) wrote:
>>> 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.
> 
> Yes, we should in general try moving all test modules out of the core.
> 
>>>
>>>> +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.
> 
> But why /dev/zero? We should understand why that would possibly be required.

This was carried over from the old test, where /dev/zero was the default
backing for mmap unless the user selected another file to back the
mapping. Now since we don't support file backed mappings, we can
directly use MAP_ANONYMOUS here. Thanks for pointing it out, I'll remove
it from this patch.

> 
>>
>>>
>>>> +
>>>> +	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 other tests do that, it's fine!
> 
> [...]
> 
>>>
>>> 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.
> It's odd that 2M gets better test coverage than 512M or 1G.
> 
> Is there really a lot of value in testing 128 2M pages? Would, like, 2 already
> be good enough?

Yup, seems like keeping 128 pages is not adding an extra value. I'll go
with 2 hugetlb pages to test both pinning within a hugetlb page and
across the hugetlb boundary. This would make things a lot simpler.