[PATCH] drm/i915: Avoid uninterruptible spinning in debugfs

[PATCH] drm/i915: Avoid uninterruptible spinning in debugfs

[Chris Wilson]

If we get caught in a kernel bug, we may never idle. Let the user regain
control of their system^Wterminal by responding to SIGINT!

Reported-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index f3fa31d840f5..baf8b548621c 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -3947,6 +3947,10 @@ i915_drop_caches_set(void *data, u64 val)
 
 	if (val & DROP_IDLE) {
 		do {
+			if (signal_pending(current)) {
+				ret = -EINTR;
+				goto out;
+			}
 			if (READ_ONCE(i915->gt.active_requests))
 				flush_delayed_work(&i915->gt.retire_work);
 			drain_delayed_work(&i915->gt.idle_work);
-- 
2.20.1

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

Re: [PATCH] drm/i915: Avoid uninterruptible spinning in debugfs

[Chris Wilson]

Quoting Chris Wilson (2019-02-01 10:22:48)
> If we get caught in a kernel bug, we may never idle. Let the user regain
> control of their system^Wterminal by responding to SIGINT!
> 
> Reported-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index f3fa31d840f5..baf8b548621c 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -3947,6 +3947,10 @@ i915_drop_caches_set(void *data, u64 val)
>  
>         if (val & DROP_IDLE) {
>                 do {
> +                       if (signal_pending(current)) {
> +                               ret = -EINTR;
> +                               goto out;
> +                       }
>                         if (READ_ONCE(i915->gt.active_requests))
>                                 flush_delayed_work(&i915->gt.retire_work);
>                         drain_delayed_work(&i915->gt.idle_work);

Drain delayed work will want similar treatment.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH] drm/i915: Avoid uninterruptible spinning in debugfs

[Chris Wilson]

If we get caught in a kernel bug, we may never idle. Let the user regain
control of their system^Wterminal by responding to SIGINT!

v2: Push the signal checking into the loop around flush_work.

Reported-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c            | 14 ++++++++------
 drivers/gpu/drm/i915/i915_gem.c                | 10 +++++++---
 drivers/gpu/drm/i915/i915_utils.h              | 18 +++++++++++++++---
 .../gpu/drm/i915/selftests/i915_gem_context.c  |  5 ++++-
 4 files changed, 34 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index f3fa31d840f5..44fa34f4ebbc 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -3945,12 +3945,14 @@ i915_drop_caches_set(void *data, u64 val)
 		i915_gem_shrink_all(i915);
 	fs_reclaim_release(GFP_KERNEL);
 
-	if (val & DROP_IDLE) {
-		do {
-			if (READ_ONCE(i915->gt.active_requests))
-				flush_delayed_work(&i915->gt.retire_work);
-			drain_delayed_work(&i915->gt.idle_work);
-		} while (READ_ONCE(i915->gt.awake));
+	if (val & DROP_IDLE && READ_ONCE(i915->gt.awake)) {
+		if (drain_delayed_work_state(&i915->gt.retire_work,
+					     TASK_INTERRUPTIBLE) ||
+		    drain_delayed_work_state(&i915->gt.idle_work,
+					     TASK_INTERRUPTIBLE)) {
+			ret = -EINTR;
+			goto out;
+		}
 	}
 
 	if (val & DROP_FREED)
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 4067eeaee78a..e47d53e9b634 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4499,13 +4499,17 @@ int i915_gem_suspend(struct drm_i915_private *i915)
 	mutex_unlock(&i915->drm.struct_mutex);
 	i915_reset_flush(i915);
 
-	drain_delayed_work(&i915->gt.retire_work);
-
 	/*
 	 * As the idle_work is rearming if it detects a race, play safe and
 	 * repeat the flush until it is definitely idle.
 	 */
-	drain_delayed_work(&i915->gt.idle_work);
+	if (drain_delayed_work_state(&i915->gt.retire_work,
+				     TASK_INTERRUPTIBLE) ||
+	    drain_delayed_work_state(&i915->gt.idle_work,
+				     TASK_INTERRUPTIBLE)) {
+		ret = -EINTR;
+		goto err_unlock;
+	}
 
 	/*
 	 * Assert that we successfully flushed all the work and
diff --git a/drivers/gpu/drm/i915/i915_utils.h b/drivers/gpu/drm/i915/i915_utils.h
index fcc751aa1ea8..6866b85e4239 100644
--- a/drivers/gpu/drm/i915/i915_utils.h
+++ b/drivers/gpu/drm/i915/i915_utils.h
@@ -25,6 +25,8 @@
 #ifndef __I915_UTILS_H
 #define __I915_UTILS_H
 
+#include <linux/sched/signal.h>
+
 #undef WARN_ON
 /* Many gcc seem to no see through this and fall over :( */
 #if 0
@@ -148,12 +150,22 @@ static inline void __list_del_many(struct list_head *head,
  * by requeueing itself. Note, that if the worker never cancels itself,
  * we will spin forever.
  */
-static inline void drain_delayed_work(struct delayed_work *dw)
+
+static inline int drain_delayed_work_state(struct delayed_work *dw, int state)
 {
 	do {
-		while (flush_delayed_work(dw))
-			;
+		do {
+			if (signal_pending_state(state, current))
+				return -EINTR;
+		} while (flush_delayed_work(dw));
 	} while (delayed_work_pending(dw));
+
+	return 0;
+}
+
+static inline void drain_delayed_work(struct delayed_work *dw)
+{
+	drain_delayed_work_state(dw, 0);
 }
 
 static inline const char *yesno(bool v)
diff --git a/drivers/gpu/drm/i915/selftests/i915_gem_context.c b/drivers/gpu/drm/i915/selftests/i915_gem_context.c
index 2b423128002c..a87998b90bf6 100644
--- a/drivers/gpu/drm/i915/selftests/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/selftests/i915_gem_context.c
@@ -1686,8 +1686,11 @@ static int __igt_switch_to_kernel_context(struct drm_i915_private *i915,
 		/* XXX Bonus points for proving we are the kernel context! */
 
 		mutex_unlock(&i915->drm.struct_mutex);
-		drain_delayed_work(&i915->gt.idle_work);
+		err = drain_delayed_work_state(&i915->gt.idle_work,
+					       TASK_KILLABLE);
 		mutex_lock(&i915->drm.struct_mutex);
+		if (err)
+			return -EINTR;
 	}
 
 	if (igt_flush_test(i915, I915_WAIT_LOCKED))
-- 
2.20.1

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

✗ Fi.CI.BAT: failure for drm/i915: Avoid uninterruptible spinning in debugfs (rev2)

[Patchwork]

== Series Details ==

Series: drm/i915: Avoid uninterruptible spinning in debugfs (rev2)
URL   : https://patchwork.freedesktop.org/series/56079/
State : failure

== Summary ==

Applying: drm/i915: Avoid uninterruptible spinning in debugfs
error: sha1 information is lacking or useless (drivers/gpu/drm/i915/i915_debugfs.c).
error: could not build fake ancestor
hint: Use 'git am --show-current-patch' to see the failed patch
Patch failed at 0001 drm/i915: Avoid uninterruptible spinning in debugfs
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

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

✓ Fi.CI.BAT: success for drm/i915: Avoid uninterruptible spinning in debugfs

[Patchwork]

== Series Details ==

Series: drm/i915: Avoid uninterruptible spinning in debugfs
URL   : https://patchwork.freedesktop.org/series/56079/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_5524 -> Patchwork_12117
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/56079/revisions/1/mbox/

Known issues
------------

  Here are the changes found in Patchwork_12117 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@amdgpu/amd_basic@userptr:
    - fi-kbl-8809g:       PASS -> DMESG-WARN [fdo#108965]

  * igt@kms_chamelium@hdmi-hpd-fast:
    - fi-kbl-7500u:       PASS -> FAIL [fdo#109485]

  * igt@kms_pipe_crc_basic@nonblocking-crc-pipe-a-frame-sequence:
    - fi-byt-clapper:     PASS -> FAIL [fdo#103191] / [fdo#107362]

  
#### Possible fixes ####

  * igt@pm_rpm@basic-pci-d3-state:
    - fi-bsw-kefka:       {SKIP} [fdo#109271] -> PASS

  * igt@pm_rpm@basic-rte:
    - fi-bsw-kefka:       FAIL [fdo#108800] -> PASS

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#103191]: https://bugs.freedesktop.org/show_bug.cgi?id=103191
  [fdo#107362]: https://bugs.freedesktop.org/show_bug.cgi?id=107362
  [fdo#108622]: https://bugs.freedesktop.org/show_bug.cgi?id=108622
  [fdo#108800]: https://bugs.freedesktop.org/show_bug.cgi?id=108800
  [fdo#108965]: https://bugs.freedesktop.org/show_bug.cgi?id=108965
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109485]: https://bugs.freedesktop.org/show_bug.cgi?id=109485


Participating hosts (49 -> 45)
------------------------------

  Additional (2): fi-hsw-4770r fi-ivb-3770 
  Missing    (6): fi-kbl-soraka fi-ilk-m540 fi-byt-squawks fi-bsw-cyan fi-ctg-p8600 fi-icl-y 


Build changes
-------------

    * Linux: CI_DRM_5524 -> Patchwork_12117

  CI_DRM_5524: 94a739eee19da904a40e612464dfa9f1326accfb @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4802: 4049adf01014af077df2174def4fadf7cecb066e @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_12117: 4bf798b7ee1a9d54d571dc9c0470be480b209c9c @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

4bf798b7ee1a drm/i915: Avoid uninterruptible spinning in debugfs

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_12117/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Avoid uninterruptible spinning in debugfs

[Tvrtko Ursulin]

On 01/02/2019 10:52, Chris Wilson wrote:
> If we get caught in a kernel bug, we may never idle. Let the user regain
> control of their system^Wterminal by responding to SIGINT!
> 
> v2: Push the signal checking into the loop around flush_work.
> 
> Reported-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_debugfs.c            | 14 ++++++++------
>   drivers/gpu/drm/i915/i915_gem.c                | 10 +++++++---
>   drivers/gpu/drm/i915/i915_utils.h              | 18 +++++++++++++++---
>   .../gpu/drm/i915/selftests/i915_gem_context.c  |  5 ++++-
>   4 files changed, 34 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index f3fa31d840f5..44fa34f4ebbc 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -3945,12 +3945,14 @@ i915_drop_caches_set(void *data, u64 val)
>   		i915_gem_shrink_all(i915);
>   	fs_reclaim_release(GFP_KERNEL);
>   
> -	if (val & DROP_IDLE) {
> -		do {
> -			if (READ_ONCE(i915->gt.active_requests))
> -				flush_delayed_work(&i915->gt.retire_work);
> -			drain_delayed_work(&i915->gt.idle_work);
> -		} while (READ_ONCE(i915->gt.awake));
> +	if (val & DROP_IDLE && READ_ONCE(i915->gt.awake)) {
> +		if (drain_delayed_work_state(&i915->gt.retire_work,
> +					     TASK_INTERRUPTIBLE) ||
> +		    drain_delayed_work_state(&i915->gt.idle_work,
> +					     TASK_INTERRUPTIBLE)) {
> +			ret = -EINTR;
> +			goto out;
> +		}

Shrinker remains only killable so still not Ctrl-C in all cases here. 
Don't know, not hot not cold. Will re-think it next week.

>   	}
>   
>   	if (val & DROP_FREED)
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 4067eeaee78a..e47d53e9b634 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -4499,13 +4499,17 @@ int i915_gem_suspend(struct drm_i915_private *i915)
>   	mutex_unlock(&i915->drm.struct_mutex);
>   	i915_reset_flush(i915);
>   
> -	drain_delayed_work(&i915->gt.retire_work);
> -
>   	/*
>   	 * As the idle_work is rearming if it detects a race, play safe and
>   	 * repeat the flush until it is definitely idle.
>   	 */
> -	drain_delayed_work(&i915->gt.idle_work);
> +	if (drain_delayed_work_state(&i915->gt.retire_work,
> +				     TASK_INTERRUPTIBLE) ||
> +	    drain_delayed_work_state(&i915->gt.idle_work,
> +				     TASK_INTERRUPTIBLE)) {
> +		ret = -EINTR;
> +		goto err_unlock;
> +	}

I'm no suspend expert but it sounds unexpected there would be signals 
involved in the process. Does it have an userspace component? We don't 
bother with interruptible mutex on this path either.

>   
>   	/*
>   	 * Assert that we successfully flushed all the work and
> diff --git a/drivers/gpu/drm/i915/i915_utils.h b/drivers/gpu/drm/i915/i915_utils.h
> index fcc751aa1ea8..6866b85e4239 100644
> --- a/drivers/gpu/drm/i915/i915_utils.h
> +++ b/drivers/gpu/drm/i915/i915_utils.h
> @@ -25,6 +25,8 @@
>   #ifndef __I915_UTILS_H
>   #define __I915_UTILS_H
>   
> +#include <linux/sched/signal.h>
> +
>   #undef WARN_ON
>   /* Many gcc seem to no see through this and fall over :( */
>   #if 0
> @@ -148,12 +150,22 @@ static inline void __list_del_many(struct list_head *head,
>    * by requeueing itself. Note, that if the worker never cancels itself,
>    * we will spin forever.
>    */
> -static inline void drain_delayed_work(struct delayed_work *dw)
> +
> +static inline int drain_delayed_work_state(struct delayed_work *dw, int state)
>   {
>   	do {
> -		while (flush_delayed_work(dw))
> -			;
> +		do {
> +			if (signal_pending_state(state, current))
> +				return -EINTR;
> +		} while (flush_delayed_work(dw));
>   	} while (delayed_work_pending(dw));
> +
> +	return 0;
> +}
> +
> +static inline void drain_delayed_work(struct delayed_work *dw)
> +{
> +	drain_delayed_work_state(dw, 0);

0 would be TASK_RUNNING. signal_pending_state bails out in this case so 
that's good.

>   }
>   
>   static inline const char *yesno(bool v)
> diff --git a/drivers/gpu/drm/i915/selftests/i915_gem_context.c b/drivers/gpu/drm/i915/selftests/i915_gem_context.c
> index 2b423128002c..a87998b90bf6 100644
> --- a/drivers/gpu/drm/i915/selftests/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/selftests/i915_gem_context.c
> @@ -1686,8 +1686,11 @@ static int __igt_switch_to_kernel_context(struct drm_i915_private *i915,
>   		/* XXX Bonus points for proving we are the kernel context! */
>   
>   		mutex_unlock(&i915->drm.struct_mutex);
> -		drain_delayed_work(&i915->gt.idle_work);
> +		err = drain_delayed_work_state(&i915->gt.idle_work,
> +					       TASK_KILLABLE);
>   		mutex_lock(&i915->drm.struct_mutex);
> +		if (err)
> +			return -EINTR;
>   	}
>   
>   	if (igt_flush_test(i915, I915_WAIT_LOCKED))
> 

Regards,

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

Re: [PATCH] drm/i915: Avoid uninterruptible spinning in debugfs

[Chris Wilson]

Quoting Tvrtko Ursulin (2019-02-01 17:29:45)
> 
> On 01/02/2019 10:52, Chris Wilson wrote:
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > index 4067eeaee78a..e47d53e9b634 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -4499,13 +4499,17 @@ int i915_gem_suspend(struct drm_i915_private *i915)
> >       mutex_unlock(&i915->drm.struct_mutex);
> >       i915_reset_flush(i915);
> >   
> > -     drain_delayed_work(&i915->gt.retire_work);
> > -
> >       /*
> >        * As the idle_work is rearming if it detects a race, play safe and
> >        * repeat the flush until it is definitely idle.
> >        */
> > -     drain_delayed_work(&i915->gt.idle_work);
> > +     if (drain_delayed_work_state(&i915->gt.retire_work,
> > +                                  TASK_INTERRUPTIBLE) ||
> > +         drain_delayed_work_state(&i915->gt.idle_work,
> > +                                  TASK_INTERRUPTIBLE)) {
> > +             ret = -EINTR;
> > +             goto err_unlock;
> > +     }
> 
> I'm no suspend expert but it sounds unexpected there would be signals 
> involved in the process. Does it have an userspace component? We don't 
> bother with interruptible mutex on this path either.

You wouldn't believe what users get up to! And they then file a bug
report that they interrupted suspend and it said EINTR...

> > diff --git a/drivers/gpu/drm/i915/i915_utils.h b/drivers/gpu/drm/i915/i915_utils.h
> > index fcc751aa1ea8..6866b85e4239 100644
> > --- a/drivers/gpu/drm/i915/i915_utils.h
> > +++ b/drivers/gpu/drm/i915/i915_utils.h
> > @@ -25,6 +25,8 @@
> >   #ifndef __I915_UTILS_H
> >   #define __I915_UTILS_H
> >   
> > +#include <linux/sched/signal.h>
> > +
> >   #undef WARN_ON
> >   /* Many gcc seem to no see through this and fall over :( */
> >   #if 0
> > @@ -148,12 +150,22 @@ static inline void __list_del_many(struct list_head *head,
> >    * by requeueing itself. Note, that if the worker never cancels itself,
> >    * we will spin forever.
> >    */
> > -static inline void drain_delayed_work(struct delayed_work *dw)
> > +
> > +static inline int drain_delayed_work_state(struct delayed_work *dw, int state)
> >   {
> >       do {
> > -             while (flush_delayed_work(dw))
> > -                     ;
> > +             do {
> > +                     if (signal_pending_state(state, current))
> > +                             return -EINTR;
> > +             } while (flush_delayed_work(dw));
> >       } while (delayed_work_pending(dw));
> > +
> > +     return 0;
> > +}
> > +
> > +static inline void drain_delayed_work(struct delayed_work *dw)
> > +{
> > +     drain_delayed_work_state(dw, 0);
> 
> 0 would be TASK_RUNNING. signal_pending_state bails out in this case so 
> that's good.

The same trick is used in mutex_lock_(interruptible|killable) to map
onto a common handler for mutex_lock, so I don't think its unintentional
;)
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx