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: Mon, 25 May 2026 16:46:28 +1000 Message-ID: In-Reply-To: <20260525050830.100254-6-mpenttil@redhat.com> References: <20260525050830.100254-1-mpenttil@redhat.com> <20260525050830.100254-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 **UAPI and testing concerns.** 1. **UAPI ioctl number renumbering is an ABI break:** ```c -#define HMM_DMIRROR_MIGRATE_TO_SYS _IOWR('H', 0x03, ...) +#define HMM_DMIRROR_MIGRATE_ON_FAULT_TO_DEV _IOWR('H', 0x03, ...) +#define HMM_DMIRROR_MIGRATE_TO_SYS _IOWR('H', 0x04, ...) ``` All existing ioctls from 0x03 onward are renumbered. While this is a tes= t driver (not general UAPI), any existing test binary compiled against the = old headers will silently call the wrong ioctl. The new ioctl should be app= ended at the end (0x09) rather than inserted in the middle. 2. **`do_fault_and_migrate()` doesn't check `dmirror_range_fault()` return = value before proceeding:** ```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); ``` If `dmirror_range_fault()` fails, the code continues to call `migrate_hm= m_range_setup()` and the full migration sequence. The `ret` value is captur= ed but not checked before proceeding. 3. **`dmirror_range_fault()` skips `mmap_read_lock` when migrating** but th= e `mmap_read_lock` is taken in the caller `do_fault_and_migrate()`. In `dmi= rror_range_fault()`: ```c if (!migrate) { mmap_read_lock(mm); ret =3D hmm_range_fault(range); mmap_read_unlock(mm); } else { ret =3D hmm_range_fault(range); } ``` And in `do_fault_and_migrate()`: ```c mmap_read_lock(dmirror->notifier.mm); ret =3D dmirror_range_fault(dmirror, range); ... mmap_read_unlock(dmirror->notifier.mm); ``` This is correct but the v10=E2=86=92v11 changelog says "Fix nested mmap_= read_lock in test suite", so this was recently fixed. The fix looks correct= =E2=80=94 the lock is held across the entire fault+migrate sequence, which= is required. 4. **Stack-allocated arrays** with fixed size: ```c unsigned long src_pfns[PFNS_ARRAY_SIZE] =3D { 0 }; // 64 entries =3D 64= * 8 =3D 512 bytes unsigned long dst_pfns[PFNS_ARRAY_SIZE] =3D { 0 }; // another 512 bytes ``` 1KB on the kernel stack is within limits but worth noting. The `range.hm= m_pfns =3D src_pfns` reuses the same array for both HMM pfns and migrate sr= c pfns, which is correct since `migrate_hmm_range_setup()` converts in-plac= e. 5. **The selftest `hmm_migrate_on_fault` only tests the happy path** =E2=80= =94 anonymous private pages that are already present (initialized before mi= gration). The cover letter's motivation is about migrating *non-present* pa= ges, but the test doesn't cover that case (it writes to pages first, making= them present). Adding a test that mmaps but doesn't touch the pages before= migrating would better validate the stated goal. --- Generated by Claude Code Patch Reviewer