Re: [PATCH] drm/ttm: Represent LRU bulk moves as nested sublists

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Missing cursor adjustment in `ttm_lru_bulk_move_add` when re-seeding an empty bulk move anchor causes recently-used resources to be prematurely evicted.
- [Medium] Computing `&man->lru[j]` when `man` is NULL in `ttm_bulk_move_drop_cursors` invokes Undefined Behavior and will trigger UBSAN panics.
--

--- Patch [1]: [PATCH] drm/ttm: Represent LRU bulk moves as nested sublists ---
commit b97657bee6dc1cddf7841bc260d33b4b040368e4
Author: Thomas Hellström <[email protected]>
Subject: drm/ttm: Represent LRU bulk moves as nested sublists

This patch changes the LRU bulk move mechanism to track each group as a
real nested sublist instead of ranges. Each group gets a persistent anchor
node linked on the manager LRU list along with a sublist holding its member
resources, simplifying cursor tracking and eliminating range-corruption bugs.

Link: https://gitlab.freedesktop.org/drm/amd/-/issues/5387

> diff --git a/drivers/gpu/drm/ttm/ttm_resource.c b/drivers/gpu/drm/ttm/ttm_resource.c
> index 4e6d3d658d858..195f960b2d738 100644
> --- a/drivers/gpu/drm/ttm/ttm_resource.c
> +++ b/drivers/gpu/drm/ttm/ttm_resource.c

[ ... ]

> +static void ttm_bulk_move_drop_cursors(struct ttm_device *bdev,
> +					struct ttm_lru_bulk_move *bulk)
> +{
> +	unsigned int i, j;
> +
> +	for (i = 0; i < TTM_NUM_MEM_TYPES; ++i) {
> +		struct ttm_resource_manager *man = ttm_manager_type(bdev, i);
> +
> +		for (j = 0; j < TTM_MAX_BO_PRIORITY; ++j) {
> +			struct ttm_lru_bulk_move_pos *pos = &bulk->pos[i][j];
> +			struct ttm_lru_item *lru, *next;
> +
> +			list_for_each_entry_safe(lru, next, &pos->sublist, link) {
> +				struct ttm_resource_cursor *cursor;
> +
> +				if (ttm_lru_item_is_res(lru)) {
> +					WARN_ON_ONCE(1);
> +					continue;
> +				}
> +				/*
> +				 * This cursor descended here; its main hitch
> +				 * already sits on the manager list, so just
> +				 * detach it from this sublist.
> +				 */
> +				cursor = container_of(lru, typeof(*cursor),
> +						      sublist_hitch);
> +				cursor->cur_list = &man->lru[j];
> +				list_del_init(&lru->link);
> +			}
> +			list_splice_tail_init(&pos->sublist, &man->lru[j]);

[Severity: Medium]
When ttm_manager_type() returns NULL for unused memory types, doesn't the
pointer arithmetic &man->lru[j] result in undefined behavior?

If the kernel is compiled with UBSAN, this can trigger a panic even if the
list operations don't actually dereference the pointer. Should there be a
check like if (!man) continue; before the inner loop?

> +			list_del_init(&pos->marker.link);
> +		}
> +	}
> +}

[ ... ]

> +/* Add the resource to a bulk_move sublist */
>  static void ttm_lru_bulk_move_add(struct ttm_lru_bulk_move *bulk,
>  				  struct ttm_resource *res)
>  {
>  	struct ttm_lru_bulk_move_pos *pos = ttm_lru_bulk_move_pos(bulk, res);
> +	struct ttm_resource *first = ttm_lru_first_res_or_null(&pos->sublist);
> +	struct ttm_buffer_object *bo = res->bo;
> +	struct ttm_resource_manager *man =
> +		ttm_manager_type(bo->bdev, res->mem_type);
>  
> -	if (!pos->first) {
> -		pos->first = res;
> -		pos->last = res;
> +	if (first) {
> -		WARN_ON(pos->first->bo->base.resv != res->bo->base.resv);
> -		ttm_lru_bulk_move_pos_tail(pos, res);
> +		WARN_ON(first->bo->base.resv != res->bo->base.resv);
>  	} else {
> +		/*
> +		 * Group empty (first activation, or all members were pinned
> +		 * or swapped out); re-seed the anchor at the tail so it
> +		 * counts as recently used.
> +		 */
> +		list_move_tail(&pos->marker.link, &man->lru[bo->priority]);
>  	}

[Severity: High]
If an active LRU traversal cursor has descended into this sublist and all
resources are subsequently removed, the sublist becomes empty except for the
cursor's hitch.

When a new resource is added and 'first' is NULL, we re-seed the anchor to
the tail of the manager's LRU list. However, could this leave the descended
cursor parked in the sublist without adjusting it?

If ttm_lru_bulk_move_pos_adjust_cursors() is not called to detach it, the
resumed traversal might immediately process and evict this newly added
resource, violating the LRU guarantee.

> +	/*
> +	 * The resource may still be on another list (manager LRU or
> +	 * bdev->unevictable); move it unconditionally to keep group
> +	 * membership consistent.
> +	 */
> +	list_move_tail(&res->lru.link, &pos->sublist);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1