[Intel-xe] [PATCH] drm/xe: Decrement fault mode counts in xe_vm_close_and_put

[Matthew Brost]

On Thu, Mar 23, 2023 at 11:03:03PM -0700, Niranjana Vishwanathapura wrote:
> On Tue, Mar 21, 2023 at 09:08:15AM -0700, Matthew Brost wrote:
> > Rather waiting for the VM to be destroyed (all refs to VM go to zero),
> > drop the fault mode counts when the VM is closed in xe_vm_close_and_put.
> > This avoids a window where user space can create a faulting VM, close
> > it, and a subsequent creation of a non-faulting VM fails.
> > 
> > v2 (Lucas): Drop VLK reference in commit message
> > 
> > Suggested-by: Niranjana Vishwanathapura <niranjana.vishwanathapura at intel.com>
> > Signed-off-by: Matthew Brost <matthew.brost at intel.com>
> 
> This patch should probably go in the 'USM locking fixes' patch series?
>

Sure, likely need at respin all of these anyways.
 
> > ---
> > drivers/gpu/drm/xe/xe_vm.c | 15 +++++++--------
> > 1 file changed, 7 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
> > index 68ab51f84bed..ab036a51d17e 100644
> > --- a/drivers/gpu/drm/xe/xe_vm.c
> > +++ b/drivers/gpu/drm/xe/xe_vm.c
> > @@ -1349,6 +1349,13 @@ void xe_vm_close_and_put(struct xe_vm *vm)
> > 	XE_WARN_ON(!list_empty(&vm->extobj.list));
> > 	up_write(&vm->lock);
> > 
> > +	mutex_lock(&xe->usm.lock);
> > +	if (vm->flags & XE_VM_FLAG_FAULT_MODE)
> > +		xe->usm.num_vm_in_fault_mode--;
> > +	else if (!(vm->flags & XE_VM_FLAG_MIGRATION))
> > +		xe->usm.num_vm_in_non_fault_mode--;
> > +	mutex_unlock(&xe->usm.lock);
> > +
> > 	xe_vm_put(vm);
> 
> My concern is that something might be holding VM reference and hence
> VM will continue to live after we decrement the counts here and thus
> might endup in a new VM created in fault mode while the old VM in
> non-fault mode still not freed (and vice versa). Is that fine?
>

The VM is effectively dead after this as we tear down all the page
tables in this function, all page faults on the VM are just dropped, and
likewise no execs can be done to this VM after this function. So yes,I
believe this is safe.

Matt

> Niranjana
> 
> > }
> > 
> > @@ -1393,18 +1400,10 @@ static void vm_destroy_work_func(struct work_struct *w)
> > 
> > 	drm_gpuva_manager_destroy(&vm->mgr);
> > 
> > -	mutex_lock(&xe->usm.lock);
> > -	if (vm->flags & XE_VM_FLAG_FAULT_MODE)
> > -		xe->usm.num_vm_in_fault_mode--;
> > -	else if (!(vm->flags & XE_VM_FLAG_MIGRATION))
> > -		xe->usm.num_vm_in_non_fault_mode--;
> > -	mutex_unlock(&xe->usm.lock);
> > -
> > 	trace_xe_vm_free(vm);
> > 	dma_fence_put(vm->rebind_fence);
> > 	dma_resv_fini(&vm->resv);
> > 	kfree(vm);
> > -
> > }
> > 
> > void xe_vm_free(struct kref *ref)
> > -- 
> > 2.34.1
> >