* [PATCH] drm/i915: Only report a wakeup if the waiter was truly asleep
@ 2017-12-09 12:47 Chris Wilson
2017-12-09 13:17 ` ✓ Fi.CI.BAT: success for drm/i915: Only report a wakeup if the waiter was truly asleep (rev4) Patchwork
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Chris Wilson @ 2017-12-09 12:47 UTC (permalink / raw)
To: intel-gfx
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. This fixes some spurious claims of
missed_breadcrumbs when running under heavy load; i.e. sufficient load to
preempt away the newly woken waiter before they complete their checks.
However, it does so at the cost of a rare false negative; where the
waiter changes between the check and ttwu -- the only way to fix that
would be to extend the reporting from ttwu where the check could be done
atomically.
v2: Defend against !CONFIG_SMP
v3: Don't filter out calls to wake_up_process
Testcase: igt/drv_missed_irq # sanity check we do detect missed_breadcrumb()
Testcase: igt/gem_concurrent_blit # for generating false positives
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 | 39 ++++++++++++++++++++++++--------
1 file changed, 30 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_breadcrumbs.c b/drivers/gpu/drm/i915/intel_breadcrumbs.c
index 24c6fefdd0b1..76e6f8e7cfd4 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)->state & TASK_NORMAL && !(tsk)->on_cpu)
+#else
+#define task_asleep(tsk) ((tsk)->state & TASK_NORMAL)
+#endif
+
static unsigned int __intel_breadcrumbs_wakeup(struct intel_breadcrumbs *b)
{
struct intel_wait *wait;
@@ -36,8 +42,20 @@ static unsigned int __intel_breadcrumbs_wakeup(struct intel_breadcrumbs *b)
wait = b->irq_wait;
if (wait) {
+ /*
+ * N.B. Since task_asleep() and ttwu are not atomic, the
+ * waiter may actually go to sleep after the check, causing
+ * us to suppress a valid wakeup. We prefer to reduce the
+ * number of false positive missed_breadcrumb() warnings
+ * at the expense of a few false negatives, as it it easy
+ * to trigger a false positive under heavy load. Enough
+ * signal should remain from genuine missed_breadcrumb()
+ * for us to detect in CI.
+ */
+ bool was_asleep = task_asleep(wait->tsk);
+
result = ENGINE_WAKEUP_WAITER;
- if (wake_up_process(wait->tsk))
+ if (wake_up_process(wait->tsk) && was_asleep)
result |= ENGINE_WAKEUP_ASLEEP;
}
@@ -47,12 +65,15 @@ static unsigned int __intel_breadcrumbs_wakeup(struct intel_breadcrumbs *b)
unsigned int intel_engine_wakeup(struct intel_engine_cs *engine)
{
struct intel_breadcrumbs *b = &engine->breadcrumbs;
- unsigned long flags;
- unsigned int result;
+ unsigned int result = 0;
- spin_lock_irqsave(&b->irq_lock, flags);
- result = __intel_breadcrumbs_wakeup(b);
- spin_unlock_irqrestore(&b->irq_lock, flags);
+ if (READ_ONCE(b->irq_wait)) {
+ unsigned long flags;
+
+ spin_lock_irqsave(&b->irq_lock, flags);
+ result = __intel_breadcrumbs_wakeup(b);
+ spin_unlock_irqrestore(&b->irq_lock, flags);
+ }
return result;
}
@@ -77,8 +98,8 @@ static noinline void missed_breadcrumb(struct intel_engine_cs *engine)
static void intel_breadcrumbs_hangcheck(struct timer_list *t)
{
- struct intel_engine_cs *engine = from_timer(engine, t,
- breadcrumbs.hangcheck);
+ struct intel_engine_cs *engine =
+ from_timer(engine, t, breadcrumbs.hangcheck);
struct intel_breadcrumbs *b = &engine->breadcrumbs;
if (!b->irq_armed)
@@ -104,7 +125,7 @@ static void intel_breadcrumbs_hangcheck(struct timer_list *t)
*/
if (intel_engine_wakeup(engine) & ENGINE_WAKEUP_ASLEEP) {
missed_breadcrumb(engine);
- mod_timer(&engine->breadcrumbs.fake_irq, jiffies + 1);
+ mod_timer(&b->fake_irq, jiffies + 1);
} else {
mod_timer(&b->hangcheck, wait_timeout());
}
--
2.15.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 9+ messages in thread
* ✓ Fi.CI.BAT: success for drm/i915: Only report a wakeup if the waiter was truly asleep (rev4)
2017-12-09 12:47 [PATCH] drm/i915: Only report a wakeup if the waiter was truly asleep Chris Wilson
@ 2017-12-09 13:17 ` Patchwork
2017-12-09 14:09 ` ✗ Fi.CI.IGT: warning " Patchwork
2017-12-11 16:10 ` [PATCH] drm/i915: Only report a wakeup if the waiter was truly asleep Tvrtko Ursulin
2 siblings, 0 replies; 9+ messages in thread
From: Patchwork @ 2017-12-09 13:17 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx
== Series Details ==
Series: drm/i915: Only report a wakeup if the waiter was truly asleep (rev4)
URL : https://patchwork.freedesktop.org/series/22445/
State : success
== Summary ==
Series 22445v4 drm/i915: Only report a wakeup if the waiter was truly asleep
https://patchwork.freedesktop.org/api/1.0/series/22445/revisions/4/mbox/
Test gem_mmap_gtt:
Subgroup basic-small-bo-tiledx:
pass -> FAIL (fi-gdg-551) fdo#102575
fdo#102575 https://bugs.freedesktop.org/show_bug.cgi?id=102575
fi-bdw-5557u total:288 pass:267 dwarn:0 dfail:0 fail:0 skip:21 time:440s
fi-bdw-gvtdvm total:288 pass:264 dwarn:0 dfail:0 fail:0 skip:24 time:449s
fi-blb-e6850 total:288 pass:223 dwarn:1 dfail:0 fail:0 skip:64 time:386s
fi-bsw-n3050 total:288 pass:242 dwarn:0 dfail:0 fail:0 skip:46 time:514s
fi-bwr-2160 total:288 pass:183 dwarn:0 dfail:0 fail:0 skip:105 time:282s
fi-bxt-dsi total:288 pass:258 dwarn:0 dfail:0 fail:0 skip:30 time:503s
fi-bxt-j4205 total:288 pass:259 dwarn:0 dfail:0 fail:0 skip:29 time:511s
fi-byt-j1900 total:288 pass:253 dwarn:0 dfail:0 fail:0 skip:35 time:489s
fi-byt-n2820 total:288 pass:249 dwarn:0 dfail:0 fail:0 skip:39 time:479s
fi-elk-e7500 total:224 pass:163 dwarn:15 dfail:0 fail:0 skip:45
fi-gdg-551 total:288 pass:178 dwarn:1 dfail:0 fail:1 skip:108 time:266s
fi-hsw-4770 total:288 pass:261 dwarn:0 dfail:0 fail:0 skip:27 time:386s
fi-hsw-4770r total:288 pass:224 dwarn:0 dfail:0 fail:0 skip:64 time:261s
fi-ilk-650 total:288 pass:228 dwarn:0 dfail:0 fail:0 skip:60 time:395s
fi-ivb-3520m total:288 pass:259 dwarn:0 dfail:0 fail:0 skip:29 time:478s
fi-ivb-3770 total:288 pass:259 dwarn:0 dfail:0 fail:0 skip:29 time:452s
fi-kbl-7500u total:288 pass:263 dwarn:1 dfail:0 fail:0 skip:24 time:489s
fi-kbl-7560u total:288 pass:269 dwarn:0 dfail:0 fail:0 skip:19 time:532s
fi-kbl-7567u total:288 pass:268 dwarn:0 dfail:0 fail:0 skip:20 time:475s
fi-kbl-r total:288 pass:260 dwarn:1 dfail:0 fail:0 skip:27 time:535s
fi-pnv-d510 total:288 pass:222 dwarn:1 dfail:0 fail:0 skip:65 time:604s
fi-skl-6260u total:288 pass:268 dwarn:0 dfail:0 fail:0 skip:20 time:451s
fi-skl-6600u total:288 pass:261 dwarn:0 dfail:0 fail:0 skip:27 time:542s
fi-skl-6700hq total:288 pass:262 dwarn:0 dfail:0 fail:0 skip:26 time:575s
fi-skl-6700k total:288 pass:264 dwarn:0 dfail:0 fail:0 skip:24 time:514s
fi-skl-6770hq total:288 pass:268 dwarn:0 dfail:0 fail:0 skip:20 time:505s
fi-skl-gvtdvm total:288 pass:265 dwarn:0 dfail:0 fail:0 skip:23 time:447s
fi-snb-2520m total:245 pass:211 dwarn:0 dfail:0 fail:0 skip:33
fi-snb-2600 total:288 pass:248 dwarn:0 dfail:0 fail:0 skip:40 time:413s
Blacklisted hosts:
fi-cfl-s2 total:288 pass:262 dwarn:0 dfail:0 fail:0 skip:26 time:610s
fi-cnl-y total:288 pass:262 dwarn:0 dfail:0 fail:0 skip:26 time:662s
fi-glk-dsi total:288 pass:258 dwarn:0 dfail:0 fail:0 skip:30 time:489s
fi-glk-1 failed to collect. IGT log at Patchwork_7459/fi-glk-1/igt.log
06dd422e3209a968c420e10504f75fbbe897f06c drm-tip: 2017y-12m-08d-21h-06m-35s UTC integration manifest
9efbac81e824 drm/i915: Only report a wakeup if the waiter was truly asleep
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_7459/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 9+ messages in thread
* ✗ Fi.CI.IGT: warning for drm/i915: Only report a wakeup if the waiter was truly asleep (rev4)
2017-12-09 12:47 [PATCH] drm/i915: Only report a wakeup if the waiter was truly asleep Chris Wilson
2017-12-09 13:17 ` ✓ Fi.CI.BAT: success for drm/i915: Only report a wakeup if the waiter was truly asleep (rev4) Patchwork
@ 2017-12-09 14:09 ` Patchwork
2017-12-11 16:10 ` [PATCH] drm/i915: Only report a wakeup if the waiter was truly asleep Tvrtko Ursulin
2 siblings, 0 replies; 9+ messages in thread
From: Patchwork @ 2017-12-09 14:09 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx
== Series Details ==
Series: drm/i915: Only report a wakeup if the waiter was truly asleep (rev4)
URL : https://patchwork.freedesktop.org/series/22445/
State : warning
== Summary ==
Test kms_plane:
Subgroup plane-position-hole-pipe-c-planes:
pass -> SKIP (shard-hsw)
Test kms_frontbuffer_tracking:
Subgroup fbc-1p-offscren-pri-shrfb-draw-render:
fail -> PASS (shard-snb) fdo#101623
Test kms_fbcon_fbt:
Subgroup fbc:
pass -> SKIP (shard-snb)
Test pm_rpm:
Subgroup system-suspend-modeset:
pass -> SKIP (shard-hsw)
Test drv_selftest:
Subgroup live_hangcheck:
incomplete -> PASS (shard-snb) fdo#103880
Test kms_fence_pin_leak:
pass -> SKIP (shard-snb)
Test gem_tiled_swapping:
Subgroup non-threaded:
incomplete -> PASS (shard-snb) fdo#104009 +1
Test gem_pwrite_pread:
Subgroup display-copy-performance:
notrun -> INCOMPLETE (shard-snb)
fdo#101623 https://bugs.freedesktop.org/show_bug.cgi?id=101623
fdo#103880 https://bugs.freedesktop.org/show_bug.cgi?id=103880
fdo#104009 https://bugs.freedesktop.org/show_bug.cgi?id=104009
shard-hsw total:2652 pass:1514 dwarn:1 dfail:0 fail:10 skip:1126 time:9105s
shard-snb total:2654 pass:1290 dwarn:1 dfail:0 fail:11 skip:1351 time:7798s
Blacklisted hosts:
shard-apl total:2605 pass:1629 dwarn:1 dfail:0 fail:22 skip:953 time:13085s
shard-kbl total:2631 pass:1766 dwarn:1 dfail:0 fail:21 skip:842 time:10543s
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_7459/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drm/i915: Only report a wakeup if the waiter was truly asleep
2017-12-09 12:47 [PATCH] drm/i915: Only report a wakeup if the waiter was truly asleep Chris Wilson
2017-12-09 13:17 ` ✓ Fi.CI.BAT: success for drm/i915: Only report a wakeup if the waiter was truly asleep (rev4) Patchwork
2017-12-09 14:09 ` ✗ Fi.CI.IGT: warning " Patchwork
@ 2017-12-11 16:10 ` Tvrtko Ursulin
2017-12-11 17:08 ` Chris Wilson
2 siblings, 1 reply; 9+ messages in thread
From: Tvrtko Ursulin @ 2017-12-11 16:10 UTC (permalink / raw)
To: Chris Wilson, intel-gfx
On 09/12/2017 12: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. This fixes some spurious claims of
> missed_breadcrumbs when running under heavy load; i.e. sufficient load to
> preempt away the newly woken waiter before they complete their checks.
> However, it does so at the cost of a rare false negative; where the
> waiter changes between the check and ttwu -- the only way to fix that
> would be to extend the reporting from ttwu where the check could be done
> atomically.
>
> v2: Defend against !CONFIG_SMP
> v3: Don't filter out calls to wake_up_process
>
> Testcase: igt/drv_missed_irq # sanity check we do detect missed_breadcrumb()
> Testcase: igt/gem_concurrent_blit # for generating false positives
> 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 | 39 ++++++++++++++++++++++++--------
> 1 file changed, 30 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_breadcrumbs.c b/drivers/gpu/drm/i915/intel_breadcrumbs.c
> index 24c6fefdd0b1..76e6f8e7cfd4 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)->state & TASK_NORMAL && !(tsk)->on_cpu)
> +#else
> +#define task_asleep(tsk) ((tsk)->state & TASK_NORMAL)
> +#endif
> +
I kind of remember the on_cpu from before and I was probably complaining
about it. Sigh, if it helps ok..
> static unsigned int __intel_breadcrumbs_wakeup(struct intel_breadcrumbs *b)
> {
> struct intel_wait *wait;
> @@ -36,8 +42,20 @@ static unsigned int __intel_breadcrumbs_wakeup(struct intel_breadcrumbs *b)
>
> wait = b->irq_wait;
> if (wait) {
> + /*
> + * N.B. Since task_asleep() and ttwu are not atomic, the
> + * waiter may actually go to sleep after the check, causing
> + * us to suppress a valid wakeup. We prefer to reduce the
> + * number of false positive missed_breadcrumb() warnings
> + * at the expense of a few false negatives, as it it easy
> + * to trigger a false positive under heavy load. Enough
> + * signal should remain from genuine missed_breadcrumb()
> + * for us to detect in CI.
> + */
> + bool was_asleep = task_asleep(wait->tsk);
> +
> result = ENGINE_WAKEUP_WAITER;
> - if (wake_up_process(wait->tsk))
> + if (wake_up_process(wait->tsk) && was_asleep)
> result |= ENGINE_WAKEUP_ASLEEP;
> }
>
> @@ -47,12 +65,15 @@ static unsigned int __intel_breadcrumbs_wakeup(struct intel_breadcrumbs *b)
> unsigned int intel_engine_wakeup(struct intel_engine_cs *engine)
> {
> struct intel_breadcrumbs *b = &engine->breadcrumbs;
> - unsigned long flags;
> - unsigned int result;
> + unsigned int result = 0;
>
> - spin_lock_irqsave(&b->irq_lock, flags);
> - result = __intel_breadcrumbs_wakeup(b);
> - spin_unlock_irqrestore(&b->irq_lock, flags);
> + if (READ_ONCE(b->irq_wait)) {
> + unsigned long flags;
> +
> + spin_lock_irqsave(&b->irq_lock, flags);
> + result = __intel_breadcrumbs_wakeup(b);
> + spin_unlock_irqrestore(&b->irq_lock, flags);
> + }
This hunk I'd leave out from the fix.
>
> return result;
> }
> @@ -77,8 +98,8 @@ static noinline void missed_breadcrumb(struct intel_engine_cs *engine)
>
> static void intel_breadcrumbs_hangcheck(struct timer_list *t)
> {
> - struct intel_engine_cs *engine = from_timer(engine, t,
> - breadcrumbs.hangcheck);
> + struct intel_engine_cs *engine =
> + from_timer(engine, t, breadcrumbs.hangcheck);
> struct intel_breadcrumbs *b = &engine->breadcrumbs;
>
> if (!b->irq_armed)
> @@ -104,7 +125,7 @@ static void intel_breadcrumbs_hangcheck(struct timer_list *t)
> */
> if (intel_engine_wakeup(engine) & ENGINE_WAKEUP_ASLEEP) {
> missed_breadcrumb(engine);
> - mod_timer(&engine->breadcrumbs.fake_irq, jiffies + 1);
> + mod_timer(&b->fake_irq, jiffies + 1);
> } else {
> mod_timer(&b->hangcheck, wait_timeout());
> }
>
I'll turn a blind eye to this one. :)
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drm/i915: Only report a wakeup if the waiter was truly asleep
2017-12-11 16:10 ` [PATCH] drm/i915: Only report a wakeup if the waiter was truly asleep Tvrtko Ursulin
@ 2017-12-11 17:08 ` Chris Wilson
2017-12-11 17:21 ` Tvrtko Ursulin
0 siblings, 1 reply; 9+ messages in thread
From: Chris Wilson @ 2017-12-11 17:08 UTC (permalink / raw)
To: Tvrtko Ursulin, intel-gfx
Quoting Tvrtko Ursulin (2017-12-11 16:10:49)
>
> On 09/12/2017 12: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. This fixes some spurious claims of
> > missed_breadcrumbs when running under heavy load; i.e. sufficient load to
> > preempt away the newly woken waiter before they complete their checks.
> > However, it does so at the cost of a rare false negative; where the
> > waiter changes between the check and ttwu -- the only way to fix that
> > would be to extend the reporting from ttwu where the check could be done
> > atomically.
> >
> > v2: Defend against !CONFIG_SMP
> > v3: Don't filter out calls to wake_up_process
> >
> > Testcase: igt/drv_missed_irq # sanity check we do detect missed_breadcrumb()
> > Testcase: igt/gem_concurrent_blit # for generating false positives
> > 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 | 39 ++++++++++++++++++++++++--------
> > 1 file changed, 30 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_breadcrumbs.c b/drivers/gpu/drm/i915/intel_breadcrumbs.c
> > index 24c6fefdd0b1..76e6f8e7cfd4 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)->state & TASK_NORMAL && !(tsk)->on_cpu)
> > +#else
> > +#define task_asleep(tsk) ((tsk)->state & TASK_NORMAL)
> > +#endif
> > +
>
> I kind of remember the on_cpu from before and I was probably complaining
> about it. Sigh, if it helps ok..
>
> > static unsigned int __intel_breadcrumbs_wakeup(struct intel_breadcrumbs *b)
> > {
> > struct intel_wait *wait;
> > @@ -36,8 +42,20 @@ static unsigned int __intel_breadcrumbs_wakeup(struct intel_breadcrumbs *b)
> >
> > wait = b->irq_wait;
> > if (wait) {
> > + /*
> > + * N.B. Since task_asleep() and ttwu are not atomic, the
> > + * waiter may actually go to sleep after the check, causing
> > + * us to suppress a valid wakeup. We prefer to reduce the
> > + * number of false positive missed_breadcrumb() warnings
> > + * at the expense of a few false negatives, as it it easy
> > + * to trigger a false positive under heavy load. Enough
> > + * signal should remain from genuine missed_breadcrumb()
> > + * for us to detect in CI.
> > + */
> > + bool was_asleep = task_asleep(wait->tsk);
> > +
> > result = ENGINE_WAKEUP_WAITER;
> > - if (wake_up_process(wait->tsk))
> > + if (wake_up_process(wait->tsk) && was_asleep)
> > result |= ENGINE_WAKEUP_ASLEEP;
> > }
> >
> > @@ -47,12 +65,15 @@ static unsigned int __intel_breadcrumbs_wakeup(struct intel_breadcrumbs *b)
> > unsigned int intel_engine_wakeup(struct intel_engine_cs *engine)
> > {
> > struct intel_breadcrumbs *b = &engine->breadcrumbs;
> > - unsigned long flags;
> > - unsigned int result;
> > + unsigned int result = 0;
> >
> > - spin_lock_irqsave(&b->irq_lock, flags);
> > - result = __intel_breadcrumbs_wakeup(b);
> > - spin_unlock_irqrestore(&b->irq_lock, flags);
> > + if (READ_ONCE(b->irq_wait)) {
> > + unsigned long flags;
> > +
> > + spin_lock_irqsave(&b->irq_lock, flags);
> > + result = __intel_breadcrumbs_wakeup(b);
> > + spin_unlock_irqrestore(&b->irq_lock, flags);
> > + }
>
> This hunk I'd leave out from the fix.
And if I postpone that hunk to tomorrow, would r-b the rest?
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drm/i915: Only report a wakeup if the waiter was truly asleep
2017-12-11 17:08 ` Chris Wilson
@ 2017-12-11 17:21 ` Tvrtko Ursulin
0 siblings, 0 replies; 9+ messages in thread
From: Tvrtko Ursulin @ 2017-12-11 17:21 UTC (permalink / raw)
To: Chris Wilson, intel-gfx
On 11/12/2017 17:08, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2017-12-11 16:10:49)
>>
>> On 09/12/2017 12: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. This fixes some spurious claims of
>>> missed_breadcrumbs when running under heavy load; i.e. sufficient load to
>>> preempt away the newly woken waiter before they complete their checks.
>>> However, it does so at the cost of a rare false negative; where the
>>> waiter changes between the check and ttwu -- the only way to fix that
>>> would be to extend the reporting from ttwu where the check could be done
>>> atomically.
>>>
>>> v2: Defend against !CONFIG_SMP
>>> v3: Don't filter out calls to wake_up_process
>>>
>>> Testcase: igt/drv_missed_irq # sanity check we do detect missed_breadcrumb()
>>> Testcase: igt/gem_concurrent_blit # for generating false positives
>>> 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 | 39 ++++++++++++++++++++++++--------
>>> 1 file changed, 30 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_breadcrumbs.c b/drivers/gpu/drm/i915/intel_breadcrumbs.c
>>> index 24c6fefdd0b1..76e6f8e7cfd4 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)->state & TASK_NORMAL && !(tsk)->on_cpu)
>>> +#else
>>> +#define task_asleep(tsk) ((tsk)->state & TASK_NORMAL)
>>> +#endif
>>> +
>>
>> I kind of remember the on_cpu from before and I was probably complaining
>> about it. Sigh, if it helps ok..
>>
>>> static unsigned int __intel_breadcrumbs_wakeup(struct intel_breadcrumbs *b)
>>> {
>>> struct intel_wait *wait;
>>> @@ -36,8 +42,20 @@ static unsigned int __intel_breadcrumbs_wakeup(struct intel_breadcrumbs *b)
>>>
>>> wait = b->irq_wait;
>>> if (wait) {
>>> + /*
>>> + * N.B. Since task_asleep() and ttwu are not atomic, the
>>> + * waiter may actually go to sleep after the check, causing
>>> + * us to suppress a valid wakeup. We prefer to reduce the
>>> + * number of false positive missed_breadcrumb() warnings
>>> + * at the expense of a few false negatives, as it it easy
>>> + * to trigger a false positive under heavy load. Enough
>>> + * signal should remain from genuine missed_breadcrumb()
>>> + * for us to detect in CI.
>>> + */
>>> + bool was_asleep = task_asleep(wait->tsk);
>>> +
>>> result = ENGINE_WAKEUP_WAITER;
>>> - if (wake_up_process(wait->tsk))
>>> + if (wake_up_process(wait->tsk) && was_asleep)
>>> result |= ENGINE_WAKEUP_ASLEEP;
>>> }
>>>
>>> @@ -47,12 +65,15 @@ static unsigned int __intel_breadcrumbs_wakeup(struct intel_breadcrumbs *b)
>>> unsigned int intel_engine_wakeup(struct intel_engine_cs *engine)
>>> {
>>> struct intel_breadcrumbs *b = &engine->breadcrumbs;
>>> - unsigned long flags;
>>> - unsigned int result;
>>> + unsigned int result = 0;
>>>
>>> - spin_lock_irqsave(&b->irq_lock, flags);
>>> - result = __intel_breadcrumbs_wakeup(b);
>>> - spin_unlock_irqrestore(&b->irq_lock, flags);
>>> + if (READ_ONCE(b->irq_wait)) {
>>> + unsigned long flags;
>>> +
>>> + spin_lock_irqsave(&b->irq_lock, flags);
>>> + result = __intel_breadcrumbs_wakeup(b);
>>> + spin_unlock_irqrestore(&b->irq_lock, flags);
>>> + }
>>
>> This hunk I'd leave out from the fix.
>
> And if I postpone that hunk to tomorrow, would r-b the rest?
Yep.
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH] drm/i915: Only report a wakeup if the waiter was truly asleep
@ 2017-04-04 14:38 Chris Wilson
2017-04-05 10:40 ` kbuild test robot
2017-04-05 12:20 ` kbuild test robot
0 siblings, 2 replies; 9+ messages in thread
From: Chris Wilson @ 2017-04-04 14:38 UTC (permalink / raw)
To: intel-gfx
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.
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 | 15 +++++++++++++--
1 file changed, 13 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_breadcrumbs.c b/drivers/gpu/drm/i915/intel_breadcrumbs.c
index 9ccbf26124c6..e8994aa1b434 100644
--- a/drivers/gpu/drm/i915/intel_breadcrumbs.c
+++ b/drivers/gpu/drm/i915/intel_breadcrumbs.c
@@ -27,6 +27,17 @@
#include "i915_drv.h"
+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 !tsk->on_cpu && wake_up_process(tsk);
+}
+
static unsigned int __intel_breadcrumbs_wakeup(struct intel_breadcrumbs *b)
{
struct intel_wait *wait;
@@ -37,7 +48,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 +209,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;
--
2.11.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] drm/i915: Only report a wakeup if the waiter was truly asleep
2017-04-04 14:38 Chris Wilson
@ 2017-04-05 10:40 ` kbuild test robot
2017-04-05 12:20 ` kbuild test robot
1 sibling, 0 replies; 9+ messages in thread
From: kbuild test robot @ 2017-04-05 10:40 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx, kbuild-all
[-- Attachment #1: Type: text/plain, Size: 1851 bytes --]
Hi Chris,
[auto build test ERROR on drm-intel/for-linux-next]
[also build test ERROR on next-20170405]
[cannot apply to v4.11-rc5]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Chris-Wilson/drm-i915-Only-report-a-wakeup-if-the-waiter-was-truly-asleep/20170405-165353
base: git://anongit.freedesktop.org/drm-intel for-linux-next
config: x86_64-randconfig-x004-201714 (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64
All error/warnings (new ones prefixed by >>):
drivers/gpu//drm/i915/intel_breadcrumbs.c: In function '__wake_up_sleeper':
>> drivers/gpu//drm/i915/intel_breadcrumbs.c:38:13: error: 'struct task_struct' has no member named 'on_cpu'; did you mean 'on_rq'?
return !tsk->on_cpu && wake_up_process(tsk);
^~
>> drivers/gpu//drm/i915/intel_breadcrumbs.c:39:1: warning: control reaches end of non-void function [-Wreturn-type]
}
^
vim +38 drivers/gpu//drm/i915/intel_breadcrumbs.c
32 /* Be careful not to report a successful wakeup if the waiter is
33 * currently processing the seqno, where it will have already
34 * called set_task_state(TASK_INTERRUPTIBLE). We first check whether
35 * the task is currently asleep before calling ttwu, and then we
36 * only report success if we were the ones to then trigger the wakeup.
37 */
> 38 return !tsk->on_cpu && wake_up_process(tsk);
> 39 }
40
41 static unsigned int __intel_breadcrumbs_wakeup(struct intel_breadcrumbs *b)
42 {
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 28439 bytes --]
[-- Attachment #3: Type: text/plain, Size: 160 bytes --]
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drm/i915: Only report a wakeup if the waiter was truly asleep
2017-04-04 14:38 Chris Wilson
2017-04-05 10:40 ` kbuild test robot
@ 2017-04-05 12:20 ` kbuild test robot
1 sibling, 0 replies; 9+ messages in thread
From: kbuild test robot @ 2017-04-05 12:20 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx, kbuild-all
[-- Attachment #1: Type: text/plain, Size: 1803 bytes --]
Hi Chris,
[auto build test ERROR on drm-intel/for-linux-next]
[also build test ERROR on next-20170405]
[cannot apply to v4.11-rc5]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Chris-Wilson/drm-i915-Only-report-a-wakeup-if-the-waiter-was-truly-asleep/20170405-165353
base: git://anongit.freedesktop.org/drm-intel for-linux-next
config: i386-randconfig-sb0-04050506 (attached as .config)
compiler: gcc-5 (Debian 5.4.1-2) 5.4.1 20160904
reproduce:
# save the attached .config to linux build tree
make ARCH=i386
All errors (new ones prefixed by >>):
drivers/gpu/drm/i915/intel_breadcrumbs.c: In function '__wake_up_sleeper':
>> drivers/gpu/drm/i915/intel_breadcrumbs.c:38:13: error: 'struct task_struct' has no member named 'on_cpu'
return !tsk->on_cpu && wake_up_process(tsk);
^
drivers/gpu/drm/i915/intel_breadcrumbs.c:39:1: warning: control reaches end of non-void function [-Wreturn-type]
}
^
vim +38 drivers/gpu/drm/i915/intel_breadcrumbs.c
32 /* Be careful not to report a successful wakeup if the waiter is
33 * currently processing the seqno, where it will have already
34 * called set_task_state(TASK_INTERRUPTIBLE). We first check whether
35 * the task is currently asleep before calling ttwu, and then we
36 * only report success if we were the ones to then trigger the wakeup.
37 */
> 38 return !tsk->on_cpu && wake_up_process(tsk);
39 }
40
41 static unsigned int __intel_breadcrumbs_wakeup(struct intel_breadcrumbs *b)
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 26843 bytes --]
[-- Attachment #3: Type: text/plain, Size: 160 bytes --]
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2017-12-11 17:21 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-12-09 12:47 [PATCH] drm/i915: Only report a wakeup if the waiter was truly asleep Chris Wilson
2017-12-09 13:17 ` ✓ Fi.CI.BAT: success for drm/i915: Only report a wakeup if the waiter was truly asleep (rev4) Patchwork
2017-12-09 14:09 ` ✗ Fi.CI.IGT: warning " Patchwork
2017-12-11 16:10 ` [PATCH] drm/i915: Only report a wakeup if the waiter was truly asleep Tvrtko Ursulin
2017-12-11 17:08 ` Chris Wilson
2017-12-11 17:21 ` Tvrtko Ursulin
-- strict thread matches above, loose matches on Subject: below --
2017-04-04 14:38 Chris Wilson
2017-04-05 10:40 ` kbuild test robot
2017-04-05 12:20 ` kbuild test robot
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.