Re: [Intel-xe] [PATCH 2/2] drm/xe: Move in fault mode / non-fault mode check to xe_vm_create

[Matthew Brost <matthew.brost@intel.com>]

On Thu, Mar 23, 2023 at 11:10:17PM -0700, Niranjana Vishwanathapura wrote:
> On Tue, Mar 21, 2023 at 04:42:17PM -0700, Matthew Brost wrote:
> > The check for fault mode / non-fault mode was in the VM create IOCTL
> > before VM creation and not under a lock. The increment was after VM
> > creation under the lock. This is racey. Move both the check and
> > increment to xe_vm_create before actual creation and have the lock for
> > both of these steps.
> > 
> > Suggested-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Signed-off-by: Matthew Brost <matthew.brost@intel.com>
> > ---
> > drivers/gpu/drm/xe/xe_vm.c | 45 ++++++++++++++++++++++++--------------
> > 1 file changed, 28 insertions(+), 17 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
> > index e7674612a57e..965cad81b02a 100644
> > --- a/drivers/gpu/drm/xe/xe_vm.c
> > +++ b/drivers/gpu/drm/xe/xe_vm.c
> > @@ -1060,9 +1060,27 @@ struct xe_vm *xe_vm_create(struct xe_device *xe, u32 flags)
> > 	struct xe_gt *gt;
> > 	u8 id;
> > 
> > +	err = mutex_lock_interruptible(&xe->usm.lock);
> > +	if (err)
> > +		return ERR_PTR(err);
> > +	if (XE_IOCTL_ERR(xe, flags & XE_VM_FLAG_FAULT_MODE &&
> > +			 xe_device_in_non_fault_mode(xe)) ||
> > +	    XE_IOCTL_ERR(xe, !(flags & XE_VM_FLAG_MIGRATION) &&
> > +			 xe_device_in_fault_mode(xe))) {
> 
> NIT...is below simplification any better?
> 
> bool fault_mode = !!(flags & XE_VM_FLAG_FAULT_MODE);
> if (XE_IOCTL_ERR(xe, fault_mode != xe_device_in_fault_mode(xe))
> 

I thought about this and this isn't the same logic, so no this doesn't work.

Matt

> > +		mutex_unlock(&xe->usm.lock);
> > +		return ERR_PTR(-EINVAL);
> > +	}
> > +	if (flags & XE_VM_FLAG_FAULT_MODE)
> > +		xe->usm.num_vm_in_fault_mode++;
> > +	else if (!(flags & XE_VM_FLAG_MIGRATION))
> > +		xe->usm.num_vm_in_non_fault_mode++;
> > +	mutex_unlock(&xe->usm.lock);
> > +
> > 	vm = kzalloc(sizeof(*vm), GFP_KERNEL);
> > -	if (!vm)
> > -		return ERR_PTR(-ENOMEM);
> > +	if (!vm) {
> > +		err = -ENOMEM;
> > +		goto err_usm;
> > +	}
> > 
> > 	vm->xe = xe;
> > 	kref_init(&vm->refcount);
> > @@ -1182,13 +1200,6 @@ struct xe_vm *xe_vm_create(struct xe_device *xe, u32 flags)
> > 	if (number_gts > 1)
> > 		vm->composite_fence_ctx = dma_fence_context_alloc(1);
> > 
> > -	mutex_lock(&xe->usm.lock);
> > -	if (flags & XE_VM_FLAG_FAULT_MODE)
> > -		xe->usm.num_vm_in_fault_mode++;
> > -	else if (!(flags & XE_VM_FLAG_MIGRATION))
> > -		xe->usm.num_vm_in_non_fault_mode++;
> > -	mutex_unlock(&xe->usm.lock);
> > -
> > 	trace_xe_vm_create(vm);
> > 
> > 	return vm;
> > @@ -1220,6 +1231,14 @@ struct xe_vm *xe_vm_create(struct xe_device *xe, u32 flags)
> > 		xe_device_mem_access_put(xe);
> > 		xe_pm_runtime_put(xe);
> > 	}
> > +err_usm:
> > +	mutex_lock(&xe->usm.lock);
> > +	if (flags & XE_VM_FLAG_FAULT_MODE)
> > +		xe->usm.num_vm_in_fault_mode--;
> > +	else if (!(flags & XE_VM_FLAG_MIGRATION))
> > +		xe->usm.num_vm_in_non_fault_mode--;
> > +	mutex_unlock(&xe->usm.lock);
> > +
> 
> Perhaps put these counts increment/decrement blocks into functions
> instead of duplicating them?
> 
> Niranjana
> 
> > 	return ERR_PTR(err);
> > }
> > 
> > @@ -1843,14 +1862,6 @@ int xe_vm_create_ioctl(struct drm_device *dev, void *data,
> > 			 args->flags & DRM_XE_VM_CREATE_FAULT_MODE))
> > 		return -EINVAL;
> > 
> > -	if (XE_IOCTL_ERR(xe, args->flags & DRM_XE_VM_CREATE_FAULT_MODE &&
> > -			 xe_device_in_non_fault_mode(xe)))
> > -		return -EINVAL;
> > -
> > -	if (XE_IOCTL_ERR(xe, !(args->flags & DRM_XE_VM_CREATE_FAULT_MODE) &&
> > -			 xe_device_in_fault_mode(xe)))
> > -		return -EINVAL;
> > -
> > 	if (XE_IOCTL_ERR(xe, args->flags & DRM_XE_VM_CREATE_FAULT_MODE &&
> > 			 !xe->info.supports_usm))
> > 		return -EINVAL;
> > -- 
> > 2.34.1
> >