[Intel-gfx] [PATCH 06/11] drm/i915: Allocate active tracking nodes from a slabcache

[Chris Wilson]

Quoting Tvrtko Ursulin (2019-01-30 15:57:06)
> 
> On 30/01/2019 02:19, Chris Wilson wrote:
> > Wrap the active tracking for a GPU references in a slabcache for faster
> > allocations, and keep track of inflight nodes so we can reap the
> > stale entries upon parking (thereby trimming our memory usage).
> 
> I suggest a two staged approach. First patch add a slab cache (you can 
> also add kmem_cache_shrink on park as we do for other caches), then add 
> the parking/reaping bit.

Not really seeing the point, the lesson learnt is that we should be
tidying up on parking and that's why having a slab makes sense in the
first place.

I can see one argument to not do the idle reaping immediately and that's
if we apply the reaping on vma idle instead. Then parking is redundant.
 
> Under what scenarios we end up not freeing active nodes sufficiently? It 
> would have to be some user which keeps many contexts around, having only 
> used them once?

Never in a typical scenario :) Almost every allocation is served from
the last_request slot, you need to have a pair of concurrent references
to a vma for it to activate, and that takes multiple engines reading the
same object within a context/ppgtt.

> >   struct i915_active {
> > -     struct drm_i915_private *i915;
> > +     struct i915_gt_active *gt;
> 
> gt_active would be better - gt is to vague.

As a backpointer though it's defined by its owner. Give me a better name
for everything.

> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > index caccff87a2a1..2bc735df408b 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -130,6 +130,7 @@ static u32 __i915_gem_park(struct drm_i915_private *i915)
> >   
> >       intel_engines_park(i915);
> >       i915_timelines_park(i915);
> > +     i915_gt_active_park(i915_gt_active(i915));
> 
> The i915_gt_active macro is just too horrible IMHO. Why? :)

Because it was called gt.active_refs and that was irking me. I really
haven't settled on the name.

> Make i915_gt_active_park take i915, or i915->gt_active.

The pretense at encapsulation was nice, and I'd like to push harder on
that front.
-Chris