[PATCH v3] drm/i915: Only report a wakeup if the waiter was truly asleep

[PATCH v3] 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
v3: Don't filter out calls to wake_up_process

References: https://bugs.freedesktop.org/show_bug.cgi?id=100007
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_breadcrumbs.c | 18 ++++++++++++++++--
 drivers/gpu/drm/i915/intel_ringbuffer.h  |  4 ++++
 2 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_breadcrumbs.c b/drivers/gpu/drm/i915/intel_breadcrumbs.c
index 9ccbf26124c6..808d3a3cda0a 100644
--- a/drivers/gpu/drm/i915/intel_breadcrumbs.c
+++ b/drivers/gpu/drm/i915/intel_breadcrumbs.c
@@ -27,6 +27,12 @@
 
 #include "i915_drv.h"
 
+#ifdef CONFIG_SMP
+#define task_asleep(tsk) (!(tsk)->on_cpu)
+#else
+#define task_asleep(tsk) ((tsk) != current)
+#endif
+
 static unsigned int __intel_breadcrumbs_wakeup(struct intel_breadcrumbs *b)
 {
 	struct intel_wait *wait;
@@ -37,8 +43,16 @@ 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))
+
+		/* 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).
+		 */
+		if (task_asleep(wait->tsk))
 			result |= ENGINE_WAKEUP_ASLEEP;
+
+		if (wake_up_process(wait->tsk))
+			result |= ENGINE_WAKEUP_SUCCESS;
 	}
 
 	return result;
@@ -98,7 +112,7 @@ static void intel_breadcrumbs_hangcheck(unsigned long data)
 	 * but we still have a waiter. Assuming all batches complete within
 	 * DRM_I915_HANGCHECK_JIFFIES [1.5s]!
 	 */
-	if (intel_engine_wakeup(engine) & ENGINE_WAKEUP_ASLEEP) {
+	if (intel_engine_wakeup(engine) == ENGINE_WAKEUP) {
 		missed_breadcrumb(engine);
 		mod_timer(&engine->breadcrumbs.fake_irq, jiffies + 1);
 	} else {
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index cbe61d3f31da..974a5928bec9 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -663,6 +663,10 @@ static inline bool intel_engine_has_waiter(const struct intel_engine_cs *engine)
 unsigned int intel_engine_wakeup(struct intel_engine_cs *engine);
 #define ENGINE_WAKEUP_WAITER BIT(0)
 #define ENGINE_WAKEUP_ASLEEP BIT(1)
+#define ENGINE_WAKEUP_SUCCESS BIT(2)
+#define ENGINE_WAKEUP (ENGINE_WAKEUP_WAITER | \
+		       ENGINE_WAKEUP_ASLEEP | \
+		       ENGINE_WAKEUP_SUCCESS)
 
 void __intel_engine_disarm_breadcrumbs(struct intel_engine_cs *engine);
 void intel_engine_disarm_breadcrumbs(struct intel_engine_cs *engine);
-- 
2.11.0

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

✓ Fi.CI.BAT: success for drm/i915: Only report a wakeup if the waiter was truly asleep (rev3)

[Patchwork]

== Series Details ==

Series: drm/i915: Only report a wakeup if the waiter was truly asleep (rev3)
URL   : https://patchwork.freedesktop.org/series/22445/
State : success

== Summary ==

Series 22445v3 drm/i915: Only report a wakeup if the waiter was truly asleep
https://patchwork.freedesktop.org/api/1.0/series/22445/revisions/3/mbox/

fi-bdw-5557u     total:278  pass:267  dwarn:0   dfail:0   fail:0   skip:11  time: 426s
fi-bdw-gvtdvm    total:278  pass:256  dwarn:8   dfail:0   fail:0   skip:14  time: 430s
fi-bsw-n3050     total:278  pass:242  dwarn:0   dfail:0   fail:0   skip:36  time: 578s
fi-bxt-j4205     total:278  pass:259  dwarn:0   dfail:0   fail:0   skip:19  time: 504s
fi-bxt-t5700     total:278  pass:258  dwarn:0   dfail:0   fail:0   skip:20  time: 546s
fi-byt-j1900     total:278  pass:254  dwarn:0   dfail:0   fail:0   skip:24  time: 484s
fi-byt-n2820     total:278  pass:250  dwarn:0   dfail:0   fail:0   skip:28  time: 477s
fi-hsw-4770      total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time: 408s
fi-hsw-4770r     total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time: 412s
fi-ilk-650       total:278  pass:228  dwarn:0   dfail:0   fail:0   skip:50  time: 432s
fi-ivb-3520m     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time: 490s
fi-ivb-3770      total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time: 473s
fi-kbl-7500u     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time: 448s
fi-kbl-7560u     total:278  pass:267  dwarn:1   dfail:0   fail:0   skip:10  time: 565s
fi-skl-6260u     total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time: 453s
fi-skl-6700hq    total:278  pass:261  dwarn:0   dfail:0   fail:0   skip:17  time: 572s
fi-skl-6700k     total:278  pass:256  dwarn:4   dfail:0   fail:0   skip:18  time: 459s
fi-skl-6770hq    total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time: 495s
fi-skl-gvtdvm    total:278  pass:265  dwarn:0   dfail:0   fail:0   skip:13  time: 437s
fi-snb-2520m     total:278  pass:250  dwarn:0   dfail:0   fail:0   skip:28  time: 534s
fi-snb-2600      total:278  pass:249  dwarn:0   dfail:0   fail:0   skip:29  time: 402s

7303b362817aa4b4c62cdc2dfa586c272982917d drm-tip: 2017y-04m-06d-08h-26m-30s UTC integration manifest
05b9f59 drm/i915: Only report a wakeup if the waiter was truly asleep

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_4418/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3] drm/i915: Only report a wakeup if the waiter was truly asleep

