From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: lib/test_hmm: add a new testcase for the migrate on fault Date: Tue, 26 May 2026 07:29:20 +1000 Message-ID: In-Reply-To: <20260525084524.139868-6-mpenttil@redhat.com> References: <20260525084524.139868-1-mpenttil@redhat.com> <20260525084524.139868-6-mpenttil@redhat.com> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Patch Review Adds the `HMM_DMIRROR_MIGRATE_ON_FAULT_TO_DEV` ioctl and a selftest exercis= ing it. **Issue 1 =E2=80=94 IOCTL number insertion breaks existing numbering:** ```c -#define HMM_DMIRROR_MIGRATE_TO_SYS _IOWR('H', 0x03, struct hmm_dmirror_cmd) +#define HMM_DMIRROR_MIGRATE_ON_FAULT_TO_DEV _IOWR('H', 0x03, struct hmm_dm= irror_cmd) +#define HMM_DMIRROR_MIGRATE_TO_SYS _IOWR('H', 0x04, struct hmm_dmirror_cm= d) ``` Inserting the new ioctl at 0x03 shifts all subsequent numbers. While this i= s a test-only driver (not a stable ABI), it breaks any existing out-of-tree= userspace using these ioctls. Appending as 0x09 would be cleaner and avoid= s renumbering. **Issue 2 =E2=80=94 Test doesn't exercise the "fault + migrate" path:** ```c + /* Initialize buffer in system memory. */ + for (i =3D 0, ptr =3D buffer->ptr; i < size / sizeof(*ptr); ++i) + ptr[i] =3D i; + + /* Fault and migrate memory to device. */ + ret =3D hmm_migrate_on_fault_sys_to_dev(self->fd, buffer, npages); ``` The write loop faults in all pages before the migrate-on-fault call. This m= eans the test exercises "migrate already-present pages via the new path" bu= t NOT "fault-in absent pages and migrate them in one walk" =E2=80=94 which = is the primary optimization this series enables. Consider adding a test tha= t calls `hmm_migrate_on_fault_sys_to_dev` WITHOUT the initialization loop (= or after an `madvise(MADV_DONTNEED)`) to exercise the true fault+migrate pa= th. **Issue 3 =E2=80=94 `do_fault_and_migrate` proceeds with migration even on = fault error:** ```c + ret =3D dmirror_range_fault(dmirror, range); + + pr_debug("Migrating from sys mem to device mem\n"); + migrate_hmm_range_setup(range); + + dmirror_migrate_alloc_and_copy(migrate, dmirror); + migrate_vma_pages(migrate); ``` If `dmirror_range_fault` fails, the code still proceeds with migration setu= p and processing. The subsequent functions handle partial state gracefully = (skipping entries without `MIGRATE_PFN_MIGRATE`), but it's still doing unne= cessary work. An early-return on error with cleanup would be cleaner. **Issue 4 =E2=80=94 Extra blank line:** ```c + dmirror_bounce_fini(&bounce); + + +out_mmput: ``` Double blank line. --- Generated by Claude Code Patch Reviewer