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

[Ruhl, Michael J]

>-----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.

Thanks,

Mike

>-Chris