[1/2] drm/i915: Comment userptr recursion on struct_mutex
diff mbox series

Message ID 20190814124933.19056-1-daniel.vetter@ffwll.ch
State New
Headers show
Series
  • [1/2] drm/i915: Comment userptr recursion on struct_mutex
Related show

Commit Message

Daniel Vetter Aug. 14, 2019, 12:49 p.m. UTC
Discussed this a bit with Chris, I think a comment here is warranted
that this will be bad once we have more than one i915 instance. And
lockdep won't catch it.

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/i915/gem/i915_gem_userptr.c | 6 ++++++
 1 file changed, 6 insertions(+)

Comments

Daniel Vetter Aug. 14, 2019, 7:22 p.m. UTC | #1
On Wed, Aug 14, 2019 at 02:49:32PM +0200, Daniel Vetter wrote:
> Discussed this a bit with Chris, I think a comment here is warranted
> that this will be bad once we have more than one i915 instance. And
> lockdep won't catch it.
> 
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>  drivers/gpu/drm/i915/gem/i915_gem_userptr.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
> index 74da35611d7c..70dc506a5426 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
> @@ -135,6 +135,12 @@ userptr_mn_invalidate_range_start(struct mmu_notifier *_mn,
>  			switch (mutex_trylock_recursive(unlock)) {
>  			default:
>  			case MUTEX_TRYLOCK_FAILED:
> +				/*
> +				 * NOTE: This only works because there's only
> +				 * ever one i915-style struct_mutex in the
> +				 * entire system. If we could have two i915
> +				 * instances, this would deadlock.
> +				 */

While fixing up annotations for the 2nd patch I though more about this,
and I'm not sold that "there's only one" makes sense. Scenario:

thread A:
get_pages
-> mutex_lock(obj->mm.lock)
-> fs_reclaim
-> mmu_notifier picks range of memory we're interested in
-> mutex_lock_killable(struct_mutex)

Why can this not deadlock against any other thread which does:

mutex_lock(struct_mutex)
-> get_pages
-> mutex_lock(obj->mm.lock)

They would both need to pick the same object, that's right now at a 0->1
transition for pages_pin_count. Plus a long list of other unhappy
circumstances ...

Note that this is different from the same annotation in shrinker_lock:
That one is only used if current_is_kswapd is, which guarantees we're not
holding a few unfortunate locks.
-Daniel

>  				if (mutex_lock_killable_nested(unlock, I915_MM_SHRINKER)) {
>  					i915_gem_object_put(obj);
>  					return -EINTR;
> -- 
> 2.22.0
>

Patch
diff mbox series

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
index 74da35611d7c..70dc506a5426 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
@@ -135,6 +135,12 @@  userptr_mn_invalidate_range_start(struct mmu_notifier *_mn,
 			switch (mutex_trylock_recursive(unlock)) {
 			default:
 			case MUTEX_TRYLOCK_FAILED:
+				/*
+				 * NOTE: This only works because there's only
+				 * ever one i915-style struct_mutex in the
+				 * entire system. If we could have two i915
+				 * instances, this would deadlock.
+				 */
 				if (mutex_lock_killable_nested(unlock, I915_MM_SHRINKER)) {
 					i915_gem_object_put(obj);
 					return -EINTR;