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 <[email protected]> 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/[email protected]?part=7
