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

[PATCH] 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.

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

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

[Patchwork]

== 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

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

[kbuild test robot]

[-- 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

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

[kbuild test robot]

[-- 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

[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=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

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

[Patchwork]

== 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

Re: [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=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

Re: [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=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

Re: [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=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

Re: [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=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

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

[Tvrtko Ursulin]

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

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

[Chris Wilson]

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

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

[Tvrtko Ursulin]

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

[PATCH] 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. 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