From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: drm/xe/userptr: Defer Waiting for TLB invalidation to the second pass if possible Date: Tue, 03 Mar 2026 13:05:44 +1000 Message-ID: In-Reply-To: <20260302163248.105454-5-thomas.hellstrom@linux.intel.com> References: <20260302163248.105454-1-thomas.hellstrom@linux.intel.com> <20260302163248.105454-5-thomas.hellstrom@linux.intel.com> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit MIME-Version: 1.0 Patch Review This is the payoff patch. It extends the two-pass model to also defer TLB invalidation waits. However, the implementation effectively creates a **three-pass** flow using a **two-pass** infrastructure, which is somewhat surprising. **Three-pass flow via two-pass API:** The commit message describes three flows: ``` pass1 (fences pending) -> invalidate_finish -> do_inval (sync TLB) pass1 (fences done) -> do_inval -> invalidate_finish -> complete_tlb_inval (deferred TLB) pass1 (finish occupied) -> do_inval (sync TLB, inline) ``` In the second flow, `xe_vma_userptr_do_inval` returns `&userptr->finish`, which means it's requesting another finish callback. But this finish pointer goes through `xe_vma_userptr_invalidate_finish` which checks `tlb_inval_submitted` and dispatches to `xe_vma_userptr_complete_tlb_inval`. This works because `xe_vma_userptr_do_inval` is called from `xe_vma_userptr_invalidate_finish` which is called from `mn_itree_finish_pass`. **Wait - this doesn't actually achieve the third pass.** Looking at `mn_itree_finish_pass`: ```c static void mn_itree_finish_pass(struct llist_head *finish_passes) { struct llist_node *first = llist_reverse_order(__llist_del_all(finish_passes)); struct mmu_interval_notifier_finish *f, *next; llist_for_each_entry_safe(f, next, first, link) f->notifier->ops->invalidate_finish(f); } ``` The `invalidate_finish` callback can return a new `finish` pointer from `xe_vma_userptr_do_inval`, but **nobody collects it**. The finish callback is `void` - it doesn't return a pointer. So the flow where `xe_vma_userptr_do_inval` returns `&userptr->finish` from within the `invalidate_finish` callback is problematic: the returned pointer goes to the caller `xe_vma_userptr_invalidate_finish`, which ignores it (the function is `void`). **Wait, re-reading `xe_vma_userptr_invalidate_finish`:** ```c static void xe_vma_userptr_invalidate_finish(struct mmu_interval_notifier_finish *finish) { ... down_write(&vm->svm.gpusvm.notifier_lock); if (uvma->userptr.tlb_inval_submitted) xe_vma_userptr_complete_tlb_inval(vm, uvma); else xe_vma_userptr_do_inval(vm, uvma, true); up_write(&vm->svm.gpusvm.notifier_lock); ... } ``` When `tlb_inval_submitted` is false (the common case for the "fences pending -> finish" path), it calls `xe_vma_userptr_do_inval(vm, uvma, true)`. That function now returns a `struct mmu_interval_notifier_finish *`, but the return value is **ignored** here. If `xe_vma_userptr_do_inval` returns a non-NULL finish (because it submitted TLB invalidation asynchronously and set `finish_inuse = true`, `tlb_inval_submitted = true`), then: 1. The TLB invalidation is submitted but never waited on 2. `finish_inuse` is set to true and never cleared 3. `tlb_inval_submitted` is set to true and never cleared **This appears to be a bug.** The deferred TLB path from within `invalidate_finish` has no mechanism to schedule another finish callback because the two-pass infrastructure only supports... two passes. The three-pass flow described in the commit message doesn't actually work as described. Let me re-examine more carefully... Actually, looking again at `xe_vma_userptr_do_inval`: ```c if (xe_vm_in_fault_mode(vm) && userptr->initial_bind) { if (!userptr->finish_inuse) { userptr->finish_inuse = true; userptr->tlb_inval_submitted = true; err = xe_vm_invalidate_vma_submit(vma, &userptr->inval_batch); XE_WARN_ON(err); return &userptr->finish; } err = xe_vm_invalidate_vma(vma); XE_WARN_ON(err); } if (is_deferred) userptr->finish_inuse = false; ``` When called from `xe_vma_userptr_invalidate_finish` (i.e., `is_deferred = true`), `finish_inuse` is still `true` (it was set in pass1). So the `!userptr->finish_inuse` check fails, and it falls through to the synchronous `xe_vm_invalidate_vma` path, then clears `finish_inuse`. So the three-pass flow **doesn't actually happen** for the "fences pending" case. The deferred TLB path only triggers when `signaled == true` in pass1 (all fences already done, so do_inval is called inline in pass1). So the actual flows are: 1. **Fences pending**: pass1 defers -> finish calls do_inval -> `finish_inuse` is true -> falls through to sync TLB -> returns NULL (ignored, but correctly) 2. **Fences done, finish_inuse == false**: pass1 calls do_inval inline -> do_inval sees `!finish_inuse` -> submits TLB async -> returns finish ptr -> pass1 returns this to notifier core -> finish calls `complete_tlb_inval` 3. **Fences done, finish_inuse == true**: pass1 calls do_inval inline -> falls through to sync TLB -> returns NULL This is actually correct! But flow #2 means the "two-pass" from the MMU notifier perspective is: pass1 (enable sw signaling + submit TLB) -> pass2 (wait for TLB). The dma_resv wait doesn't happen because fences are already signaled. My initial analysis was wrong about the bug. However, this is still confusing. The `is_deferred` parameter controls whether `finish_inuse` is cleared. In flow #2, `is_deferred = false` (called from pass1), so `finish_inuse` is NOT cleared at the bottom of `do_inval`. It remains true, and is cleared later in `xe_vma_userptr_complete_tlb_inval`. This is correct but tricky. **`xe_vma_userptr_force_invalidate` chain in patch 4:** ```c finish = xe_vma_userptr_invalidate_pass1(vm, uvma); if (finish) finish = xe_vma_userptr_do_inval(vm, uvma, true); if (finish) xe_vma_userptr_complete_tlb_inval(vm, uvma); ``` Note that `finish` here is the `static` variable from patch 2 (still a bug). But the logic is: if pass1 deferred, do the inval; if that deferred too, complete TLB. This correctly handles all three steps synchronously for the force-invalidate testing path. **Embedding `xe_tlb_inval_batch` in `xe_userptr`:** ```c struct xe_tlb_inval_batch inval_batch; ``` This embeds 4 `xe_tlb_inval_fence` structs (~400-500 bytes) in every `xe_userptr`. This is a significant memory overhead per-VMA. Consider whether this should be dynamically allocated when needed, or whether the cost is justified by avoiding allocation in the notifier callback path. **Summary of issues in patch 4:** 1. The `static` variable from patch 2 is still present (bug, should be fixed in patch 2) 2. The code flow is complex and the commit message's three-flow description is somewhat misleading about when each flow actually triggers 3. The return value of `xe_vma_userptr_do_inval` is ignored in `xe_vma_userptr_invalidate_finish` - this is actually correct behavior (it will always return NULL in that context because `finish_inuse` is already set), but deserves a comment 4. Per-VMA memory overhead from embedding `xe_tlb_inval_batch` --- **Summary of actionable items:** 1. **Bug (Patch 2):** Remove `static` from `finish` variable in `xe_vma_userptr_force_invalidate` 2. **Minor (Patch 1):** Consider adding `WARN_ON` if both `invalidate` and `invalidate_start` are set in ops during registration 3. **Minor (Patch 3):** Inconsistent local variable naming (`_batch` vs `batch`) 4. **Minor (Patch 3):** `xe_tlb_inval_types.h` including `xe_device_types.h` is heavy for a types header 5. **Documentation (Patch 4):** Add a comment in `xe_vma_userptr_invalidate_finish` explaining that `xe_vma_userptr_do_inval` always returns NULL when called from the finish path (because `finish_inuse` is already set) 6. **Design consideration (Patch 4):** Memory cost of embedding `xe_tlb_inval_batch` in every `xe_userptr` --- Generated by Claude Code Patch Reviewer