On Fri, 2026-08-14 at 13:46 +0000, [email protected] wrote: > 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.
Both issues verified as real and reachable. Will update for v3. Thanks, Thomas > -- > > --- 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); > > }
