Re: [PATCH v2 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 and zero-refcount BOs, failing to completely evict the manager.
- [Medium] cond_resched() is called while the buffer object's dma_resv lock is held.
--

commit ff3394ed91bf06ac0115f1da5665b9edeeb4a66b
Author: Natalie Vock <nat@pixelcluster.dev>

drm/ttm: use ttm_bo_lru_for_each_reserved_guarded in evict_all

Use the for_each loop to evict all BOs of an resource manager as well.
Greately simplifying the handling and finally allows us to remove
ttm_bo_evict_first().

> diff --git a/drivers/gpu/drm/ttm/ttm_resource.c b/drivers/gpu/drm/ttm/ttm_resource.c
> index 154d6739256f8..107b6cfea66b3 100644
> --- a/drivers/gpu/drm/ttm/ttm_resource.c
> +++ b/drivers/gpu/drm/ttm/ttm_resource.c

[ ... ]

> @@ -561,17 +561,24 @@ 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,
> +	};
>  	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) {
> +		ret = ttm_bo_evict(bo, &ctx);
> +		if (ret)
> +			return ret;

[Severity: High]
Does this new loop successfully evict all buffers if some are locked by other
threads?

With the previous do/while loop using ttm_bo_evict_first(), it would keep
retrying until the manager was genuinely empty. 

Using ttm_bo_lru_for_each_reserved_guarded() with an arg where
arg.ticket = NULL, __ttm_bo_lru_cursor_next() skips locked or zero-refcount BOs:

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) {
            spin_unlock(lru_lock);
            ttm_bo_put(bo);
            spin_lock(lru_lock);
            continue;
        }
        ...
}

Because the new loop is a single-pass iteration, will these skipped BOs be left
permanently in the resource manager, potentially causing resource leaks or
kernel crashes (like a WARN_ON for non-empty LRU lists) during driver unload or
device suspend?

> -		cond_resched();
> -	} while (!ret);
> -
> -	if (ret && ret != -ENOENT)
> -		return ret;
> +		cond_resched();

[Severity: Medium]
Is cond_resched() being called here while holding the buffer object's
dma_resv lock?

The ttm_bo_lru_for_each_reserved_guarded iterator yields each BO with its
dma_resv lock held, and only releases it during the implicit next call at the
end of the loop.

Yielding the CPU while holding this lock might stall other kernel paths waiting
for the lock. 

In the previous implementation, the lock was explicitly released inside
ttm_bo_evict_first() before cond_resched() was invoked.

> +	}
>  
>  	ret = 0;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260706-ttm_2_drm_exec-v2-0-4bf6bfc0d320@pixelcluster.dev?part=7