[Intel-gfx] [PATCH 2/2] drm/i915: Move stale context reaping to common i915_gem_context_create

[Chris Wilson]

Quoting Tvrtko Ursulin (2017-07-05 14:50:57)
> 
> On 05/07/2017 14:18, Chris Wilson wrote:
> > We need to reap the stale contexts for all new contexts, be they created
> > by user in i915_gem_context_ioctl or from opening a new file in
> > i915_gem_context_open. Both paths may be called very frequently
> > accumulating many stale contexts before any worker has a chance to run
> > and free their memory.
> > 
> > Fixes: 1acfc104cdf8 ("drm/i915: Enable rcu-only context lookups")
> > Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> > Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> > ---
> >   drivers/gpu/drm/i915/i915_gem_context.c | 8 ++++----
> >   1 file changed, 4 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> > index 98d2ce98f467..c58a95c33c25 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_context.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> > @@ -383,6 +383,10 @@ i915_gem_create_context(struct drm_i915_private *dev_priv,
> >   
> >       lockdep_assert_held(&dev_priv->drm.struct_mutex);
> >   
> > +     /* Reap stale contexts */
> > +     i915_gem_retire_requests(dev_priv);
> > +     contexts_free(dev_priv);
> > +
> >       ctx = __create_hw_context(dev_priv, file_priv);
> >       if (IS_ERR(ctx))
> >               return ctx;
> > @@ -989,10 +993,6 @@ int i915_gem_context_create_ioctl(struct drm_device *dev, void *data,
> >       if (ret)
> >               return ret;
> >   
> > -     /* Reap stale contexts */
> > -     i915_gem_retire_requests(dev_priv);
> > -     contexts_free(dev_priv);
> > -
> >       ctx = i915_gem_create_context(dev_priv, file_priv);
> >       mutex_unlock(&dev->struct_mutex);
> >       if (IS_ERR(ctx))
> > 
> 
> Can't make myself concerned about the context creation slowdown with 
> this solution. I guess it highlights the same old RCU/delayed worker 
> issues, but it could be an option to somehow generalize these things 
> based on some threshold, like:

We are just moving the cost from delete (where we don't care that much)
to create (where we do care a bit more). However, the cost of context
destruction isn't that great and the likelihood of there being contexts
to destroy is marginal.

Retiring requests is overkill really, doing so won't immediately give us
contexts to free (since they will instead go onto the RCU list) so that
is the first thing to delete if we are concerned.

> maybe_pruge_deleted_contexts()
> {
>         if (nr_contexts_on_free_list > some_threshold) {
>                 retire...
>                 free...
>         }
> }

If pushed, I think something like where we only free the first in the
list just to keep a cap (one in, one out) on the list. Let's give that
a try... But first I'm going to see if Tomi is finally happy again.
-Chris