[Intel-gfx] [PATCH 2/7] drm/i915/gt: Protect context lifetime with RCU

[Chris Wilson]

Quoting Tvrtko Ursulin (2020-08-07 11:08:59)
> 
> On 07/08/2020 09:32, Chris Wilson wrote:
> > +static void __intel_context_ctor(void *arg)
> > +{
> > +     struct intel_context *ce = arg;
> > +
> > +     INIT_LIST_HEAD(&ce->signal_link);
> > +     INIT_LIST_HEAD(&ce->signals);
> > +
> > +     atomic_set(&ce->pin_count, 0);
> > +     mutex_init(&ce->pin_mutex);
> > +
> > +     ce->active_count = 0;
> > +     i915_active_init(&ce->active,
> > +                      __intel_context_active, __intel_context_retire);
> > +
> > +     ce->inflight = NULL;
> > +     ce->lrc_reg_state = NULL;
> > +     ce->lrc.desc = 0;
> > +}
> > +
> > +static void
> > +__intel_context_init(struct intel_context *ce, struct intel_engine_cs *engine)
> > +{
> > +     GEM_BUG_ON(!engine->cops);
> > +     GEM_BUG_ON(!engine->gt->vm);
> > +
> > +     kref_init(&ce->ref);
> > +     i915_active_reinit(&ce->active);
> > +
> > +     ce->engine = engine;
> > +     ce->ops = engine->cops;
> > +     ce->sseu = engine->sseu;
> > +
> > +     ce->wa_bb_page = 0;
> > +     ce->flags = 0;
> > +     ce->tag = 0;
> > +
> > +     memset(&ce->runtime, 0, sizeof(ce->runtime));
> > +
> > +     ce->vm = i915_vm_get(engine->gt->vm);
> > +     ce->gem_context = NULL;
> > +
> > +     ce->ring = __intel_context_ring_size(SZ_4K);
> > +     ce->timeline = NULL;
> > +     ce->state = NULL;
> > +
> > +     GEM_BUG_ON(atomic_read(&ce->pin_count));
> > +     GEM_BUG_ON(ce->active_count);
> > +     GEM_BUG_ON(ce->inflight);
> > +}
> > +
> > +void
> > +intel_context_init(struct intel_context *ce, struct intel_engine_cs *engine)
> > +{
> > +     __intel_context_ctor(ce);
> > +     __intel_context_init(ce, engine);
> 
> Which bits are accessed from outside context free (inside the RCU 
> period) and based on which ones is the decision taken in the caller that 
> the context is free so should be skipped?
> 
> And arent' both _ctor and _init fields re-initialized in the same go 
> with this approach (no RCU period between them) - that is - object can 
> get recycled instantly so what is the point in this case between the 
> _ctor and _init split?

ctor is once per slab-page allocation, init is once per object
allocation. Once upon a time it was said that "DESTROY_BY_RCU without ctor is
inherently wrong" (which was immediately contradicted by others), but
the point still resonates in that since the object may be reused within
an rcu grace period, one should consider what access may still be
inflight at that time. Here, I was just going through the motions of
minimising the amount we reset during init.

We don't have to use TYPESAFE_BY_RCU, it has some nice properties in
freelist management (at least without measuring they sound nice on
paper), we could just use add a manual call_rcu() to defer the
kmem_cache_free. Or we could just ignore the _ctor since we don't
yet have a conflicting access pattern.
-Chris