diff mbox series

[2/2] drm/i915: Removing polling for struct_mutex from vmap shrinker

Message ID 20190109164204.23935-2-chris@chris-wilson.co.uk (mailing list archive)
State New, archived
Headers show
Series [1/2] drm/i915: Use mutex_lock_killable() from inside the shrinker | expand

Commit Message

Chris Wilson Jan. 9, 2019, 4:42 p.m. UTC
The wait-for-idle used from within the shrinker_lock_uninterruptible
depends on the struct_mutex locking state being known and declared to
i915_request_wait(). As it is conceivable that we reach the vmap
notifier from underneath struct_mutex (and so keep on relying on the
mutex_trylock_recursive), we should not blindly call i915_request_wait.

In the process we can remove the dubious polling to acquire
struct_mutex, and simply act, or not, on a successful trylock.

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

Comments

Tvrtko Ursulin Jan. 10, 2019, 10:28 a.m. UTC | #1
On 09/01/2019 16:42, Chris Wilson wrote:
> The wait-for-idle used from within the shrinker_lock_uninterruptible
> depends on the struct_mutex locking state being known and declared to
> i915_request_wait(). As it is conceivable that we reach the vmap
> notifier from underneath struct_mutex (and so keep on relying on the
> mutex_trylock_recursive), we should not blindly call i915_request_wait.
> 
> In the process we can remove the dubious polling to acquire
> struct_mutex, and simply act, or not, on a successful trylock.

Makes sense and removes the special casing. I only miss the historical 
reference as to why did vmap notifier need this special handling?

> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_gem_shrinker.c | 35 +++---------------------
>   1 file changed, 4 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_shrinker.c b/drivers/gpu/drm/i915/i915_gem_shrinker.c
> index 8ad9519779cc..6cc2b964c955 100644
> --- a/drivers/gpu/drm/i915/i915_gem_shrinker.c
> +++ b/drivers/gpu/drm/i915/i915_gem_shrinker.c
> @@ -385,31 +385,6 @@ i915_gem_shrinker_scan(struct shrinker *shrinker, struct shrink_control *sc)
>   	return sc->nr_scanned ? freed : SHRINK_STOP;
>   }
>   
> -static bool
> -shrinker_lock_uninterruptible(struct drm_i915_private *i915, bool *unlock,
> -			      int timeout_ms)
> -{
> -	unsigned long timeout = jiffies + msecs_to_jiffies_timeout(timeout_ms);
> -
> -	do {
> -		if (i915_gem_wait_for_idle(i915,
> -					   0, MAX_SCHEDULE_TIMEOUT) == 0 &&
> -		    shrinker_lock(i915, 0, unlock))
> -			break;
> -
> -		schedule_timeout_killable(1);
> -		if (fatal_signal_pending(current))
> -			return false;
> -
> -		if (time_after(jiffies, timeout)) {
> -			pr_err("Unable to lock GPU to purge memory.\n");
> -			return false;
> -		}
> -	} while (1);
> -
> -	return true;
> -}
> -
>   static int
>   i915_gem_shrinker_oom(struct notifier_block *nb, unsigned long event, void *ptr)
>   {
> @@ -461,16 +436,14 @@ i915_gem_shrinker_vmap(struct notifier_block *nb, unsigned long event, void *ptr
>   	struct i915_vma *vma, *next;
>   	unsigned long freed_pages = 0;
>   	bool unlock;
> -	int ret;
>   
> -	if (!shrinker_lock_uninterruptible(i915, &unlock, 5000))
> +	if (!shrinker_lock(i915, 0, &unlock))
>   		return NOTIFY_DONE;
>   
>   	/* Force everything onto the inactive lists */
> -	ret = i915_gem_wait_for_idle(i915,
> -				     I915_WAIT_LOCKED,
> -				     MAX_SCHEDULE_TIMEOUT);
> -	if (ret)
> +	if (i915_gem_wait_for_idle(i915,
> +				   I915_WAIT_LOCKED,
> +				   MAX_SCHEDULE_TIMEOUT))
>   		goto out;
>   
>   	intel_runtime_pm_get(i915);
> 

Regards,

Tvrtko
Chris Wilson Jan. 10, 2019, 10:39 a.m. UTC | #2
Quoting Tvrtko Ursulin (2019-01-10 10:28:14)
> 
> On 09/01/2019 16:42, Chris Wilson wrote:
> > The wait-for-idle used from within the shrinker_lock_uninterruptible
> > depends on the struct_mutex locking state being known and declared to
> > i915_request_wait(). As it is conceivable that we reach the vmap
> > notifier from underneath struct_mutex (and so keep on relying on the
> > mutex_trylock_recursive), we should not blindly call i915_request_wait.
> > 
> > In the process we can remove the dubious polling to acquire
> > struct_mutex, and simply act, or not, on a successful trylock.
> 
> Makes sense and removes the special casing. I only miss the historical 
> reference as to why did vmap notifier need this special handling?

Cargo culting. I copied the code I had for oom-notifier, thinking the
heavier the hammer the better.
-Chris
Tvrtko Ursulin Jan. 10, 2019, 1:16 p.m. UTC | #3
On 10/01/2019 10:39, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2019-01-10 10:28:14)
>>
>> On 09/01/2019 16:42, Chris Wilson wrote:
>>> The wait-for-idle used from within the shrinker_lock_uninterruptible
>>> depends on the struct_mutex locking state being known and declared to
>>> i915_request_wait(). As it is conceivable that we reach the vmap
>>> notifier from underneath struct_mutex (and so keep on relying on the
>>> mutex_trylock_recursive), we should not blindly call i915_request_wait.
>>>
>>> In the process we can remove the dubious polling to acquire
>>> struct_mutex, and simply act, or not, on a successful trylock.
>>
>> Makes sense and removes the special casing. I only miss the historical
>> reference as to why did vmap notifier need this special handling?
> 
> Cargo culting. I copied the code I had for oom-notifier, thinking the
> heavier the hammer the better.

Go for it.

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

Tvrtko
Chris Wilson Jan. 10, 2019, 1:48 p.m. UTC | #4
Quoting Tvrtko Ursulin (2019-01-10 13:16:30)
> 
> On 10/01/2019 10:39, Chris Wilson wrote:
> > Quoting Tvrtko Ursulin (2019-01-10 10:28:14)
> >>
> >> On 09/01/2019 16:42, Chris Wilson wrote:
> >>> The wait-for-idle used from within the shrinker_lock_uninterruptible
> >>> depends on the struct_mutex locking state being known and declared to
> >>> i915_request_wait(). As it is conceivable that we reach the vmap
> >>> notifier from underneath struct_mutex (and so keep on relying on the
> >>> mutex_trylock_recursive), we should not blindly call i915_request_wait.
> >>>
> >>> In the process we can remove the dubious polling to acquire
> >>> struct_mutex, and simply act, or not, on a successful trylock.
> >>
> >> Makes sense and removes the special casing. I only miss the historical
> >> reference as to why did vmap notifier need this special handling?
> > 
> > Cargo culting. I copied the code I had for oom-notifier, thinking the
> > heavier the hammer the better.
> 
> Go for it.
> 
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Definitely a long way from perfection! Hopefully not one filled with
later regret.

Ta,
-Chris
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/i915_gem_shrinker.c b/drivers/gpu/drm/i915/i915_gem_shrinker.c
index 8ad9519779cc..6cc2b964c955 100644
--- a/drivers/gpu/drm/i915/i915_gem_shrinker.c
+++ b/drivers/gpu/drm/i915/i915_gem_shrinker.c
@@ -385,31 +385,6 @@  i915_gem_shrinker_scan(struct shrinker *shrinker, struct shrink_control *sc)
 	return sc->nr_scanned ? freed : SHRINK_STOP;
 }
 
