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

[Chris Wilson]

Quoting Ruhl, Michael J (2020-01-22 16:15:17)
> 
> 
> >-----Original Message-----
> >From: Chris Wilson <chris at chris-wilson.co.uk>
> >Sent: Wednesday, January 22, 2020 11:09 AM
> >To: Ruhl, Michael J <michael.j.ruhl at intel.com>; intel-
> >gfx at lists.freedesktop.org
> >Cc: Auld, Matthew <matthew.auld at intel.com>
> >Subject: RE: [Intel-gfx] [PATCH 4/5] drm/i915/gem: Convert vm idr to xarray
> >
> >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).
> 
> Yup.  Following that rule, xa_array() will only update the value on success.
> 
> I was mostly commenting on it because you had done that in the previous
> xa_alloc call.

Went back and changed that so both paths look very similar, begging to
be refactored...
-Chris