[Intel-gfx] [PATCH v2] drm/i915: Only report a wakeup if the waiter was truly asleep

[Tvrtko Ursulin]

On 06/04/2017 09:16, Chris Wilson wrote:
> On Thu, Apr 06, 2017 at 09:04:23AM +0100, Tvrtko Ursulin wrote:
>>
>> On 05/04/2017 13:47, Chris Wilson wrote:
>>> If we attempt to wake up a waiter, who is currently checking the seqno
>>> it will be in the TASK_INTERRUPTIBLE state and ttwu will report success.
>>> However, it is actually awake and functioning -- so delay reporting the
>>> actual wake up until it sleeps.
>>>
>>> v2: Defend against !CONFIG_SMP
>>>
>>> References: https://bugs.freedesktop.org/show_bug.cgi?id=100007
>>> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
>>> Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
>>> Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
>>> ---
>>> drivers/gpu/drm/i915/intel_breadcrumbs.c | 21 +++++++++++++++++++--
>>> 1 file changed, 19 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_breadcrumbs.c b/drivers/gpu/drm/i915/intel_breadcrumbs.c
>>> index 9ccbf26124c6..4fdf7868e2f1 100644
>>> --- a/drivers/gpu/drm/i915/intel_breadcrumbs.c
>>> +++ b/drivers/gpu/drm/i915/intel_breadcrumbs.c
>>> @@ -27,6 +27,23 @@
>>>
>>> #include "i915_drv.h"
>>>
>>> +#ifdef CONFIG_SMP
>>> +#define task_asleep(tsk) (!(tsk)->on_cpu)
>>> +#else
>>> +#define task_asleep(tsk) ((tsk) != current)
>>> +#endif
>>
>> I've looked and on_cpu seems to be a boolean indicating whether a
>> task is currently running. Which means on UP tsk != current is a
>> correct replacement. However ...
>>
>>> +
>>> +static inline bool __wake_up_sleeper(struct task_struct *tsk)
>>> +{
>>> +	/* Be careful not to report a successful wakeup if the waiter is
>>> +	 * currently processing the seqno, where it will have already
>>> +	 * called set_task_state(TASK_INTERRUPTIBLE). We first check whether
>>> +	 * the task is currently asleep before calling ttwu, and then we
>>> +	 * only report success if we were the ones to then trigger the wakeup.
>>> +	 */
>>> +	return task_asleep(tsk) && wake_up_process(tsk);
>>
>> ... I don't see why on SMP task couldn't get "on_cpu" between the
>> task_asleep() and wake_up_process? That would then foil the test and
>> just shrink the race window a bit.
>
> Two scenarios:
> on_cpu 1 -> 0, we wait until next the timer expiry and check again
> on_cpu 0 -> 1 (before wake_up_process), we were not the ones to wake it
> up, so we wait until the next timer expiry and check again.
>
> Someone else waking up the process after us doesn't affect our decision
> that the irq counter was stable and yet the waiter is still asleep.
>
> So I don't it matters.

What about the other call sites? Like the wakeup from irq which may not 
have the opportunity to check again?

Regards,

Tvrtko