Re: [PATCH] drm/i915: Reduce context HW ID lifetime

[Chris Wilson <chris@chris-wilson.co.uk>]

Quoting Tvrtko Ursulin (2018-08-30 17:23:43)
> 
> On 30/08/2018 11:24, Chris Wilson wrote:
> > +static int steal_hw_id(struct drm_i915_private *i915)
> > +{
> > +     struct i915_gem_context *ctx, *cn;
> > +     LIST_HEAD(pinned);
> > +     int id = -ENOSPC;
> > +
> > +     lockdep_assert_held(&i915->contexts.mutex);
> > +
> > +     list_for_each_entry_safe(ctx, cn,
> > +                              &i915->contexts.hw_id_list, hw_id_link) {
> > +             if (atomic_read(&ctx->pin_hw_id)) {
> > +                     list_move_tail(&ctx->hw_id_link, &pinned);
> > +                     continue;
> > +             }
> > +
> > +             GEM_BUG_ON(!ctx->hw_id); /* perma-pinned kernel context */
> > +             list_del_init(&ctx->hw_id_link);
> > +             id = ctx->hw_id;
> > +             break;
> > +     }
> > +
> > +     list_splice_tail(&pinned, &i915->contexts.hw_id_list);
> 
> Put a comment what is this code doing please. Trying to create some sort 
> of LRU order?

LRSearched. Same as the shrinker, and eviction code if you would also
review that ;)

