Re: [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=17
> >>>Signed-off-by: Chris Wilson 
> >>>Cc: Tvrtko Ursulin 
> >>>Cc: Joonas Lahtinen 
> >>>---
> >>>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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Tvrtko Ursulin]

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=17
Signed-off-by: Chris Wilson 
Cc: Tvrtko Ursulin 
Cc: Joonas Lahtinen 
---
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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [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=17
> >Signed-off-by: Chris Wilson 
> >Cc: Tvrtko Ursulin 
> >Cc: Joonas Lahtinen 
> >---
> > 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
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Tvrtko Ursulin]

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=17
Signed-off-by: Chris Wilson 
Cc: Tvrtko Ursulin 
Cc: Joonas Lahtinen 
---
 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, >waiters, node) {
RB_CLEAR_NODE(>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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Chris Wilson]

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=17
Signed-off-by: Chris Wilson 
Cc: Tvrtko Ursulin 
Cc: Joonas Lahtinen 
---
 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
+
+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);
+}
+
 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, >waiters, node) {
RB_CLEAR_NODE(>node);
-   if (wake_up_process(wait->tsk) && wait == first)
+   if (__wake_up_sleeper(wait->tsk) && wait == first)
missed_breadcrumb(engine);
}
b->waiters = RB_ROOT;
-- 
2.11.0

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx