From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: drm/ttm: Use common ancestor of evictor and evictee as limit pool Date: Sat, 14 Mar 2026 07:10:16 +1000 Message-ID: In-Reply-To: <20260313-dmemcg-aggressive-protect-v6-6-7c71cc1492db@gmx.de> References: <20260313-dmemcg-aggressive-protect-v6-0-7c71cc1492db@gmx.de> <20260313-dmemcg-aggressive-protect-v6-6-7c71cc1492db@gmx.de> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Patch Review This patch fixes protection calculation for sibling cgroups by using their = common ancestor as the limit pool root. Without this, recursive protection = propagation makes all children of a protected parent appear protected, prev= enting a protected sibling from evicting an unprotected sibling. **The self-eviction guard**: ```c if (!evict_walk->alloc_state->may_try_low && bo->resource->css =3D=3D evict_walk->alloc_state->charge_pool) return 0; ``` This prevents a protected cgroup from evicting its own buffers during the "= aggressive protected" eviction path. The pointer comparison works because `= charge_pool` was obtained from `get_current_dmemcs()` and `bo->resource->cs= s` is the pool charged during allocation. However, this is a **pointer comp= arison** =E2=80=94 if two different pool_state objects refer to the same cg= roup (e.g., obtained at different times), this could fail to match. Looking= at `dmem_cgroup_try_charge`, the pool is obtained via `get_cg_pool_unlocke= d` which caches pool states per cgroup, so the same cgroup should yield the= same pointer. This should be safe. **The ancestor lookup**: ```c if (!limit_pool) { ancestor =3D dmem_cgroup_get_common_ancestor(bo->resource->css, evict_walk->alloc_state->charge_pool); limit_pool =3D ancestor; } ``` When there's no explicit limit_pool (the aggressive protection path), the c= ommon ancestor is computed per-eviction-candidate. This is O(depth) per can= didate, which seems acceptable for typical cgroup hierarchies. The referenc= e is properly put after the `dmem_cgroup_state_evict_valuable` call. **Indentation**: The `&&` continuation at line 1852 uses tabs for alignment= rather than aligning under the `(`. This may or may not match local style;= the existing code in this file seems to use tab-based continuation which i= s kernel style. Overall this is a well-thought-out series. The main actionable items are: 1. The `@b: First pool` =E2=86=92 `@b: Second pool` typo in patch 2 2. The potential RCU concern around `cgroup_e_css()` usage in `dmem_cgroup_= get_common_ancestor()` in patch 2 3. The "pendant" =E2=86=92 "counterpart" wording in patch 1 --- Generated by Claude Code Patch Reviewer