Hi, Christian, On Thu, 2024-07-04 at 11:21 +0200, Christian König wrote: > Am 03.07.24 um 17:38 schrieb Thomas Hellström: > > To address the problem with hitches moving when bulk move > > sublists are lru-bumped, register the list cursors with the > > ttm_lru_bulk_move structure when traversing its list, and > > when lru-bumping the list, move the cursor hitch to the tail. > > This also means it's mandatory for drivers to call > > ttm_lru_bulk_move_init() and ttm_lru_bulk_move_fini() when > > initializing and finalizing the bulk move structure, so add > > those calls to the amdgpu- and xe driver. > > > > Compared to v1 this is slightly more code but less fragile > > and hopefully easier to understand. > > This is the only patch in the series which I see critical. > > I think the final goal when using drm_exec in TTMs eviction path is > to > keep all evicted (or evicting) BOs locked until we have enough space. > > This means that for bulk move sections on the LRU we would lock the > first BO and would only drop that lock again if we have gone over the > full bulk move section and know that all BOs are not valuable for > eviction. > > Because of this the issue of having to consider hitches move with a > bulk > move section on the LRU doesn't even occur because for that a > concurrent > process would need to grab the common lock of the BOs in the bulk > move > section. While I agree that this is something we should strive towards, following the previous discussion I already reworked this patch completely to remove the dual hitches and make it less fragile. After that you mentioned you were ok with the high level approach for these first four patches here: https://lists.freedesktop.org/archives/dri-devel/2024-April/450288.html So is that not any longer the case? To recap, the concerns I'm seeing with the "kept common lock" approach are a) Since when we release the LRU lock and the common bulk bo lock is not yet locked, a LRU bump may happen and the hitch will go with it. So to avoid that we need to place the hitch *before* then considered resource in the LRU list rather than *after*. Now on the next iteration we need to come up with some way to choose what's really the next resource? If the next resource pointer is the same we already considered, should we assume it might have been freed and re-alloced with the same virtual address? b) It will be up to the user of the lru traversal to actually guarantee that locks are held across a bulk part, to make the resource traversal reasonably self-contained. In this case the LRU walker, because there's where the bo locking happens. This means that any other code that aims to walk the LRUs for various reasons, and doesn't provide any held lock guarantees, may be subject to unexpected results if someone bumped the LRU. So we would basically tailor the resource iteration here for a single use-case and not make it robust for various use-cases. So my suggestion is we keep this until we've come up with a bullet- proof way to sort out a) and b) above and then we can rip it out. /Thomas > > Regards, > Christian. > > > > > > Changes in previous series: > > - Completely rework the functionality > > - Avoid a NULL pointer dereference assigning manager->mem_type > > - Remove some leftover code causing build problems > > v2: > > - For hitch bulk tail moves, store the mem_type in the cursor > > instead of with the manager. > > v3: > > - Remove leftover mem_type member from change in v2. > > v6: > > - Add some lockdep asserts (Matthew Brost) > > - Avoid NULL pointer dereference (Matthew Brost) > > - No need to check bo->resource before dereferencing > > bo->bulk_move (Matthew Brost) > > > > Cc: Christian König <christian.koenig@xxxxxxx> > > Cc: Somalapuram Amaranath <Amaranath.Somalapuram@xxxxxxx> > > Cc: Matthew Brost <matthew.brost@xxxxxxxxx> > > Cc: <dri-devel@xxxxxxxxxxxxxxxxxxxxx> > > Signed-off-by: Thomas Hellström <thomas.hellstrom@xxxxxxxxxxxxxxx> > > --- > > drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 4 ++ > > drivers/gpu/drm/ttm/ttm_resource.c | 92 > > ++++++++++++++++++++++++++ > > drivers/gpu/drm/xe/xe_vm.c | 4 ++ > > include/drm/ttm/ttm_resource.h | 56 ++++++++++------ > > 4 files changed, 135 insertions(+), 21 deletions(-) > > > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c > > b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c > > index 3abfa66d72a2..97743993d711 100644 > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c > > @@ -2420,6 +2420,8 @@ int amdgpu_vm_init(struct amdgpu_device > > *adev, struct amdgpu_vm *vm, > > if (r) > > return r; > > > > + ttm_lru_bulk_move_init(&vm->lru_bulk_move); > > + > > vm->is_compute_context = false; > > > > vm->use_cpu_for_update = !!(adev- > > >vm_manager.vm_update_mode & > > @@ -2484,6 +2486,7 @@ int amdgpu_vm_init(struct amdgpu_device > > *adev, struct amdgpu_vm *vm, > > error_free_delayed: > > dma_fence_put(vm->last_tlb_flush); > > dma_fence_put(vm->last_unlocked); > > + ttm_lru_bulk_move_fini(&adev->mman.bdev, &vm- > > >lru_bulk_move); > > amdgpu_vm_fini_entities(vm); > > > > return r; > > @@ -2640,6 +2643,7 @@ void amdgpu_vm_fini(struct amdgpu_device > > *adev, struct amdgpu_vm *vm) > > } > > } > > > > + ttm_lru_bulk_move_fini(&adev->mman.bdev, &vm- > > >lru_bulk_move); > > } > > > > /** > > diff --git a/drivers/gpu/drm/ttm/ttm_resource.c > > b/drivers/gpu/drm/ttm/ttm_resource.c > > index 9c8b6499edfb..b6a2daac5518 100644 > > --- a/drivers/gpu/drm/ttm/ttm_resource.c > > +++ b/drivers/gpu/drm/ttm/ttm_resource.c > > @@ -33,6 +33,53 @@ > > > > #include <drm/drm_util.h> > > > > +/* Detach the cursor from the bulk move list*/ > > +static void > > +ttm_resource_cursor_clear_bulk(struct ttm_resource_cursor *cursor) > > +{ > > + lockdep_assert_held(&cursor->man->bdev->lru_lock); > > + > > + cursor->bulk = NULL; > > + list_del_init(&cursor->bulk_link); > > +} > > + > > +/* Move the cursor to the end of the bulk move list it's in */ > > +static void ttm_resource_cursor_move_bulk_tail(struct > > ttm_lru_bulk_move *bulk, > > + struct > > ttm_resource_cursor *cursor) > > +{ > > + struct ttm_lru_bulk_move_pos *pos; > > + > > + lockdep_assert_held(&cursor->man->bdev->lru_lock); > > + > > + if (WARN_ON_ONCE(bulk != cursor->bulk)) { > > + list_del_init(&cursor->bulk_link); > > + return; > > + } > > + > > + pos = &bulk->pos[cursor->mem_type][cursor->priority]; > > + if (pos->last) > > + list_move(&cursor->hitch.link, &pos->last- > > >lru.link); > > + ttm_resource_cursor_clear_bulk(cursor); > > +} > > + > > +/* Move all cursors attached to a bulk move to its end */ > > +static void ttm_bulk_move_adjust_cursors(struct ttm_lru_bulk_move > > *bulk) > > +{ > > + struct ttm_resource_cursor *cursor, *next; > > + > > + list_for_each_entry_safe(cursor, next, &bulk->cursor_list, > > bulk_link) > > + ttm_resource_cursor_move_bulk_tail(bulk, cursor); > > +} > > + > > +/* Remove a cursor from an empty bulk move list */ > > +static void ttm_bulk_move_drop_cursors(struct ttm_lru_bulk_move > > *bulk) > > +{ > > + struct ttm_resource_cursor *cursor, *next; > > + > > + list_for_each_entry_safe(cursor, next, &bulk->cursor_list, > > bulk_link) > > + ttm_resource_cursor_clear_bulk(cursor); > > +} > > + > > /** > > * ttm_resource_cursor_fini_locked() - Finalize the LRU list > > cursor usage > > * @cursor: The struct ttm_resource_cursor to finalize. > > @@ -45,6 +92,7 @@ void ttm_resource_cursor_fini_locked(struct > > ttm_resource_cursor *cursor) > > { > > lockdep_assert_held(&cursor->man->bdev->lru_lock); > > list_del_init(&cursor->hitch.link); > > + ttm_resource_cursor_clear_bulk(cursor); > > } > > > > /** > > @@ -73,9 +121,27 @@ void ttm_resource_cursor_fini(struct > > ttm_resource_cursor *cursor) > > void ttm_lru_bulk_move_init(struct ttm_lru_bulk_move *bulk) > > { > > memset(bulk, 0, sizeof(*bulk)); > > + INIT_LIST_HEAD(&bulk->cursor_list); > > } > > EXPORT_SYMBOL(ttm_lru_bulk_move_init); > > > > +/** > > + * ttm_lru_bulk_move_fini - finalize a bulk move structure > > + * @bdev: The struct ttm_device > > + * @bulk: the structure to finalize > > + * > > + * Sanity checks that bulk moves don't have any > > + * resources left and hence no cursors attached. > > + */ > > +void ttm_lru_bulk_move_fini(struct ttm_device *bdev, > > + struct ttm_lru_bulk_move *bulk) > > +{ > > + spin_lock(&bdev->lru_lock); > > + ttm_bulk_move_drop_cursors(bulk); > > + spin_unlock(&bdev->lru_lock); > > +} > > +EXPORT_SYMBOL(ttm_lru_bulk_move_fini); > > + > > /** > > * ttm_lru_bulk_move_tail - bulk move range of resources to the > > LRU tail. > > * > > @@ -88,6 +154,7 @@ void ttm_lru_bulk_move_tail(struct > > ttm_lru_bulk_move *bulk) > > { > > unsigned i, j; > > > > + ttm_bulk_move_adjust_cursors(bulk); > > for (i = 0; i < TTM_NUM_MEM_TYPES; ++i) { > > for (j = 0; j < TTM_MAX_BO_PRIORITY; ++j) { > > struct ttm_lru_bulk_move_pos *pos = &bulk- > > >pos[i][j]; > > @@ -515,6 +582,28 @@ void ttm_resource_manager_debug(struct > > ttm_resource_manager *man, > > } > > EXPORT_SYMBOL(ttm_resource_manager_debug); > > > > +static void > > +ttm_resource_cursor_check_bulk(struct ttm_resource_cursor *cursor, > > + struct ttm_lru_item *next_lru) > > +{ > > + struct ttm_resource *next = ttm_lru_item_to_res(next_lru); > > + struct ttm_lru_bulk_move *bulk = NULL; > > + struct ttm_buffer_object *bo = next->bo; > > + > > + lockdep_assert_held(&cursor->man->bdev->lru_lock); > > + bulk = bo->bulk_move; > > + > > + if (cursor->bulk != bulk) { > > + if (bulk) { > > + list_move_tail(&cursor->bulk_link, &bulk- > > >cursor_list); > > + cursor->mem_type = next->mem_type; > > + } else { > > + list_del_init(&cursor->bulk_link); > > + } > > + cursor->bulk = bulk; > > + } > > +} > > + > > /** > > * ttm_resource_manager_first() - Start iterating over the > > resources > > * of a resource manager > > @@ -535,6 +624,7 @@ ttm_resource_manager_first(struct > > ttm_resource_manager *man, > > cursor->priority = 0; > > cursor->man = man; > > ttm_lru_item_init(&cursor->hitch, TTM_LRU_HITCH); > > + INIT_LIST_HEAD(&cursor->bulk_link); > > list_add(&cursor->hitch.link, &man->lru[cursor- > > >priority]); > > > > return ttm_resource_manager_next(cursor); > > @@ -559,6 +649,7 @@ ttm_resource_manager_next(struct > > ttm_resource_cursor *cursor) > > lru = &cursor->hitch; > > list_for_each_entry_continue(lru, &man- > > >lru[cursor->priority], link) { > > if (ttm_lru_item_is_res(lru)) { > > + ttm_resource_cursor_check_bulk(cur > > sor, lru); > > list_move(&cursor->hitch.link, > > &lru->link); > > return ttm_lru_item_to_res(lru); > > } > > @@ -568,6 +659,7 @@ ttm_resource_manager_next(struct > > ttm_resource_cursor *cursor) > > break; > > > > list_move(&cursor->hitch.link, &man->lru[cursor- > > >priority]); > > + ttm_resource_cursor_clear_bulk(cursor); > > } > > > > ttm_resource_cursor_fini_locked(cursor); > > diff --git a/drivers/gpu/drm/xe/xe_vm.c > > b/drivers/gpu/drm/xe/xe_vm.c > > index 5b166fa03684..0c7e327bc9a2 100644 > > --- a/drivers/gpu/drm/xe/xe_vm.c > > +++ b/drivers/gpu/drm/xe/xe_vm.c > > @@ -1335,6 +1335,8 @@ struct xe_vm *xe_vm_create(struct xe_device > > *xe, u32 flags) > > > > INIT_WORK(&vm->destroy_work, vm_destroy_work_func); > > > > + ttm_lru_bulk_move_init(&vm->lru_bulk_move); > > + > > INIT_LIST_HEAD(&vm->preempt.exec_queues); > > vm->preempt.min_run_period_ms = 10; /* FIXME: Wire up > > to uAPI */ > > > > @@ -1458,6 +1460,7 @@ struct xe_vm *xe_vm_create(struct xe_device > > *xe, u32 flags) > > mutex_destroy(&vm->snap_mutex); > > for_each_tile(tile, xe, id) > > xe_range_fence_tree_fini(&vm->rftree[id]); > > + ttm_lru_bulk_move_fini(&xe->ttm, &vm->lru_bulk_move); > > kfree(vm); > > if (flags & XE_VM_FLAG_LR_MODE) > > xe_pm_runtime_put(xe); > > @@ -1601,6 +1604,7 @@ static void vm_destroy_work_func(struct > > work_struct *w) > > XE_WARN_ON(vm->pt_root[id]); > > > > trace_xe_vm_free(vm); > > + ttm_lru_bulk_move_fini(&xe->ttm, &vm->lru_bulk_move); > > kfree(vm); > > } > > > > diff --git a/include/drm/ttm/ttm_resource.h > > b/include/drm/ttm/ttm_resource.h > > index 8fac781f641e..571abb4861a6 100644 > > --- a/include/drm/ttm/ttm_resource.h > > +++ b/include/drm/ttm/ttm_resource.h > > @@ -269,26 +269,6 @@ ttm_lru_item_to_res(struct ttm_lru_item *item) > > return container_of(item, struct ttm_resource, lru); > > } > > > > -/** > > - * struct ttm_resource_cursor > > - * > > - * @man: The resource manager currently being iterated over. > > - * @hitch: A hitch list node inserted before the next resource > > - * to iterate over. > > - * @priority: the current priority > > - * > > - * Cursor to iterate over the resources in a manager. > > - */ > > -struct ttm_resource_cursor { > > - struct ttm_resource_manager *man; > > - struct ttm_lru_item hitch; > > - unsigned int priority; > > -}; > > - > > -void ttm_resource_cursor_fini_locked(struct ttm_resource_cursor > > *cursor); > > - > > -void ttm_resource_cursor_fini(struct ttm_resource_cursor *cursor); > > - > > /** > > * struct ttm_lru_bulk_move_pos > > * > > @@ -304,8 +284,9 @@ struct ttm_lru_bulk_move_pos { > > > > /** > > * struct ttm_lru_bulk_move > > - * > > * @pos: first/last lru entry for resources in the each > > domain/priority > > + * @cursor_list: The list of cursors currently traversing any of > > + * the sublists of @pos. Protected by the ttm device's lru_lock. > > * > > * Container for the current bulk move state. Should be used with > > * ttm_lru_bulk_move_init() and ttm_bo_set_bulk_move(). > > @@ -315,8 +296,39 @@ struct ttm_lru_bulk_move_pos { > > */ > > struct ttm_lru_bulk_move { > > struct ttm_lru_bulk_move_pos > > pos[TTM_NUM_MEM_TYPES][TTM_MAX_BO_PRIORITY]; > > + struct list_head cursor_list; > > }; > > > > +/** > > + * struct ttm_resource_cursor > > + * @man: The resource manager currently being iterated over > > + * @hitch: A hitch list node inserted before the next resource > > + * to iterate over. > > + * @bulk_link: A list link for the list of cursors traversing the > > + * bulk sublist of @bulk. Protected by the ttm device's lru_lock. > > + * @bulk: Pointer to struct ttm_lru_bulk_move whose subrange > > @hitch is > > + * inserted to. NULL if none. Never dereference this pointer since > > + * the struct ttm_lru_bulk_move object pointed to might have been > > + * freed. The pointer is only for comparison. > > + * @mem_type: The memory type of the LRU list being traversed. > > + * This field is valid iff @bulk != NULL. > > + * @priority: the current priority > > + * > > + * Cursor to iterate over the resources in a manager. > > + */ > > +struct ttm_resource_cursor { > > + struct ttm_resource_manager *man; > > + struct ttm_lru_item hitch; > > + struct list_head bulk_link; > > + struct ttm_lru_bulk_move *bulk; > > + unsigned int mem_type; > > + unsigned int priority; > > +}; > > + > > +void ttm_resource_cursor_fini_locked(struct ttm_resource_cursor > > *cursor); > > + > > +void ttm_resource_cursor_fini(struct ttm_resource_cursor *cursor); > > + > > /** > > * struct ttm_kmap_iter_iomap - Specialization for a struct > > io_mapping + > > * struct sg_table backed struct ttm_resource. > > @@ -405,6 +417,8 @@ ttm_resource_manager_cleanup(struct > > ttm_resource_manager *man) > > > > void ttm_lru_bulk_move_init(struct ttm_lru_bulk_move *bulk); > > void ttm_lru_bulk_move_tail(struct ttm_lru_bulk_move *bulk); > > +void ttm_lru_bulk_move_fini(struct ttm_device *bdev, > > + struct ttm_lru_bulk_move *bulk); > > > > void ttm_resource_add_bulk_move(struct ttm_resource *res, > > struct ttm_buffer_object *bo); >
