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

[Tvrtko Ursulin <tursulin@ursulin.net>]

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