Re: [Intel-gfx] [PATCH 14/28] drm/i915/gt: Free stale request on destroying the virtual engine

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

On 18/11/2020 11:24, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2020-11-18 11:05:24)
>>
>> On 17/11/2020 11:30, Chris Wilson wrote:
>>> Since preempt-to-busy, we may unsubmit a request while it is still on
>>> the HW and completes asynchronously. That means it may be retired and in
>>> the process destroy the virtual engine (as the user has closed their
>>> context), but that engine may still be holding onto the unsubmitted
>>> compelted request. Therefore we need to potentially cleanup the old
>>> request on destroying the virtual engine. We also have to keep the
>>> virtual_engine alive until after the sibling's execlists_dequeue() have
>>> finished peeking into the virtual engines, for which we serialise with
>>> RCU.
>>>
>>> v2: Be paranoid and flush the tasklet as well.
>>> v3: And flush the tasklet before the engines, as the tasklet may
>>> re-attach an rb_node after our removal from the siblings.
>>>
>>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>> ---
>>>    drivers/gpu/drm/i915/gt/intel_lrc.c | 61 +++++++++++++++++++++++++----
>>>    1 file changed, 54 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
>>> index 17cb7060eb29..c11433884cf6 100644
>>> --- a/drivers/gpu/drm/i915/gt/intel_lrc.c
>>> +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
>>> @@ -182,6 +182,7 @@
>>>    struct virtual_engine {
>>>        struct intel_engine_cs base;
>>>        struct intel_context context;
>>> +     struct rcu_work rcu;
>>>    
>>>        /*
>>>         * We allow only a single request through the virtual engine at a time
>>> @@ -5470,44 +5471,90 @@ static struct list_head *virtual_queue(struct virtual_engine *ve)
>>>        return &ve->base.execlists.default_priolist.requests[0];
>>>    }
>>>    
>>> -static void virtual_context_destroy(struct kref *kref)
>>> +static void rcu_virtual_context_destroy(struct work_struct *wrk)
>>>    {
>>>        struct virtual_engine *ve =
>>> -             container_of(kref, typeof(*ve), context.ref);
>>> +             container_of(wrk, typeof(*ve), rcu.work);
>>>        unsigned int n;
>>>    
>>> -     GEM_BUG_ON(!list_empty(virtual_queue(ve)));
>>> -     GEM_BUG_ON(ve->request);
>>>        GEM_BUG_ON(ve->context.inflight);
>>>    
>>> +     /* Preempt-to-busy may leave a stale request behind. */
>>> +     if (unlikely(ve->request)) {
>>> +             struct i915_request *old;
>>> +
>>> +             spin_lock_irq(&ve->base.active.lock);
>>> +
>>> +             old = fetch_and_zero(&ve->request);
>>> +             if (old) {
>>> +                     GEM_BUG_ON(!i915_request_completed(old));
>>> +                     __i915_request_submit(old);
>>> +                     i915_request_put(old);
>>> +             }
>>> +
>>> +             spin_unlock_irq(&ve->base.active.lock);
>>> +     }
>>> +
>>> +     /*
>>> +      * Flush the tasklet in case it is still running on another core.
>>> +      *
>>> +      * This needs to be done before we remove ourselves from the siblings'
>>> +      * rbtrees as in the case it is running in parallel, it may reinsert
>>> +      * the rb_node into a sibling.
>>> +      */
>>> +     tasklet_kill(&ve->base.execlists.tasklet);
>>
>> Can it still be running after an RCU period?
> 
> I think there is a window between checking to see if the request is
> completed and kicking the tasklet, that is not under the rcu lock and
> opportunity for the request to be retired, and barrier flushed to drop
> the context references.

 From where would that check come?

> I observed the leaked ve->request, but the tasklet_kill, iirc, is
> speculation about possible windows. Admittedly all long test runs have
> been with this patch in place for most of the last year.
> 
>>> +     /* Decouple ourselves from the siblings, no more access allowed. */
>>>        for (n = 0; n < ve->num_siblings; n++) {
>>>                struct intel_engine_cs *sibling = ve->siblings[n];
>>>                struct rb_node *node = &ve->nodes[sibling->id].rb;
>>> -             unsigned long flags;
>>>    
>>>                if (RB_EMPTY_NODE(node))
>>>                        continue;
>>>    
>>> -             spin_lock_irqsave(&sibling->active.lock, flags);
>>> +             spin_lock_irq(&sibling->active.lock);
>>>    
>>>                /* Detachment is lazily performed in the execlists tasklet */
>>>                if (!RB_EMPTY_NODE(node))
>>>                        rb_erase_cached(node, &sibling->execlists.virtual);
>>>    
>>> -             spin_unlock_irqrestore(&sibling->active.lock, flags);
>>> +             spin_unlock_irq(&sibling->active.lock);
>>>        }
>>>        GEM_BUG_ON(__tasklet_is_scheduled(&ve->base.execlists.tasklet));
>>> +     GEM_BUG_ON(!list_empty(virtual_queue(ve)));
>>>    
>>>        if (ve->context.state)
>>>                __execlists_context_fini(&ve->context);
>>>        intel_context_fini(&ve->context);
>>>    
>>>        intel_engine_free_request_pool(&ve->base);
>>> +     intel_breadcrumbs_free(ve->base.breadcrumbs);
>>
>> This looks to belong to some other patch.
> 
> Some might say I was fixing up an earlier oversight.

Separate patch would be good, with Fixes: probably since it is a memory 
leak and one liner.

Regards,

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