public inbox for drm-ai-reviews@public-inbox.freedesktop.org
 help / color / mirror / Atom feed
From: Claude Code Review Bot <claude-review@example.com>
To: dri-devel-reviews@example.com
Subject: Claude review: mm/mmu_notifier: Allow two-pass struct mmu_interval_notifiers
Date: Tue, 03 Mar 2026 13:05:44 +1000	[thread overview]
Message-ID: <review-patch1-20260302163248.105454-2-thomas.hellstrom@linux.intel.com> (raw)
In-Reply-To: <20260302163248.105454-2-thomas.hellstrom@linux.intel.com>

Patch Review

This is the core infrastructure patch. The approach of using a lockless list (`llist`) to collect finish callbacks during the first-pass tree walk and then iterating them for the second pass is clean and avoids a redundant second interval tree traversal.

**Design observations:**

The documentation in `struct mmu_interval_notifier_finish` is good about recommending GFP_NOWAIT allocation and pools. However, the xe driver (in patch 2) embeds the struct instead, which is the superior approach for a single-notifier-at-a-time design.

**Concern: `invalidate_start` vs `invalidate` mutual exclusivity not enforced:**

The ops struct allows both `invalidate` and `invalidate_start` to be set simultaneously. The code checks `invalidate_start` first (lines 364, 404), so `invalidate` would never be called if both are present. This is fine in practice but a `WARN_ON(ops->invalidate && ops->invalidate_start)` during registration would be defensive.

**Correctness of error path in `mn_itree_invalidate`:**

```c
		if (!ret) {
			if (WARN_ON(mmu_notifier_range_blockable(range)))
				continue;
			err = -EAGAIN;
			break;
		}
```

When a non-blockable notifier returns false, the loop breaks, and then:
```c
	mn_itree_finish_pass(&finish_passes);
	if (err)
		mn_itree_inv_end(subscriptions);
```

This correctly calls the finish callbacks for any notifiers that already succeeded before the failure. Good.

**Minor: llist reversal for ordering:**

```c
	struct llist_node *first = llist_reverse_order(__llist_del_all(finish_passes));
```

Since `__llist_add` prepends, the list is in reverse iteration order. The reversal here restores original order, which is the right thing to do. However, the comment in the cover letter says this uses "lockless list for list management" - note that `__llist_add` (double-underscore) is actually *not* the lockless/atomic version, it's the non-atomic helper. This is fine since the list is local to each invocation, but the v2 changelog entry "Use lockless list for list management" is slightly misleading. The structure uses `llist_node`/`llist_head` types but doesn't use the atomic operations.

**Nit:** The v2 changelog entry says "Update documentation (GitHub Copilot:claude-sonnet-4.6)" - the Assisted-by tag uses "GitHub Copilot:claude-sonnet-4.6" which is an unusual attribution format.

---

---
Generated by Claude Code Patch Reviewer

  parent reply	other threads:[~2026-03-03  3:05 UTC|newest]

Thread overview: 21+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-03-02 16:32 [PATCH v2 0/4] Two-pass MMU interval notifiers Thomas Hellström
2026-03-02 16:32 ` [PATCH v2 1/4] mm/mmu_notifier: Allow two-pass struct mmu_interval_notifiers Thomas Hellström
2026-03-02 19:48   ` Matthew Brost
2026-03-02 21:12     ` Thomas Hellström
2026-03-03  3:05   ` Claude Code Review Bot [this message]
2026-03-02 16:32 ` [PATCH v2 2/4] drm/xe/userptr: Convert invalidation to two-pass MMU notifier Thomas Hellström
2026-03-02 18:57   ` Matthew Brost
2026-03-02 21:22     ` Thomas Hellström
2026-03-02 21:30       ` Matthew Brost
2026-03-03  3:05   ` Claude review: " Claude Code Review Bot
2026-03-02 16:32 ` [PATCH v2 3/4] drm/xe: Split TLB invalidation into submit and wait steps Thomas Hellström
2026-03-02 19:06   ` Matthew Brost
2026-03-02 21:29     ` Thomas Hellström
2026-03-02 21:31       ` Matthew Brost
2026-03-03  3:05   ` Claude review: " Claude Code Review Bot
2026-03-02 16:32 ` [PATCH v2 4/4] drm/xe/userptr: Defer Waiting for TLB invalidation to the second pass if possible Thomas Hellström
2026-03-02 19:14   ` Matthew Brost
2026-03-02 21:33     ` Thomas Hellström
2026-03-03  3:05   ` Claude review: " Claude Code Review Bot
2026-03-03  3:05 ` Claude review: Two-pass MMU interval notifiers Claude Code Review Bot
  -- strict thread matches above, loose matches on Subject: below --
2026-03-03 13:34 [PATCH v3 0/4] " Thomas Hellström
2026-03-03 13:34 ` [PATCH v3 1/4] mm/mmu_notifier: Allow two-pass struct mmu_interval_notifiers Thomas Hellström
2026-03-03 21:09   ` Claude review: " Claude Code Review Bot

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=review-patch1-20260302163248.105454-2-thomas.hellstrom@linux.intel.com \
    --to=claude-review@example.com \
    --cc=dri-devel-reviews@example.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox