| 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 |
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
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
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
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 --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);
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(-)