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 <thomas.hellstrom@xxxxxxxxxxxxxxx>
> > > ---
> > > 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 <matthew.brost@xxxxxxxxx>
> /Thomas
>
>
> >
> > Matt
> >
> > > + spin_unlock(&vm->notifier.list_lock);
> > > if (!vma->bo->vm)
> > > vm_remove_extobj(vma);
> > > }
> > > --
> > > 2.39.2
> > >
