Re: [PATCH v4 4/6] drm/ttm: Split cgroup charge and resource allocation

Wed, 25 Feb 2026 08:01:38 -0800 


On 25/02/2026 15:33, Tvrtko Ursulin wrote:


On 25/02/2026 12:10, Natalie Vock wrote:

Coupling resource allocation and cgroup charging is racy when charging
succeeds, but subsequent resource allocation fails. Certain eviction
decisions are made on the basis of whether the allocating cgroup is
protected, i.e. within its min/low limits, but with the charge being
tied to resource allocation (and uncharged when the resource allocation
fails), this check is done at a poin where the allocation is not actually


s/poin/point/


charged to the cgroup.

This is subtly wrong if the allocation were to cause the cgroup to exceed
the min/low protection, but it's even more wrong if the same cgroup tries
allocating multiple buffers concurrently: In this case, the min/low
protection may pass for all allocation attempts when the real min/low
protection covers only some, or potentially none of the allocated
buffers.



Interesting! Do I understand correctly this would be a scenario with 
multi-threaded buffer allocation or there is another path to it?



In any case moving the charge to before allocation makes sense to me. 
With a caveat that I wasn't involved in the dmem cgroup controller 
design so may be missing something.



Instead, charge the allocation to the cgroup once and keep the charge

for as long as we try to allocate a ttm_resource, and only undo the 
charge

if allocating the resource is ultimately unsuccessful and we move on to
a different ttm_place.

Signed-off-by: Natalie Vock <[email protected]>
---
  drivers/gpu/drm/ttm/ttm_bo.c       | 28 +++++++++++++++-------

  drivers/gpu/drm/ttm/ttm_resource.c | 48 ++++++++++++++++++++++++++ 
+-----------

  include/drm/ttm/ttm_resource.h     |  6 ++++-
  3 files changed, 60 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
index 48dbaaa46824c..a8914d20b0c32 100644
--- a/drivers/gpu/drm/ttm/ttm_bo.c
+++ b/drivers/gpu/drm/ttm/ttm_bo.c

@@ -490,6 +490,8 @@ 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;
  };

@@ -546,7 +548,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:

@@ -724,10 +726,8 @@ static int ttm_bo_alloc_at_place(struct 
ttm_buffer_object *bo,

      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);
-
+    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

@@ -737,14 +737,23 @@ static int ttm_bo_alloc_at_place(struct 
ttm_buffer_object *bo,

           * attempt.
           */
          if (ret == -EAGAIN)
-            return may_evict ? -EBUSY : -ENOSPC;
+            ret = may_evict ? -EBUSY : -ENOSPC;
+        return ret;
+    }
-        if (ret == -ENOSPC && may_evict)
-            return -EBUSY;
+    ret = ttm_resource_alloc(bo, place, res, alloc_state->charge_pool);


No need for a blank line here.


+    if (ret) {
+        if (ret == -ENOSPC && may_evict)
+            ret = -EBUSY;


Why did you remove EAGAIN handling from after ttm_resource_alloc()?



I figured this part out. I guess EAGAIN can only come out 
dmem_cgroup_try_charge() which is no longer here. Makes sense.



Return code handling changes look fine to me in this case. Just the 
question of uncharging remains.


Regards,

Tvrtko




          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;
  }

@@ -799,6 +808,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) {

@@ -808,11 +818,13 @@ 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);
                  if (ret != -EBUSY)
                      return ret;
                  continue;
              }
          } else if (ret) {
+            dmem_cgroup_pool_state_put(alloc_state.charge_pool);



Is uncharge in the failure case hidden in dmem_cgroup_pool_state_put() 
somehow?


Regards,

Tvrtko


              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.c

index 192fca24f37e4..a8a836f6e376a 100644
--- a/drivers/gpu/drm/ttm/ttm_resource.c
+++ b/drivers/gpu/drm/ttm/ttm_resource.c

@@ -373,30 +373,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;
      spin_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.h

index 33e80f30b8b82..549b5b796884d 100644
--- a/include/drm/ttm/ttm_resource.h
+++ b/include/drm/ttm/ttm_resource.h

@@ -456,10 +456,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,




























					Reply via email to