Quoting Tvrtko Ursulin (2019-09-18 16:54:36)
>
> On 17/09/2019 16:17, Chris Wilson wrote:
> > Quoting Tvrtko Ursulin (2019-09-17 15:59:25)
> >>
> >> On 16/09/2019 12:38, Chris Wilson wrote:
> >>> When using virtual engines, the rq->engine is not stable until we hold
> >>> the engine->active.lock (as the virtual engine may be exchanged with the
> >>> sibling). Since commit 22b7a426bbe1 ("drm/i915/execlists: Preempt-to-busy")
> >>> we may retire a request concurrently with resubmitting it to HW, we need
> >>> to be extra careful to verify we are holding the correct lock for the
> >>> request's active list. This is similar to the issue we saw with
> >>> rescheduling the virtual requests, see sched_lock_engine().
> >>>
> >>> Or else:
> >>>
> >>> <4> [876.736126] list_add corruption. prev->next should be next (ffff8883f931a1f8), but was dead000000000100. (prev=ffff888361ffa610).
...
> >>> <4> [876.736415] list_del corruption. prev->next should be ffff888361ffca10, but was ffff88840ac2c730
> > Yes. So preempt-to-busy introduces a window where the request is still
> > on HW but we have returned it back to the submission queue. We catch up
> > with the HW on the next process_csb, but it may have completed the
> > request in the mean time (it is just not allowed to advance beyond the
> > subsequent breadcrumb and so prevented from overtaking our knowledge of
> > RING_TAIL and so we avoid telling the HW to go "backwards".).
>
> Would it be sufficient to do:
>
> engine = READ_ONCE(rq->engine);
> spin_lock(...);
> list_del(...);
> spin_unlock(engine->active.lock);
>
> To ensure the same engine is used? Although the oops is not about
> spinlock but list corruption. How does the list get corrupt though?
> list_del does not care on which list the request is.. If it is really
> key to have the correct lock, then why it is enough to re-check the
> engine after taking the lock? Why rq->engine couldn't change under the
> lock again? rq->engine does get updated under the very lock, no?
Don't forget that list_del changes the list around it:
list_del() {
list->prev->next = list->next;
list->next->prev = list->prev;
}
rq->engine can't change under the real->active.lock, as the assignment
to rq->engine = (virtual, real) is made under the real->active.lock.
execlists_dequeue:
real->active.lock
ve->active.lock
__unwind_incomplete_requests:
real->active.lock
Hmm. I trust the trick employed in the patch is well proven by this
point, but if we took the nested ve lock inside __unwind, do we need to
worry. Hmm.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
