Claude review: cgroup/dmem: implement dmem.high soft limit via prioritized eviction

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

Patch Review

#### kernel/cgroup/dmem.c — `dmem_cgroup_state_evict_valuable()`

**Issue 1 (medium): `limit_pool == test_pool` path in try_high mode skips emin protection**

```c
	if (limit_pool == test_pool) {
		if (try_high) {
			ctest = &test_pool->cnt;
			used = page_counter_read(ctest);
			return used > READ_ONCE(ctest->high);
		}
		return true;
	}
```

When a cgroup hits its own limit and `try_high` is set, this returns true solely based on `used > high` without checking `emin`. In contrast, the cross-cgroup `try_high` path (added later in the function) explicitly calls `dmem_cgroup_calculate_protection()` and checks `used > min`. This inconsistency means that in the self-eviction case during Pass 1, BOs below `emin` protection could be evicted. While `limit_pool == test_pool` implies this is the cgroup evicting its own BOs (so protection may be less relevant since the cgroup is over its own limits), it is still an inconsistency with the later code path and should at minimum be documented as intentional.

**Issue 2 (medium): try_high path does not respect elow**

The try_high cross-cgroup path checks `emin` but not `elow`:

```c
	if (try_high) {
		...
		dmem_cgroup_calculate_protection(limit_pool, test_pool);
		min = READ_ONCE(ctest->emin);

		return used > min;
	}
```

The normal (non-try_high) path checks both `emin` and `elow` (with `ret_hit_low` signaling). The try_high path ignores `elow` entirely. This means during Pass 1, a BO from a cgroup that is over its parent's `dmem.high` but protected by `dmem.low` could be evicted. If this is intentional (i.e., "high overrides low for the penalization pass"), it should be documented. If not, it's a correctness issue.

**Issue 3 (minor): `used` variable read twice unnecessarily**

```c
	ctest = &test_pool->cnt;
	used = page_counter_read(ctest);

	if (try_high) {
		...
		if (used <= READ_ONCE(ctest->high)) {
```

The `used = page_counter_read(ctest)` is hoisted before the `try_high` block. This read was previously placed after `dmem_cgroup_calculate_protection()`. Moving it earlier means the value is read before protection calculation in the non-try_high path. Looking at the diff more carefully, the original code had:

```c
	ctest = &test_pool->cnt;
	dmem_cgroup_calculate_protection(limit_pool, test_pool);
	used = page_counter_read(ctest);
```

The patch changes this to:

```c
	ctest = &test_pool->cnt;
	used = page_counter_read(ctest);

	if (try_high) { ... }

	dmem_cgroup_calculate_protection(limit_pool, test_pool);
	min = READ_ONCE(ctest->emin);
```

This reordering means `used` is now read before `dmem_cgroup_calculate_protection()` in the normal path too. In practice, `page_counter_read()` just reads an atomic counter and `dmem_cgroup_calculate_protection()` only writes `emin`/`elow`, so this reordering is harmless. But it's a subtle behavioral change that's not called out.

#### kernel/cgroup/dmem.c — New accessors and cgroupfs file

The `set_resource_high()`, `get_resource_high()`, `reset_all_resource_limits()` changes, and the new cgroupfs entries all look correct and follow the existing patterns exactly. `get_resource_high()` correctly returns `PAGE_COUNTER_MAX` for `NULL` pool, matching `get_resource_max()`.

The new cftype entry is placed between `low` and `max` in the files array, which is the natural ordering (min < low < high < max). Good.

#### include/linux/cgroup_dmem.h

The stub for `!CONFIG_CGROUP_DMEM` correctly adds the `try_high` parameter and ignores it (always returns `true`). This is correct — when dmem cgroups are disabled, all BOs are evictable.

#### drivers/gpu/drm/ttm/ttm_bo.c — 3-pass eviction strategy

**Issue 4 (low): Pass 1 blocking semantics could still cause latency issues**

```c
	evict_walk.walk.arg.trylock_only = !ticket;
	evict_walk.try_high = true;
	lret = ttm_lru_walk_for_evict(&evict_walk.walk, bdev, man, 1);
```

When a ticket is available, Pass 1 uses blocking locks. This is intentional (v4 changelog says "as requested by Maarten Lankhorst") to ensure over-limit cgroups are actually penalized. However, this means Pass 1 could block waiting for a BO that ultimately turns out not to be in an over-high cgroup. The `dmem_cgroup_state_evict_valuable()` check happens inside `ttm_bo_evict_cb()`, which is called after the BO lock is acquired. So the blocking lock is acquired before knowing whether this BO is even a candidate for try_high eviction. This is a pre-existing pattern in the TTM LRU walk (the low/min checks also happen after locking), so it's not a new issue introduced by this patch, but it's worth noting for the commit message.

**Issue 5 (low): hit_low not reset between Pass 1 and Pass 2**

```c
	evict_walk.try_high = false;
	if (lret)
		goto out;

	/* Pass 2 */
	evict_walk.walk.arg.trylock_only = true;
	lret = ttm_lru_walk_for_evict(&evict_walk.walk, bdev, man, 1);
	if (!lret && evict_walk.hit_low) {
```

`hit_low` is not reset between Pass 1 and Pass 2. If Pass 1 happened to set `hit_low` (which can't actually happen since the try_high path never sets `*ret_hit_low`), this would cause Pass 2 to immediately try the low fallback. Since the try_high path in `dmem_cgroup_state_evict_valuable()` returns before reaching the `*ret_hit_low = true` line, this is not a bug in practice. But it relies on the invariant that try_high never touches hit_low — fragile if the code evolves.

**Overall comment on the ttm_bo.c changes:** The 3-pass structure is clean and readable. The comments added are appropriate and helpful. The removal of several now-redundant comments from the original code is fine.

#### Commit message

The commit message is well-written and clearly explains the motivation and approach. The below-the-`---` section duplicates much of the above-the-`---` content, which is unusual — typically the section below `---` is for changelog and notes, not a re-summary. The v4→v5 changelog is valuable and clearly describes the fixes.

#### Missing: Documentation

There is no update to any documentation file. The `dmem` cgroup controller should have its `high` knob documented alongside `min`, `low`, and `max`. This is typically expected for new cgroupfs control files.

---

**Summary:** The patch is well-motivated and has clearly improved through the v1→v5 iterations. The main substantive concerns are (1) inconsistent `emin`/`elow` protection in the try_high paths and (2) missing documentation for the new cgroupfs control file. The TTM eviction refactor is clean and correct. I'd suggest addressing the protection consistency issues and adding documentation before merging.

---
Generated by Claude Code Patch Reviewer