Re: [PATCH v6 6/6] drm/ttm: Use common ancestor of evictor and evictee as limit pool

[Tvrtko Ursulin <tursulin@ursulin.net>]

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 <natalie.vock@gmx.de>
> ---
>   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?

> +
> +	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?

Regards,

Tvrtko

>   		return 0;
>   
>   	if (bo->pin_count || !bo->bdev->funcs->eviction_valuable(bo, evict_walk->place))
>