Re: [PATCH 07/10] drm/ttm: use ttm_bo_lru_for_each_reserved_guarded in evict_all

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] ttm_resource_manager_evict_all() silently skips locked BOs instead of waiting for them, failing to ensure the resource manager is completely empty.
- [Low] The function prototype for ttm_bo_evict_first() was left in the header despite its implementation being removed.
--

commit 916bbbd30917f83e8ac0aca7e1ff8e726b30968b
Author: Christian König <christian.koenig@amd.com>

drm/ttm: use ttm_bo_lru_for_each_reserved_guarded in evict_all

This patch replaces the while loop in ttm_resource_manager_evict_all()
with the ttm_bo_lru_for_each_reserved_guarded() iterator. It also removes
the ttm_bo_evict_first() function and makes ttm_bo_evict() available
externally.

> diff --git a/drivers/gpu/drm/ttm/ttm_resource.c b/drivers/gpu/drm/ttm/ttm_resource.c
> index 154d6739256f8..cf3fc3a594388 100644
> --- a/drivers/gpu/drm/ttm/ttm_resource.c
> +++ b/drivers/gpu/drm/ttm/ttm_resource.c
> @@ -561,17 +561,25 @@ EXPORT_SYMBOL(ttm_resource_manager_init);
>  int ttm_resource_manager_evict_all(struct ttm_device *bdev,
>  				   struct ttm_resource_manager *man)
>  {
> -	struct ttm_operation_ctx ctx = { };
> +	struct ttm_bo_lru_cursor cursor;
> +	struct ttm_buffer_object *bo;
> +	struct ttm_operation_ctx ctx = {
> +		.interruptible = false,
> +		.no_wait_gpu = false,
> +	};
> +	struct ttm_lru_walk_arg arg = {
> +		.ctx = &ctx,
> +		.trylock_only = true
> +	};
>  	struct dma_fence *fence;
>  	int ret, i;
>  
> -	do {
> -		ret = ttm_bo_evict_first(bdev, man, &ctx);
> +	ttm_bo_lru_for_each_reserved_guarded(&cursor, man, &arg, bo) {

[Severity: High]
Does this new iterator loop silently skip locked BOs instead of waiting for them?

Because arg.trylock_only is set to true, the iterator will fail to trylock
any BO currently reserved by another thread and simply continue to the next one:

drivers/gpu/drm/ttm/ttm_bo_util.c:__ttm_bo_lru_cursor_next() {
        ...
		} else if ((!arg->ticket || arg->ctx->no_wait_gpu ||
			    arg->trylock_only) && !arg->ctx->exec) {
			spin_unlock(lru_lock);
			ttm_bo_put(bo);
			spin_lock(lru_lock);
			continue;
		}
        ...
}

The previous while loop would repeatedly try to evict everything until
ttm_bo_evict_first() returned -ENOENT. Will this new single-pass approach cause
ttm_resource_manager_evict_all() to return success (0) while leaving un-evicted
BOs behind if they are concurrently locked?

> +		ret = ttm_bo_evict(bo, &ctx);
> +		if (ret)
> +			return ret;
>  		cond_resched();
> -	} while (!ret);
> -
> -	if (ret && ret != -ENOENT)
> -		return ret;
>  
>  	ret = 0;
>  
> diff --git a/include/drm/ttm/ttm_bo.h b/include/drm/ttm/ttm_bo.h
> index bbed63064c9a9..a4060e44d23d0 100644
> --- a/include/drm/ttm/ttm_bo.h
> +++ b/include/drm/ttm/ttm_bo.h
> @@ -389,6 +389,7 @@ int ttm_bo_validate(struct ttm_buffer_object *bo,
>  void ttm_bo_fini(struct ttm_buffer_object *bo);
>  void ttm_bo_set_bulk_move(struct ttm_buffer_object *bo,
>  			  struct ttm_lru_bulk_move *bulk);
> +int ttm_bo_evict(struct ttm_buffer_object *bo, struct ttm_operation_ctx *ctx);

[Severity: Low]
This isn't a bug, but it looks like the prototype for ttm_bo_evict_first()
was left in include/drm/ttm/ttm_bo.h even though its implementation is removed
by this patch.

>  bool ttm_bo_eviction_valuable(struct ttm_buffer_object *bo,
>  			      const struct ttm_place *place);
>  int ttm_bo_init_reserved(struct ttm_device *bdev, struct ttm_buffer_object *bo,

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260703-ttm_2_drm_exec-v1-0-43685ac1286b@gmx.de?part=7