On 25/02/2026 12:10, Natalie Vock wrote:
Move all code for attempting allocation for a specific place to
ttm_bo_alloc_place. With subsequent patches, this logic is going to get
more complicated, so it helps readability to have this separate.
ttm_bo_alloc_at_place takes a pointer to a struct ttm_bo_alloc_state.
This struct holds various state produced by the allocation (e.g. cgroup
resource associated with the allocation) that the caller needs to keep
track of (and potentially dispose of). This is just the limiting cgroup
pool for now, but future patches will add more state needing to be tracked.
ttm_bo_alloc_at_place also communicates via return codes if eviction
using ttm_bo_evict_alloc should be attempted. This is preparation for
attempting eviction in more cases than just force_space being set.
No functional change intended.
Signed-off-by: Natalie Vock <[email protected]>
---
drivers/gpu/drm/ttm/ttm_bo.c | 109 +++++++++++++++++++++++++++++++++----------
1 file changed, 84 insertions(+), 25 deletions(-)
diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
index acb9197db8798..48dbaaa46824c 100644
--- a/drivers/gpu/drm/ttm/ttm_bo.c
+++ b/drivers/gpu/drm/ttm/ttm_bo.c
@@ -489,6 +489,11 @@ int ttm_bo_evict_first(struct ttm_device *bdev, struct
ttm_resource_manager *man
return ret;
}
+struct ttm_bo_alloc_state {
+ /** @limit_pool: Which pool limit we should test against */
+ struct dmem_cgroup_pool_state *limit_pool;
+};
+
/**
* struct ttm_bo_evict_walk - Parameters for the evict walk.
*/
@@ -504,12 +509,13 @@ struct ttm_bo_evict_walk {
/** @evicted: Number of successful evictions. */
unsigned long evicted;
- /** @limit_pool: Which pool limit we should test against */
- struct dmem_cgroup_pool_state *limit_pool;
/** @try_low: Whether we should attempt to evict BO's with low
watermark threshold */
bool try_low;
/** @hit_low: If we cannot evict a bo when @try_low is false (first
pass) */
bool hit_low;
+
+ /** @alloc_state: State associated with the allocation attempt. */
+ struct ttm_bo_alloc_state *alloc_state;
};
static s64 ttm_bo_evict_cb(struct ttm_lru_walk *walk, struct ttm_buffer_object *bo)
@@ -518,8 +524,9 @@ static s64 ttm_bo_evict_cb(struct ttm_lru_walk *walk,
struct ttm_buffer_object *
container_of(walk, typeof(*evict_walk), walk);
s64 lret;
- if (!dmem_cgroup_state_evict_valuable(evict_walk->limit_pool, bo->resource->css,
- evict_walk->try_low,
&evict_walk->hit_low))
+ if
(!dmem_cgroup_state_evict_valuable(evict_walk->alloc_state->limit_pool,
+ bo->resource->css,
evict_walk->try_low,
+ &evict_walk->hit_low))
return 0;
if (bo->pin_count || !bo->bdev->funcs->eviction_valuable(bo, evict_walk->place))
@@ -561,7 +568,7 @@ static int ttm_bo_evict_alloc(struct ttm_device *bdev,
struct ttm_operation_ctx *ctx,
struct ww_acquire_ctx *ticket,
struct ttm_resource **res,
- struct dmem_cgroup_pool_state *limit_pool)
+ struct ttm_bo_alloc_state *state)
{
struct ttm_bo_evict_walk evict_walk = {
.walk = {
@@ -574,7 +581,7 @@ static int ttm_bo_evict_alloc(struct ttm_device *bdev,
.place = place,
.evictor = evictor,
.res = res,
- .limit_pool = limit_pool,
+ .alloc_state = state,
};
s64 lret;
@@ -689,6 +696,58 @@ static int ttm_bo_add_pipelined_eviction_fences(struct ttm_buffer_object *bo,
return dma_resv_reserve_fences(bo->base.resv, 1);
}
+
+/**
+ * ttm_bo_alloc_at_place - Attempt allocating a BO's backing store in a place
+ *
+ * @bo: The buffer to allocate the backing store of
+ * @place: The place to attempt allocation in
+ * @ctx: ttm_operation_ctx associated with this allocation
+ * @force_space: If we should evict buffers to force space
+ * @res: On allocation success, the resulting struct ttm_resource.
+ * @alloc_state: Object holding allocation state such as charged cgroups.
+ *
+ * Returns:
+ * -EBUSY: No space available, but allocation should be retried with
ttm_bo_evict_alloc.
+ * -ENOSPC: No space available, allocation should not be retried.
+ * -ERESTARTSYS: An interruptible sleep was interrupted by a signal.
+ *
+ */
+static int ttm_bo_alloc_at_place(struct ttm_buffer_object *bo,
+ const struct ttm_place *place,
+ struct ttm_operation_ctx *ctx,
+ bool force_space,
+ struct ttm_resource **res,
+ struct ttm_bo_alloc_state *alloc_state)
+{
+ bool may_evict;
+ int ret;
+
+ may_evict = (force_space && place->mem_type != TTM_PL_SYSTEM);
+
+ ret = ttm_resource_alloc(bo, place, res,
+ 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)
+ return may_evict ? -EBUSY : -ENOSPC;
+
+ if (ret == -ENOSPC && may_evict)
+ return -EBUSY;
+
+ return ret;
+ }
+
+ return 0;
+}
+
/**
* ttm_bo_alloc_resource - Allocate backing store for a BO
*
@@ -714,7 +773,9 @@ static int ttm_bo_alloc_resource(struct ttm_buffer_object
*bo,
bool force_space,
struct ttm_resource **res)
{
+ struct ttm_bo_alloc_state alloc_state = {0};
= {};
struct ttm_device *bdev = bo->bdev;
+ struct ttm_resource_manager *man;
I don't mind if you pull the above two out of the loop too much, but I
have to re-point it out since I am sure you know the principle of not
making changes which are not strictly needed, especially if they are not
a clear win on readability or something.
struct ww_acquire_ctx *ticket;
int i, ret;
@@ -725,9 +786,6 @@ static int ttm_bo_alloc_resource(struct ttm_buffer_object *bo,
for (i = 0; i < placement->num_placement; ++i) {
const struct ttm_place *place = &placement->placement[i];
- struct dmem_cgroup_pool_state *limit_pool = NULL;
- struct ttm_resource_manager *man;
- bool may_evict;
man = ttm_manager_type(bdev, place->mem_type);
if (!man || !ttm_resource_manager_used(man))
@@ -737,25 +795,26 @@ static int ttm_bo_alloc_resource(struct ttm_buffer_object
*bo,
TTM_PL_FLAG_FALLBACK))
continue;
- may_evict = (force_space && place->mem_type != TTM_PL_SYSTEM);
- ret = ttm_resource_alloc(bo, place, res, force_space ?
&limit_pool : NULL);
- if (ret) {
- if (ret != -ENOSPC && ret != -EAGAIN) {
- dmem_cgroup_pool_state_put(limit_pool);
- return ret;
- }
- if (!may_evict) {
- dmem_cgroup_pool_state_put(limit_pool);
- continue;
- }
+ ret = ttm_bo_alloc_at_place(bo, place, ctx, force_space,
+ res, &alloc_state);
+ if (ret == -ENOSPC) {
+ dmem_cgroup_pool_state_put(alloc_state.limit_pool);
+ continue;
+ } else if (ret == -EBUSY) {
ret = ttm_bo_evict_alloc(bdev, man, place, bo, ctx,
- ticket, res, limit_pool);
- dmem_cgroup_pool_state_put(limit_pool);
- if (ret == -EBUSY)
+ ticket, res, &alloc_state);
+
+ dmem_cgroup_pool_state_put(alloc_state.limit_pool);
+
+ if (ret) {
+ if (ret != -EBUSY)
+ return ret;
continue;
- if (ret)
- return ret;
+ }
Would keeping the ret checks at one level of indentation look better? Eg
like the current version:
if (ret == -EBUSY)
continue;
else if (ret)
return ret;
Up to you.
Btw, it is an interesting design that there are eviction errors which
prevent trying the next placement. A bit surprising to me but it is out
of scope here.
Anyway, I went back and forth a few times over the logic and it indeed
looks to me that there are no functional changes. Thanks for improving
the commit message as well, now it is completely clear what the patch is
about. With or without the nitpicks:
Reviewed-by: Tvrtko Ursulin <[email protected]>
Regards,
Tvrtko
+ } else if (ret) {
+ dmem_cgroup_pool_state_put(alloc_state.limit_pool);
+ return ret;
}
ret = ttm_bo_add_pipelined_eviction_fences(bo, man, ctx->no_wait_gpu);
