From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: Migrate on fault for device pages Date: Mon, 25 May 2026 16:46:27 +1000 Message-ID: In-Reply-To: <20260525050830.100254-1-mpenttil@redhat.com> References: <20260525050830.100254-1-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 Overall Series Review Subject: Migrate on fault for device pages Author: mpenttil@redhat.com Patches: 6 Reviewed: 2026-05-25T16:46:27.367330 --- This is v11 of a series to add "migrate on fault" for device pages, unifyin= g the HMM fault path (`hmm_range_fault()`) with the migration path (`migrat= e_vma_*`) to reduce redundant page table walks. The goal is sound and well-= motivated =E2=80=94 avoiding 2=E2=80=933 page table walks when a single wal= k could do both faulting and migration. **Strengths:** - The core idea of combining fault + migration in a single page table walk = is a clear performance win. - Extensive testing across 11 versions shows good diligence on locking edge= cases. - The Kconfig dependency fix (patch 1) and the pfn conversion helper (patch= 2) are clean. - David Hildenbrand's Acked-by on patch 1 is a positive signal. **Concerns:** - **Duplicate `hmm_select_migrate()` definitions in patch 3**: Two static i= nline definitions exist in the same header (`include/linux/migrate.h`) =E2= =80=94 one under `CONFIG_MIGRATION` and one under `CONFIG_DEVICE_MIGRATION`= . This will cause a compilation error for certain configs. - **`migrate_vma_unmap()` is static but called from `migrate_device.c` exte= rnally in patch 2**: `migrate_hmm_range_setup()` calls `migrate_vma_unmap()= ` which is `static` in `migrate_device.c`. This only works because both are= in the same file, but it's fragile and needs a forward declaration or expo= rt. - **`// TODO` comments and C++-style comments** remain in patch 3, which sh= ould be removed before merge. - **UAPI ioctl renumbering in patch 5** is an ABI break for existing users = of the test driver. - **Error handling in `migrate_vma_setup()` (patch 4)** calls `migrate_vma_= pages()` + `migrate_vma_finalize()` on error to clean up migration entries,= but this inverts the previous contract where `migrate_vma_setup()` always = returned 0. - The locking complexity in `hmm_vma_walk_pmd()` (patch 3/4) is substantial= and hard to audit =E2=80=94 the pmd/pte lock tracking via booleans is erro= r-prone. --- --- Generated by Claude Code Patch Reviewer