From mboxrd@z Thu Jan  1 00:00:00 1970
From: Claude Code Review Bot <claude-review@example.com>
To: dri-devel-reviews@example.com
Subject: Claude review: mm/hmm: do the plumbing for HMM to participate in migration
Date: Mon, 25 May 2026 16:46:28 +1000
Message-ID: <review-patch3-20260525050830.100254-4-mpenttil@redhat.com>
In-Reply-To: <20260525050830.100254-4-mpenttil@redhat.com>
References: <20260525050830.100254-1-mpenttil@redhat.com> <20260525050830.100254-4-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

**Several issues.**

1. **Duplicate `hmm_select_migrate()` definitions will cause build errors.*=
* Patch 3 adds two definitions of `hmm_select_migrate()` in `include/linux/=
migrate.h`:
   - One at line ~880 inside `#ifdef CONFIG_MIGRATION` / `#else`:
     ```c
     static inline enum migrate_vma_info hmm_select_migrate(struct hmm_rang=
e *range)
     {
         return MIGRATE_VMA_SELECT_NONE;
     }
     ```
   - One at line ~902 inside the `CONFIG_DEVICE_MIGRATION` section:
     ```c
     // TODO: enable migration
     static inline enum migrate_vma_info hmm_select_migrate(struct hmm_rang=
e *range)
     {
         return 0;
     }
     ```
   When `CONFIG_DEVICE_MIGRATION` is enabled, both definitions are visible,=
 producing a redefinition error. The first one (returning `MIGRATE_VMA_SELE=
CT_NONE`) is only needed as a stub when `CONFIG_DEVICE_MIGRATION` is off. T=
he second has a `// TODO` comment that should not be present in submitted c=
ode (and is a C++-style comment).

2. **`migrate_vma_direction` renamed to `migrate_vma_info` in two places** =
=E2=80=94 this is the actual rename, but the `MIGRATE_VMA_SELECT_NONE` / `M=
IGRATE_VMA_SELECT_COMPOUND` definitions are questionable:
   ```c
   enum migrate_vma_info {
       MIGRATE_VMA_SELECT_NONE =3D 0,
       MIGRATE_VMA_SELECT_COMPOUND =3D MIGRATE_VMA_SELECT_NONE,
   };
   ```
   Setting `MIGRATE_VMA_SELECT_COMPOUND =3D MIGRATE_VMA_SELECT_NONE =3D 0` =
means the compound check `(minfo & MIGRATE_VMA_SELECT_COMPOUND)` in `hmm_pf=
ns_fill()` will always be false. This is intentional for the non-migration =
stub, but confusing because the real `MIGRATE_VMA_SELECT_COMPOUND` (defined=
 elsewhere as `1 << 3`) has a different value.

3. **C++-style comments** throughout (`//` instead of `/* */`). Linux kerne=
l coding style requires C-style comments.

4. **`hmm_vma_walk_pmd()` rewrite is complex and hard to verify.** The func=
tion grows from ~50 lines to ~130+ lines with interleaved lock management v=
ia booleans. Key concern =E2=80=94 after the `pmd_trans_huge()` block proce=
sses the PMD:
   ```c
   if (pmd_trans_huge(pmd)) {
       ...
       r =3D hmm_vma_handle_pmd(walk, addr, end, hmm_pfns, pmd);
       // If not migrating we are done
       if (r || !minfo) {
           if (hmm_vma_walk->pmdlocked) {
               spin_unlock(hmm_vma_walk->ptl);
               hmm_vma_walk->pmdlocked =3D false;
           }
           return r;
       }
   }
   r =3D hmm_vma_handle_migrate_prepare_pmd(walk, pmdp, start, end, hmm_pfn=
s);
   ```
   If `hmm_vma_handle_pmd()` returns 0 and `minfo` is set, execution falls =
through to `hmm_vma_handle_migrate_prepare_pmd()` with the PMD lock still h=
eld. This is correct but the flow is non-obvious =E2=80=94 the comment "If =
not migrating we are done" only makes sense if you understand the full stat=
e machine.

5. **`hmm_vma_capture_migrate_range()` issues:**
   ```c
   if (!hmm_vma_walk->mmu_range.owner) {
       mmu_notifier_range_init_owner(&hmm_vma_walk->mmu_range, ...);
       mmu_notifier_invalidate_range_start(&hmm_vma_walk->mmu_range);
   }
   ```
   The `mmu_notifier_invalidate_range_start()` is called but the correspond=
ing `_end()` is done later in `hmm_range_fault()` only when `hmm_select_mig=
rate(range) && range->migrate && hmm_vma_walk.mmu_range.owner`. If `hmm_ran=
ge_fault()` returns early (e.g., `-EBUSY` from `mmu_interval_check_retry()`=
), the break at line 1530 jumps past the retry loop but still reaches the `=
#ifdef CONFIG_DEVICE_MIGRATION` cleanup. This appears correct but is fragil=
e.

6. **`hmm_vma_handle_pte()` has an `if/else` without braces mismatch:**
   ```c
   if (!hmm_select_migrate(range)) {
       ...
       return -EBUSY;
   } else
       goto out;
   ```
   Kernel coding style requires braces on the `else` when the `if` has them.

---

---
Generated by Claude Code Patch Reviewer