From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: cgroup/dmem: implement dmem.high soft limit via prioritized eviction Date: Thu, 04 Jun 2026 15:52:23 +1000 Message-ID: In-Reply-To: <20260530-feature-dmem-high-v4-1-ee7c6ec1c8da@gmail.com> References: <20260530-feature-dmem-high-v4-1-ee7c6ec1c8da@gmail.com> <20260530-feature-dmem-high-v4-1-ee7c6ec1c8da@gmail.com> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Patch Review #### Bug: Pass 2 uses blocking lock instead of trylock In `ttm_bo_evict_alloc()`, Pass 1 sets `trylock_only =3D false` for the blo= cking high-priority pass, but this is never restored before Pass 2: ```c + evict_walk.walk.arg.trylock_only =3D false; + evict_walk.try_high =3D true; lret =3D ttm_lru_walk_for_evict(&evict_walk.walk, bdev, man, 1); + evict_walk.try_high =3D false; + if (lret) + goto out; + + /* + * Pass 2 (trylock): Evict BOs above the effective low watermark. + */ lret =3D ttm_lru_walk_for_evict(&evict_walk.walk, bdev, man, 1); ``` The comment says "Pass 2 (trylock)" but `trylock_only` is still `false` fro= m Pass 1. The original code had `trylock_only =3D true` here. This changes = the locking behavior for the standard eviction pass =E2=80=94 it will now t= ake blocking locks on BOs that it shouldn't, potentially causing excessive = blocking or deadlocks. A `evict_walk.walk.arg.trylock_only =3D true;` is ne= eded before Pass 2. #### Bug: try_high path skips limit_pool/test_pool hierarchy check The existing `dmem_cgroup_state_evict_valuable()` verifies that `test_pool`= is a descendant of `limit_pool` before allowing eviction: ```c for (pool =3D test_pool; pool && limit_pool !=3D pool; pool =3D pool_paren= t(pool)) {} if (!pool) return false; ``` The new `try_high` path bypasses this entirely. When `limit_pool !=3D test_= pool`, the code goes straight into the ancestor high-limit walk without ver= ifying the hierarchy relationship: ```c + if (try_high) { + if (limit_pool =3D=3D test_pool) + return used > READ_ONCE(ctest->high); + + { + struct page_counter *c; + if (used <=3D READ_ONCE(ctest->high)) { + for (c =3D ctest->parent; c; c =3D c->parent) { + if (page_counter_read(c) > + READ_ONCE(c->high)) + break; + } + if (!c) + return false; + } + } ``` This means if cgroup A hits its `max` limit and triggers eviction, a BO fro= m completely unrelated cgroup B can be evicted if B (or any of B's ancestor= s) exceeds its high limit. This is unfair =E2=80=94 A's over-allocation sho= uldn't cause eviction from an unrelated cgroup tree. Furthermore, when the hierarchy isn't verified, the subsequent call to `dme= m_cgroup_calculate_protection(limit_pool, test_pool)` iterates `limit_pool`= 's descendants looking for `test_pool`, won't find it, and returns without = updating `test_pool->cnt.emin`. The `used > min` check then reads a stale `= emin` value. If the intent is to only evict over-high descendants of the limit-hitting c= group, the hierarchy check from the non-try_high path should be included. I= f the intent is truly global (evict any over-high BO regardless of hierarch= y), that's a design decision that should be explicitly documented, and the = `dmem_cgroup_calculate_protection` call needs to use the correct root for t= est_pool. #### Style: Bare block for variable declaration ```c + { + struct page_counter *c; + ... + } ``` This nested scope block is unusual for kernel style. The variable `c` could= be declared at the top of the `if (try_high)` block or at function scope a= longside the existing local variables. #### Commit message typo The commit message says "above-elow" in two places =E2=80=94 should be "abo= ve-low": > Pass 2 falls back to the standard above-elow trylock eviction. #### Minor: cgroupfs plumbing looks correct The `set_resource_high()`, `get_resource_high()`, `reset_all_resource_limit= s()` addition, and the `cftype` file registration all follow the establishe= d pattern for `min`/`low`/`max` correctly. The `get_resource_high(NULL)` re= turning `PAGE_COUNTER_MAX` (matching `get_resource_max()` behavior) is the = right default. #### Missing: No documentation update The kernel typically documents cgroup control files in `Documentation/admin= -guide/cgroup-v2.rst`. A new user-facing control file like `dmem.high` shou= ld have corresponding documentation describing its semantics, default value= , and interaction with `dmem.max`, `dmem.low`, and `dmem.min`. --- Generated by Claude Code Patch Reviewer