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@xxxxxxxxxxxxxxxxxx> > >>>Cc: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx> > >>>Cc: Joonas Lahtinen <joonas.lahtinen@xxxxxxxxxxxxxxx> > >>>--- > >>>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 _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
