| Message ID | 20190103122329.19948-5-tvrtko.ursulin@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Shrinker tweaks/fixes? | expand |
Quoting Tvrtko Ursulin (2019-01-03 12:23:29) > From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > In a patch named "drm/i915: Return immediately if trylock fails for > direct-reclaim" Chris Wilson implicitly suggests it is safe to use a plain > mutex_lock instead of trylock on the I915_SHRINK_ACTIVE code paths. > > Explore this to see how the lockdep traces will look. > > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > --- > drivers/gpu/drm/i915/i915_gem_shrinker.c | 11 ++--------- > 1 file changed, 2 insertions(+), 9 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem_shrinker.c b/drivers/gpu/drm/i915/i915_gem_shrinker.c > index 97deec775f40..2d4a54851c0a 100644 > --- a/drivers/gpu/drm/i915/i915_gem_shrinker.c > +++ b/drivers/gpu/drm/i915/i915_gem_shrinker.c > @@ -47,15 +47,8 @@ shrinker_lock(struct drm_i915_private *i915, unsigned int flags, bool *unlock) > case MUTEX_TRYLOCK_FAILED: > *unlock = false; > if (flags & I915_SHRINK_ACTIVE) { > - preempt_disable(); > - do { > - cpu_relax(); > - if (mutex_trylock(&i915->drm.struct_mutex)) { > - *unlock = true; > - break; > - } > - } while (!need_resched()); > - preempt_enable(); > + mutex_lock(&i915->drm.struct_mutex); You seemed to have lost some annotation here. -Chris
On 03/01/2019 12:56, Chris Wilson wrote: > Quoting Tvrtko Ursulin (2019-01-03 12:23:29) >> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >> >> In a patch named "drm/i915: Return immediately if trylock fails for >> direct-reclaim" Chris Wilson implicitly suggests it is safe to use a plain >> mutex_lock instead of trylock on the I915_SHRINK_ACTIVE code paths. >> >> Explore this to see how the lockdep traces will look. >> >> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >> Cc: Chris Wilson <chris@chris-wilson.co.uk> >> --- >> drivers/gpu/drm/i915/i915_gem_shrinker.c | 11 ++--------- >> 1 file changed, 2 insertions(+), 9 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_gem_shrinker.c b/drivers/gpu/drm/i915/i915_gem_shrinker.c >> index 97deec775f40..2d4a54851c0a 100644 >> --- a/drivers/gpu/drm/i915/i915_gem_shrinker.c >> +++ b/drivers/gpu/drm/i915/i915_gem_shrinker.c >> @@ -47,15 +47,8 @@ shrinker_lock(struct drm_i915_private *i915, unsigned int flags, bool *unlock) >> case MUTEX_TRYLOCK_FAILED: >> *unlock = false; >> if (flags & I915_SHRINK_ACTIVE) { >> - preempt_disable(); >> - do { >> - cpu_relax(); >> - if (mutex_trylock(&i915->drm.struct_mutex)) { >> - *unlock = true; >> - break; >> - } >> - } while (!need_resched()); >> - preempt_enable(); >> + mutex_lock(&i915->drm.struct_mutex); > > You seemed to have lost some annotation here. I deliberately omitted mutex_lock_nested from this series to avoid hiding any potential lockdep issues. If there won't be any hit then we really need to improve on the test coverage. Regards, Tvrtko
diff --git a/drivers/gpu/drm/i915/i915_gem_shrinker.c b/drivers/gpu/drm/i915/i915_gem_shrinker.c index 97deec775f40..2d4a54851c0a 100644 --- a/drivers/gpu/drm/i915/i915_gem_shrinker.c +++ b/drivers/gpu/drm/i915/i915_gem_shrinker.c @@ -47,15 +47,8 @@ shrinker_lock(struct drm_i915_private *i915, unsigned int flags, bool *unlock) case MUTEX_TRYLOCK_FAILED: *unlock = false; if (flags & I915_SHRINK_ACTIVE) { - preempt_disable(); - do { - cpu_relax(); - if (mutex_trylock(&i915->drm.struct_mutex)) { - *unlock = true; - break; - } - } while (!need_resched()); - preempt_enable(); + mutex_lock(&i915->drm.struct_mutex); + *unlock = true; } return *unlock;