[Tvrtko Ursulin]

On 06/04/2017 10:30, 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
> v3: Don't filter out calls to wake_up_process
>
> References: https://bugs.freedesktop.org/show_bug.cgi?id=100007
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_breadcrumbs.c | 18 ++++++++++++++++--
>  drivers/gpu/drm/i915/intel_ringbuffer.h  |  4 ++++
>  2 files changed, 20 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_breadcrumbs.c b/drivers/gpu/drm/i915/intel_breadcrumbs.c
> index 9ccbf26124c6..808d3a3cda0a 100644
> --- a/drivers/gpu/drm/i915/intel_breadcrumbs.c
> +++ b/drivers/gpu/drm/i915/intel_breadcrumbs.c
> @@ -27,6 +27,12 @@
>
>  #include "i915_drv.h"
>
> +#ifdef CONFIG_SMP
> +#define task_asleep(tsk) (!(tsk)->on_cpu)
> +#else
> +#define task_asleep(tsk) ((tsk) != current)
> +#endif

It really bothers to fish into this low level info which probably isn't 
intended to be used from the outside.

> +
>  static unsigned int __intel_breadcrumbs_wakeup(struct intel_breadcrumbs *b)
>  {
>  	struct intel_wait *wait;
> @@ -37,8 +43,16 @@ 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))
> +
> +		/* 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).
> +		 */
> +		if (task_asleep(wait->tsk))
>  			result |= ENGINE_WAKEUP_ASLEEP;

And this still has the problem of not being atomic between reporting the 
two flags. So the reported status can be false which also bothers me.

I will need to take some more time thinking about this.

Regards,

Tvrtko

