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
