* [PATCH] drm/i915: Only report a wakeup if the waiter was truly asleep
@ 2017-04-04 14:38 Chris Wilson
2017-04-04 15:10 ` ✓ Fi.CI.BAT: success for " Patchwork
` (4 more replies)
0 siblings, 5 replies; 10+ 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] 10+ messages in thread
* ✓ Fi.CI.BAT: success for drm/i915: Only report a wakeup if the waiter was truly asleep
2017-04-04 14:38 [PATCH] drm/i915: Only report a wakeup if the waiter was truly asleep Chris Wilson
@ 2017-04-04 15:10 ` Patchwork
2017-04-05 10:40 ` [PATCH] " kbuild test robot
` (3 subsequent siblings)
4 siblings, 0 replies; 10+ messages in thread
From: Patchwork @ 2017-04-04 15:10 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx
== Series Details ==
Series: drm/i915: Only report a wakeup if the waiter was truly asleep
URL : https://patchwork.freedesktop.org/series/22445/
State : success
== Summary ==
Series 22445v1 drm/i915: Only report a wakeup if the waiter was truly asleep
https://patchwork.freedesktop.org/api/1.0/series/22445/revisions/1/mbox/
Test gem_exec_flush:
Subgroup basic-batch-kernel-default-uc:
fail -> PASS (fi-snb-2600) fdo#100007
Test gem_exec_suspend:
Subgroup basic-s4-devices:
pass -> DMESG-WARN (fi-kbl-7560u) fdo#100125
Test kms_pipe_crc_basic:
Subgroup suspend-read-crc-pipe-a:
fail -> PASS (fi-skl-6700k) fdo#100367
Test prime_vgem:
Subgroup basic-fence-flip:
dmesg-warn -> PASS (fi-byt-n2820) fdo#100094
fdo#100007 https://bugs.freedesktop.org/show_bug.cgi?id=100007
fdo#100125 https://bugs.freedesktop.org/show_bug.cgi?id=100125
fdo#100367 https://bugs.freedesktop.org/show_bug.cgi?id=100367
fdo#100094 https://bugs.freedesktop.org/show_bug.cgi?id=100094
fi-bdw-5557u total:278 pass:267 dwarn:0 dfail:0 fail:0 skip:11 time: 431s
fi-bdw-gvtdvm total:278 pass:256 dwarn:8 dfail:0 fail:0 skip:14 time: 424s
fi-bsw-n3050 total:278 pass:239 dwarn:0 dfail:0 fail:0 skip:39 time: 572s
fi-bxt-j4205 total:278 pass:259 dwarn:0 dfail:0 fail:0 skip:19 time: 511s
fi-bxt-t5700 total:278 pass:258 dwarn:0 dfail:0 fail:0 skip:20 time: 551s
fi-byt-j1900 total:278 pass:251 dwarn:0 dfail:0 fail:0 skip:27 time: 481s
fi-byt-n2820 total:278 pass:247 dwarn:0 dfail:0 fail:0 skip:31 time: 484s
fi-hsw-4770 total:278 pass:262 dwarn:0 dfail:0 fail:0 skip:16 time: 411s
fi-hsw-4770r total:278 pass:262 dwarn:0 dfail:0 fail:0 skip:16 time: 407s
fi-ilk-650 total:278 pass:228 dwarn:0 dfail:0 fail:0 skip:50 time: 423s
fi-ivb-3520m total:278 pass:260 dwarn:0 dfail:0 fail:0 skip:18 time: 494s
fi-ivb-3770 total:278 pass:260 dwarn:0 dfail:0 fail:0 skip:18 time: 465s
fi-kbl-7500u total:278 pass:260 dwarn:0 dfail:0 fail:0 skip:18 time: 453s
fi-kbl-7560u total:278 pass:267 dwarn:1 dfail:0 fail:0 skip:10 time: 571s
fi-skl-6260u total:278 pass:268 dwarn:0 dfail:0 fail:0 skip:10 time: 459s
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: 462s
fi-skl-6770hq total:278 pass:268 dwarn:0 dfail:0 fail:0 skip:10 time: 489s
fi-skl-gvtdvm total:278 pass:265 dwarn:0 dfail:0 fail:0 skip:13 time: 434s
fi-snb-2520m total:278 pass:250 dwarn:0 dfail:0 fail:0 skip:28 time: 527s
fi-snb-2600 total:278 pass:249 dwarn:0 dfail:0 fail:0 skip:29 time: 407s
38a5db0d3643f519d86f42029b7a93e624807a71 drm-tip: 2017y-04m-04d-13h-24m-06s UTC integration manifest
8da28f9 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_4397/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] drm/i915: Only report a wakeup if the waiter was truly asleep
2017-04-04 14:38 [PATCH] drm/i915: Only report a wakeup if the waiter was truly asleep Chris Wilson
2017-04-04 15:10 ` ✓ Fi.CI.BAT: success for " Patchwork
@ 2017-04-05 10:40 ` kbuild test robot
2017-04-05 12:20 ` kbuild test robot
` (2 subsequent siblings)
4 siblings, 0 replies; 10+ 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] 10+ messages in thread
* Re: [PATCH] drm/i915: Only report a wakeup if the waiter was truly asleep
2017-04-04 14:38 [PATCH] drm/i915: Only report a wakeup if the waiter was truly asleep Chris Wilson
2017-04-04 15:10 ` ✓ Fi.CI.BAT: success for " Patchwork
2017-04-05 10:40 ` [PATCH] " kbuild test robot
@ 2017-04-05 12:20 ` kbuild test robot
2017-04-05 12:47 ` [PATCH v2] " Chris Wilson
2017-04-05 13:06 ` ✓ Fi.CI.BAT: success for drm/i915: Only report a wakeup if the waiter was truly asleep (rev2) Patchwork
4 siblings, 0 replies; 10+ 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] 10+ messages in thread
* [PATCH v2] drm/i915: Only report a wakeup if the waiter was truly asleep
2017-04-04 14:38 [PATCH] drm/i915: Only report a wakeup if the waiter was truly asleep Chris Wilson
` (2 preceding siblings ...)
2017-04-05 12:20 ` kbuild test robot
@ 2017-04-05 12:47 ` Chris Wilson
2017-04-06 8:04 ` Tvrtko Ursulin
2017-04-05 13:06 ` ✓ Fi.CI.BAT: success for drm/i915: Only report a wakeup if the waiter was truly asleep (rev2) Patchwork
4 siblings, 1 reply; 10+ messages in thread
From: Chris Wilson @ 2017-04-05 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.
v2: Defend against !CONFIG_SMP
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 | 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, &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] 10+ messages in thread
* Re: [PATCH v2] drm/i915: Only report a wakeup if the waiter was truly asleep
2017-04-05 12:47 ` [PATCH v2] " Chris Wilson
@ 2017-04-06 8:04 ` Tvrtko Ursulin
2017-04-06 8:16 ` Chris Wilson
0 siblings, 1 reply; 10+ messages in thread
From: Tvrtko Ursulin @ 2017-04-06 8:04 UTC (permalink / raw)
To: Chris Wilson, intel-gfx
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@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 | 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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] drm/i915: Only report a wakeup if the waiter was truly asleep
2017-04-06 8:04 ` Tvrtko Ursulin
@ 2017-04-06 8:16 ` Chris Wilson
2017-04-06 8:55 ` Tvrtko Ursulin
0 siblings, 1 reply; 10+ messages in thread
From: Chris Wilson @ 2017-04-06 8:16 UTC (permalink / raw)
To: Tvrtko Ursulin; +Cc: intel-gfx
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@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 | 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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] drm/i915: Only report a wakeup if the waiter was truly asleep
2017-04-06 8:16 ` Chris Wilson
@ 2017-04-06 8:55 ` Tvrtko Ursulin
2017-04-06 9:18 ` Chris Wilson
0 siblings, 1 reply; 10+ messages in thread
From: Tvrtko Ursulin @ 2017-04-06 8:55 UTC (permalink / raw)
To: Chris Wilson, intel-gfx
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@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 | 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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] drm/i915: Only report a wakeup if the waiter was truly asleep
2017-04-06 8:55 ` Tvrtko Ursulin
@ 2017-04-06 9:18 ` Chris Wilson
0 siblings, 0 replies; 10+ messages in thread
From: Chris Wilson @ 2017-04-06 9:18 UTC (permalink / raw)
To: Tvrtko Ursulin; +Cc: intel-gfx
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=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 | 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
^ permalink raw reply [flat|nested] 10+ messages in thread
* ✓ Fi.CI.BAT: success for drm/i915: Only report a wakeup if the waiter was truly asleep (rev2)
2017-04-04 14:38 [PATCH] drm/i915: Only report a wakeup if the waiter was truly asleep Chris Wilson
` (3 preceding siblings ...)
2017-04-05 12:47 ` [PATCH v2] " Chris Wilson
@ 2017-04-05 13:06 ` Patchwork
4 siblings, 0 replies; 10+ messages in thread
From: Patchwork @ 2017-04-05 13:06 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx
== Series Details ==
Series: drm/i915: Only report a wakeup if the waiter was truly asleep (rev2)
URL : https://patchwork.freedesktop.org/series/22445/
State : success
== Summary ==
Series 22445v2 drm/i915: Only report a wakeup if the waiter was truly asleep
https://patchwork.freedesktop.org/api/1.0/series/22445/revisions/2/mbox/
Test gem_exec_suspend:
Subgroup basic-s4-devices:
dmesg-warn -> PASS (fi-kbl-7560u) fdo#100125
Test kms_pipe_crc_basic:
Subgroup hang-read-crc-pipe-a:
incomplete -> PASS (fi-snb-2520m)
fdo#100125 https://bugs.freedesktop.org/show_bug.cgi?id=100125
fi-bdw-5557u total:278 pass:267 dwarn:0 dfail:0 fail:0 skip:11 time: 430s
fi-bdw-gvtdvm total:278 pass:256 dwarn:8 dfail:0 fail:0 skip:14 time: 429s
fi-bsw-n3050 total:278 pass:242 dwarn:0 dfail:0 fail:0 skip:36 time: 583s
fi-bxt-j4205 total:278 pass:259 dwarn:0 dfail:0 fail:0 skip:19 time: 505s
fi-bxt-t5700 total:278 pass:258 dwarn:0 dfail:0 fail:0 skip:20 time: 560s
fi-byt-j1900 total:278 pass:254 dwarn:0 dfail:0 fail:0 skip:24 time: 495s
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: 409s
fi-hsw-4770r total:278 pass:262 dwarn:0 dfail:0 fail:0 skip:16 time: 405s
fi-ilk-650 total:278 pass:228 dwarn:0 dfail:0 fail:0 skip:50 time: 415s
fi-ivb-3520m total:278 pass:260 dwarn:0 dfail:0 fail:0 skip:18 time: 488s
fi-ivb-3770 total:278 pass:260 dwarn:0 dfail:0 fail:0 skip:18 time: 465s
fi-kbl-7500u total:278 pass:260 dwarn:0 dfail:0 fail:0 skip:18 time: 455s
fi-kbl-7560u total:278 pass:268 dwarn:0 dfail:0 fail:0 skip:10 time: 572s
fi-skl-6260u total:278 pass:268 dwarn:0 dfail:0 fail:0 skip:10 time: 469s
fi-skl-6700hq total:278 pass:261 dwarn:0 dfail:0 fail:0 skip:17 time: 577s
fi-skl-6700k total:278 pass:256 dwarn:4 dfail:0 fail:0 skip:18 time: 461s
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: 436s
fi-snb-2520m total:278 pass:250 dwarn:0 dfail:0 fail:0 skip:28 time: 527s
fi-snb-2600 total:278 pass:249 dwarn:0 dfail:0 fail:0 skip:29 time: 397s
3c5f424b9e2697352118ee33514b29b7cd57f69c drm-tip: 2017y-04m-05d-11h-46m-45s UTC integration manifest
b1843ef 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_4407/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2017-04-06 9:18 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-04-04 14:38 [PATCH] drm/i915: Only report a wakeup if the waiter was truly asleep Chris Wilson
2017-04-04 15:10 ` ✓ Fi.CI.BAT: success for " Patchwork
2017-04-05 10:40 ` [PATCH] " kbuild test robot
2017-04-05 12:20 ` kbuild test robot
2017-04-05 12:47 ` [PATCH v2] " Chris Wilson
2017-04-06 8:04 ` Tvrtko Ursulin
2017-04-06 8:16 ` Chris Wilson
2017-04-06 8:55 ` Tvrtko Ursulin
2017-04-06 9:18 ` Chris Wilson
2017-04-05 13:06 ` ✓ Fi.CI.BAT: success for drm/i915: Only report a wakeup if the waiter was truly asleep (rev2) Patchwork
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.