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:03:19 +1000 Message-ID: In-Reply-To: <20260531-feature-dmem-high-v5-1-1c6c532b26a9@gmail.com> References: <20260531-feature-dmem-high-v5-1-1c6c532b26a9@gmail.com> <20260531-feature-dmem-high-v5-1-1c6c532b26a9@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 #### kernel/cgroup/dmem.c =E2=80=94 `dmem_cgroup_state_evict_valuable()` **Issue 1 (medium): `limit_pool =3D=3D test_pool` path in try_high mode ski= ps emin protection** ```c if (limit_pool =3D=3D test_pool) { if (try_high) { ctest =3D &test_pool->cnt; used =3D 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 s= olely based on `used > high` without checking `emin`. In contrast, the cros= s-cgroup `try_high` path (added later in the function) explicitly calls `dm= em_cgroup_calculate_protection()` and checks `used > min`. This inconsisten= cy means that in the self-eviction case during Pass 1, BOs below `emin` pro= tection could be evicted. While `limit_pool =3D=3D test_pool` implies this = is the cgroup evicting its own BOs (so protection may be less relevant sinc= e 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 =3D 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 dur= ing Pass 1, a BO from a cgroup that is over its parent's `dmem.high` but pr= otected 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 =3D &test_pool->cnt; used =3D page_counter_read(ctest); if (try_high) { ... if (used <=3D READ_ONCE(ctest->high)) { ``` The `used =3D page_counter_read(ctest)` is hoisted before the `try_high` bl= ock. This read was previously placed after `dmem_cgroup_calculate_protectio= n()`. Moving it earlier means the value is read before protection calculati= on in the non-try_high path. Looking at the diff more carefully, the origin= al code had: ```c ctest =3D &test_pool->cnt; dmem_cgroup_calculate_protection(limit_pool, test_pool); used =3D page_counter_read(ctest); ``` The patch changes this to: ```c ctest =3D &test_pool->cnt; used =3D page_counter_read(ctest); if (try_high) { ... } dmem_cgroup_calculate_protection(limit_pool, test_pool); min =3D READ_ONCE(ctest->emin); ``` This reordering means `used` is now read before `dmem_cgroup_calculate_prot= ection()` in the normal path too. In practice, `page_counter_read()` just r= eads an atomic counter and `dmem_cgroup_calculate_protection()` only writes= `emin`/`elow`, so this reordering is harmless. But it's a subtle behaviora= l change that's not called out. #### kernel/cgroup/dmem.c =E2=80=94 New accessors and cgroupfs file The `set_resource_high()`, `get_resource_high()`, `reset_all_resource_limit= s()` changes, and the new cgroupfs entries all look correct and follow the = existing patterns exactly. `get_resource_high()` correctly returns `PAGE_CO= UNTER_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 =E2=80=94 when dmem= cgroups are disabled, all BOs are evictable. #### drivers/gpu/drm/ttm/ttm_bo.c =E2=80=94 3-pass eviction strategy **Issue 4 (low): Pass 1 blocking semantics could still cause latency issues= ** ```c evict_walk.walk.arg.trylock_only =3D !ticket; evict_walk.try_high =3D true; lret =3D 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-lim= it cgroups are actually penalized. However, this means Pass 1 could block w= aiting 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_evic= t_cb()`, which is called after the BO lock is acquired. So the blocking loc= k is acquired before knowing whether this BO is even a candidate for try_hi= gh eviction. This is a pre-existing pattern in the TTM LRU walk (the low/mi= n 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 =3D false; if (lret) goto out; /* Pass 2 */ evict_walk.walk.arg.trylock_only =3D true; lret =3D 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 fallbac= k. Since the try_high path in `dmem_cgroup_state_evict_valuable()` returns = before reaching the `*ret_hit_low =3D true` line, this is not a bug in prac= tice. But it relies on the invariant that try_high never touches hit_low = =E2=80=94 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 o= f 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 =E2=80=94 typically the section below `---` is = for changelog and notes, not a re-summary. The v4=E2=86=92v5 changelog is v= aluable 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`. T= his is typically expected for new cgroupfs control files. --- **Summary:** The patch is well-motivated and has clearly improved through t= he v1=E2=86=92v5 iterations. The main substantive concerns are (1) inconsis= tent `emin`/`elow` protection in the try_high paths and (2) missing documen= tation for the new cgroupfs control file. The TTM eviction refactor is clea= n and correct. I'd suggest addressing the protection consistency issues and= adding documentation before merging. --- Generated by Claude Code Patch Reviewer