On Thu, May 11, 2023 at 05:38:11PM +0200, Thomas Hellström wrote: > > On 5/11/23 16:54, Matthew Brost wrote: > > On Wed, May 10, 2023 at 04:19:32PM +0200, Thomas Hellström wrote: > > > If a vma was destroyed with the bo evicted, it might happen that we forget > > > to remove it from the notifer::rebind_list. Fix to make sure that really > > > happens. > > > > > > Signed-off-by: Thomas Hellström <[email protected]> > > > --- > > > drivers/gpu/drm/xe/xe_vm.c | 9 +++++++++ > > > 1 file changed, 9 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c > > > index 5f93d78c2e58..f54b3b7566c9 100644 > > > --- a/drivers/gpu/drm/xe/xe_vm.c > > > +++ b/drivers/gpu/drm/xe/xe_vm.c > > > @@ -978,6 +978,15 @@ static void xe_vma_destroy(struct xe_vma *vma, > > > struct dma_fence *fence) > > > } else { > > > xe_bo_assert_held(vma->bo); > > > list_del(&vma->bo_link); > > > + /* > > > + * TODO: We can do an advisory check for list link empty here, > > > + * if this lock becomes too costly. Nobody can re-add to the > > > + * bo to the vm::notifier::rebind_list at this point since we > > > + * have the bo lock. > > > + */ > > IMO grab isn't a big deal, not sure this is worth such a lengthly comment. > > Ok, I'll remove it. > > > > > > > + spin_lock(&vm->notifier.list_lock); > > > + list_del(&vma->notifier.rebind_link); > > Can you safe call list_del on an empty list? I thought that call blows > > up hence we have a bunch of if (!list_empty()) checks before calling > > list_del all over the driver. > > Good question. Looking at the implementation it definitely looks possible, > and I have LIST_DEBUG turned on when testing, so I assume it would have > blown up otherwise. >
It looks like 2 deletes will blow up but delete on a empty list is fine. With that: Reviewed-by: Matthew Brost <[email protected]> > /Thomas > > > > > > Matt > > > > > + spin_unlock(&vm->notifier.list_lock); > > > if (!vma->bo->vm) > > > vm_remove_extobj(vma); > > > } > > > -- > > > 2.39.2 > > >
