[Intel-xe] [PATCH 2/2] drm/xe: Properly remove the vma from the vm::notifer::rebind_list when destroyed

[Matthew Brost]

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 at linux.intel.com>
> > > ---
> > >   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 at intel.com>

> /Thomas
> 
> 
> > 
> > Matt
> > 
> > > +		 spin_unlock(&vm->notifier.list_lock);
> > >   		if (!vma->bo->vm)
> > >   			vm_remove_extobj(vma);
> > >   	}
> > > -- 
> > > 2.39.2
> > >