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

["David Hildenbrand (Arm)" <david@kernel.org>]

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

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

-- 
Cheers,

David