On 15/03/2017 19:10, Chris Wilson wrote:
On Wed, Mar 15, 2017 at 06:58:27PM +0000, Tvrtko Ursulin wrote:
On 15/03/2017 14:01, Chris Wilson wrote:
When adding a new request to the breadcrumb rbtree, we mark all those
requests inside the rbtree that are already completed as complete. This
wakes those waiters up and allows them to skip the spinlock before
returning to userspace. If one of those is the current bottom-half, it
may then overwrite intel_wait as the interrupt handler dereferences it.
Last sentence sounds suspicious. The interrupts are disabled when
this runs and locking is in place. And since the fix is to move the
"completed" block after the "first", I wonder what can get
overwritten by who?
Oh.. __intel_breadcrumbs_finish. But how does re-ordering help?
Shouldn't the fix be to skip the bottom-half assignment if the
"complete" loop has processed the waiter getting added?
Thread A runs intel_engine_add_wait, marks an earlier waiter complete
and wakes up thread B. Thread C is processing the interrupt and grabs
the irq_wait. However, thread B sees that is is complete and exits
i915_aait_request() invalidating the irq_wait as it is being used by
thread C.
Moving the irq_wait update before we wakeup thread B, ensures that
thread C has a valid irq_wait.
As discussed on the IRC, it is not that the exiting waiter explicitly
destroys the intel_wait state, but it happens implicitly because the
struct is on the waiter's stack. I was misled by the short-circuit in
intel_engine_remove_wait to think commit message was incorrect so I
suggest describing the stack situation explicitly in the commit.
With a line added to do so:
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