> 
> > +     return id;
> > +}
> > +
> > +static int assign_hw_id(struct drm_i915_private *i915, unsigned int *out)
> > +{
> > +     int ret;
> > +
> > +     lockdep_assert_held(&i915->contexts.mutex);
> > +
> > +     ret = new_hw_id(i915, GFP_KERNEL | __GFP_RETRY_MAYFAIL | __GFP_NOWARN);
> > +     if (unlikely(ret < 0)) {
> > +             ret = steal_hw_id(i915);
> > +             if (ret < 0) /* once again for the correct erro code */
> 
> errno
> 
> > +                     ret = new_hw_id(i915, GFP_KERNEL);
> 
> Hmm.. shouldn't you try GFP_KERNEL before attempting to steal? Actually 
> I think you should branch based on -ENOSPC (steal) vs -ENOMEM (retry 
> with GFP_KERNEL). Which would actually mean something like:

I was applying the same strategy as we use elsewhere. Penalise any
driver cache before hitting reclaim.

I think that is fair from an application of soft backpressure point of
view. (Lack of backpressure is probably a sore point for many.)

> > -     ret = ida_simple_get(&dev_priv->contexts.hw_ida,
> > -                          0, max, GFP_KERNEL);
> 
> Although now that I see this I am struggling not to say the change to 
> try a lighter weight allocation strategy first (gfp may fail) needs to 
> be split out to a separate patch.

Pardon? I appear to suddenly be hard of hearing.

The patch was all about the steal_hw_id().

> > -     if (ret < 0) {
> > -             /* Contexts are only released when no longer active.
> > -              * Flush any pending retires to hopefully release some
> > -              * stale contexts and try again.
> > -              */
> > -             i915_retire_requests(dev_priv);
> > -             ret = ida_simple_get(&dev_priv->contexts.hw_ida,
> > -                                  0, max, GFP_KERNEL);
> > -             if (ret < 0)
> > -                     return ret;
> > -     }
> > -
> > -     *out = ret;
> > -     return 0;
> > -}
> > -
> >   static u32 default_desc_template(const struct drm_i915_private *i915,
> >                                const struct i915_hw_ppgtt *ppgtt)
> >   {
> > @@ -276,12 +324,6 @@ __create_hw_context(struct drm_i915_private *dev_priv,
> >       if (ctx == NULL)
> >               return ERR_PTR(-ENOMEM);
> >   
> > -     ret = assign_hw_id(dev_priv, &ctx->hw_id);
> > -     if (ret) {
> > -             kfree(ctx);
> > -             return ERR_PTR(ret);
> > -     }
> > -
> >       kref_init(&ctx->ref);
> >       list_add_tail(&ctx->link, &dev_priv->contexts.list);
> >       ctx->i915 = dev_priv;
> > @@ -295,6 +337,7 @@ __create_hw_context(struct drm_i915_private *dev_priv,
> >   
> >       INIT_RADIX_TREE(&ctx->handles_vma, GFP_KERNEL);
> >       INIT_LIST_HEAD(&ctx->handles_list);
> > +     INIT_LIST_HEAD(&ctx->hw_id_link);
> >   
> >       /* Default context will never have a file_priv */
> >       ret = DEFAULT_CONTEXT_HANDLE;
> > @@ -421,15 +464,35 @@ i915_gem_context_create_gvt(struct drm_device *dev)
> >       return ctx;
> >   }
> >   
> > +static void
> > +destroy_kernel_context(struct i915_gem_context **ctxp)
> > +{
> > +     struct i915_gem_context *ctx;
> > +
> > +     /* Keep the context ref so that we can free it immediately ourselves */
> > +     ctx = i915_gem_context_get(fetch_and_zero(ctxp));
> > +     GEM_BUG_ON(!i915_gem_context_is_kernel(ctx));
> > +
> > +     context_close(ctx);
> > +     i915_gem_context_free(ctx);
> > +}
> > +
> >   struct i915_gem_context *
> >   i915_gem_context_create_kernel(struct drm_i915_private *i915, int prio)
> >   {
> >       struct i915_gem_context *ctx;
> > +     int err;
> >   
> >       ctx = i915_gem_create_context(i915, NULL);
> >       if (IS_ERR(ctx))
> >               return ctx;
> >   
> > +     err = i915_gem_context_pin_hw_id(ctx);
> > +     if (err) {
> > +             destroy_kernel_context(&ctx);
> > +             return ERR_PTR(err);
> > +     }
> > +
> >       i915_gem_context_clear_bannable(ctx);
> >       ctx->sched.priority = prio;
> >       ctx->ring_size = PAGE_SIZE;
> > @@ -439,17 +502,19 @@ i915_gem_context_create_kernel(struct drm_i915_private *i915, int prio)
> >       return ctx;
> >   }
> >   
> > -static void
> > -destroy_kernel_context(struct i915_gem_context **ctxp)
> > +static void init_contexts(struct drm_i915_private *i915)
> >   {
> > -     struct i915_gem_context *ctx;
> > +     mutex_init(&i915->contexts.mutex);
> > +     INIT_LIST_HEAD(&i915->contexts.list);
> >   
> > -     /* Keep the context ref so that we can free it immediately ourselves */
> > -     ctx = i915_gem_context_get(fetch_and_zero(ctxp));
> > -     GEM_BUG_ON(!i915_gem_context_is_kernel(ctx));
> > +     /* Using the simple ida interface, the max is limited by sizeof(int) */
> > +     BUILD_BUG_ON(MAX_CONTEXT_HW_ID > INT_MAX);
> > +     BUILD_BUG_ON(GEN11_MAX_CONTEXT_HW_ID > INT_MAX);
> > +     ida_init(&i915->contexts.hw_ida);
> > +     INIT_LIST_HEAD(&i915->contexts.hw_id_list);
> >   
> > -     context_close(ctx);
> > -     i915_gem_context_free(ctx);
> > +     INIT_WORK(&i915->contexts.free_work, contexts_free_worker);
> > +     init_llist_head(&i915->contexts.free_list);
> 
> ugh diff.. :) looks like pure movement from perspective of 
> destroy_kernel_context.
> 
> >   }
> >   
> >   static bool needs_preempt_context(struct drm_i915_private *i915)
> > @@ -470,14 +535,7 @@ int i915_gem_contexts_init(struct drm_i915_private *dev_priv)
> >       if (ret)
> >               return ret;
> >   
> > -     INIT_LIST_HEAD(&dev_priv->contexts.list);
> > -     INIT_WORK(&dev_priv->contexts.free_work, contexts_free_worker);
> > -     init_llist_head(&dev_priv->contexts.free_list);
> > -
> > -     /* Using the simple ida interface, the max is limited by sizeof(int) */
> > -     BUILD_BUG_ON(MAX_CONTEXT_HW_ID > INT_MAX);
> > -     BUILD_BUG_ON(GEN11_MAX_CONTEXT_HW_ID > INT_MAX);
> > -     ida_init(&dev_priv->contexts.hw_ida);
> > +     init_contexts(dev_priv);
> >   
> >       /* lowest priority; idle task */
> >       ctx = i915_gem_context_create_kernel(dev_priv, I915_PRIORITY_MIN);
> > @@ -490,6 +548,7 @@ int i915_gem_contexts_init(struct drm_i915_private *dev_priv)
> >        * all user contexts will have non-zero hw_id.
> >        */
> >       GEM_BUG_ON(ctx->hw_id);
> > +     GEM_BUG_ON(!atomic_read(&ctx->pin_hw_id));
> 
> /* Kernel context is perma-pinned */
> 
> >       dev_priv->kernel_context = ctx;
> >   
> >       /* highest priority; preempting task */
> > @@ -527,6 +586,7 @@ void i915_gem_contexts_fini(struct drm_i915_private *i915)
> >       destroy_kernel_context(&i915->kernel_context);
> >   
> >       /* Must free all deferred contexts (via flush_workqueue) first */
> > +     GEM_BUG_ON(!list_empty(&i915->contexts.hw_id_list));
> >       ida_destroy(&i915->contexts.hw_ida);
> >   }
> >   
> > @@ -932,6 +992,33 @@ int i915_gem_context_reset_stats_ioctl(struct drm_device *dev,
> >       return ret;
> >   }
> >   
> > +int __i915_gem_context_pin_hw_id(struct i915_gem_context *ctx)
> > +{
> > +     struct drm_i915_private *i915 = ctx->i915;
> > +     int err = 0;
> > +
> > +     mutex_lock(&i915->contexts.mutex);
> > +
> > +     GEM_BUG_ON(i915_gem_context_is_closed(ctx));
> > +
> > +     if (list_empty(&ctx->hw_id_link)) {
> > +             GEM_BUG_ON(atomic_read(&ctx->pin_hw_id));
> > +
> > +             err = assign_hw_id(i915, &ctx->hw_id);
> > +             if (err)
> > +                     goto out_unlock;
> > +
> > +             list_add_tail(&ctx->hw_id_link, &i915->contexts.hw_id_list);
> > +     }
> > +
> > +     GEM_BUG_ON(atomic_read(&ctx->pin_hw_id) == ~0u);
> > +     atomic_inc(&ctx->pin_hw_id);
> > +
> > +out_unlock:
> > +     mutex_unlock(&i915->contexts.mutex);
> > +     return err;
> > +}
> > +
> >   #if IS_ENABLED(CONFIG_DRM_I915_SELFTEST)
> >   #include "selftests/mock_context.c"
> >   #include "selftests/i915_gem_context.c"
> > diff --git a/drivers/gpu/drm/i915/i915_gem_context.h b/drivers/gpu/drm/i915/i915_gem_context.h
> > index 851dad6decd7..c73ac614f58c 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_context.h
> > +++ b/drivers/gpu/drm/i915/i915_gem_context.h
> > @@ -136,6 +136,8 @@ struct i915_gem_context {
> >        * id for the lifetime of the context.
> >        */
> >       unsigned int hw_id;
> > +     atomic_t pin_hw_id;
> 
> I think now we need short comments describing the difference between the 
> two.

One is 32bits unsigned, unserialised. The other is 32bits signed, and
very loosely serialised :)

> > +     struct list_head hw_id_link;
> 
> And for this one.

[snip]

> So in essence there will be a little bit more cost when pinning in the 
> normal case, or a bit bit more in the stealing/pathological case, but as 
> long as we stay below over-subscription the cost is only on first pin. 
> No complaints there. Debug also won't be confusing in the normal case 
> since numbers will be stable.

Yup. Nice addition to the changelog, thanks.
 
> Does it have any negative connotations in the world of OA is the 
> question for Lionel?

Lionel kept promising me this was ok, that he/gputop was quite ready for
shorter lived ctx id, and reuse.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx