[Intel-xe] [PATCH 2/2] drm/xe: Move in fault mode / non-fault mode check to xe_vm_create
Matthew Brost
matthew.brost at intel.com
Fri Mar 24 16:27:08 UTC 2023
On Fri, Mar 24, 2023 at 05:25:42PM +0100, Maarten Lankhorst wrote:
> Reviewed-by: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
>
> On 2023-03-24 08:20, Matthew Brost wrote:
> > 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 at linux.intel.com>
> > > > Signed-off-by: Matthew Brost <matthew.brost at 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))
> > >
> > Sure.
>
> This will not work correctly when fault_mode and non_fault_mode are both
> false (eg first context created).
>
Yep, realized this last night too and just sent an email stating that.
Matt
> I'd jsut leave this patch as it is now.
>
> ~Maarten
>
> > > > + 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?
> > >
> > Sure.
> >
> > Matt
> > > 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
> > > >
More information about the Intel-xe
mailing list