-static bool
-shrinker_lock_uninterruptible(struct drm_i915_private *i915, bool *unlock,
-			      int timeout_ms)
-{
-	unsigned long timeout = jiffies + msecs_to_jiffies_timeout(timeout_ms);
-
-	do {
-		if (i915_gem_wait_for_idle(i915,
-					   0, MAX_SCHEDULE_TIMEOUT) == 0 &&
-		    shrinker_lock(i915, 0, unlock))
-			break;
-
-		schedule_timeout_killable(1);
-		if (fatal_signal_pending(current))
-			return false;
-
-		if (time_after(jiffies, timeout)) {
-			pr_err("Unable to lock GPU to purge memory.\n");
-			return false;
-		}
-	} while (1);
-
-	return true;
-}
-
 static int
 i915_gem_shrinker_oom(struct notifier_block *nb, unsigned long event, void *ptr)
 {
@@ -461,16 +436,14 @@  i915_gem_shrinker_vmap(struct notifier_block *nb, unsigned long event, void *ptr
 	struct i915_vma *vma, *next;
 	unsigned long freed_pages = 0;
 	bool unlock;
-	int ret;
 
-	if (!shrinker_lock_uninterruptible(i915, &unlock, 5000))
+	if (!shrinker_lock(i915, 0, &unlock))
 		return NOTIFY_DONE;
 
 	/* Force everything onto the inactive lists */
-	ret = i915_gem_wait_for_idle(i915,
-				     I915_WAIT_LOCKED,
-				     MAX_SCHEDULE_TIMEOUT);
-	if (ret)
+	if (i915_gem_wait_for_idle(i915,
+				   I915_WAIT_LOCKED,
+				   MAX_SCHEDULE_TIMEOUT))
 		goto out;
 
 	intel_runtime_pm_get(i915);