On 16/03/2026 09:05, Natalie Vock wrote:
On 3/13/26 15:11, Tvrtko Ursulin wrote:
On 13/03/2026 11:40, Natalie Vock wrote:
When checking whether to skip certain buffers because they're protected
by dmem.low, we're checking the effective protection of the evictee's
cgroup, but depending on how the evictor's cgroup relates to the
evictee's, the semantics of effective protection values change.
When testing against cgroups from different subtrees, page_counter's
recursive protection propagates memory protection afforded to a parent
down to the child cgroups, even if the children were not explicitly
protected. This prevents cgroups whose parents were afforded no
protection from stealing memory from cgroups whose parents were afforded
more protection, without users having to explicitly propagate this
protection.
However, if we always calculate protection from the root cgroup, this
breaks prioritization of sibling cgroups: If one cgroup was explicitly
protected and its siblings were not, the protected cgroup should get
higher priority, i.e. the protected cgroup should be able to steal from
unprotected siblings. This only works if we restrict the protection
calculation to the subtree shared by evictor and evictee.
Signed-off-by: Natalie Vock <[email protected]>
---
drivers/gpu/drm/ttm/ttm_bo.c | 43 +++++++++++++++++++++++++++++++++
+ ++++++---
1 file changed, 40 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
index 7300b91b77dd3..df4f4633a3a53 100644
--- a/drivers/gpu/drm/ttm/ttm_bo.c
+++ b/drivers/gpu/drm/ttm/ttm_bo.c
@@ -628,11 +628,48 @@ static s64 ttm_bo_evict_cb(struct ttm_lru_walk
*walk, struct ttm_buffer_object *
{
struct ttm_bo_evict_walk *evict_walk =
container_of(walk, typeof(*evict_walk), walk);
+ struct dmem_cgroup_pool_state *limit_pool, *ancestor = NULL;
+ bool evict_valuable;
s64 lret;
- if (!dmem_cgroup_state_evict_valuable(evict_walk->alloc_state-
>limit_pool,
- bo->resource->css, evict_walk->try_low,
- &evict_walk->hit_low))
+ /*
+ * If may_try_low is not set, then we're trying to evict
unprotected
+ * buffers in favor of a protected allocation for charge_pool.
Explicitly skip
+ * buffers belonging to the same cgroup here - that cgroup is
definitely protected,
+ * even though dmem_cgroup_state_evict_valuable would allow the
eviction because a
+ * cgroup is always allowed to evict from itself even if it is
protected.
+ */
+ if (!evict_walk->alloc_state->may_try_low &&
+ bo->resource->css == evict_walk->alloc_state->charge_pool)
+ return 0;
Hm.. should this hunk go into the previous patch?
Hm. Maybe, I can move it there in a v7.
Do you see it as logically belonging to 5/6, or not?
+
+ limit_pool = evict_walk->alloc_state->limit_pool;
+ /*
+ * If there is no explicit limit pool, find the root of the
shared subtree between
+ * evictor and evictee. This is important so that recursive
protection rules can
+ * apply properly: Recursive protection distributes cgroup
protection afforded
+ * to a parent cgroup but not used explicitly by a child cgroup
between all child
+ * cgroups (see docs of effective_protection in mm/
page_counter.c). However, when
+ * direct siblings compete for memory, siblings that were
explicitly protected
+ * should get prioritized over siblings that weren't. This only
happens correctly
+ * when the root of the shared subtree is passed to
+ * dmem_cgroup_state_evict_valuable. Otherwise, the effective-
protection
+ * calculation cannot distinguish direct siblings from unrelated
subtrees and the
+ * calculated protection ends up wrong.
+ */
+ if (!limit_pool) {
+ ancestor = dmem_cgroup_get_common_ancestor(bo->resource->css,
+ evict_walk->alloc_state->charge_pool);
+ limit_pool = ancestor;
+ }
+
+ evict_valuable = dmem_cgroup_state_evict_valuable(limit_pool,
bo- >resource->css,
+ evict_walk->try_low,
+ &evict_walk->hit_low);
+ if (ancestor)
+ dmem_cgroup_pool_state_put(ancestor);
+
+ if (!evict_valuable)
This part is probably better reviewed by someone more familiar with
the dmem controller. One question I have though is whether this patch
is independent from the rest of the series or it really makes sense
for it to be last?
It depends on patch 2/6 (cgroup,cgroup/dmem: Add
(dmem_)cgroup_common_ancestor helper). I could potentially reorder it,
though then there's likely going to be quite a few rebase conflicts.
I will not insist on it, ordering is probably not that critical in this
case. It just looked to me the added condition is an independent fix
from the charge accounting and unprotected eviction changes. Up to you
for what I am concerned at least.
Regards,
Tvrtko
