[Intel-gfx] [PATCH 4/5] drm/i915/gem: Convert vm idr to xarray

[Chris Wilson]

Quoting Ruhl, Michael J (2020-01-22 16:00:25)
> >-----Original Message-----
> >From: Intel-gfx <intel-gfx-bounces at lists.freedesktop.org> On Behalf Of Chris
> >Wilson
> >@@ -876,23 +868,13 @@ int i915_gem_vm_create_ioctl(struct drm_device
> >*dev, void *data,
> >                       goto err_put;
> >       }
> >
> >-      err = mutex_lock_interruptible(&file_priv->vm_idr_lock);
> >+      err = xa_alloc(&file_priv->vm_xa, &args->vm_id,
> >+                     &ppgtt->vm, xa_limit_32b, GFP_KERNEL);
> >       if (err)
> >               goto err_put;
> >
> >-      err = idr_alloc(&file_priv->vm_idr, &ppgtt->vm, 0, 0, GFP_KERNEL);
> >-      if (err < 0)
> >-              goto err_unlock;
> >-
> >-      GEM_BUG_ON(err == 0); /* reserved for invalid/unassigned ppgtt */
> 
> Moving this comment to the xa_init_flags() would help me understand
> why the index started at 1.

Hey, I take 0 being reserved for granted, and had to think about why
the context_xa was not 1-biased!

> >@@ -1021,35 +991,27 @@ static int get_ppgtt(struct drm_i915_file_private
> >*file_priv,
> >                    struct drm_i915_gem_context_param *args)
> > {
> >       struct i915_address_space *vm;
> >-      int ret;
> >+      int err = -ENODEV;
> >+      u32 id;
> >
> >       if (!rcu_access_pointer(ctx->vm))
> >               return -ENODEV;
> >
> >       rcu_read_lock();
> >       vm = context_get_vm_rcu(ctx);
> >+      if (vm)
> >+              err = xa_alloc(&file_priv->vm_xa, &id, vm,
> >+                             xa_limit_32b, GFP_KERNEL);
> >       rcu_read_unlock();
> >+      if (!err) {
> >+              i915_vm_open(vm);
> 
> Why did you switch to success path in the if here?

No good reason, just simple enough to fit inside one if {}.

> Can you do:
> 
> if (err)
>         goto err_put;
> 
> ?
> 
> >-      ret = mutex_lock_interruptible(&file_priv->vm_idr_lock);
> >-      if (ret)
> >-              goto err_put;
> >-
> >-      ret = idr_alloc(&file_priv->vm_idr, vm, 0, 0, GFP_KERNEL);
> >-      GEM_BUG_ON(!ret);
> >-      if (ret < 0)
> >-              goto err_unlock;
> >-
> >-      i915_vm_open(vm);
> >-
> >-      args->size = 0;
> >-      args->value = ret;
> >+              args->size = 0;
> >+              args->value = id;
> 
> Would passing args->value to the xa_alloc be a useful?

General rule is not to alter user params except on success. While not
always required, the pattern does help to avoid common pitfalls where
userspace has to repeat an ioctl (e.g. SIGINT).
-Chris