Claude review: drm/xe/userptr: Defer Waiting for TLB invalidation to the second pass if possible

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

Patch Review

This is the most complex patch, adding a *third* effective pass: pass1 defers fence wait, finish-pass does fence wait + submits TLB invalidation, then another finish-pass waits for TLB invalidation. The cover letter documents 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)
```

**Concern — three-pass through a two-pass API:**

The mmu_notifier infrastructure in patch 1 is designed for two passes. This patch achieves a third pass by having `xe_vma_userptr_do_inval` (called from `invalidate_finish`) return *another* `mmu_interval_notifier_finish*`, effectively re-queuing the notifier for another finish pass. However, looking at the mm core code in `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);
}
```

This drains the list and calls `invalidate_finish` for each entry. There's **no mechanism** for `invalidate_finish` to re-queue itself for yet another pass — the list is already drained when finish runs. Looking more carefully at `xe_vma_userptr_do_inval` in this patch:

```c
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;
}
```

This returns `&userptr->finish` from within `xe_vma_userptr_do_inval`, which was called from `xe_vma_userptr_invalidate_finish`. But `invalidate_finish` in the mm core doesn't look at any return value — it's `void`:

```c
void (*invalidate_finish)(struct mmu_interval_notifier_finish *finish);
```

So the return value from `xe_vma_userptr_do_inval` in the `invalidate_finish` path goes... where? Let's trace the call:

```c
static void xe_vma_userptr_invalidate_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);  // return value IGNORED
    up_write(&vm->svm.gpusvm.notifier_lock);
}
```

The return value of `xe_vma_userptr_do_inval` is **ignored** in `invalidate_finish`. So when `xe_vma_userptr_do_inval` is called with `is_deferred=true` and `!finish_inuse`, it sets `finish_inuse=true`, `tlb_inval_submitted=true`, submits the TLB invalidation, and returns `&userptr->finish`. But nobody picks up this return value to schedule another finish pass.

**This means the TLB invalidation batch submitted in this path is never waited on.** The `xe_vma_userptr_complete_tlb_inval` function would need to be called, but it's only called when `tlb_inval_submitted` is true at the *entry* of `xe_vma_userptr_invalidate_finish`. Since the finish callback is only invoked once per `__llist_add`, this deferred TLB path appears to leak — `finish_inuse` remains true, `tlb_inval_submitted` remains true, and the TLB batch is never waited on. The next notifier call would see `finish_inuse=true` and fall back to the synchronous path, which works, but `tlb_inval_submitted` being stale would cause `xe_vma_userptr_invalidate_finish` to call `xe_vma_userptr_complete_tlb_inval` on a stale/reused batch.

Wait — let me re-read more carefully. The `is_deferred=true` path enters with `finish_inuse` already set (the assert checks it):
```c
if (is_deferred)
    xe_assert(vm->xe, userptr->finish_inuse && !userptr->tlb_inval_submitted);
```

So when `xe_vma_userptr_do_inval` is called with `is_deferred=true`, `finish_inuse` is already true. Then the `if (!userptr->finish_inuse)` check is false, so it falls through to the synchronous `xe_vm_invalidate_vma()` path. So actually the deferred TLB submit via `do_inval` **only happens in the non-deferred `is_deferred=false` path** where `finish_inuse` is initially false. Let me re-trace:

In `xe_vma_userptr_invalidate_pass1`, when `signaled || finish_inuse`:
```c
if (signaled || userptr->finish_inuse)
    return xe_vma_userptr_do_inval(vm, uvma, false);  // is_deferred=false
```

Here `is_deferred=false`. If `!finish_inuse` (i.e., we entered because `signaled` was true), then `xe_vma_userptr_do_inval` may return `&userptr->finish` after setting `finish_inuse=true` and `tlb_inval_submitted=true`. This return value goes back to `xe_vma_userptr_invalidate_pass1`, then to `xe_vma_userptr_invalidate_start`, which sets `*p_finish` to it. This **does** schedule the finish pass in the mm core via `__llist_add`. Then `xe_vma_userptr_invalidate_finish` is called, sees `tlb_inval_submitted=true`, and calls `xe_vma_userptr_complete_tlb_inval`. This path is **correct**.

The other case: when `finish_inuse` is already true (concurrent invalidation), `xe_vma_userptr_do_inval(vm, uvma, false)` with `finish_inuse=true` falls through to sync `xe_vm_invalidate_vma` and returns NULL. Correct.

And the deferred path from pass1: when `!signaled && !finish_inuse`, pass1 returns `&userptr->finish` directly, finish is called, and `do_inval` with `is_deferred=true` does the sync path (since `finish_inuse` is already true). So the deferred TLB only happens in the "fences already signaled but TLB flush needed" case. That makes sense — if fences are done, you can immediately submit TLB flush and defer the wait.

After re-analysis: **The logic is correct but extremely convoluted.** The three interleaved flows with the `finish_inuse` and `tlb_inval_submitted` booleans create a complex state machine that's hard to reason about. The assertions help, but this would benefit from a state diagram in a comment.

**Per-userptr struct bloat:**

Patch 4 embeds `struct xe_tlb_inval_batch` (~450 bytes) into every `struct xe_userptr`. This is added to the already-present `struct mmu_interval_notifier_finish` (from patch 2). For systems with many userptr VMAs, this is significant memory overhead for a feature that only activates in fault mode with multi-GPU. Consider whether the batch could be dynamically allocated (with fallback to sync) instead of embedded.

**`xe_vma_userptr_force_invalidate` 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);
```

This still uses the `static` variable from patch 2 (which is a bug as noted above). Also, the second `if (finish)` can never be true because `do_inval` with `is_deferred=true` always falls through to the sync path (since `finish_inuse` is already true when `is_deferred=true`). The third line is dead code in the current implementation. The `static` bug and this dead code suggest the force_invalidate path hasn't been fully thought through.

Overall: **Logic is correct after careful analysis, but the state machine is very complex. Bug inherited from patch 2 (static variable). Per-userptr memory overhead is significant. Dead code in force_invalidate.**

---

## Summary of Issues

| Severity | Patch | Issue |
|----------|-------|-------|
| Bug | 2/4 | `static struct mmu_interval_notifier_finish *finish` in `xe_vma_userptr_force_invalidate` — should be a local variable |
| Medium | 4/4 | ~450 bytes of `xe_tlb_inval_batch` embedded per-userptr for a feature only used in fault mode multi-GPU |
| Medium | 4/4 | Three-pass state machine through a two-pass API is hard to follow; would benefit from a state diagram comment |
| Low | 3/4 | `tile_invalidated` is now set before TLB flush completes — semantics should be documented |
| Low | 3/4 | `xe_tlb_inval_types.h` pulls in heavyweight `xe_device_types.h` for a constant |
| Nit | 1/4 | Duplicated dispatch logic between `mn_itree_release` and `mn_itree_invalidate` could use a helper |
| Nit | 4/4 | Third `if (finish)` in `xe_vma_userptr_force_invalidate` is dead code |

---
Generated by Claude Code Patch Reviewer