> +
> +		if (wake_up_process(wait->tsk))
> +			result |= ENGINE_WAKEUP_SUCCESS;
>  	}
>
>  	return result;
> @@ -98,7 +112,7 @@ static void intel_breadcrumbs_hangcheck(unsigned long data)
>  	 * but we still have a waiter. Assuming all batches complete within
>  	 * DRM_I915_HANGCHECK_JIFFIES [1.5s]!
>  	 */
> -	if (intel_engine_wakeup(engine) & ENGINE_WAKEUP_ASLEEP) {
> +	if (intel_engine_wakeup(engine) == ENGINE_WAKEUP) {
>  		missed_breadcrumb(engine);
>  		mod_timer(&engine->breadcrumbs.fake_irq, jiffies + 1);
>  	} else {
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
> index cbe61d3f31da..974a5928bec9 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> @@ -663,6 +663,10 @@ static inline bool intel_engine_has_waiter(const struct intel_engine_cs *engine)
>  unsigned int intel_engine_wakeup(struct intel_engine_cs *engine);
>  #define ENGINE_WAKEUP_WAITER BIT(0)
>  #define ENGINE_WAKEUP_ASLEEP BIT(1)
> +#define ENGINE_WAKEUP_SUCCESS BIT(2)
> +#define ENGINE_WAKEUP (ENGINE_WAKEUP_WAITER | \
> +		       ENGINE_WAKEUP_ASLEEP | \
> +		       ENGINE_WAKEUP_SUCCESS)
>
>  void __intel_engine_disarm_breadcrumbs(struct intel_engine_cs *engine);
>  void intel_engine_disarm_breadcrumbs(struct intel_engine_cs *engine);
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3] drm/i915: Only report a wakeup if the waiter was truly asleep

[Tvrtko Ursulin]

On 06/04/2017 18:40, Tvrtko Ursulin wrote:
> On 06/04/2017 10:30, 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
>> v3: Don't filter out calls to wake_up_process
>>
>> References: https://bugs.freedesktop.org/show_bug.cgi?id=100007
>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_breadcrumbs.c | 18 ++++++++++++++++--
>>  drivers/gpu/drm/i915/intel_ringbuffer.h  |  4 ++++
>>  2 files changed, 20 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_breadcrumbs.c
>> b/drivers/gpu/drm/i915/intel_breadcrumbs.c
>> index 9ccbf26124c6..808d3a3cda0a 100644
>> --- a/drivers/gpu/drm/i915/intel_breadcrumbs.c
>> +++ b/drivers/gpu/drm/i915/intel_breadcrumbs.c
>> @@ -27,6 +27,12 @@
>>
>>  #include "i915_drv.h"
>>
>> +#ifdef CONFIG_SMP
>> +#define task_asleep(tsk) (!(tsk)->on_cpu)
>> +#else
>> +#define task_asleep(tsk) ((tsk) != current)
>> +#endif
>
> It really bothers to fish into this low level info which probably isn't
> intended to be used from the outside.
>
>> +
>>  static unsigned int __intel_breadcrumbs_wakeup(struct
>> intel_breadcrumbs *b)
>>  {
>>      struct intel_wait *wait;
>> @@ -37,8 +43,16 @@ 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))
>> +
>> +        /* 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).
>> +         */
>> +        if (task_asleep(wait->tsk))
>>              result |= ENGINE_WAKEUP_ASLEEP;
>
> And this still has the problem of not being atomic between reporting the
> two flags. So the reported status can be false which also bothers me.
>
> I will need to take some more time thinking about this.

Warning, the idea below is potentially unrefined! :)

How about a scheme where on wake_up we would atomic_inc our own wakeup 
counter, and then the signaller keeps atomic_dec_and_test one item at a 
time until it has consumed all the wakeups? The code in 
intel_breadcrumbs_hangcheck only declares a missed breadcrumb if the 
wakeup counter is one, meaning this was the first wakeup?

Regards,

Tvrtko

