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?
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
