[Intel-gfx] [PATCH 3/6] drm/i915/breadcrumbs: Update bottom-half before marking as complete

[Tvrtko Ursulin]

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 at intel.com>

Regards,

Tvrtko