>> +
>> +        if (wake_up_process(wait->tsk))
>> +            result |= ENGINE_WAKEUP_SUCCESS;
>>      }
>>
>>      return result;
>> @@ -98,7 +112,7 @@ static void intel_breadcrumbs_hangcheck(unsigned
>> long data)
>>       * but we still have a waiter. Assuming all batches complete within
>>       * DRM_I915_HANGCHECK_JIFFIES [1.5s]!
>>       */
>> -    if (intel_engine_wakeup(engine) & ENGINE_WAKEUP_ASLEEP) {
>> +    if (intel_engine_wakeup(engine) == ENGINE_WAKEUP) {
>>          missed_breadcrumb(engine);
>>          mod_timer(&engine->breadcrumbs.fake_irq, jiffies + 1);
>>      } else {
>> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h
>> b/drivers/gpu/drm/i915/intel_ringbuffer.h
>> index cbe61d3f31da..974a5928bec9 100644
>> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
>> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
>> @@ -663,6 +663,10 @@ static inline bool intel_engine_has_waiter(const
>> struct intel_engine_cs *engine)
>>  unsigned int intel_engine_wakeup(struct intel_engine_cs *engine);
>>  #define ENGINE_WAKEUP_WAITER BIT(0)
>>  #define ENGINE_WAKEUP_ASLEEP BIT(1)
>> +#define ENGINE_WAKEUP_SUCCESS BIT(2)
>> +#define ENGINE_WAKEUP (ENGINE_WAKEUP_WAITER | \
>> +               ENGINE_WAKEUP_ASLEEP | \
>> +               ENGINE_WAKEUP_SUCCESS)
>>
>>  void __intel_engine_disarm_breadcrumbs(struct intel_engine_cs *engine);
>>  void intel_engine_disarm_breadcrumbs(struct intel_engine_cs *engine);
>>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3] drm/i915: Only report a wakeup if the waiter was truly asleep

[Chris Wilson]

On Fri, Apr 07, 2017 at 09:23:26AM +0100, Tvrtko Ursulin wrote:
> 
> On 06/04/2017 18:40, Tvrtko Ursulin wrote:
> >On 06/04/2017 10:30, 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
> >>v3: Don't filter out calls to wake_up_process
> >>
> >>References: https://bugs.freedesktop.org/show_bug.cgi?id=100007
> >>Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> >>Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >>Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> >>---
> >> drivers/gpu/drm/i915/intel_breadcrumbs.c | 18 ++++++++++++++++--
> >> drivers/gpu/drm/i915/intel_ringbuffer.h  |  4 ++++
> >> 2 files changed, 20 insertions(+), 2 deletions(-)
> >>
> >>diff --git a/drivers/gpu/drm/i915/intel_breadcrumbs.c
> >>b/drivers/gpu/drm/i915/intel_breadcrumbs.c
> >>index 9ccbf26124c6..808d3a3cda0a 100644
> >>--- a/drivers/gpu/drm/i915/intel_breadcrumbs.c
> >>+++ b/drivers/gpu/drm/i915/intel_breadcrumbs.c
> >>@@ -27,6 +27,12 @@
> >>
> >> #include "i915_drv.h"
> >>
> >>+#ifdef CONFIG_SMP
> >>+#define task_asleep(tsk) (!(tsk)->on_cpu)
> >>+#else
> >>+#define task_asleep(tsk) ((tsk) != current)
> >>+#endif
> >
> >It really bothers to fish into this low level info which probably isn't
> >intended to be used from the outside.
> >
> >>+
> >> static unsigned int __intel_breadcrumbs_wakeup(struct
> >>intel_breadcrumbs *b)
> >> {
> >>     struct intel_wait *wait;
> >>@@ -37,8 +43,16 @@ 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))
> >>+
> >>+        /* 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).
> >>+         */
> >>+        if (task_asleep(wait->tsk))
> >>             result |= ENGINE_WAKEUP_ASLEEP;
> >
> >And this still has the problem of not being atomic between reporting the
> >two flags. So the reported status can be false which also bothers me.
> >
> >I will need to take some more time thinking about this.
> 
> Warning, the idea below is potentially unrefined! :)
> 
> How about a scheme where on wake_up we would atomic_inc our own
> wakeup counter, and then the signaller keeps atomic_dec_and_test one
> item at a time until it has consumed all the wakeups? The code in
> intel_breadcrumbs_hangcheck only declares a missed breadcrumb if the
> wakeup counter is one, meaning this was the first wakeup?

Yeah, I'd been trying to convince myself to be happy with something like
that as well. I've been contemplating an "on-cpu" bit inside the struct
intel_wait. You will still have the complaint that the two tests are not
atomic, and I'll be handwaving that the worst-case outcome is one extra
timer period before the missed breadcrumb is spotted.

Just can't convince myself that it is worth it. Whilst I can see that if
we starve the system it is quite possible that the bottom-half never
gets run and that it may be preempted before we perform our checks, it
just seems so unlikely.
-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