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

[Chris Wilson]

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.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre