On 25/02/2026 09:49, Natalie Vock wrote:
On 2/24/26 17:40, Tvrtko Ursulin wrote:On 10/11/2025 12:37, Natalie Vock wrote:When the cgroup's memory usage is below the low/min limit and allocation fails, try evicting some unprotected buffers to make space. Otherwise, application buffers may be forced to go into GTT even though usage is below the corresponding low/min limit, if other applications filled VRAM with their allocations first. Signed-off-by: Natalie Vock <[email protected]> ---drivers/gpu/drm/ttm/ttm_bo.c | 75 +++++++++++++++++++++++++++ + ++++++----drivers/gpu/drm/ttm/ttm_resource.c | 48 +++++++++++++++++------- include/drm/ttm/ttm_resource.h | 6 ++- 3 files changed, 108 insertions(+), 21 deletions(-) diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c index 829d994798835..bd467c965e1bc 100644 --- a/drivers/gpu/drm/ttm/ttm_bo.c +++ b/drivers/gpu/drm/ttm/ttm_bo.c@@ -490,8 +490,12 @@ int ttm_bo_evict_first(struct ttm_device *bdev, struct ttm_resource_manager *man} struct ttm_bo_alloc_state { + /** @charge_pool: The memory pool the resource is charged to */ + struct dmem_cgroup_pool_state *charge_pool; /** @limit_pool: Which pool limit we should test against */ struct dmem_cgroup_pool_state *limit_pool;+ /** @only_evict_unprotected: If eviction should be restricted to unprotected BOs */+ bool only_evict_unprotected; }; /**@@ -546,7 +550,7 @@ static s64 ttm_bo_evict_cb(struct ttm_lru_walk *walk, struct ttm_buffer_object *evict_walk->evicted++; if (evict_walk->res)lret = ttm_resource_alloc(evict_walk->evictor, evict_walk- >place,- evict_walk->res, NULL);+ evict_walk->res, evict_walk->alloc_state- >charge_pool);if (lret == 0) return 1; out:@@ -589,7 +593,7 @@ static int ttm_bo_evict_alloc(struct ttm_device *bdev,lret = ttm_lru_walk_for_evict(&evict_walk.walk, bdev, man, 1); /* One more attempt if we hit low limit? */ - if (!lret && evict_walk.hit_low) {+ if (!lret && evict_walk.hit_low && !state- >only_evict_unprotected) {What is unprotected synonymous with? No low watermark set? Should dmem_cgroup_state_evict_valuable() even set *hit_low = true for if low is not set to begin with?In terms of cgroup usage, a cgroup (and by extension, its BOs) is protected as long as its usage stays under the low watermark (if not set, that watermark is zero and any BO is trivially unprotected). If the usage exceeds the low watermark, the cgroup/its BOs become unprotected and can be evicted (more easily), until the usage goes below the watermark again.
Got it thanks, so either no low set, or usage above low. Makes sense.
With only_evict_unprotected, what we're trying to do is evict buffers from any cgroup that currently exceeds its low (or min) watermark, but leave alone cgroups that are within their limits. I've elaborated on the rationale more in the cover letter, but essentially, this is supposed to make TTM honor the low/min protection better for cgroups that are allocating and currently within their low/min watermark, by allowing them to push out BOs from cgroups that exceed their respective watermarks.
Yep, I got that part. Just that I will need a second pass to fully grasp the extended logic. Problem being more booleans and passes make things more complex. That is why I made this side question on whether it even makes sense for dmem_cgroup_state_evict_valuable() to set hit_low if the low is not even set. Assuming I got it right it can happen:
if (!ignore_low) {
low = READ_ONCE(ctest->elow);
if (used > low)
return true;
*ret_hit_low = true;
return false;
}
So I was wondering what would be the effect of making that like this:
if (!ignore_low) {
low = READ_ONCE(ctest->elow);
if (used > low)
return true;
+ if (low)
+ *ret_hit_low = true;
return false;
}
Could that somehow simplify the logic, maybe allow for not having to add
the additional condition above. Possibly not, it seems more complex than
that. But I am just thinking out loud at this point. Again, I need to
make a second reading pass.
I'll add some comments to the only_evict_unprotected docs to explain what "unprotected" means here.evict_walk.try_low = true; lret = ttm_lru_walk_for_evict(&evict_walk.walk, bdev, man, 1); }@@ -610,7 +614,8 @@ static int ttm_bo_evict_alloc(struct ttm_device *bdev,} while (!lret && evict_walk.evicted); /* We hit the low limit? Try once more */ - if (!lret && evict_walk.hit_low && !evict_walk.try_low) { + if (!lret && evict_walk.hit_low && !evict_walk.try_low && + !state->only_evict_unprotected) { evict_walk.try_low = true; goto retry; }@@ -719,20 +724,72 @@ static int ttm_bo_alloc_at_place(struct ttm_buffer_object *bo,struct ttm_resource **res, struct ttm_bo_alloc_state *alloc_state) { - bool may_evict; + bool may_evict, below_low = false; int ret; may_evict = (force_space && place->mem_type != TTM_PL_SYSTEM); + ret = ttm_resource_try_charge(bo, place, &alloc_state->charge_pool, + force_space ? &alloc_state->limit_pool : NULL); + if (ret) { + /* + * -EAGAIN means the charge failed, which we treat like an+ * allocation failure. Therefore, return an error code indicating + * the allocation failed - either -EBUSY if the allocation should + * be retried with eviction, or -ENOSPC if there should be no second+ * attempt. + */ + if (ret == -EAGAIN) + ret = may_evict ? -EBUSY : -ENOSPC; + return ret; + } - ret = ttm_resource_alloc(bo, place, res, - force_space ? &alloc_state->limit_pool : NULL); + /* + * cgroup protection plays a special role in eviction.+ * Conceptually, protection of memory via the dmem cgroup controller+ * entitles the protected cgroup to use a certain amount of memory. + * There are two types of protection - the 'low' limit is a+ * "best-effort" protection, whereas the 'min' limit provides a hard+ * guarantee that memory within the cgroup's allowance will not be + * evicted under any circumstance. + *+ * To faithfully model this concept in TTM, we also need to take cgroup+ * protection into account when allocating. When allocation in one+ * place fails, TTM will default to trying other places first before+ * evicting. + * If the allocation is covered by dmem cgroup protection, however,+ * this prevents the allocation from using the memory it is "entitled" + * to. To make sure unprotected allocations cannot push new protected+ * allocations out of places they are "entitled" to use, we should + * evict buffers not covered by any cgroup protection, if this + * allocation is covered by cgroup protection. + *+ * Buffers covered by 'min' protection are a special case - the 'min' + * limit is a stronger guarantee than 'low', and thus buffers protected+ * by 'low' but not 'min' should also be considered for eviction. + * Buffers protected by 'min' will never be considered for eviction + * anyway, so the regular eviction path should be triggered here. + * Buffers protected by 'low' but not 'min' will take a special+ * eviction path that only evicts buffers covered by neither 'low' or+ * 'min' protections. + */ + may_evict |= dmem_cgroup_below_min(NULL, alloc_state->charge_pool); + below_low = dmem_cgroup_below_low(NULL, alloc_state->charge_pool); + alloc_state->only_evict_unprotected = !may_evict && below_low; + + ret = ttm_resource_alloc(bo, place, res, alloc_state->charge_pool); if (ret) { - if ((ret == -ENOSPC || ret == -EAGAIN) && may_evict) + if ((ret == -ENOSPC || ret == -EAGAIN) && + (may_evict || below_low)) ret = -EBUSY; return ret; } + /*+ * Ownership of charge_pool has been transferred to the TTM resource,+ * don't make the caller think we still hold a reference to it. + */ + alloc_state->charge_pool = NULL; return 0; }@@ -787,6 +844,7 @@ static int ttm_bo_alloc_resource(struct ttm_buffer_object *bo,res, &alloc_state); if (ret == -ENOSPC) { + dmem_cgroup_pool_state_put(alloc_state.charge_pool); dmem_cgroup_pool_state_put(alloc_state.limit_pool); continue; } else if (ret == -EBUSY) {@@ -796,11 +854,14 @@ static int ttm_bo_alloc_resource(struct ttm_buffer_object *bo,dmem_cgroup_pool_state_put(alloc_state.limit_pool); if (ret) { + dmem_cgroup_pool_state_put( + alloc_state.charge_pool);Funky line break.Will fix.if (ret != -ENOSPC && ret != -EBUSY) return ret; continue; } } else if (ret) { + dmem_cgroup_pool_state_put(alloc_state.charge_pool); dmem_cgroup_pool_state_put(alloc_state.limit_pool); return ret; }diff --git a/drivers/gpu/drm/ttm/ttm_resource.c b/drivers/gpu/drm/ ttm/ ttm_resource.cindex e2c82ad07eb44..fcfa8b51b0337 100644 --- a/drivers/gpu/drm/ttm/ttm_resource.c +++ b/drivers/gpu/drm/ttm/ttm_resource.c@@ -372,30 +372,52 @@ void ttm_resource_fini(struct ttm_resource_manager *man,} EXPORT_SYMBOL(ttm_resource_fini); +/** + * ttm_resource_try_charge - charge a resource manager's cgroup pool + * @bo: buffer for which an allocation should be charged + * @place: where the allocation is attempted to be placed + * @ret_pool: on charge success, the pool that was charged+ * @ret_limit_pool: on charge failure, the pool responsible for the failure+ *+ * Should be used to charge cgroups before attempting resource allocation.+ * When charging succeeds, the value of ret_pool should be passed to + * ttm_resource_alloc. + * + * Returns: 0 on charge success, negative errno on failure. + */ +int ttm_resource_try_charge(struct ttm_buffer_object *bo, + const struct ttm_place *place, + struct dmem_cgroup_pool_state **ret_pool, + struct dmem_cgroup_pool_state **ret_limit_pool) +{ + struct ttm_resource_manager *man = + ttm_manager_type(bo->bdev, place->mem_type); + + if (!man->cg) { + *ret_pool = NULL; + if (ret_limit_pool) + *ret_limit_pool = NULL; + return 0; + } + + return dmem_cgroup_try_charge(man->cg, bo->base.size, ret_pool, + ret_limit_pool); +} + int ttm_resource_alloc(struct ttm_buffer_object *bo, const struct ttm_place *place, struct ttm_resource **res_ptr, - struct dmem_cgroup_pool_state **ret_limit_pool) + struct dmem_cgroup_pool_state *charge_pool) { struct ttm_resource_manager *man = ttm_manager_type(bo->bdev, place->mem_type); - struct dmem_cgroup_pool_state *pool = NULL; int ret; - if (man->cg) {- ret = dmem_cgroup_try_charge(man->cg, bo->base.size, &pool, ret_limit_pool);- if (ret) - return ret; - } - ret = man->func->alloc(man, bo, place, res_ptr); - if (ret) { - if (pool) - dmem_cgroup_uncharge(pool, bo->base.size); + if (ret) return ret; - } - (*res_ptr)->css = pool; + (*res_ptr)->css = charge_pool;Is it possible to somehow split this patch into two? I mean first a patch which changes the prototype of ttm_resource_alloc(), adjusting the callers, set out new rules for owning the charge pool, etc, then the patch which only adds the cgroup smarts to ttm_bo_alloc_at_place(). If that could be made without creating any functional difference to the eviction alone I think it could make it easier to review.Will try.
Only if it sounds plausible that it can be sensibly done. Otherwise don't spend too much time if you think it makes no sense. I'll wait for the verdict, or for v4 to appear and then have another go of making sense of the existing vs new eviction logic.
Regards, Tvrtko
Thanks, NatalieRegards, Tvrtkospin_lock(&bo->bdev->lru_lock); ttm_resource_add_bulk_move(*res_ptr, bo);diff --git a/include/drm/ttm/ttm_resource.h b/include/drm/ttm/ ttm_resource.hindex e52bba15012f7..3aef7efdd7cfb 100644 --- a/include/drm/ttm/ttm_resource.h +++ b/include/drm/ttm/ttm_resource.h@@ -442,10 +442,14 @@ void ttm_resource_init(struct ttm_buffer_object *bo,void ttm_resource_fini(struct ttm_resource_manager *man, struct ttm_resource *res); +int ttm_resource_try_charge(struct ttm_buffer_object *bo, + const struct ttm_place *place, + struct dmem_cgroup_pool_state **ret_pool, + struct dmem_cgroup_pool_state **ret_limit_pool); int ttm_resource_alloc(struct ttm_buffer_object *bo, const struct ttm_place *place, struct ttm_resource **res, - struct dmem_cgroup_pool_state **ret_limit_pool); + struct dmem_cgroup_pool_state *charge_pool);void ttm_resource_free(struct ttm_buffer_object *bo, struct ttm_resource **res);bool ttm_resource_intersects(struct ttm_device *bdev, struct ttm_resource *res,
