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

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 07/08/2020 12:14, Chris Wilson wrote:
> 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.

Ugh sorry then, I was thinking ctor was called on giving out the new object.

TYPESAFE_BY_RCU does have the advantage compared to kfree_rcu/call_rcu, 
but just please document in comments how the callers are using this.

Like with requests we have a clear kref_get_unless_zero via 
dma_fence_get_rcu, so we need to know what is the "equivalent" for 
intel_context. And which stale data can get accessed, by whom, and why 
it is safe.

Regards,

Tvrtko


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx