Re: [PATCH 11/22] drm/i915: Introduce a mutex for file_priv->context_idr

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 18/03/2019 16:35, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2019-03-18 16:28:35)
>>
>> On 18/03/2019 09:51, Chris Wilson wrote:
>>> Define a mutex for the exclusive use of interacting with the per-file
>>> context-idr, that was previously guarded by struct_mutex. This allows us
>>> to reduce the coverage of struct_mutex, with a view to removing the last
>>> bits coordinating GEM context later. (In the short term, we avoid taking
>>> struct_mutex while using the extended constructor functions, preventing
>>> some nasty recursion.)
>>>
>>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>> ---
>>>    drivers/gpu/drm/i915/i915_drv.h         |  2 ++
>>>    drivers/gpu/drm/i915/i915_gem_context.c | 43 +++++++++++--------------
>>>    2 files changed, 21 insertions(+), 24 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>>> index 86080a6e0f45..90389333dd47 100644
>>> --- a/drivers/gpu/drm/i915/i915_drv.h
>>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>>> @@ -216,7 +216,9 @@ struct drm_i915_file_private {
>>>     */
>>>    #define DRM_I915_THROTTLE_JIFFIES msecs_to_jiffies(20)
>>>        } mm;
>>> +
>>>        struct idr context_idr;
>>> +     struct mutex context_lock; /* guards context_idr */
>>
>> context_idr_lock then?
>>
>>>    
>>>        unsigned int bsd_engine;
>>>    
>>> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
>>> index 5df3d423ec6c..94c466d4b29e 100644
>>> --- a/drivers/gpu/drm/i915/i915_gem_context.c
>>> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
>>> @@ -579,9 +579,7 @@ void i915_gem_contexts_fini(struct drm_i915_private *i915)
>>>    
>>>    static int context_idr_cleanup(int id, void *p, void *data)
>>>    {
>>> -     struct i915_gem_context *ctx = p;
>>> -
>>> -     context_close(ctx);
>>> +     context_close(p);
>>>        return 0;
>>>    }
>>>    
>>> @@ -596,8 +594,10 @@ static int gem_context_register(struct i915_gem_context *ctx,
>>>                ctx->ppgtt->vm.file = fpriv;
>>>    
>>>        /* And (nearly) finally expose ourselves to userspace via the idr */
>>> +     mutex_lock(&fpriv->context_lock);
>>>        ret = idr_alloc(&fpriv->context_idr, ctx,
>>>                        DEFAULT_CONTEXT_HANDLE, 0, GFP_KERNEL);
>>> +     mutex_unlock(&fpriv->context_lock);
>>>        if (ret < 0)
>>>                goto err_pid;
>>>    
>>> @@ -616,7 +616,9 @@ static int gem_context_register(struct i915_gem_context *ctx,
>>>        return 0;
>>>    
>>>    err_idr:
>>> +     mutex_lock(&fpriv->context_lock);
>>>        idr_remove(&fpriv->context_idr, ctx->user_handle);
>>> +     mutex_unlock(&fpriv->context_lock);
>>>        ctx->file_priv = NULL;
>>>    err_pid:
>>>        put_pid(ctx->pid);
>>> @@ -632,10 +634,11 @@ int i915_gem_context_open(struct drm_i915_private *i915,
>>>        int err;
>>>    
>>>        idr_init(&file_priv->context_idr);
>>> +     mutex_init(&file_priv->context_lock);
>>>    
>>>        mutex_lock(&i915->drm.struct_mutex);
>>> -
>>>        ctx = i915_gem_create_context(i915);
>>> +     mutex_unlock(&i915->drm.struct_mutex);
>>>        if (IS_ERR(ctx)) {
>>>                err = PTR_ERR(ctx);
>>>                goto err;
>>> @@ -648,14 +651,14 @@ int i915_gem_context_open(struct drm_i915_private *i915,
>>>        GEM_BUG_ON(ctx->user_handle != DEFAULT_CONTEXT_HANDLE);
>>>        GEM_BUG_ON(i915_gem_context_is_kernel(ctx));
>>>    
>>> -     mutex_unlock(&i915->drm.struct_mutex);
>>> -
>>>        return 0;
>>>    
>>>    err_ctx:
>>> +     mutex_lock(&i915->drm.struct_mutex);
>>>        context_close(ctx);
>>> -err:
>>>        mutex_unlock(&i915->drm.struct_mutex);
>>> +err:
>>> +     mutex_destroy(&file_priv->context_lock);
>>>        idr_destroy(&file_priv->context_idr);
>>>        return PTR_ERR(ctx);
>>>    }
>>> @@ -668,6 +671,7 @@ void i915_gem_context_close(struct drm_file *file)
>>>    
>>>        idr_for_each(&file_priv->context_idr, context_idr_cleanup, NULL);
>>>        idr_destroy(&file_priv->context_idr);
>>> +     mutex_destroy(&file_priv->context_lock);
>>>    }
>>>    
>>>    static struct i915_request *
>>> @@ -850,25 +854,22 @@ int i915_gem_context_create_ioctl(struct drm_device *dev, void *data,
>>>                return ret;
>>>    
>>>        ctx = i915_gem_create_context(i915);
>>> -     if (IS_ERR(ctx)) {
>>> -             ret = PTR_ERR(ctx);
>>> -             goto err_unlock;
>>> -     }
>>> +     mutex_unlock(&dev->struct_mutex);
>>> +     if (IS_ERR(ctx))
>>> +             return PTR_ERR(ctx);
>>>    
>>>        ret = gem_context_register(ctx, file_priv);
>>>        if (ret)
>>>                goto err_ctx;
>>>    
>>> -     mutex_unlock(&dev->struct_mutex);
>>> -
>>>        args->ctx_id = ctx->user_handle;
>>>        DRM_DEBUG("HW context %d created\n", args->ctx_id);
>>>    
>>>        return 0;
>>>    
>>>    err_ctx:
>>> +     mutex_lock(&dev->struct_mutex);
>>>        context_close(ctx);
>>> -err_unlock:
>>>        mutex_unlock(&dev->struct_mutex);
>>>        return ret;
>>>    }
>>> @@ -879,7 +880,6 @@ int i915_gem_context_destroy_ioctl(struct drm_device *dev, void *data,
>>>        struct drm_i915_gem_context_destroy *args = data;
>>>        struct drm_i915_file_private *file_priv = file->driver_priv;
>>>        struct i915_gem_context *ctx;
>>> -     int ret;
>>>    
>>>        if (args->pad != 0)
>>>                return -EINVAL;
>>> @@ -887,21 +887,16 @@ int i915_gem_context_destroy_ioctl(struct drm_device *dev, void *data,
>>>        if (args->ctx_id == DEFAULT_CONTEXT_HANDLE)
>>>                return -ENOENT;
>>>    
>>> -     ctx = i915_gem_context_lookup(file_priv, args->ctx_id);
>>> +     mutex_lock(&file_priv->context_lock);
>>> +     ctx = idr_remove(&file_priv->context_idr, args->ctx_id);
>>> +     mutex_lock(&file_priv->context_lock);
>>>        if (!ctx)
>>>                return -ENOENT;
>>>    
>>> -     ret = mutex_lock_interruptible(&dev->struct_mutex);
>>> -     if (ret)
>>> -             goto out;
>>> -
>>> -     idr_remove(&file_priv->context_idr, ctx->user_handle);
>>> +     mutex_lock(&dev->struct_mutex);
>>
>> I'd keep this one interruptible. Hm bummer, there was more of them before..
> 
> At this point, interrupt handling becomes problematic, as we have to
> then re-insert the ctx_id into the idr and that may have already been
> claimed elsewhere.

Ugh, bad.. Can we have struct_mutex nest under the context_idr_lock?

Regards,

Tvrtko

>> I mean the new mutex we can probably get away by not bothering, since it
>> guards so little, but struct mutex, since you are touching those lines
>> anyway, what do you think about making it interruptible in all ioctl paths?
> 
> Not practical imo; but we won't need struct_mutex here in about another 20
> patches :)
> -Chris
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx