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.
Regards,
Tvrtko
+}
+
static unsigned int __intel_breadcrumbs_wakeup(struct intel_breadcrumbs *b)
{
struct intel_wait *wait;
@@ -37,7 +54,7 @@ static unsigned int __intel_breadcrumbs_wakeup(struct intel_breadcrumbs *b)
wait = b->irq_wait;
if (wait) {
result = ENGINE_WAKEUP_WAITER;
- if (wake_up_process(wait->tsk))
+ if (__wake_up_sleeper(wait->tsk))
result |= ENGINE_WAKEUP_ASLEEP;
}
@@ -198,7 +215,7 @@ void intel_engine_disarm_breadcrumbs(struct intel_engine_cs *engine)
rbtree_postorder_for_each_entry_safe(wait, n, &b->waiters, node) {
RB_CLEAR_NODE(&wait->node);
- if (wake_up_process(wait->tsk) && wait == first)
+ if (__wake_up_sleeper(wait->tsk) && wait == first)
missed_breadcrumb(engine);
}
b->waiters = RB_ROOT;
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
