[Intel-gfx] [PATCH 23/24] drm/i915: Keep a recent cache of freed contexts objects for reuse

[Chris Wilson]

On Thu, May 18, 2017 at 02:52:41PM +0100, Tvrtko Ursulin wrote:
> 
> On 18/05/2017 10:46, Chris Wilson wrote:
> >Keep the recently freed context objects for reuse. This allows us to use
> >the current GGTT bindings and dma bound pages, avoiding any clflushes as
> >required. We mark the objects as purgeable under memory pressure, and
> >reap the list of freed objects as soon as the device is idle.
> 
> Some description on when is this interesting and by how much?

No interesting workload (a few synthetic benchmarks including the GL).
Best argument would be a minor improvement in application startup.

> Since you drop everything on idle it must be for some workload which
> likes to go through context while keeping busy?

Yup, it's just the rule of thumb I have (from experience with the
cmdparser batch caching).
 
> >Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> >---
> > drivers/gpu/drm/i915/i915_drv.h                  |  2 +
> > drivers/gpu/drm/i915/i915_gem.c                  |  1 +
> > drivers/gpu/drm/i915/i915_gem_context.c          | 59 ++++++++++++++++++++++--
> > drivers/gpu/drm/i915/i915_gem_context.h          |  5 ++
> > drivers/gpu/drm/i915/intel_lrc.c                 |  2 +-
> > drivers/gpu/drm/i915/intel_ringbuffer.c          |  2 +-
> > drivers/gpu/drm/i915/selftests/mock_context.c    |  1 +
> > drivers/gpu/drm/i915/selftests/mock_gem_device.c |  1 +
> > 8 files changed, 68 insertions(+), 5 deletions(-)
> >
> >diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> >index 1d55bbde68df..1fa1e7d48f02 100644
> >--- a/drivers/gpu/drm/i915/i915_drv.h
> >+++ b/drivers/gpu/drm/i915/i915_drv.h
> >@@ -2316,6 +2316,8 @@ struct drm_i915_private {
> > 		struct llist_head free_list;
> > 		struct work_struct free_work;
> >
> >+		struct list_head freed_objects;
> 
> free_ctx_objects maybe?

This is i915->contexts.freed_objects (new contexts substruct). There is
already a precedent for using freed_ (mainly because I think its a
better description for that phase, free is too similar to avail,
although you may persuade me that free is better).

> Knowing you I am surprised this is not bucketed! :)

I didn't bother since we only really have two sizes - also reason why I
didn't make it per-engine so that we could share the xcs context
objects. My expectation is that we won't be caught up in slow list
searches, so went for simplicity for a change.

> >+struct drm_i915_gem_object *
> >+i915_gem_context_create_object(struct drm_i915_private *i915,
> >+			       unsigned long size)
> >+{
> >+	struct drm_i915_gem_object *obj, *on;
> >+
> >+	lockdep_assert_held(&i915->drm.struct_mutex);
> >+
> >+	list_for_each_entry_safe(obj, on,
> >+				 &i915->contexts.freed_objects,
> >+				 batch_pool_link) {
> >+		if (obj->mm.madv != I915_MADV_DONTNEED) {
> >+			/* Purge the heretic! */
> 
> I don't see this can happen in the current version?

The power of the shrinker. Remember it can and will stab you in the back
at any time.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre