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