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