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.
