Claude review: mm/hmm: do the plumbing for HMM to participate in migration

[Claude Code Review Bot <claude-review@example.com>]

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_range *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_range *range)
     {
         return 0;
     }
     ```
   When `CONFIG_DEVICE_MIGRATION` is enabled, both definitions are visible, producing a redefinition error. The first one (returning `MIGRATE_VMA_SELECT_NONE`) is only needed as a stub when `CONFIG_DEVICE_MIGRATION` is off. The second has a `// TODO` comment that should not be present in submitted code (and is a C++-style comment).

2. **`migrate_vma_direction` renamed to `migrate_vma_info` in two places** — this is the actual rename, but the `MIGRATE_VMA_SELECT_NONE` / `MIGRATE_VMA_SELECT_COMPOUND` definitions are questionable:
   ```c
   enum migrate_vma_info {
       MIGRATE_VMA_SELECT_NONE = 0,
       MIGRATE_VMA_SELECT_COMPOUND = MIGRATE_VMA_SELECT_NONE,
   };
   ```
   Setting `MIGRATE_VMA_SELECT_COMPOUND = MIGRATE_VMA_SELECT_NONE = 0` means the compound check `(minfo & MIGRATE_VMA_SELECT_COMPOUND)` in `hmm_pfns_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 kernel coding style requires C-style comments.

4. **`hmm_vma_walk_pmd()` rewrite is complex and hard to verify.** The function grows from ~50 lines to ~130+ lines with interleaved lock management via booleans. Key concern — after the `pmd_trans_huge()` block processes the PMD:
   ```c
   if (pmd_trans_huge(pmd)) {
       ...
       r = 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 = false;
           }
           return r;
       }
   }
   r = hmm_vma_handle_migrate_prepare_pmd(walk, pmdp, start, end, hmm_pfns);
   ```
   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 held. This is correct but the flow is non-obvious — the comment "If not migrating we are done" only makes sense if you understand the full state 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 corresponding `_end()` is done later in `hmm_range_fault()` only when `hmm_select_migrate(range) && range->migrate && hmm_vma_walk.mmu_range.owner`. If `hmm_range_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 fragile.

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