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: drm/ttm: Hook up a cgroup-aware reclaim callback for the dmem controller
Date: Sat, 16 May 2026 14:04:00 +1000
Message-ID: <review-patch3-20260512082406.44470-4-thomas.hellstrom@linux.intel.com>
In-Reply-To: <20260512082406.44470-4-thomas.hellstrom@linux.intel.com>
References: <20260512082406.44470-1-thomas.hellstrom@linux.intel.com> <20260512082406.44470-4-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

**Verdict: Good. The TTM integration is solid.**

**NULL place handling in `ttm_bo_evict_cb`:**

```c
+	/*
+	 * evict_walk->place is NULL in cgroup drain mode.  Drivers'
+	 * eviction_valuable() callbacks must handle a NULL place, treating it
+	 * as "any placement": the TTM base implementation already does so via
+	 * ttm_resource_intersects().
+	 */
 	if (bo->pin_count || !bo->bdev->funcs->eviction_valuable(bo, evict_walk->place))
```

I verified that `ttm_resource_intersects()` returns `true` for NULL place, so the base `ttm_bo_eviction_valuable()` correctly treats NULL as "any placement." The xe driver's `xe_bo_eviction_valuable()` calls the base first, so it's also safe. The amdgpu driver is handled in patch 5.

However, the comment says "Drivers' eviction_valuable() callbacks **must** handle a NULL place" but there's no compile-time or runtime enforcement. Any out-of-tree or future driver that dereferences `place` without a NULL check would crash. A comment in the `eviction_valuable` callback documentation (`struct ttm_device_funcs`) would be valuable to prevent future regressions.

**`bo_size` capture:**

```c
+	s64 bo_size = bo->base.size;
```

This correctly captures the GEM object size before eviction may clear the resource. Note that `bo->base.size` is the GEM size, not the VRAM allocation size (which could differ for padded allocations). For progress tracking this is fine since the actual cgroup accounting uses `page_counter_read()`.

**`sleeping_lock` flag:**

```c
+	/**
+	 * @sleeping_lock: Use sleeping locks even with %NULL @ticket.
+	 * @trylock_only has precedence over this field.
+	 */
+	bool sleeping_lock;
```

The change in `ttm_bo_util.c` correctly allows the `ttm_lru_walk_ticketlock` path when `sleeping_lock` is true and no WW ticket is present:

```c
-		else if (!arg->ticket || arg->ctx->no_wait_gpu || arg->trylock_only)
+		else if ((!arg->ticket && !arg->sleeping_lock) || arg->ctx->no_wait_gpu ||
+			 arg->trylock_only)
```

This is correct: without this, the cgroup reclaim path (which has no WW ticket) would only trylock and skip contended BOs, potentially making no progress. The comment in the patch about "sooner than later be converted to full WW transactions" is noted.

**`ttm_resource_manager_set_dmem_region` defensive check:**

```c
+	if (!IS_ERR_OR_NULL(region)) {
+		man->cg = region;
+		dmem_cgroup_region_set_reclaim(region, ...);
+	}
```

The `IS_ERR_OR_NULL` check is defensive since callers already validate, but it prevents storing error pointers (the v3 bug). Reasonable.

---

---
Generated by Claude Code Patch Reviewer