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

[Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxxxxxxx>]

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@xxxxxxxxxxxxxxxxxx>
> > > Cc: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
> > > ---
> > >    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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx