[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:55:27AM +0100, Tvrtko Ursulin wrote:
> 
> 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?

Fortunately, it wasn't used from that critical path, but point taken I
had missed that it escaped via intel_engine_wakeup.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre