Re: [PATCH v7 3/6] drm/ttm: Extract code for attempting allocation in a place

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Low] Incomplete refactoring: `ttm_bo_evict_cb` bypasses `ttm_bo_alloc_at_place`.
- [Low] Unreachable dead code in `ttm_bo_alloc_at_place` handling `-EAGAIN`.
- [Low] Unused parameter `ctx` in `ttm_bo_alloc_at_place`.
--

--- Patch [3]: [PATCH v7 3/6] drm/ttm: Extract code for attempting allocation in a place ---
commit 6da6afe8040fede46543a08f8c3a3c9b15da624e
Author: Natalie Vock <natalie.vock@gmx.de>
Date:   Tue Jul 21 19:43:22 2026 +0000

    drm/ttm: Extract code for attempting allocation in a place
    
    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.

> diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
> index 3980f376e3ba4..7192c15f2fcef 100644
> --- a/drivers/gpu/drm/ttm/ttm_bo.c
> +++ b/drivers/gpu/drm/ttm/ttm_bo.c

[ ... ]

> @@ -488,6 +488,62 @@ 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;
> +};
> +
> +/**
> + * 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,
                                 ^^^^

[Severity: Low]
Is the ctx parameter needed here? It doesn't appear to be used anywhere within
the body of ttm_bo_alloc_at_place().

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

[Severity: Low]
Can ttm_resource_alloc() return -EAGAIN here? If it intercepts -EAGAIN
internally and converts it to -ENOSPC, this condition might be unreachable.

> +			return may_evict ? -EBUSY : -ENOSPC;
> +
> +		if (ret == -ENOSPC && may_evict)
> +			return -EBUSY;
> +
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
>  /**
>   * struct ttm_bo_evict_walk - Parameters for the evict walk.
>   */
> @@ -503,12 +559,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)
> @@ -517,8 +574,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))
                                              ^^^^

[Severity: Low]
Later in ttm_bo_evict_cb(), the code still calls ttm_resource_alloc()
directly:

	if (evict_walk->res)
		lret = ttm_resource_alloc(evict_walk->evictor, evict_walk->place,
					  evict_walk->res, NULL);

Should this have been updated to use the newly introduced
ttm_bo_alloc_at_place() as part of the refactoring?

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260721-dmemcg-aggressive-protect-v7-0-97c4b1014315@gmx.de?part=3