[Intel-gfx] [PATCH 06/16] drm/i915/gt: Decouple completed requests on unwind

[Tvrtko Ursulin]

On 24/11/2020 17:31, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2020-11-24 17:13:02)
>>
>> On 24/11/2020 11:42, Chris Wilson wrote:
>>> Since the introduction of preempt-to-busy, requests can complete in the
>>> background, even while they are not on the engine->active.requests list.
>>> As such, the engine->active.request list itself is not in strict
>>> retirement order, and we have to scan the entire list while unwinding to
>>> not miss any. However, if the request is completed we currently leave it
>>> on the list [until retirement], but we could just as simply remove it
>>> and stop treating it as active. We would only have to then traverse it
>>> once while unwinding in quick succession.
>>>
>>> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
>>> ---
>>>    drivers/gpu/drm/i915/gt/intel_lrc.c | 6 ++++--
>>>    drivers/gpu/drm/i915/i915_request.c | 3 ++-
>>>    2 files changed, 6 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
>>> index 30aa59fb7271..cf11cbac241b 100644
>>> --- a/drivers/gpu/drm/i915/gt/intel_lrc.c
>>> +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
>>> @@ -1116,8 +1116,10 @@ __unwind_incomplete_requests(struct intel_engine_cs *engine)
>>>        list_for_each_entry_safe_reverse(rq, rn,
>>>                                         &engine->active.requests,
>>>                                         sched.link) {
>>> -             if (i915_request_completed(rq))
>>> -                     continue; /* XXX */
>>> +             if (i915_request_completed(rq)) {
>>> +                     list_del_init(&rq->sched.link);
>>> +                     continue;
>>> +             }
>>>    
>>>                __i915_request_unsubmit(rq);
>>>    
>>> diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
>>> index 8d7d29c9e375..a9db1376b996 100644
>>> --- a/drivers/gpu/drm/i915/i915_request.c
>>> +++ b/drivers/gpu/drm/i915/i915_request.c
>>> @@ -321,7 +321,8 @@ bool i915_request_retire(struct i915_request *rq)
>>>         * after removing the breadcrumb and signaling it, so that we do not
>>>         * inadvertently attach the breadcrumb to a completed request.
>>>         */
>>> -     remove_from_engine(rq);
>>> +     if (!list_empty(&rq->sched.link))
>>> +             remove_from_engine(rq);
>>
>> The list_empty check is unlocked so is list_del_init in
>> remove_from_engine safe on potentially already unlinked request or it
>> needs to re-check under the lock?
> 
> It's safe. The unwind is under the lock, and remove_from_engine takes
> the lock, and both do list_del_init() which is a no-op if already
> removed. And the end state of the flag bits is the same on each path. We
> can skip the __notify_execute_cb_imm() since we know in unwind it is
> executing and there should be no cb.
> 
> The test before we take the lock is only allowed to skip the active.lock
> if it sees the list is already decoupled, in which case we can leave it
> to the unwind to remove it from the engine (and we know that the request
> can only have been inflight prior to completion). Since the test is not
> locked, we don't serialise with the removal, but the list_del_init is
> the last action on the request so there is no window where the unwind is
> accessing the request after it may have been retired.
> 
> list_move() will not confuse list_empty(), as although it does a
> list_del_entry, it is not temporarily re-initialised to an empty list.

List_del_init is indeed safe. List_move.. which one you think can race 
with retire? Preempt-to-busy unwinding an almost completed request yet 
again? Or even preempt timeout racing with completion?

Regards,

Tvrtko