All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] drm/i915/userptr: Avoid struct_mutex recursion for mmu_invalidate_range_start
@ 2018-10-25 12:45 Chris Wilson
  2018-10-25 13:32 ` ✗ Fi.CI.CHECKPATCH: warning for drm/i915/userptr: Avoid struct_mutex recursion for mmu_invalidate_range_start (rev2) Patchwork
                   ` (4 more replies)
  0 siblings, 5 replies; 13+ messages in thread
From: Chris Wilson @ 2018-10-25 12:45 UTC (permalink / raw)
  To: intel-gfx

Since commit 93065ac753e4 ("mm, oom: distinguish blockable mode for mmu
notifiers") we have been able to report failure from
mmu_invalidate_range_start which allows us to use a trylock on the
struct_mutex to avoid potential recursion and report -EBUSY instead.
Furthermore, this allows us to pull the work into the main callback and
avoid the sleight-of-hand in using a workqueue to avoid lockdep.

However, not all paths to mmu_invalidate_range_start are prepared to
handle failure, so instead of reporting the recursion, deal with it.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=108375
References: 93065ac753e4 ("mm, oom: distinguish blockable mode for mmu notifiers")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_drv.h         |   4 +-
 drivers/gpu/drm/i915/i915_gem.c         |  12 +-
 drivers/gpu/drm/i915/i915_gem_userptr.c | 219 ++++++++++++------------
 3 files changed, 115 insertions(+), 120 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 2d7761b8ac07..c3b94c3f930f 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -3076,8 +3076,8 @@ enum i915_mm_subclass { /* lockdep subclass for obj->mm.lock */
 	I915_MM_SHRINKER
 };
 
-void __i915_gem_object_put_pages(struct drm_i915_gem_object *obj,
-				 enum i915_mm_subclass subclass);
+int __i915_gem_object_put_pages(struct drm_i915_gem_object *obj,
+				enum i915_mm_subclass subclass);
 void __i915_gem_object_invalidate(struct drm_i915_gem_object *obj);
 
 enum i915_map_type {
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 93d09282710d..de7ccb3eb7b8 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2454,17 +2454,18 @@ __i915_gem_object_unset_pages(struct drm_i915_gem_object *obj)
 	return pages;
 }
 
-void __i915_gem_object_put_pages(struct drm_i915_gem_object *obj,
-				 enum i915_mm_subclass subclass)
+int __i915_gem_object_put_pages(struct drm_i915_gem_object *obj,
+				enum i915_mm_subclass subclass)
 {
 	struct sg_table *pages;
+	int ret = -EBUSY;
 
 	if (i915_gem_object_has_pinned_pages(obj))
-		return;
+		return -EBUSY;
 
 	GEM_BUG_ON(obj->bind_count);
 	if (!i915_gem_object_has_pages(obj))
-		return;
+		return 0;
 
 	/* May be called by shrinker from within get_pages() (on another bo) */
 	mutex_lock_nested(&obj->mm.lock, subclass);
@@ -2480,8 +2481,11 @@ void __i915_gem_object_put_pages(struct drm_i915_gem_object *obj,
 	if (!IS_ERR(pages))
 		obj->ops->put_pages(obj, pages);
 
+	ret = 0;
 unlock:
 	mutex_unlock(&obj->mm.lock);
+
+	return ret;
 }
 
 bool i915_sg_trim(struct sg_table *orig_st)
diff --git a/drivers/gpu/drm/i915/i915_gem_userptr.c b/drivers/gpu/drm/i915/i915_gem_userptr.c
index 2c9b284036d1..a8f3c304db55 100644
--- a/drivers/gpu/drm/i915/i915_gem_userptr.c
+++ b/drivers/gpu/drm/i915/i915_gem_userptr.c
@@ -50,79 +50,84 @@ struct i915_mmu_notifier {
 	struct hlist_node node;
 	struct mmu_notifier mn;
 	struct rb_root_cached objects;
-	struct workqueue_struct *wq;
+	struct i915_mm_struct *mm;
 };
 
 struct i915_mmu_object {
 	struct i915_mmu_notifier *mn;
 	struct drm_i915_gem_object *obj;
 	struct interval_tree_node it;
-	struct list_head link;
-	struct work_struct work;
-	bool attached;
 };
 
-static void cancel_userptr(struct work_struct *work)
-{
-	struct i915_mmu_object *mo = container_of(work, typeof(*mo), work);
-	struct drm_i915_gem_object *obj = mo->obj;
-	struct work_struct *active;
-
-	/* Cancel any active worker and force us to re-evaluate gup */
-	mutex_lock(&obj->mm.lock);
-	active = fetch_and_zero(&obj->userptr.work);
-	mutex_unlock(&obj->mm.lock);
-	if (active)
-		goto out;
-
-	i915_gem_object_wait(obj, I915_WAIT_ALL, MAX_SCHEDULE_TIMEOUT, NULL);
-
-	mutex_lock(&obj->base.dev->struct_mutex);
-
-	/* We are inside a kthread context and can't be interrupted */
-	if (i915_gem_object_unbind(obj) == 0)
-		__i915_gem_object_put_pages(obj, I915_MM_NORMAL);
-	WARN_ONCE(i915_gem_object_has_pages(obj),
-		  "Failed to release pages: bind_count=%d, pages_pin_count=%d, pin_global=%d\n",
-		  obj->bind_count,
-		  atomic_read(&obj->mm.pages_pin_count),
-		  obj->pin_global);
-
-	mutex_unlock(&obj->base.dev->struct_mutex);
-
-out:
-	i915_gem_object_put(obj);
-}
-
 static void add_object(struct i915_mmu_object *mo)
 {
-	if (mo->attached)
+	if (!RB_EMPTY_NODE(&mo->it.rb))
 		return;
 
 	interval_tree_insert(&mo->it, &mo->mn->objects);
-	mo->attached = true;
 }
 
 static void del_object(struct i915_mmu_object *mo)
 {
-	if (!mo->attached)
+	if (RB_EMPTY_NODE(&mo->it.rb))
 		return;
 
 	interval_tree_remove(&mo->it, &mo->mn->objects);
-	mo->attached = false;
+	RB_CLEAR_NODE(&mo->it.rb);
+}
+
+static void
+__i915_gem_userptr_set_active(struct drm_i915_gem_object *obj, bool value)
+{
+	struct i915_mmu_object *mo = obj->userptr.mmu_object;
+
+	/*
+	 * During mm_invalidate_range we need to cancel any userptr that
+	 * overlaps the range being invalidated. Doing so requires the
+	 * struct_mutex, and that risks recursion. In order to cause
+	 * recursion, the user must alias the userptr address space with
+	 * a GTT mmapping (possible with a MAP_FIXED) - then when we have
+	 * to invalidate that mmaping, mm_invalidate_range is called with
+	 * the userptr address *and* the struct_mutex held.  To prevent that
+	 * we set a flag under the i915_mmu_notifier spinlock to indicate
+	 * whether this object is valid.
+	 */
+	if (!mo)
+		return;
+
+	spin_lock(&mo->mn->lock);
+	if (value)
+		add_object(mo);
+	else
+		del_object(mo);
+	spin_unlock(&mo->mn->lock);
+}
+
+static struct mutex *__i915_mutex_lock_recursive(struct mutex *m)
+{
+	switch (mutex_trylock_recursive(m)) {
+	default:
+	case MUTEX_TRYLOCK_FAILED:
+		mutex_lock(m);
+	case MUTEX_TRYLOCK_SUCCESS:
+		return m;
+
+	case MUTEX_TRYLOCK_RECURSIVE:
+		return ERR_PTR(-EEXIST);
+	}
 }
 
 static int i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn,
-						       struct mm_struct *mm,
-						       unsigned long start,
-						       unsigned long end,
-						       bool blockable)
+						      struct mm_struct *mm,
+						      unsigned long start,
+						      unsigned long end,
+						      bool blockable)
 {
 	struct i915_mmu_notifier *mn =
 		container_of(_mn, struct i915_mmu_notifier, mn);
-	struct i915_mmu_object *mo;
 	struct interval_tree_node *it;
-	LIST_HEAD(cancelled);
+	struct mutex *unlock = NULL;
+	int ret = 0;
 
 	if (RB_EMPTY_ROOT(&mn->objects.rb_root))
 		return 0;
@@ -133,11 +138,16 @@ static int i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn,
 	spin_lock(&mn->lock);
 	it = interval_tree_iter_first(&mn->objects, start, end);
 	while (it) {
+		struct drm_i915_gem_object *obj;
+		bool has_pages = false;
+
 		if (!blockable) {
-			spin_unlock(&mn->lock);
-			return -EAGAIN;
+			ret = -EAGAIN;
+			break;
 		}
-		/* The mmu_object is released late when destroying the
+
+		/*
+		 * The mmu_object is released late when destroying the
 		 * GEM object so it is entirely possible to gain a
 		 * reference on an object in the process of being freed
 		 * since our serialisation is via the spinlock and not
@@ -146,21 +156,44 @@ static int i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn,
 		 * use-after-free we only acquire a reference on the
 		 * object if it is not in the process of being destroyed.
 		 */
-		mo = container_of(it, struct i915_mmu_object, it);
-		if (kref_get_unless_zero(&mo->obj->base.refcount))
-			queue_work(mn->wq, &mo->work);
+		obj = container_of(it, struct i915_mmu_object, it)->obj;
+		if (!kref_get_unless_zero(&obj->base.refcount)) {
+			it = interval_tree_iter_next(it, start, end);
+			continue;
+		}
+		spin_unlock(&mn->lock);
 
-		list_add(&mo->link, &cancelled);
-		it = interval_tree_iter_next(it, start, end);
+		/* Cancel any pending worker and force us to re-evaluate gup */
+		mutex_lock_nested(&obj->mm.lock, I915_MM_SHRINKER);
+		if (fetch_and_zero(&obj->userptr.work))
+			__i915_gem_userptr_set_active(obj, false);
+		else
+			has_pages = i915_gem_object_has_pages(obj);
+		mutex_unlock(&obj->mm.lock);
+
+		if (has_pages) {
+			if (!unlock)
+				unlock = __i915_mutex_lock_recursive(&mn->mm->i915->drm.struct_mutex);
+			ret = i915_gem_object_unbind(obj);
+			if (ret == 0)
+				ret = __i915_gem_object_put_pages(obj, I915_MM_SHRINKER);
+		}
+
+		i915_gem_object_put(obj);
+		if (ret)
+			goto unlock;
+
+		spin_lock(&mn->lock);
+		it = interval_tree_iter_first(&mn->objects, start, end);
 	}
-	list_for_each_entry(mo, &cancelled, link)
-		del_object(mo);
 	spin_unlock(&mn->lock);
 
-	if (!list_empty(&cancelled))
-		flush_workqueue(mn->wq);
+unlock:
+	if (!IS_ERR_OR_NULL(unlock))
+		mutex_unlock(unlock);
+
+	return ret;
 
-	return 0;
 }
 
 static const struct mmu_notifier_ops i915_gem_userptr_notifier = {
@@ -168,7 +201,7 @@ static const struct mmu_notifier_ops i915_gem_userptr_notifier = {
 };
 
 static struct i915_mmu_notifier *
-i915_mmu_notifier_create(struct mm_struct *mm)
+i915_mmu_notifier_create(struct i915_mm_struct *mm)
 {
 	struct i915_mmu_notifier *mn;
 
@@ -179,13 +212,7 @@ i915_mmu_notifier_create(struct mm_struct *mm)
 	spin_lock_init(&mn->lock);
 	mn->mn.ops = &i915_gem_userptr_notifier;
 	mn->objects = RB_ROOT_CACHED;
-	mn->wq = alloc_workqueue("i915-userptr-release",
-				 WQ_UNBOUND | WQ_MEM_RECLAIM,
-				 0);
-	if (mn->wq == NULL) {
-		kfree(mn);
-		return ERR_PTR(-ENOMEM);
-	}
+	mn->mm = mm;
 
 	return mn;
 }
@@ -195,16 +222,14 @@ i915_gem_userptr_release__mmu_notifier(struct drm_i915_gem_object *obj)
 {
 	struct i915_mmu_object *mo;
 
-	mo = obj->userptr.mmu_object;
-	if (mo == NULL)
+	mo = fetch_and_zero(&obj->userptr.mmu_object);
+	if (!mo)
 		return;
 
 	spin_lock(&mo->mn->lock);
 	del_object(mo);
 	spin_unlock(&mo->mn->lock);
 	kfree(mo);
-
-	obj->userptr.mmu_object = NULL;
 }
 
 static struct i915_mmu_notifier *
@@ -217,7 +242,7 @@ i915_mmu_notifier_find(struct i915_mm_struct *mm)
 	if (mn)
 		return mn;
 
-	mn = i915_mmu_notifier_create(mm->mm);
+	mn = i915_mmu_notifier_create(mm);
 	if (IS_ERR(mn))
 		err = PTR_ERR(mn);
 
@@ -240,10 +265,8 @@ i915_mmu_notifier_find(struct i915_mm_struct *mm)
 	mutex_unlock(&mm->i915->mm_lock);
 	up_write(&mm->mm->mmap_sem);
 
-	if (mn && !IS_ERR(mn)) {
-		destroy_workqueue(mn->wq);
+	if (mn && !IS_ERR(mn))
 		kfree(mn);
-	}
 
 	return err ? ERR_PTR(err) : mm->mn;
 }
@@ -266,14 +289,14 @@ i915_gem_userptr_init__mmu_notifier(struct drm_i915_gem_object *obj,
 		return PTR_ERR(mn);
 
 	mo = kzalloc(sizeof(*mo), GFP_KERNEL);
-	if (mo == NULL)
+	if (!mo)
 		return -ENOMEM;
 
 	mo->mn = mn;
 	mo->obj = obj;
 	mo->it.start = obj->userptr.ptr;
 	mo->it.last = obj->userptr.ptr + obj->base.size - 1;
-	INIT_WORK(&mo->work, cancel_userptr);
+	RB_CLEAR_NODE(&mo->it.rb);
 
 	obj->userptr.mmu_object = mo;
 	return 0;
@@ -287,12 +310,16 @@ i915_mmu_notifier_free(struct i915_mmu_notifier *mn,
 		return;
 
 	mmu_notifier_unregister(&mn->mn, mm);
-	destroy_workqueue(mn->wq);
 	kfree(mn);
 }
 
 #else
 
+static void
+__i915_gem_userptr_set_active(struct drm_i915_gem_object *obj, bool value)
+{
+}
+
 static void
 i915_gem_userptr_release__mmu_notifier(struct drm_i915_gem_object *obj)
 {
@@ -461,42 +488,6 @@ __i915_gem_userptr_alloc_pages(struct drm_i915_gem_object *obj,
 	return st;
 }
 
-static int
-__i915_gem_userptr_set_active(struct drm_i915_gem_object *obj,
-			      bool value)
-{
-	int ret = 0;
-
-	/* During mm_invalidate_range we need to cancel any userptr that
-	 * overlaps the range being invalidated. Doing so requires the
-	 * struct_mutex, and that risks recursion. In order to cause
-	 * recursion, the user must alias the userptr address space with
-	 * a GTT mmapping (possible with a MAP_FIXED) - then when we have
-	 * to invalidate that mmaping, mm_invalidate_range is called with
-	 * the userptr address *and* the struct_mutex held.  To prevent that
-	 * we set a flag under the i915_mmu_notifier spinlock to indicate
-	 * whether this object is valid.
-	 */
-#if defined(CONFIG_MMU_NOTIFIER)
-	if (obj->userptr.mmu_object == NULL)
-		return 0;
-
-	spin_lock(&obj->userptr.mmu_object->mn->lock);
-	/* In order to serialise get_pages with an outstanding
-	 * cancel_userptr, we must drop the struct_mutex and try again.
-	 */
-	if (!value)
-		del_object(obj->userptr.mmu_object);
-	else if (!work_pending(&obj->userptr.mmu_object->work))
-		add_object(obj->userptr.mmu_object);
-	else
-		ret = -EAGAIN;
-	spin_unlock(&obj->userptr.mmu_object->mn->lock);
-#endif
-
-	return ret;
-}
-
 static void
 __i915_gem_userptr_get_pages_worker(struct work_struct *_work)
 {
-- 
2.19.1

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

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* ✗ Fi.CI.CHECKPATCH: warning for drm/i915/userptr: Avoid struct_mutex recursion for mmu_invalidate_range_start (rev2)
  2018-10-25 12:45 [PATCH] drm/i915/userptr: Avoid struct_mutex recursion for mmu_invalidate_range_start Chris Wilson
@ 2018-10-25 13:32 ` Patchwork
  2018-10-25 13:50 ` ✓ Fi.CI.BAT: success " Patchwork
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 13+ messages in thread
From: Patchwork @ 2018-10-25 13:32 UTC (permalink / raw)
  To: Chris Wilson; +Cc: intel-gfx

== Series Details ==

Series: drm/i915/userptr: Avoid struct_mutex recursion for mmu_invalidate_range_start (rev2)
URL   : https://patchwork.freedesktop.org/series/51362/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
7c70af9afdbf drm/i915/userptr: Avoid struct_mutex recursion for mmu_invalidate_range_start
-:18: WARNING:COMMIT_LOG_LONG_LINE: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#18: 
References: 93065ac753e4 ("mm, oom: distinguish blockable mode for mmu notifiers")

-:18: ERROR:GIT_COMMIT_ID: Please use git commit description style 'commit <12+ chars of sha1> ("<title line>")' - ie: 'commit 93065ac753e4 ("mm, oom: distinguish blockable mode for mmu notifiers")'
#18: 
References: 93065ac753e4 ("mm, oom: distinguish blockable mode for mmu notifiers")

-:178: ERROR:LOCKING: recursive locking is bad, do not use this ever.
#178: FILE: drivers/gpu/drm/i915/i915_gem_userptr.c:108:
+	switch (mutex_trylock_recursive(m)) {

-:256: WARNING:LONG_LINE: line over 100 characters
#256: FILE: drivers/gpu/drm/i915/i915_gem_userptr.c:176:
+				unlock = __i915_mutex_lock_recursive(&mn->mm->i915->drm.struct_mutex);

total: 2 errors, 2 warnings, 0 checks, 380 lines checked

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* ✓ Fi.CI.BAT: success for drm/i915/userptr: Avoid struct_mutex recursion for mmu_invalidate_range_start (rev2)
  2018-10-25 12:45 [PATCH] drm/i915/userptr: Avoid struct_mutex recursion for mmu_invalidate_range_start Chris Wilson
  2018-10-25 13:32 ` ✗ Fi.CI.CHECKPATCH: warning for drm/i915/userptr: Avoid struct_mutex recursion for mmu_invalidate_range_start (rev2) Patchwork
@ 2018-10-25 13:50 ` Patchwork
  2018-10-25 18:57 ` ✗ Fi.CI.IGT: failure " Patchwork
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 13+ messages in thread
From: Patchwork @ 2018-10-25 13:50 UTC (permalink / raw)
  To: Chris Wilson; +Cc: intel-gfx

== Series Details ==

Series: drm/i915/userptr: Avoid struct_mutex recursion for mmu_invalidate_range_start (rev2)
URL   : https://patchwork.freedesktop.org/series/51362/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_5035 -> Patchwork_10577 =

== Summary - SUCCESS ==

  No regressions found.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/51362/revisions/2/mbox/

== Known issues ==

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

  === IGT changes ===

    ==== Issues hit ====

    igt@drv_module_reload@basic-reload-inject:
      fi-glk-j4005:       PASS -> DMESG-WARN (fdo#106725, fdo#106248)

    igt@gem_exec_suspend@basic-s3:
      fi-kbl-soraka:      NOTRUN -> INCOMPLETE (fdo#107556, fdo#107774, fdo#107859)

    igt@kms_cursor_legacy@basic-busy-flip-before-cursor-legacy:
      fi-glk-j4005:       PASS -> FAIL (fdo#106765)

    igt@kms_flip@basic-flip-vs-modeset:
      fi-skl-6700hq:      PASS -> DMESG-WARN (fdo#105998)

    igt@kms_flip@basic-plain-flip:
      fi-glk-j4005:       PASS -> DMESG-WARN (fdo#106097)

    igt@kms_pipe_crc_basic@nonblocking-crc-pipe-b-frame-sequence:
      fi-byt-clapper:     PASS -> FAIL (fdo#103191, fdo#107362)

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-b:
      fi-blb-e6850:       NOTRUN -> INCOMPLETE (fdo#107718)

    igt@pm_rps@basic-api:
      fi-glk-j4005:       PASS -> DMESG-WARN (fdo#105719)

    
    ==== Possible fixes ====

    igt@drv_selftest@live_hangcheck:
      fi-skl-gvtdvm:      INCOMPLETE (fdo#105600) -> PASS

    igt@gem_exec_suspend@basic-s4-devices:
      fi-blb-e6850:       INCOMPLETE (fdo#107718) -> PASS

    igt@pm_rpm@module-reload:
      fi-glk-j4005:       DMESG-WARN (fdo#107726) -> PASS

    
  fdo#103191 https://bugs.freedesktop.org/show_bug.cgi?id=103191
  fdo#105600 https://bugs.freedesktop.org/show_bug.cgi?id=105600
  fdo#105719 https://bugs.freedesktop.org/show_bug.cgi?id=105719
  fdo#105998 https://bugs.freedesktop.org/show_bug.cgi?id=105998
  fdo#106097 https://bugs.freedesktop.org/show_bug.cgi?id=106097
  fdo#106248 https://bugs.freedesktop.org/show_bug.cgi?id=106248
  fdo#106725 https://bugs.freedesktop.org/show_bug.cgi?id=106725
  fdo#106765 https://bugs.freedesktop.org/show_bug.cgi?id=106765
  fdo#107362 https://bugs.freedesktop.org/show_bug.cgi?id=107362
  fdo#107556 https://bugs.freedesktop.org/show_bug.cgi?id=107556
  fdo#107718 https://bugs.freedesktop.org/show_bug.cgi?id=107718
  fdo#107726 https://bugs.freedesktop.org/show_bug.cgi?id=107726
  fdo#107774 https://bugs.freedesktop.org/show_bug.cgi?id=107774
  fdo#107859 https://bugs.freedesktop.org/show_bug.cgi?id=107859


== Participating hosts (46 -> 42) ==

  Additional (1): fi-kbl-soraka 
  Missing    (5): fi-ilk-m540 fi-byt-squawks fi-bsw-cyan fi-apl-guc fi-ctg-p8600 


== Build changes ==

    * Linux: CI_DRM_5035 -> Patchwork_10577

  CI_DRM_5035: 33294493a897f20670882844840c2205955aa0ca @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4694: ff8d1156723f235e82cb4fcfd2cd6e5a5bb211fa @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_10577: 7c70af9afdbf13ec641204195b85a391336fc533 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

7c70af9afdbf drm/i915/userptr: Avoid struct_mutex recursion for mmu_invalidate_range_start

== Logs ==

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* ✗ Fi.CI.IGT: failure for drm/i915/userptr: Avoid struct_mutex recursion for mmu_invalidate_range_start (rev2)
  2018-10-25 12:45 [PATCH] drm/i915/userptr: Avoid struct_mutex recursion for mmu_invalidate_range_start Chris Wilson
  2018-10-25 13:32 ` ✗ Fi.CI.CHECKPATCH: warning for drm/i915/userptr: Avoid struct_mutex recursion for mmu_invalidate_range_start (rev2) Patchwork
  2018-10-25 13:50 ` ✓ Fi.CI.BAT: success " Patchwork
@ 2018-10-25 18:57 ` Patchwork
  2018-10-25 19:16 ` [PATCH] drm/i915/userptr: Avoid struct_mutex recursion for mmu_invalidate_range_start Daniel Vetter
  2018-10-25 21:04 ` Mark Janes
  4 siblings, 0 replies; 13+ messages in thread
From: Patchwork @ 2018-10-25 18:57 UTC (permalink / raw)
  To: Chris Wilson; +Cc: intel-gfx

== Series Details ==

Series: drm/i915/userptr: Avoid struct_mutex recursion for mmu_invalidate_range_start (rev2)
URL   : https://patchwork.freedesktop.org/series/51362/
State : failure

== Summary ==

= CI Bug Log - changes from CI_DRM_5035_full -> Patchwork_10577_full =

== Summary - FAILURE ==

  Serious unknown changes coming with Patchwork_10577_full absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_10577_full, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  

== Possible new issues ==

  Here are the unknown changes that may have been introduced in Patchwork_10577_full:

  === IGT changes ===

    ==== Possible regressions ====

    igt@gem_busy@close-race:
      shard-glk:          PASS -> DMESG-FAIL

    
== Known issues ==

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

  === IGT changes ===

    ==== Issues hit ====

    igt@drv_suspend@shrink:
      shard-kbl:          PASS -> INCOMPLETE (fdo#103665, fdo#106886)

    igt@gem_eio@in-flight-suspend:
      shard-apl:          PASS -> DMESG-WARN (fdo#107957)

    igt@gem_exec_schedule@pi-ringfull-bsd:
      shard-skl:          NOTRUN -> FAIL (fdo#103158)

    igt@kms_atomic_transition@1x-modeset-transitions-nonblocking:
      shard-apl:          PASS -> DMESG-FAIL (fdo#108549) +2

    igt@kms_atomic_transition@plane-all-modeset-transition:
      shard-apl:          PASS -> INCOMPLETE (fdo#103927)

    igt@kms_color@pipe-a-degamma:
      shard-apl:          PASS -> FAIL (fdo#104782, fdo#108145)

    igt@kms_concurrent@pipe-a:
      shard-apl:          PASS -> DMESG-WARN (fdo#108549) +21

    igt@kms_cursor_crc@cursor-size-change:
      shard-glk:          PASS -> FAIL (fdo#103232) +2

    igt@kms_flip_tiling@flip-changes-tiling-yf:
      shard-skl:          PASS -> FAIL (fdo#108303, fdo#108228)

    igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-draw-mmap-cpu:
      shard-apl:          PASS -> FAIL (fdo#103167) +1

    igt@kms_frontbuffer_tracking@fbc-2p-primscrn-cur-indfb-draw-mmap-gtt:
      shard-glk:          PASS -> FAIL (fdo#103167) +3

    igt@kms_plane_alpha_blend@pipe-c-alpha-opaque-fb:
      shard-skl:          NOTRUN -> FAIL (fdo#108145)
      shard-glk:          PASS -> FAIL (fdo#108145)

    igt@kms_universal_plane@universal-plane-pipe-a-functional:
      shard-apl:          PASS -> FAIL (fdo#103166)

    
    ==== Possible fixes ====

    igt@kms_busy@extended-modeset-hang-newfb-render-c:
      shard-skl:          DMESG-WARN (fdo#107956) -> PASS

    igt@kms_ccs@pipe-a-crc-sprite-planes-basic:
      shard-glk:          FAIL (fdo#108145) -> PASS

    igt@kms_chv_cursor_fail@pipe-c-128x128-bottom-edge:
      shard-apl:          DMESG-WARN (fdo#108549) -> PASS +23

    igt@kms_cursor_crc@cursor-128x128-random:
      shard-apl:          FAIL (fdo#103232) -> PASS +1
      shard-kbl:          DMESG-WARN (fdo#103558, fdo#105602) -> PASS +7

    igt@kms_cursor_crc@cursor-256x256-random:
      shard-glk:          FAIL (fdo#103232) -> PASS +2

    igt@kms_cursor_crc@cursor-256x256-suspend:
      shard-kbl:          INCOMPLETE (fdo#103665) -> PASS
      shard-apl:          FAIL (fdo#103232, fdo#103191) -> PASS

    igt@kms_frontbuffer_tracking@fbc-1p-offscren-pri-indfb-draw-mmap-wc:
      shard-skl:          FAIL (fdo#103167) -> PASS

    igt@kms_frontbuffer_tracking@fbc-1p-primscrn-cur-indfb-draw-blt:
      shard-glk:          FAIL (fdo#103167) -> PASS +3

    igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-draw-blt:
      shard-apl:          FAIL (fdo#103167) -> PASS +1

    igt@kms_frontbuffer_tracking@fbc-1p-rte:
      shard-apl:          FAIL (fdo#105682, fdo#103167) -> PASS

    igt@kms_plane@plane-panning-bottom-right-suspend-pipe-a-planes:
      shard-kbl:          DMESG-FAIL (fdo#103166, fdo#103558, fdo#105602) -> PASS

    igt@kms_plane_alpha_blend@pipe-a-coverage-7efc:
      shard-skl:          FAIL (fdo#108145, fdo#107815) -> PASS

    igt@kms_plane_multiple@atomic-pipe-a-tiling-y:
      shard-glk:          FAIL (fdo#103166) -> PASS +1

    igt@kms_vblank@pipe-b-ts-continuation-suspend:
      shard-apl:          DMESG-FAIL (fdo#108549) -> PASS

    
    ==== Warnings ====

    igt@gem_ppgtt@blt-vs-render-ctx0:
      shard-skl:          INCOMPLETE (fdo#106887) -> TIMEOUT (fdo#108039)

    igt@kms_cursor_crc@cursor-64x64-random:
      shard-apl:          DMESG-WARN (fdo#108549) -> FAIL (fdo#103232)

    igt@kms_plane_alpha_blend@pipe-a-alpha-7efc:
      shard-kbl:          DMESG-FAIL (fdo#108145, fdo#103558, fdo#105602) -> FAIL (fdo#108145)

    igt@kms_plane_alpha_blend@pipe-c-alpha-transparant-fb:
      shard-apl:          FAIL (fdo#108145) -> DMESG-FAIL (fdo#108145, fdo#108549)

    igt@kms_setmode@basic:
      shard-apl:          DMESG-WARN (fdo#108549) -> FAIL (fdo#99912)

    
  fdo#103158 https://bugs.freedesktop.org/show_bug.cgi?id=103158
  fdo#103166 https://bugs.freedesktop.org/show_bug.cgi?id=103166
  fdo#103167 https://bugs.freedesktop.org/show_bug.cgi?id=103167
  fdo#103191 https://bugs.freedesktop.org/show_bug.cgi?id=103191
  fdo#103232 https://bugs.freedesktop.org/show_bug.cgi?id=103232
  fdo#103558 https://bugs.freedesktop.org/show_bug.cgi?id=103558
  fdo#103665 https://bugs.freedesktop.org/show_bug.cgi?id=103665
  fdo#103927 https://bugs.freedesktop.org/show_bug.cgi?id=103927
  fdo#104782 https://bugs.freedesktop.org/show_bug.cgi?id=104782
  fdo#105602 https://bugs.freedesktop.org/show_bug.cgi?id=105602
  fdo#105682 https://bugs.freedesktop.org/show_bug.cgi?id=105682
  fdo#106886 https://bugs.freedesktop.org/show_bug.cgi?id=106886
  fdo#106887 https://bugs.freedesktop.org/show_bug.cgi?id=106887
  fdo#107815 https://bugs.freedesktop.org/show_bug.cgi?id=107815
  fdo#107956 https://bugs.freedesktop.org/show_bug.cgi?id=107956
  fdo#107957 https://bugs.freedesktop.org/show_bug.cgi?id=107957
  fdo#108039 https://bugs.freedesktop.org/show_bug.cgi?id=108039
  fdo#108145 https://bugs.freedesktop.org/show_bug.cgi?id=108145
  fdo#108228 https://bugs.freedesktop.org/show_bug.cgi?id=108228
  fdo#108303 https://bugs.freedesktop.org/show_bug.cgi?id=108303
  fdo#108549 https://bugs.freedesktop.org/show_bug.cgi?id=108549
  fdo#99912 https://bugs.freedesktop.org/show_bug.cgi?id=99912


== Participating hosts (6 -> 6) ==

  No changes in participating hosts


== Build changes ==

    * Linux: CI_DRM_5035 -> Patchwork_10577

  CI_DRM_5035: 33294493a897f20670882844840c2205955aa0ca @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4694: ff8d1156723f235e82cb4fcfd2cd6e5a5bb211fa @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_10577: 7c70af9afdbf13ec641204195b85a391336fc533 @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit

== Logs ==

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] drm/i915/userptr: Avoid struct_mutex recursion for mmu_invalidate_range_start
  2018-10-25 12:45 [PATCH] drm/i915/userptr: Avoid struct_mutex recursion for mmu_invalidate_range_start Chris Wilson
                   ` (2 preceding siblings ...)
  2018-10-25 18:57 ` ✗ Fi.CI.IGT: failure " Patchwork
@ 2018-10-25 19:16 ` Daniel Vetter
  2018-10-25 19:39   ` Chris Wilson
  2018-10-25 21:04 ` Mark Janes
  4 siblings, 1 reply; 13+ messages in thread
From: Daniel Vetter @ 2018-10-25 19:16 UTC (permalink / raw)
  To: Chris Wilson; +Cc: intel-gfx

On Thu, Oct 25, 2018 at 01:45:42PM +0100, Chris Wilson wrote:
> Since commit 93065ac753e4 ("mm, oom: distinguish blockable mode for mmu
> notifiers") we have been able to report failure from
> mmu_invalidate_range_start which allows us to use a trylock on the
> struct_mutex to avoid potential recursion and report -EBUSY instead.
> Furthermore, this allows us to pull the work into the main callback and
> avoid the sleight-of-hand in using a workqueue to avoid lockdep.
> 
> However, not all paths to mmu_invalidate_range_start are prepared to
> handle failure, so instead of reporting the recursion, deal with it.
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=108375
> References: 93065ac753e4 ("mm, oom: distinguish blockable mode for mmu notifiers")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/i915_drv.h         |   4 +-
>  drivers/gpu/drm/i915/i915_gem.c         |  12 +-
>  drivers/gpu/drm/i915/i915_gem_userptr.c | 219 ++++++++++++------------
>  3 files changed, 115 insertions(+), 120 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 2d7761b8ac07..c3b94c3f930f 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -3076,8 +3076,8 @@ enum i915_mm_subclass { /* lockdep subclass for obj->mm.lock */
>  	I915_MM_SHRINKER
>  };
>  
> -void __i915_gem_object_put_pages(struct drm_i915_gem_object *obj,
> -				 enum i915_mm_subclass subclass);
> +int __i915_gem_object_put_pages(struct drm_i915_gem_object *obj,
> +				enum i915_mm_subclass subclass);
>  void __i915_gem_object_invalidate(struct drm_i915_gem_object *obj);
>  
>  enum i915_map_type {
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 93d09282710d..de7ccb3eb7b8 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2454,17 +2454,18 @@ __i915_gem_object_unset_pages(struct drm_i915_gem_object *obj)
>  	return pages;
>  }
>  
> -void __i915_gem_object_put_pages(struct drm_i915_gem_object *obj,
> -				 enum i915_mm_subclass subclass)
> +int __i915_gem_object_put_pages(struct drm_i915_gem_object *obj,
> +				enum i915_mm_subclass subclass)
>  {
>  	struct sg_table *pages;
> +	int ret = -EBUSY;
>  
>  	if (i915_gem_object_has_pinned_pages(obj))
> -		return;
> +		return -EBUSY;
>  
>  	GEM_BUG_ON(obj->bind_count);
>  	if (!i915_gem_object_has_pages(obj))
> -		return;
> +		return 0;
>  
>  	/* May be called by shrinker from within get_pages() (on another bo) */
>  	mutex_lock_nested(&obj->mm.lock, subclass);
> @@ -2480,8 +2481,11 @@ void __i915_gem_object_put_pages(struct drm_i915_gem_object *obj,
>  	if (!IS_ERR(pages))
>  		obj->ops->put_pages(obj, pages);
>  
> +	ret = 0;
>  unlock:
>  	mutex_unlock(&obj->mm.lock);
> +
> +	return ret;
>  }
>  
>  bool i915_sg_trim(struct sg_table *orig_st)
> diff --git a/drivers/gpu/drm/i915/i915_gem_userptr.c b/drivers/gpu/drm/i915/i915_gem_userptr.c
> index 2c9b284036d1..a8f3c304db55 100644
> --- a/drivers/gpu/drm/i915/i915_gem_userptr.c
> +++ b/drivers/gpu/drm/i915/i915_gem_userptr.c
> @@ -50,79 +50,84 @@ struct i915_mmu_notifier {
>  	struct hlist_node node;
>  	struct mmu_notifier mn;
>  	struct rb_root_cached objects;
> -	struct workqueue_struct *wq;
> +	struct i915_mm_struct *mm;
>  };
>  
>  struct i915_mmu_object {
>  	struct i915_mmu_notifier *mn;
>  	struct drm_i915_gem_object *obj;
>  	struct interval_tree_node it;
> -	struct list_head link;
> -	struct work_struct work;
> -	bool attached;
>  };
>  
> -static void cancel_userptr(struct work_struct *work)
> -{
> -	struct i915_mmu_object *mo = container_of(work, typeof(*mo), work);
> -	struct drm_i915_gem_object *obj = mo->obj;
> -	struct work_struct *active;
> -
> -	/* Cancel any active worker and force us to re-evaluate gup */
> -	mutex_lock(&obj->mm.lock);
> -	active = fetch_and_zero(&obj->userptr.work);
> -	mutex_unlock(&obj->mm.lock);
> -	if (active)
> -		goto out;
> -
> -	i915_gem_object_wait(obj, I915_WAIT_ALL, MAX_SCHEDULE_TIMEOUT, NULL);
> -
> -	mutex_lock(&obj->base.dev->struct_mutex);
> -
> -	/* We are inside a kthread context and can't be interrupted */
> -	if (i915_gem_object_unbind(obj) == 0)
> -		__i915_gem_object_put_pages(obj, I915_MM_NORMAL);
> -	WARN_ONCE(i915_gem_object_has_pages(obj),
> -		  "Failed to release pages: bind_count=%d, pages_pin_count=%d, pin_global=%d\n",
> -		  obj->bind_count,
> -		  atomic_read(&obj->mm.pages_pin_count),
> -		  obj->pin_global);
> -
> -	mutex_unlock(&obj->base.dev->struct_mutex);
> -
> -out:
> -	i915_gem_object_put(obj);
> -}
> -
>  static void add_object(struct i915_mmu_object *mo)
>  {
> -	if (mo->attached)
> +	if (!RB_EMPTY_NODE(&mo->it.rb))
>  		return;
>  
>  	interval_tree_insert(&mo->it, &mo->mn->objects);
> -	mo->attached = true;
>  }
>  
>  static void del_object(struct i915_mmu_object *mo)
>  {
> -	if (!mo->attached)
> +	if (RB_EMPTY_NODE(&mo->it.rb))
>  		return;
>  
>  	interval_tree_remove(&mo->it, &mo->mn->objects);
> -	mo->attached = false;
> +	RB_CLEAR_NODE(&mo->it.rb);
> +}
> +
> +static void
> +__i915_gem_userptr_set_active(struct drm_i915_gem_object *obj, bool value)
> +{
> +	struct i915_mmu_object *mo = obj->userptr.mmu_object;
> +
> +	/*
> +	 * During mm_invalidate_range we need to cancel any userptr that
> +	 * overlaps the range being invalidated. Doing so requires the
> +	 * struct_mutex, and that risks recursion. In order to cause
> +	 * recursion, the user must alias the userptr address space with
> +	 * a GTT mmapping (possible with a MAP_FIXED) - then when we have
> +	 * to invalidate that mmaping, mm_invalidate_range is called with
> +	 * the userptr address *and* the struct_mutex held.  To prevent that
> +	 * we set a flag under the i915_mmu_notifier spinlock to indicate
> +	 * whether this object is valid.
> +	 */
> +	if (!mo)
> +		return;
> +
> +	spin_lock(&mo->mn->lock);
> +	if (value)
> +		add_object(mo);
> +	else
> +		del_object(mo);
> +	spin_unlock(&mo->mn->lock);
> +}
> +
> +static struct mutex *__i915_mutex_lock_recursive(struct mutex *m)
> +{
> +	switch (mutex_trylock_recursive(m)) {
> +	default:
> +	case MUTEX_TRYLOCK_FAILED:
> +		mutex_lock(m);
> +	case MUTEX_TRYLOCK_SUCCESS:
> +		return m;
> +
> +	case MUTEX_TRYLOCK_RECURSIVE:
> +		return ERR_PTR(-EEXIST);
> +	}
>  }
>  
>  static int i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn,
> -						       struct mm_struct *mm,
> -						       unsigned long start,
> -						       unsigned long end,
> -						       bool blockable)
> +						      struct mm_struct *mm,
> +						      unsigned long start,
> +						      unsigned long end,
> +						      bool blockable)
>  {
>  	struct i915_mmu_notifier *mn =
>  		container_of(_mn, struct i915_mmu_notifier, mn);
> -	struct i915_mmu_object *mo;
>  	struct interval_tree_node *it;
> -	LIST_HEAD(cancelled);
> +	struct mutex *unlock = NULL;
> +	int ret = 0;
>  
>  	if (RB_EMPTY_ROOT(&mn->objects.rb_root))
>  		return 0;
> @@ -133,11 +138,16 @@ static int i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn,
>  	spin_lock(&mn->lock);
>  	it = interval_tree_iter_first(&mn->objects, start, end);
>  	while (it) {
> +		struct drm_i915_gem_object *obj;
> +		bool has_pages = false;
> +
>  		if (!blockable) {
> -			spin_unlock(&mn->lock);
> -			return -EAGAIN;
> +			ret = -EAGAIN;
> +			break;
>  		}
> -		/* The mmu_object is released late when destroying the
> +
> +		/*
> +		 * The mmu_object is released late when destroying the
>  		 * GEM object so it is entirely possible to gain a
>  		 * reference on an object in the process of being freed
>  		 * since our serialisation is via the spinlock and not
> @@ -146,21 +156,44 @@ static int i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn,
>  		 * use-after-free we only acquire a reference on the
>  		 * object if it is not in the process of being destroyed.
>  		 */
> -		mo = container_of(it, struct i915_mmu_object, it);
> -		if (kref_get_unless_zero(&mo->obj->base.refcount))
> -			queue_work(mn->wq, &mo->work);
> +		obj = container_of(it, struct i915_mmu_object, it)->obj;
> +		if (!kref_get_unless_zero(&obj->base.refcount)) {
> +			it = interval_tree_iter_next(it, start, end);
> +			continue;
> +		}
> +		spin_unlock(&mn->lock);
>  
> -		list_add(&mo->link, &cancelled);
> -		it = interval_tree_iter_next(it, start, end);
> +		/* Cancel any pending worker and force us to re-evaluate gup */
> +		mutex_lock_nested(&obj->mm.lock, I915_MM_SHRINKER);

mutex_lock_nested is meant for static nesting. This here is rather
dynamic, and I don't really see where the guarantee is that the classic
deadlock isn't possible:

thread 1: get_pages on object A -> mmu_notifier (or shrinker) on object B
thread 2: get_pages on object B -> mmu_notifier (or shrinker) on object A

Only when you have some guarantee for a global order can you nest locks
using mutex_lock_nested.

There's a few checks and stuff that might work, but they all seem rather
racy. What's the guarantee that prevents the above?
-Daniel

> +		if (fetch_and_zero(&obj->userptr.work))
> +			__i915_gem_userptr_set_active(obj, false);
> +		else
> +			has_pages = i915_gem_object_has_pages(obj);
> +		mutex_unlock(&obj->mm.lock);
> +
> +		if (has_pages) {
> +			if (!unlock)
> +				unlock = __i915_mutex_lock_recursive(&mn->mm->i915->drm.struct_mutex);
> +			ret = i915_gem_object_unbind(obj);
> +			if (ret == 0)
> +				ret = __i915_gem_object_put_pages(obj, I915_MM_SHRINKER);
> +		}
> +
> +		i915_gem_object_put(obj);
> +		if (ret)
> +			goto unlock;
> +
> +		spin_lock(&mn->lock);
> +		it = interval_tree_iter_first(&mn->objects, start, end);
>  	}
> -	list_for_each_entry(mo, &cancelled, link)
> -		del_object(mo);
>  	spin_unlock(&mn->lock);
>  
> -	if (!list_empty(&cancelled))
> -		flush_workqueue(mn->wq);
> +unlock:
> +	if (!IS_ERR_OR_NULL(unlock))
> +		mutex_unlock(unlock);
> +
> +	return ret;
>  
> -	return 0;
>  }
>  
>  static const struct mmu_notifier_ops i915_gem_userptr_notifier = {
> @@ -168,7 +201,7 @@ static const struct mmu_notifier_ops i915_gem_userptr_notifier = {
>  };
>  
>  static struct i915_mmu_notifier *
> -i915_mmu_notifier_create(struct mm_struct *mm)
> +i915_mmu_notifier_create(struct i915_mm_struct *mm)
>  {
>  	struct i915_mmu_notifier *mn;
>  
> @@ -179,13 +212,7 @@ i915_mmu_notifier_create(struct mm_struct *mm)
>  	spin_lock_init(&mn->lock);
>  	mn->mn.ops = &i915_gem_userptr_notifier;
>  	mn->objects = RB_ROOT_CACHED;
> -	mn->wq = alloc_workqueue("i915-userptr-release",
> -				 WQ_UNBOUND | WQ_MEM_RECLAIM,
> -				 0);
> -	if (mn->wq == NULL) {
> -		kfree(mn);
> -		return ERR_PTR(-ENOMEM);
> -	}
> +	mn->mm = mm;
>  
>  	return mn;
>  }
> @@ -195,16 +222,14 @@ i915_gem_userptr_release__mmu_notifier(struct drm_i915_gem_object *obj)
>  {
>  	struct i915_mmu_object *mo;
>  
> -	mo = obj->userptr.mmu_object;
> -	if (mo == NULL)
> +	mo = fetch_and_zero(&obj->userptr.mmu_object);
> +	if (!mo)
>  		return;
>  
>  	spin_lock(&mo->mn->lock);
>  	del_object(mo);
>  	spin_unlock(&mo->mn->lock);
>  	kfree(mo);
> -
> -	obj->userptr.mmu_object = NULL;
>  }
>  
>  static struct i915_mmu_notifier *
> @@ -217,7 +242,7 @@ i915_mmu_notifier_find(struct i915_mm_struct *mm)
>  	if (mn)
>  		return mn;
>  
> -	mn = i915_mmu_notifier_create(mm->mm);
> +	mn = i915_mmu_notifier_create(mm);
>  	if (IS_ERR(mn))
>  		err = PTR_ERR(mn);
>  
> @@ -240,10 +265,8 @@ i915_mmu_notifier_find(struct i915_mm_struct *mm)
>  	mutex_unlock(&mm->i915->mm_lock);
>  	up_write(&mm->mm->mmap_sem);
>  
> -	if (mn && !IS_ERR(mn)) {
> -		destroy_workqueue(mn->wq);
> +	if (mn && !IS_ERR(mn))
>  		kfree(mn);
> -	}
>  
>  	return err ? ERR_PTR(err) : mm->mn;
>  }
> @@ -266,14 +289,14 @@ i915_gem_userptr_init__mmu_notifier(struct drm_i915_gem_object *obj,
>  		return PTR_ERR(mn);
>  
>  	mo = kzalloc(sizeof(*mo), GFP_KERNEL);
> -	if (mo == NULL)
> +	if (!mo)
>  		return -ENOMEM;
>  
>  	mo->mn = mn;
>  	mo->obj = obj;
>  	mo->it.start = obj->userptr.ptr;
>  	mo->it.last = obj->userptr.ptr + obj->base.size - 1;
> -	INIT_WORK(&mo->work, cancel_userptr);
> +	RB_CLEAR_NODE(&mo->it.rb);
>  
>  	obj->userptr.mmu_object = mo;
>  	return 0;
> @@ -287,12 +310,16 @@ i915_mmu_notifier_free(struct i915_mmu_notifier *mn,
>  		return;
>  
>  	mmu_notifier_unregister(&mn->mn, mm);
> -	destroy_workqueue(mn->wq);
>  	kfree(mn);
>  }
>  
>  #else
>  
> +static void
> +__i915_gem_userptr_set_active(struct drm_i915_gem_object *obj, bool value)
> +{
> +}
> +
>  static void
>  i915_gem_userptr_release__mmu_notifier(struct drm_i915_gem_object *obj)
>  {
> @@ -461,42 +488,6 @@ __i915_gem_userptr_alloc_pages(struct drm_i915_gem_object *obj,
>  	return st;
>  }
>  
> -static int
> -__i915_gem_userptr_set_active(struct drm_i915_gem_object *obj,
> -			      bool value)
> -{
> -	int ret = 0;
> -
> -	/* During mm_invalidate_range we need to cancel any userptr that
> -	 * overlaps the range being invalidated. Doing so requires the
> -	 * struct_mutex, and that risks recursion. In order to cause
> -	 * recursion, the user must alias the userptr address space with
> -	 * a GTT mmapping (possible with a MAP_FIXED) - then when we have
> -	 * to invalidate that mmaping, mm_invalidate_range is called with
> -	 * the userptr address *and* the struct_mutex held.  To prevent that
> -	 * we set a flag under the i915_mmu_notifier spinlock to indicate
> -	 * whether this object is valid.
> -	 */
> -#if defined(CONFIG_MMU_NOTIFIER)
> -	if (obj->userptr.mmu_object == NULL)
> -		return 0;
> -
> -	spin_lock(&obj->userptr.mmu_object->mn->lock);
> -	/* In order to serialise get_pages with an outstanding
> -	 * cancel_userptr, we must drop the struct_mutex and try again.
> -	 */
> -	if (!value)
> -		del_object(obj->userptr.mmu_object);
> -	else if (!work_pending(&obj->userptr.mmu_object->work))
> -		add_object(obj->userptr.mmu_object);
> -	else
> -		ret = -EAGAIN;
> -	spin_unlock(&obj->userptr.mmu_object->mn->lock);
> -#endif
> -
> -	return ret;
> -}
> -
>  static void
>  __i915_gem_userptr_get_pages_worker(struct work_struct *_work)
>  {
> -- 
> 2.19.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] drm/i915/userptr: Avoid struct_mutex recursion for mmu_invalidate_range_start
  2018-10-25 19:16 ` [PATCH] drm/i915/userptr: Avoid struct_mutex recursion for mmu_invalidate_range_start Daniel Vetter
@ 2018-10-25 19:39   ` Chris Wilson
  2018-10-26  9:02     ` Daniel Vetter
  0 siblings, 1 reply; 13+ messages in thread
From: Chris Wilson @ 2018-10-25 19:39 UTC (permalink / raw)
  To: Daniel Vetter; +Cc: intel-gfx

Quoting Daniel Vetter (2018-10-25 20:16:50)
> On Thu, Oct 25, 2018 at 01:45:42PM +0100, Chris Wilson wrote:
> > Since commit 93065ac753e4 ("mm, oom: distinguish blockable mode for mmu
> > notifiers") we have been able to report failure from
> > mmu_invalidate_range_start which allows us to use a trylock on the
> > struct_mutex to avoid potential recursion and report -EBUSY instead.
> > Furthermore, this allows us to pull the work into the main callback and
> > avoid the sleight-of-hand in using a workqueue to avoid lockdep.
> > 
> > However, not all paths to mmu_invalidate_range_start are prepared to
> > handle failure, so instead of reporting the recursion, deal with it.
> > 
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=108375
> > References: 93065ac753e4 ("mm, oom: distinguish blockable mode for mmu notifiers")
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h         |   4 +-
> >  drivers/gpu/drm/i915/i915_gem.c         |  12 +-
> >  drivers/gpu/drm/i915/i915_gem_userptr.c | 219 ++++++++++++------------
> >  3 files changed, 115 insertions(+), 120 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 2d7761b8ac07..c3b94c3f930f 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -3076,8 +3076,8 @@ enum i915_mm_subclass { /* lockdep subclass for obj->mm.lock */
> >       I915_MM_SHRINKER
> >  };
> >  
> > -void __i915_gem_object_put_pages(struct drm_i915_gem_object *obj,
> > -                              enum i915_mm_subclass subclass);
> > +int __i915_gem_object_put_pages(struct drm_i915_gem_object *obj,
> > +                             enum i915_mm_subclass subclass);
> >  void __i915_gem_object_invalidate(struct drm_i915_gem_object *obj);
> >  
> >  enum i915_map_type {
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > index 93d09282710d..de7ccb3eb7b8 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -2454,17 +2454,18 @@ __i915_gem_object_unset_pages(struct drm_i915_gem_object *obj)
> >       return pages;
> >  }
> >  
> > -void __i915_gem_object_put_pages(struct drm_i915_gem_object *obj,
> > -                              enum i915_mm_subclass subclass)
> > +int __i915_gem_object_put_pages(struct drm_i915_gem_object *obj,
> > +                             enum i915_mm_subclass subclass)
> >  {
> >       struct sg_table *pages;
> > +     int ret = -EBUSY;
> >  
> >       if (i915_gem_object_has_pinned_pages(obj))
> > -             return;
> > +             return -EBUSY;
> >  
> >       GEM_BUG_ON(obj->bind_count);
> >       if (!i915_gem_object_has_pages(obj))
> > -             return;
> > +             return 0;
> >  
> >       /* May be called by shrinker from within get_pages() (on another bo) */
> >       mutex_lock_nested(&obj->mm.lock, subclass);
> > @@ -2480,8 +2481,11 @@ void __i915_gem_object_put_pages(struct drm_i915_gem_object *obj,
> >       if (!IS_ERR(pages))
> >               obj->ops->put_pages(obj, pages);
> >  
> > +     ret = 0;
> >  unlock:
> >       mutex_unlock(&obj->mm.lock);
> > +
> > +     return ret;
> >  }
> >  
> >  bool i915_sg_trim(struct sg_table *orig_st)
> > diff --git a/drivers/gpu/drm/i915/i915_gem_userptr.c b/drivers/gpu/drm/i915/i915_gem_userptr.c
> > index 2c9b284036d1..a8f3c304db55 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_userptr.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_userptr.c
> > @@ -50,79 +50,84 @@ struct i915_mmu_notifier {
> >       struct hlist_node node;
> >       struct mmu_notifier mn;
> >       struct rb_root_cached objects;
> > -     struct workqueue_struct *wq;
> > +     struct i915_mm_struct *mm;
> >  };
> >  
> >  struct i915_mmu_object {
> >       struct i915_mmu_notifier *mn;
> >       struct drm_i915_gem_object *obj;
> >       struct interval_tree_node it;
> > -     struct list_head link;
> > -     struct work_struct work;
> > -     bool attached;
> >  };
> >  
> > -static void cancel_userptr(struct work_struct *work)
> > -{
> > -     struct i915_mmu_object *mo = container_of(work, typeof(*mo), work);
> > -     struct drm_i915_gem_object *obj = mo->obj;
> > -     struct work_struct *active;
> > -
> > -     /* Cancel any active worker and force us to re-evaluate gup */
> > -     mutex_lock(&obj->mm.lock);
> > -     active = fetch_and_zero(&obj->userptr.work);
> > -     mutex_unlock(&obj->mm.lock);
> > -     if (active)
> > -             goto out;
> > -
> > -     i915_gem_object_wait(obj, I915_WAIT_ALL, MAX_SCHEDULE_TIMEOUT, NULL);
> > -
> > -     mutex_lock(&obj->base.dev->struct_mutex);
> > -
> > -     /* We are inside a kthread context and can't be interrupted */
> > -     if (i915_gem_object_unbind(obj) == 0)
> > -             __i915_gem_object_put_pages(obj, I915_MM_NORMAL);
> > -     WARN_ONCE(i915_gem_object_has_pages(obj),
> > -               "Failed to release pages: bind_count=%d, pages_pin_count=%d, pin_global=%d\n",
> > -               obj->bind_count,
> > -               atomic_read(&obj->mm.pages_pin_count),
> > -               obj->pin_global);
> > -
> > -     mutex_unlock(&obj->base.dev->struct_mutex);
> > -
> > -out:
> > -     i915_gem_object_put(obj);
> > -}
> > -
> >  static void add_object(struct i915_mmu_object *mo)
> >  {
> > -     if (mo->attached)
> > +     if (!RB_EMPTY_NODE(&mo->it.rb))
> >               return;
> >  
> >       interval_tree_insert(&mo->it, &mo->mn->objects);
> > -     mo->attached = true;
> >  }
> >  
> >  static void del_object(struct i915_mmu_object *mo)
> >  {
> > -     if (!mo->attached)
> > +     if (RB_EMPTY_NODE(&mo->it.rb))
> >               return;
> >  
> >       interval_tree_remove(&mo->it, &mo->mn->objects);
> > -     mo->attached = false;
> > +     RB_CLEAR_NODE(&mo->it.rb);
> > +}
> > +
> > +static void
> > +__i915_gem_userptr_set_active(struct drm_i915_gem_object *obj, bool value)
> > +{
> > +     struct i915_mmu_object *mo = obj->userptr.mmu_object;
> > +
> > +     /*
> > +      * During mm_invalidate_range we need to cancel any userptr that
> > +      * overlaps the range being invalidated. Doing so requires the
> > +      * struct_mutex, and that risks recursion. In order to cause
> > +      * recursion, the user must alias the userptr address space with
> > +      * a GTT mmapping (possible with a MAP_FIXED) - then when we have
> > +      * to invalidate that mmaping, mm_invalidate_range is called with
> > +      * the userptr address *and* the struct_mutex held.  To prevent that
> > +      * we set a flag under the i915_mmu_notifier spinlock to indicate
> > +      * whether this object is valid.
> > +      */
> > +     if (!mo)
> > +             return;
> > +
> > +     spin_lock(&mo->mn->lock);
> > +     if (value)
> > +             add_object(mo);
> > +     else
> > +             del_object(mo);
> > +     spin_unlock(&mo->mn->lock);
> > +}
> > +
> > +static struct mutex *__i915_mutex_lock_recursive(struct mutex *m)
> > +{
> > +     switch (mutex_trylock_recursive(m)) {
> > +     default:
> > +     case MUTEX_TRYLOCK_FAILED:
> > +             mutex_lock(m);
> > +     case MUTEX_TRYLOCK_SUCCESS:
> > +             return m;
> > +
> > +     case MUTEX_TRYLOCK_RECURSIVE:
> > +             return ERR_PTR(-EEXIST);
> > +     }
> >  }
> >  
> >  static int i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn,
> > -                                                    struct mm_struct *mm,
> > -                                                    unsigned long start,
> > -                                                    unsigned long end,
> > -                                                    bool blockable)
> > +                                                   struct mm_struct *mm,
> > +                                                   unsigned long start,
> > +                                                   unsigned long end,
> > +                                                   bool blockable)
> >  {
> >       struct i915_mmu_notifier *mn =
> >               container_of(_mn, struct i915_mmu_notifier, mn);
> > -     struct i915_mmu_object *mo;
> >       struct interval_tree_node *it;
> > -     LIST_HEAD(cancelled);
> > +     struct mutex *unlock = NULL;
> > +     int ret = 0;
> >  
> >       if (RB_EMPTY_ROOT(&mn->objects.rb_root))
> >               return 0;
> > @@ -133,11 +138,16 @@ static int i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn,
> >       spin_lock(&mn->lock);
> >       it = interval_tree_iter_first(&mn->objects, start, end);
> >       while (it) {
> > +             struct drm_i915_gem_object *obj;
> > +             bool has_pages = false;
> > +
> >               if (!blockable) {
> > -                     spin_unlock(&mn->lock);
> > -                     return -EAGAIN;
> > +                     ret = -EAGAIN;
> > +                     break;
> >               }
> > -             /* The mmu_object is released late when destroying the
> > +
> > +             /*
> > +              * The mmu_object is released late when destroying the
> >                * GEM object so it is entirely possible to gain a
> >                * reference on an object in the process of being freed
> >                * since our serialisation is via the spinlock and not
> > @@ -146,21 +156,44 @@ static int i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn,
> >                * use-after-free we only acquire a reference on the
> >                * object if it is not in the process of being destroyed.
> >                */
> > -             mo = container_of(it, struct i915_mmu_object, it);
> > -             if (kref_get_unless_zero(&mo->obj->base.refcount))
> > -                     queue_work(mn->wq, &mo->work);
> > +             obj = container_of(it, struct i915_mmu_object, it)->obj;
> > +             if (!kref_get_unless_zero(&obj->base.refcount)) {
> > +                     it = interval_tree_iter_next(it, start, end);
> > +                     continue;
> > +             }
> > +             spin_unlock(&mn->lock);
> >  
> > -             list_add(&mo->link, &cancelled);
> > -             it = interval_tree_iter_next(it, start, end);
> > +             /* Cancel any pending worker and force us to re-evaluate gup */
> > +             mutex_lock_nested(&obj->mm.lock, I915_MM_SHRINKER);
> 
> mutex_lock_nested is meant for static nesting. This here is rather
> dynamic, and I don't really see where the guarantee is that the classic
> deadlock isn't possible:

It's meant to be after put_pages == 0 to ensure it only applied to a
different object. A bit more involved to do it exactly how I want, that
is to basically incorporate it into put_pages itself. Actually, if it is
in the put_pages it should just work...
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] drm/i915/userptr: Avoid struct_mutex recursion for mmu_invalidate_range_start
  2018-10-25 12:45 [PATCH] drm/i915/userptr: Avoid struct_mutex recursion for mmu_invalidate_range_start Chris Wilson
                   ` (3 preceding siblings ...)
  2018-10-25 19:16 ` [PATCH] drm/i915/userptr: Avoid struct_mutex recursion for mmu_invalidate_range_start Daniel Vetter
@ 2018-10-25 21:04 ` Mark Janes
  4 siblings, 0 replies; 13+ messages in thread
From: Mark Janes @ 2018-10-25 21:04 UTC (permalink / raw)
  To: Chris Wilson, intel-gfx

Chris Wilson <chris@chris-wilson.co.uk> writes:

> Since commit 93065ac753e4 ("mm, oom: distinguish blockable mode for mmu
> notifiers") we have been able to report failure from
> mmu_invalidate_range_start which allows us to use a trylock on the
> struct_mutex to avoid potential recursion and report -EBUSY instead.
> Furthermore, this allows us to pull the work into the main callback and
> avoid the sleight-of-hand in using a workqueue to avoid lockdep.
>
> However, not all paths to mmu_invalidate_range_start are prepared to
> handle failure, so instead of reporting the recursion, deal with it.
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=108375

Does this address https://bugs.freedesktop.org/show_bug.cgi?id=108456 ?

> References: 93065ac753e4 ("mm, oom: distinguish blockable mode for mmu notifiers")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/i915_drv.h         |   4 +-
>  drivers/gpu/drm/i915/i915_gem.c         |  12 +-
>  drivers/gpu/drm/i915/i915_gem_userptr.c | 219 ++++++++++++------------
>  3 files changed, 115 insertions(+), 120 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 2d7761b8ac07..c3b94c3f930f 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -3076,8 +3076,8 @@ enum i915_mm_subclass { /* lockdep subclass for obj->mm.lock */
>  	I915_MM_SHRINKER
>  };
>  
> -void __i915_gem_object_put_pages(struct drm_i915_gem_object *obj,
> -				 enum i915_mm_subclass subclass);
> +int __i915_gem_object_put_pages(struct drm_i915_gem_object *obj,
> +				enum i915_mm_subclass subclass);
>  void __i915_gem_object_invalidate(struct drm_i915_gem_object *obj);
>  
>  enum i915_map_type {
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 93d09282710d..de7ccb3eb7b8 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2454,17 +2454,18 @@ __i915_gem_object_unset_pages(struct drm_i915_gem_object *obj)
>  	return pages;
>  }
>  
> -void __i915_gem_object_put_pages(struct drm_i915_gem_object *obj,
> -				 enum i915_mm_subclass subclass)
> +int __i915_gem_object_put_pages(struct drm_i915_gem_object *obj,
> +				enum i915_mm_subclass subclass)
>  {
>  	struct sg_table *pages;
> +	int ret = -EBUSY;
>  
>  	if (i915_gem_object_has_pinned_pages(obj))
> -		return;
> +		return -EBUSY;
>  
>  	GEM_BUG_ON(obj->bind_count);
>  	if (!i915_gem_object_has_pages(obj))
> -		return;
> +		return 0;
>  
>  	/* May be called by shrinker from within get_pages() (on another bo) */
>  	mutex_lock_nested(&obj->mm.lock, subclass);
> @@ -2480,8 +2481,11 @@ void __i915_gem_object_put_pages(struct drm_i915_gem_object *obj,
>  	if (!IS_ERR(pages))
>  		obj->ops->put_pages(obj, pages);
>  
> +	ret = 0;
>  unlock:
>  	mutex_unlock(&obj->mm.lock);
> +
> +	return ret;
>  }
>  
>  bool i915_sg_trim(struct sg_table *orig_st)
> diff --git a/drivers/gpu/drm/i915/i915_gem_userptr.c b/drivers/gpu/drm/i915/i915_gem_userptr.c
> index 2c9b284036d1..a8f3c304db55 100644
> --- a/drivers/gpu/drm/i915/i915_gem_userptr.c
> +++ b/drivers/gpu/drm/i915/i915_gem_userptr.c
> @@ -50,79 +50,84 @@ struct i915_mmu_notifier {
>  	struct hlist_node node;
>  	struct mmu_notifier mn;
>  	struct rb_root_cached objects;
> -	struct workqueue_struct *wq;
> +	struct i915_mm_struct *mm;
>  };
>  
>  struct i915_mmu_object {
>  	struct i915_mmu_notifier *mn;
>  	struct drm_i915_gem_object *obj;
>  	struct interval_tree_node it;
> -	struct list_head link;
> -	struct work_struct work;
> -	bool attached;
>  };
>  
> -static void cancel_userptr(struct work_struct *work)
> -{
> -	struct i915_mmu_object *mo = container_of(work, typeof(*mo), work);
> -	struct drm_i915_gem_object *obj = mo->obj;
> -	struct work_struct *active;
> -
> -	/* Cancel any active worker and force us to re-evaluate gup */
> -	mutex_lock(&obj->mm.lock);
> -	active = fetch_and_zero(&obj->userptr.work);
> -	mutex_unlock(&obj->mm.lock);
> -	if (active)
> -		goto out;
> -
> -	i915_gem_object_wait(obj, I915_WAIT_ALL, MAX_SCHEDULE_TIMEOUT, NULL);
> -
> -	mutex_lock(&obj->base.dev->struct_mutex);
> -
> -	/* We are inside a kthread context and can't be interrupted */
> -	if (i915_gem_object_unbind(obj) == 0)
> -		__i915_gem_object_put_pages(obj, I915_MM_NORMAL);
> -	WARN_ONCE(i915_gem_object_has_pages(obj),
> -		  "Failed to release pages: bind_count=%d, pages_pin_count=%d, pin_global=%d\n",
> -		  obj->bind_count,
> -		  atomic_read(&obj->mm.pages_pin_count),
> -		  obj->pin_global);
> -
> -	mutex_unlock(&obj->base.dev->struct_mutex);
> -
> -out:
> -	i915_gem_object_put(obj);
> -}
> -
>  static void add_object(struct i915_mmu_object *mo)
>  {
> -	if (mo->attached)
> +	if (!RB_EMPTY_NODE(&mo->it.rb))
>  		return;
>  
>  	interval_tree_insert(&mo->it, &mo->mn->objects);
> -	mo->attached = true;
>  }
>  
>  static void del_object(struct i915_mmu_object *mo)
>  {
> -	if (!mo->attached)
> +	if (RB_EMPTY_NODE(&mo->it.rb))
>  		return;
>  
>  	interval_tree_remove(&mo->it, &mo->mn->objects);
> -	mo->attached = false;
> +	RB_CLEAR_NODE(&mo->it.rb);
> +}
> +
> +static void
> +__i915_gem_userptr_set_active(struct drm_i915_gem_object *obj, bool value)
> +{
> +	struct i915_mmu_object *mo = obj->userptr.mmu_object;
> +
> +	/*
> +	 * During mm_invalidate_range we need to cancel any userptr that
> +	 * overlaps the range being invalidated. Doing so requires the
> +	 * struct_mutex, and that risks recursion. In order to cause
> +	 * recursion, the user must alias the userptr address space with
> +	 * a GTT mmapping (possible with a MAP_FIXED) - then when we have
> +	 * to invalidate that mmaping, mm_invalidate_range is called with
> +	 * the userptr address *and* the struct_mutex held.  To prevent that
> +	 * we set a flag under the i915_mmu_notifier spinlock to indicate
> +	 * whether this object is valid.
> +	 */
> +	if (!mo)
> +		return;
> +
> +	spin_lock(&mo->mn->lock);
> +	if (value)
> +		add_object(mo);
> +	else
> +		del_object(mo);
> +	spin_unlock(&mo->mn->lock);
> +}
> +
> +static struct mutex *__i915_mutex_lock_recursive(struct mutex *m)
> +{
> +	switch (mutex_trylock_recursive(m)) {
> +	default:
> +	case MUTEX_TRYLOCK_FAILED:
> +		mutex_lock(m);
> +	case MUTEX_TRYLOCK_SUCCESS:
> +		return m;
> +
> +	case MUTEX_TRYLOCK_RECURSIVE:
> +		return ERR_PTR(-EEXIST);
> +	}
>  }
>  
>  static int i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn,
> -						       struct mm_struct *mm,
> -						       unsigned long start,
> -						       unsigned long end,
> -						       bool blockable)
> +						      struct mm_struct *mm,
> +						      unsigned long start,
> +						      unsigned long end,
> +						      bool blockable)
>  {
>  	struct i915_mmu_notifier *mn =
>  		container_of(_mn, struct i915_mmu_notifier, mn);
> -	struct i915_mmu_object *mo;
>  	struct interval_tree_node *it;
> -	LIST_HEAD(cancelled);
> +	struct mutex *unlock = NULL;
> +	int ret = 0;
>  
>  	if (RB_EMPTY_ROOT(&mn->objects.rb_root))
>  		return 0;
> @@ -133,11 +138,16 @@ static int i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn,
>  	spin_lock(&mn->lock);
>  	it = interval_tree_iter_first(&mn->objects, start, end);
>  	while (it) {
> +		struct drm_i915_gem_object *obj;
> +		bool has_pages = false;
> +
>  		if (!blockable) {
> -			spin_unlock(&mn->lock);
> -			return -EAGAIN;
> +			ret = -EAGAIN;
> +			break;
>  		}
> -		/* The mmu_object is released late when destroying the
> +
> +		/*
> +		 * The mmu_object is released late when destroying the
>  		 * GEM object so it is entirely possible to gain a
>  		 * reference on an object in the process of being freed
>  		 * since our serialisation is via the spinlock and not
> @@ -146,21 +156,44 @@ static int i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn,
>  		 * use-after-free we only acquire a reference on the
>  		 * object if it is not in the process of being destroyed.
>  		 */
> -		mo = container_of(it, struct i915_mmu_object, it);
> -		if (kref_get_unless_zero(&mo->obj->base.refcount))
> -			queue_work(mn->wq, &mo->work);
> +		obj = container_of(it, struct i915_mmu_object, it)->obj;
> +		if (!kref_get_unless_zero(&obj->base.refcount)) {
> +			it = interval_tree_iter_next(it, start, end);
> +			continue;
> +		}
> +		spin_unlock(&mn->lock);
>  
> -		list_add(&mo->link, &cancelled);
> -		it = interval_tree_iter_next(it, start, end);
> +		/* Cancel any pending worker and force us to re-evaluate gup */
> +		mutex_lock_nested(&obj->mm.lock, I915_MM_SHRINKER);
> +		if (fetch_and_zero(&obj->userptr.work))
> +			__i915_gem_userptr_set_active(obj, false);
> +		else
> +			has_pages = i915_gem_object_has_pages(obj);
> +		mutex_unlock(&obj->mm.lock);
> +
> +		if (has_pages) {
> +			if (!unlock)
> +				unlock = __i915_mutex_lock_recursive(&mn->mm->i915->drm.struct_mutex);
> +			ret = i915_gem_object_unbind(obj);
> +			if (ret == 0)
> +				ret = __i915_gem_object_put_pages(obj, I915_MM_SHRINKER);
> +		}
> +
> +		i915_gem_object_put(obj);
> +		if (ret)
> +			goto unlock;
> +
> +		spin_lock(&mn->lock);
> +		it = interval_tree_iter_first(&mn->objects, start, end);
>  	}
> -	list_for_each_entry(mo, &cancelled, link)
> -		del_object(mo);
>  	spin_unlock(&mn->lock);
>  
> -	if (!list_empty(&cancelled))
> -		flush_workqueue(mn->wq);
> +unlock:
> +	if (!IS_ERR_OR_NULL(unlock))
> +		mutex_unlock(unlock);
> +
> +	return ret;
>  
> -	return 0;
>  }
>  
>  static const struct mmu_notifier_ops i915_gem_userptr_notifier = {
> @@ -168,7 +201,7 @@ static const struct mmu_notifier_ops i915_gem_userptr_notifier = {
>  };
>  
>  static struct i915_mmu_notifier *
> -i915_mmu_notifier_create(struct mm_struct *mm)
> +i915_mmu_notifier_create(struct i915_mm_struct *mm)
>  {
>  	struct i915_mmu_notifier *mn;
>  
> @@ -179,13 +212,7 @@ i915_mmu_notifier_create(struct mm_struct *mm)
>  	spin_lock_init(&mn->lock);
>  	mn->mn.ops = &i915_gem_userptr_notifier;
>  	mn->objects = RB_ROOT_CACHED;
> -	mn->wq = alloc_workqueue("i915-userptr-release",
> -				 WQ_UNBOUND | WQ_MEM_RECLAIM,
> -				 0);
> -	if (mn->wq == NULL) {
> -		kfree(mn);
> -		return ERR_PTR(-ENOMEM);
> -	}
> +	mn->mm = mm;
>  
>  	return mn;
>  }
> @@ -195,16 +222,14 @@ i915_gem_userptr_release__mmu_notifier(struct drm_i915_gem_object *obj)
>  {
>  	struct i915_mmu_object *mo;
>  
> -	mo = obj->userptr.mmu_object;
> -	if (mo == NULL)
> +	mo = fetch_and_zero(&obj->userptr.mmu_object);
> +	if (!mo)
>  		return;
>  
>  	spin_lock(&mo->mn->lock);
>  	del_object(mo);
>  	spin_unlock(&mo->mn->lock);
>  	kfree(mo);
> -
> -	obj->userptr.mmu_object = NULL;
>  }
>  
>  static struct i915_mmu_notifier *
> @@ -217,7 +242,7 @@ i915_mmu_notifier_find(struct i915_mm_struct *mm)
>  	if (mn)
>  		return mn;
>  
> -	mn = i915_mmu_notifier_create(mm->mm);
> +	mn = i915_mmu_notifier_create(mm);
>  	if (IS_ERR(mn))
>  		err = PTR_ERR(mn);
>  
> @@ -240,10 +265,8 @@ i915_mmu_notifier_find(struct i915_mm_struct *mm)
>  	mutex_unlock(&mm->i915->mm_lock);
>  	up_write(&mm->mm->mmap_sem);
>  
> -	if (mn && !IS_ERR(mn)) {
> -		destroy_workqueue(mn->wq);
> +	if (mn && !IS_ERR(mn))
>  		kfree(mn);
> -	}
>  
>  	return err ? ERR_PTR(err) : mm->mn;
>  }
> @@ -266,14 +289,14 @@ i915_gem_userptr_init__mmu_notifier(struct drm_i915_gem_object *obj,
>  		return PTR_ERR(mn);
>  
>  	mo = kzalloc(sizeof(*mo), GFP_KERNEL);
> -	if (mo == NULL)
> +	if (!mo)
>  		return -ENOMEM;
>  
>  	mo->mn = mn;
>  	mo->obj = obj;
>  	mo->it.start = obj->userptr.ptr;
>  	mo->it.last = obj->userptr.ptr + obj->base.size - 1;
> -	INIT_WORK(&mo->work, cancel_userptr);
> +	RB_CLEAR_NODE(&mo->it.rb);
>  
>  	obj->userptr.mmu_object = mo;
>  	return 0;
> @@ -287,12 +310,16 @@ i915_mmu_notifier_free(struct i915_mmu_notifier *mn,
>  		return;
>  
>  	mmu_notifier_unregister(&mn->mn, mm);
> -	destroy_workqueue(mn->wq);
>  	kfree(mn);
>  }
>  
>  #else
>  
> +static void
> +__i915_gem_userptr_set_active(struct drm_i915_gem_object *obj, bool value)
> +{
> +}
> +
>  static void
>  i915_gem_userptr_release__mmu_notifier(struct drm_i915_gem_object *obj)
>  {
> @@ -461,42 +488,6 @@ __i915_gem_userptr_alloc_pages(struct drm_i915_gem_object *obj,
>  	return st;
>  }
>  
> -static int
> -__i915_gem_userptr_set_active(struct drm_i915_gem_object *obj,
> -			      bool value)
> -{
> -	int ret = 0;
> -
> -	/* During mm_invalidate_range we need to cancel any userptr that
> -	 * overlaps the range being invalidated. Doing so requires the
> -	 * struct_mutex, and that risks recursion. In order to cause
> -	 * recursion, the user must alias the userptr address space with
> -	 * a GTT mmapping (possible with a MAP_FIXED) - then when we have
> -	 * to invalidate that mmaping, mm_invalidate_range is called with
> -	 * the userptr address *and* the struct_mutex held.  To prevent that
> -	 * we set a flag under the i915_mmu_notifier spinlock to indicate
> -	 * whether this object is valid.
> -	 */
> -#if defined(CONFIG_MMU_NOTIFIER)
> -	if (obj->userptr.mmu_object == NULL)
> -		return 0;
> -
> -	spin_lock(&obj->userptr.mmu_object->mn->lock);
> -	/* In order to serialise get_pages with an outstanding
> -	 * cancel_userptr, we must drop the struct_mutex and try again.
> -	 */
> -	if (!value)
> -		del_object(obj->userptr.mmu_object);
> -	else if (!work_pending(&obj->userptr.mmu_object->work))
> -		add_object(obj->userptr.mmu_object);
> -	else
> -		ret = -EAGAIN;
> -	spin_unlock(&obj->userptr.mmu_object->mn->lock);
> -#endif
> -
> -	return ret;
> -}
> -
>  static void
>  __i915_gem_userptr_get_pages_worker(struct work_struct *_work)
>  {
> -- 
> 2.19.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] drm/i915/userptr: Avoid struct_mutex recursion for mmu_invalidate_range_start
  2018-10-25 19:39   ` Chris Wilson
@ 2018-10-26  9:02     ` Daniel Vetter
  2018-10-26 13:48       ` Daniel Vetter
  0 siblings, 1 reply; 13+ messages in thread
From: Daniel Vetter @ 2018-10-26  9:02 UTC (permalink / raw)
  To: Chris Wilson; +Cc: intel-gfx

On Thu, Oct 25, 2018 at 9:39 PM Chris Wilson <chris@chris-wilson.co.uk> wrote:
>
> Quoting Daniel Vetter (2018-10-25 20:16:50)
> > On Thu, Oct 25, 2018 at 01:45:42PM +0100, Chris Wilson wrote:
> > > Since commit 93065ac753e4 ("mm, oom: distinguish blockable mode for mmu
> > > notifiers") we have been able to report failure from
> > > mmu_invalidate_range_start which allows us to use a trylock on the
> > > struct_mutex to avoid potential recursion and report -EBUSY instead.
> > > Furthermore, this allows us to pull the work into the main callback and
> > > avoid the sleight-of-hand in using a workqueue to avoid lockdep.
> > >
> > > However, not all paths to mmu_invalidate_range_start are prepared to
> > > handle failure, so instead of reporting the recursion, deal with it.
> > >
> > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=108375
> > > References: 93065ac753e4 ("mm, oom: distinguish blockable mode for mmu notifiers")
> > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > ---
> > >  drivers/gpu/drm/i915/i915_drv.h         |   4 +-
> > >  drivers/gpu/drm/i915/i915_gem.c         |  12 +-
> > >  drivers/gpu/drm/i915/i915_gem_userptr.c | 219 ++++++++++++------------
> > >  3 files changed, 115 insertions(+), 120 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > > index 2d7761b8ac07..c3b94c3f930f 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > @@ -3076,8 +3076,8 @@ enum i915_mm_subclass { /* lockdep subclass for obj->mm.lock */
> > >       I915_MM_SHRINKER
> > >  };
> > >
> > > -void __i915_gem_object_put_pages(struct drm_i915_gem_object *obj,
> > > -                              enum i915_mm_subclass subclass);
> > > +int __i915_gem_object_put_pages(struct drm_i915_gem_object *obj,
> > > +                             enum i915_mm_subclass subclass);
> > >  void __i915_gem_object_invalidate(struct drm_i915_gem_object *obj);
> > >
> > >  enum i915_map_type {
> > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > > index 93d09282710d..de7ccb3eb7b8 100644
> > > --- a/drivers/gpu/drm/i915/i915_gem.c
> > > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > > @@ -2454,17 +2454,18 @@ __i915_gem_object_unset_pages(struct drm_i915_gem_object *obj)
> > >       return pages;
> > >  }
> > >
> > > -void __i915_gem_object_put_pages(struct drm_i915_gem_object *obj,
> > > -                              enum i915_mm_subclass subclass)
> > > +int __i915_gem_object_put_pages(struct drm_i915_gem_object *obj,
> > > +                             enum i915_mm_subclass subclass)
> > >  {
> > >       struct sg_table *pages;
> > > +     int ret = -EBUSY;
> > >
> > >       if (i915_gem_object_has_pinned_pages(obj))
> > > -             return;
> > > +             return -EBUSY;
> > >
> > >       GEM_BUG_ON(obj->bind_count);
> > >       if (!i915_gem_object_has_pages(obj))
> > > -             return;
> > > +             return 0;
> > >
> > >       /* May be called by shrinker from within get_pages() (on another bo) */
> > >       mutex_lock_nested(&obj->mm.lock, subclass);
> > > @@ -2480,8 +2481,11 @@ void __i915_gem_object_put_pages(struct drm_i915_gem_object *obj,
> > >       if (!IS_ERR(pages))
> > >               obj->ops->put_pages(obj, pages);
> > >
> > > +     ret = 0;
> > >  unlock:
> > >       mutex_unlock(&obj->mm.lock);
> > > +
> > > +     return ret;
> > >  }
> > >
> > >  bool i915_sg_trim(struct sg_table *orig_st)
> > > diff --git a/drivers/gpu/drm/i915/i915_gem_userptr.c b/drivers/gpu/drm/i915/i915_gem_userptr.c
> > > index 2c9b284036d1..a8f3c304db55 100644
> > > --- a/drivers/gpu/drm/i915/i915_gem_userptr.c
> > > +++ b/drivers/gpu/drm/i915/i915_gem_userptr.c
> > > @@ -50,79 +50,84 @@ struct i915_mmu_notifier {
> > >       struct hlist_node node;
> > >       struct mmu_notifier mn;
> > >       struct rb_root_cached objects;
> > > -     struct workqueue_struct *wq;
> > > +     struct i915_mm_struct *mm;
> > >  };
> > >
> > >  struct i915_mmu_object {
> > >       struct i915_mmu_notifier *mn;
> > >       struct drm_i915_gem_object *obj;
> > >       struct interval_tree_node it;
> > > -     struct list_head link;
> > > -     struct work_struct work;
> > > -     bool attached;
> > >  };
> > >
> > > -static void cancel_userptr(struct work_struct *work)
> > > -{
> > > -     struct i915_mmu_object *mo = container_of(work, typeof(*mo), work);
> > > -     struct drm_i915_gem_object *obj = mo->obj;
> > > -     struct work_struct *active;
> > > -
> > > -     /* Cancel any active worker and force us to re-evaluate gup */
> > > -     mutex_lock(&obj->mm.lock);
> > > -     active = fetch_and_zero(&obj->userptr.work);
> > > -     mutex_unlock(&obj->mm.lock);
> > > -     if (active)
> > > -             goto out;
> > > -
> > > -     i915_gem_object_wait(obj, I915_WAIT_ALL, MAX_SCHEDULE_TIMEOUT, NULL);
> > > -
> > > -     mutex_lock(&obj->base.dev->struct_mutex);
> > > -
> > > -     /* We are inside a kthread context and can't be interrupted */
> > > -     if (i915_gem_object_unbind(obj) == 0)
> > > -             __i915_gem_object_put_pages(obj, I915_MM_NORMAL);
> > > -     WARN_ONCE(i915_gem_object_has_pages(obj),
> > > -               "Failed to release pages: bind_count=%d, pages_pin_count=%d, pin_global=%d\n",
> > > -               obj->bind_count,
> > > -               atomic_read(&obj->mm.pages_pin_count),
> > > -               obj->pin_global);
> > > -
> > > -     mutex_unlock(&obj->base.dev->struct_mutex);
> > > -
> > > -out:
> > > -     i915_gem_object_put(obj);
> > > -}
> > > -
> > >  static void add_object(struct i915_mmu_object *mo)
> > >  {
> > > -     if (mo->attached)
> > > +     if (!RB_EMPTY_NODE(&mo->it.rb))
> > >               return;
> > >
> > >       interval_tree_insert(&mo->it, &mo->mn->objects);
> > > -     mo->attached = true;
> > >  }
> > >
> > >  static void del_object(struct i915_mmu_object *mo)
> > >  {
> > > -     if (!mo->attached)
> > > +     if (RB_EMPTY_NODE(&mo->it.rb))
> > >               return;
> > >
> > >       interval_tree_remove(&mo->it, &mo->mn->objects);
> > > -     mo->attached = false;
> > > +     RB_CLEAR_NODE(&mo->it.rb);
> > > +}
> > > +
> > > +static void
> > > +__i915_gem_userptr_set_active(struct drm_i915_gem_object *obj, bool value)
> > > +{
> > > +     struct i915_mmu_object *mo = obj->userptr.mmu_object;
> > > +
> > > +     /*
> > > +      * During mm_invalidate_range we need to cancel any userptr that
> > > +      * overlaps the range being invalidated. Doing so requires the
> > > +      * struct_mutex, and that risks recursion. In order to cause
> > > +      * recursion, the user must alias the userptr address space with
> > > +      * a GTT mmapping (possible with a MAP_FIXED) - then when we have
> > > +      * to invalidate that mmaping, mm_invalidate_range is called with
> > > +      * the userptr address *and* the struct_mutex held.  To prevent that
> > > +      * we set a flag under the i915_mmu_notifier spinlock to indicate
> > > +      * whether this object is valid.
> > > +      */
> > > +     if (!mo)
> > > +             return;
> > > +
> > > +     spin_lock(&mo->mn->lock);
> > > +     if (value)
> > > +             add_object(mo);
> > > +     else
> > > +             del_object(mo);
> > > +     spin_unlock(&mo->mn->lock);
> > > +}
> > > +
> > > +static struct mutex *__i915_mutex_lock_recursive(struct mutex *m)
> > > +{
> > > +     switch (mutex_trylock_recursive(m)) {
> > > +     default:
> > > +     case MUTEX_TRYLOCK_FAILED:
> > > +             mutex_lock(m);
> > > +     case MUTEX_TRYLOCK_SUCCESS:
> > > +             return m;
> > > +
> > > +     case MUTEX_TRYLOCK_RECURSIVE:
> > > +             return ERR_PTR(-EEXIST);
> > > +     }
> > >  }
> > >
> > >  static int i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn,
> > > -                                                    struct mm_struct *mm,
> > > -                                                    unsigned long start,
> > > -                                                    unsigned long end,
> > > -                                                    bool blockable)
> > > +                                                   struct mm_struct *mm,
> > > +                                                   unsigned long start,
> > > +                                                   unsigned long end,
> > > +                                                   bool blockable)
> > >  {
> > >       struct i915_mmu_notifier *mn =
> > >               container_of(_mn, struct i915_mmu_notifier, mn);
> > > -     struct i915_mmu_object *mo;
> > >       struct interval_tree_node *it;
> > > -     LIST_HEAD(cancelled);
> > > +     struct mutex *unlock = NULL;
> > > +     int ret = 0;
> > >
> > >       if (RB_EMPTY_ROOT(&mn->objects.rb_root))
> > >               return 0;
> > > @@ -133,11 +138,16 @@ static int i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn,
> > >       spin_lock(&mn->lock);
> > >       it = interval_tree_iter_first(&mn->objects, start, end);
> > >       while (it) {
> > > +             struct drm_i915_gem_object *obj;
> > > +             bool has_pages = false;
> > > +
> > >               if (!blockable) {
> > > -                     spin_unlock(&mn->lock);
> > > -                     return -EAGAIN;
> > > +                     ret = -EAGAIN;
> > > +                     break;
> > >               }
> > > -             /* The mmu_object is released late when destroying the
> > > +
> > > +             /*
> > > +              * The mmu_object is released late when destroying the
> > >                * GEM object so it is entirely possible to gain a
> > >                * reference on an object in the process of being freed
> > >                * since our serialisation is via the spinlock and not
> > > @@ -146,21 +156,44 @@ static int i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn,
> > >                * use-after-free we only acquire a reference on the
> > >                * object if it is not in the process of being destroyed.
> > >                */
> > > -             mo = container_of(it, struct i915_mmu_object, it);
> > > -             if (kref_get_unless_zero(&mo->obj->base.refcount))
> > > -                     queue_work(mn->wq, &mo->work);
> > > +             obj = container_of(it, struct i915_mmu_object, it)->obj;
> > > +             if (!kref_get_unless_zero(&obj->base.refcount)) {
> > > +                     it = interval_tree_iter_next(it, start, end);
> > > +                     continue;
> > > +             }
> > > +             spin_unlock(&mn->lock);
> > >
> > > -             list_add(&mo->link, &cancelled);
> > > -             it = interval_tree_iter_next(it, start, end);
> > > +             /* Cancel any pending worker and force us to re-evaluate gup */
> > > +             mutex_lock_nested(&obj->mm.lock, I915_MM_SHRINKER);
> >
> > mutex_lock_nested is meant for static nesting. This here is rather
> > dynamic, and I don't really see where the guarantee is that the classic
> > deadlock isn't possible:
>
> It's meant to be after put_pages == 0 to ensure it only applied to a
> different object. A bit more involved to do it exactly how I want, that
> is to basically incorporate it into put_pages itself. Actually, if it is
> in the put_pages it should just work...

So for the shrinker I guess you can make this work. For my taste
there's not enough GEM_WARN_ON to check the invariants of the nesting
scheme (I assume it is list_empty(obj->mm.link) -> use I915_MM_NORMAL,
and I915_MM_SHRINKER for anyone who discovers an object through the
obj->mm.link lists, but would be nice to double-check that every time
we take the obj->mm.lock with a GEM_WARN_ON).

But for the mmu notifier I'm not seeing that same guarantee, and
didn't spot any other, so not clear why this works. And assuming I'm
reading the code correctly, we do insert the object into the rb tree
already when gup worker is still doing it's thing.

Or maybe there's another invariant that guarnatees the nesting, but I
checked a few and they all don't seem to work (like pages_pin_count).
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] drm/i915/userptr: Avoid struct_mutex recursion for mmu_invalidate_range_start
  2018-10-26  9:02     ` Daniel Vetter
@ 2018-10-26 13:48       ` Daniel Vetter
  2018-10-29 10:56         ` Chris Wilson
  0 siblings, 1 reply; 13+ messages in thread
From: Daniel Vetter @ 2018-10-26 13:48 UTC (permalink / raw)
  To: Chris Wilson; +Cc: intel-gfx

On Fri, Oct 26, 2018 at 11:02 AM Daniel Vetter <daniel@ffwll.ch> wrote:
>
> On Thu, Oct 25, 2018 at 9:39 PM Chris Wilson <chris@chris-wilson.co.uk> wrote:
> >
> > Quoting Daniel Vetter (2018-10-25 20:16:50)
> > > On Thu, Oct 25, 2018 at 01:45:42PM +0100, Chris Wilson wrote:
> > > > Since commit 93065ac753e4 ("mm, oom: distinguish blockable mode for mmu
> > > > notifiers") we have been able to report failure from
> > > > mmu_invalidate_range_start which allows us to use a trylock on the
> > > > struct_mutex to avoid potential recursion and report -EBUSY instead.
> > > > Furthermore, this allows us to pull the work into the main callback and
> > > > avoid the sleight-of-hand in using a workqueue to avoid lockdep.
> > > >
> > > > However, not all paths to mmu_invalidate_range_start are prepared to
> > > > handle failure, so instead of reporting the recursion, deal with it.
> > > >
> > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=108375
> > > > References: 93065ac753e4 ("mm, oom: distinguish blockable mode for mmu notifiers")
> > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > > ---
> > > >  drivers/gpu/drm/i915/i915_drv.h         |   4 +-
> > > >  drivers/gpu/drm/i915/i915_gem.c         |  12 +-
> > > >  drivers/gpu/drm/i915/i915_gem_userptr.c | 219 ++++++++++++------------
> > > >  3 files changed, 115 insertions(+), 120 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > > > index 2d7761b8ac07..c3b94c3f930f 100644
> > > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > > @@ -3076,8 +3076,8 @@ enum i915_mm_subclass { /* lockdep subclass for obj->mm.lock */
> > > >       I915_MM_SHRINKER
> > > >  };
> > > >
> > > > -void __i915_gem_object_put_pages(struct drm_i915_gem_object *obj,
> > > > -                              enum i915_mm_subclass subclass);
> > > > +int __i915_gem_object_put_pages(struct drm_i915_gem_object *obj,
> > > > +                             enum i915_mm_subclass subclass);
> > > >  void __i915_gem_object_invalidate(struct drm_i915_gem_object *obj);
> > > >
> > > >  enum i915_map_type {
> > > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > > > index 93d09282710d..de7ccb3eb7b8 100644
> > > > --- a/drivers/gpu/drm/i915/i915_gem.c
> > > > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > > > @@ -2454,17 +2454,18 @@ __i915_gem_object_unset_pages(struct drm_i915_gem_object *obj)
> > > >       return pages;
> > > >  }
> > > >
> > > > -void __i915_gem_object_put_pages(struct drm_i915_gem_object *obj,
> > > > -                              enum i915_mm_subclass subclass)
> > > > +int __i915_gem_object_put_pages(struct drm_i915_gem_object *obj,
> > > > +                             enum i915_mm_subclass subclass)
> > > >  {
> > > >       struct sg_table *pages;
> > > > +     int ret = -EBUSY;
> > > >
> > > >       if (i915_gem_object_has_pinned_pages(obj))
> > > > -             return;
> > > > +             return -EBUSY;
> > > >
> > > >       GEM_BUG_ON(obj->bind_count);
> > > >       if (!i915_gem_object_has_pages(obj))
> > > > -             return;
> > > > +             return 0;
> > > >
> > > >       /* May be called by shrinker from within get_pages() (on another bo) */
> > > >       mutex_lock_nested(&obj->mm.lock, subclass);
> > > > @@ -2480,8 +2481,11 @@ void __i915_gem_object_put_pages(struct drm_i915_gem_object *obj,
> > > >       if (!IS_ERR(pages))
> > > >               obj->ops->put_pages(obj, pages);
> > > >
> > > > +     ret = 0;
> > > >  unlock:
> > > >       mutex_unlock(&obj->mm.lock);
> > > > +
> > > > +     return ret;
> > > >  }
> > > >
> > > >  bool i915_sg_trim(struct sg_table *orig_st)
> > > > diff --git a/drivers/gpu/drm/i915/i915_gem_userptr.c b/drivers/gpu/drm/i915/i915_gem_userptr.c
> > > > index 2c9b284036d1..a8f3c304db55 100644
> > > > --- a/drivers/gpu/drm/i915/i915_gem_userptr.c
> > > > +++ b/drivers/gpu/drm/i915/i915_gem_userptr.c
> > > > @@ -50,79 +50,84 @@ struct i915_mmu_notifier {
> > > >       struct hlist_node node;
> > > >       struct mmu_notifier mn;
> > > >       struct rb_root_cached objects;
> > > > -     struct workqueue_struct *wq;
> > > > +     struct i915_mm_struct *mm;
> > > >  };
> > > >
> > > >  struct i915_mmu_object {
> > > >       struct i915_mmu_notifier *mn;
> > > >       struct drm_i915_gem_object *obj;
> > > >       struct interval_tree_node it;
> > > > -     struct list_head link;
> > > > -     struct work_struct work;
> > > > -     bool attached;
> > > >  };
> > > >
> > > > -static void cancel_userptr(struct work_struct *work)
> > > > -{
> > > > -     struct i915_mmu_object *mo = container_of(work, typeof(*mo), work);
> > > > -     struct drm_i915_gem_object *obj = mo->obj;
> > > > -     struct work_struct *active;
> > > > -
> > > > -     /* Cancel any active worker and force us to re-evaluate gup */
> > > > -     mutex_lock(&obj->mm.lock);
> > > > -     active = fetch_and_zero(&obj->userptr.work);
> > > > -     mutex_unlock(&obj->mm.lock);
> > > > -     if (active)
> > > > -             goto out;
> > > > -
> > > > -     i915_gem_object_wait(obj, I915_WAIT_ALL, MAX_SCHEDULE_TIMEOUT, NULL);
> > > > -
> > > > -     mutex_lock(&obj->base.dev->struct_mutex);
> > > > -
> > > > -     /* We are inside a kthread context and can't be interrupted */
> > > > -     if (i915_gem_object_unbind(obj) == 0)
> > > > -             __i915_gem_object_put_pages(obj, I915_MM_NORMAL);
> > > > -     WARN_ONCE(i915_gem_object_has_pages(obj),
> > > > -               "Failed to release pages: bind_count=%d, pages_pin_count=%d, pin_global=%d\n",
> > > > -               obj->bind_count,
> > > > -               atomic_read(&obj->mm.pages_pin_count),
> > > > -               obj->pin_global);
> > > > -
> > > > -     mutex_unlock(&obj->base.dev->struct_mutex);
> > > > -
> > > > -out:
> > > > -     i915_gem_object_put(obj);
> > > > -}
> > > > -
> > > >  static void add_object(struct i915_mmu_object *mo)
> > > >  {
> > > > -     if (mo->attached)
> > > > +     if (!RB_EMPTY_NODE(&mo->it.rb))
> > > >               return;
> > > >
> > > >       interval_tree_insert(&mo->it, &mo->mn->objects);
> > > > -     mo->attached = true;
> > > >  }
> > > >
> > > >  static void del_object(struct i915_mmu_object *mo)
> > > >  {
> > > > -     if (!mo->attached)
> > > > +     if (RB_EMPTY_NODE(&mo->it.rb))
> > > >               return;
> > > >
> > > >       interval_tree_remove(&mo->it, &mo->mn->objects);
> > > > -     mo->attached = false;
> > > > +     RB_CLEAR_NODE(&mo->it.rb);
> > > > +}
> > > > +
> > > > +static void
> > > > +__i915_gem_userptr_set_active(struct drm_i915_gem_object *obj, bool value)
> > > > +{
> > > > +     struct i915_mmu_object *mo = obj->userptr.mmu_object;
> > > > +
> > > > +     /*
> > > > +      * During mm_invalidate_range we need to cancel any userptr that
> > > > +      * overlaps the range being invalidated. Doing so requires the
> > > > +      * struct_mutex, and that risks recursion. In order to cause
> > > > +      * recursion, the user must alias the userptr address space with
> > > > +      * a GTT mmapping (possible with a MAP_FIXED) - then when we have
> > > > +      * to invalidate that mmaping, mm_invalidate_range is called with
> > > > +      * the userptr address *and* the struct_mutex held.  To prevent that
> > > > +      * we set a flag under the i915_mmu_notifier spinlock to indicate
> > > > +      * whether this object is valid.
> > > > +      */
> > > > +     if (!mo)
> > > > +             return;
> > > > +
> > > > +     spin_lock(&mo->mn->lock);
> > > > +     if (value)
> > > > +             add_object(mo);
> > > > +     else
> > > > +             del_object(mo);
> > > > +     spin_unlock(&mo->mn->lock);
> > > > +}
> > > > +
> > > > +static struct mutex *__i915_mutex_lock_recursive(struct mutex *m)
> > > > +{
> > > > +     switch (mutex_trylock_recursive(m)) {
> > > > +     default:
> > > > +     case MUTEX_TRYLOCK_FAILED:
> > > > +             mutex_lock(m);
> > > > +     case MUTEX_TRYLOCK_SUCCESS:
> > > > +             return m;
> > > > +
> > > > +     case MUTEX_TRYLOCK_RECURSIVE:
> > > > +             return ERR_PTR(-EEXIST);
> > > > +     }
> > > >  }
> > > >
> > > >  static int i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn,
> > > > -                                                    struct mm_struct *mm,
> > > > -                                                    unsigned long start,
> > > > -                                                    unsigned long end,
> > > > -                                                    bool blockable)
> > > > +                                                   struct mm_struct *mm,
> > > > +                                                   unsigned long start,
> > > > +                                                   unsigned long end,
> > > > +                                                   bool blockable)
> > > >  {
> > > >       struct i915_mmu_notifier *mn =
> > > >               container_of(_mn, struct i915_mmu_notifier, mn);
> > > > -     struct i915_mmu_object *mo;
> > > >       struct interval_tree_node *it;
> > > > -     LIST_HEAD(cancelled);
> > > > +     struct mutex *unlock = NULL;
> > > > +     int ret = 0;
> > > >
> > > >       if (RB_EMPTY_ROOT(&mn->objects.rb_root))
> > > >               return 0;
> > > > @@ -133,11 +138,16 @@ static int i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn,
> > > >       spin_lock(&mn->lock);
> > > >       it = interval_tree_iter_first(&mn->objects, start, end);
> > > >       while (it) {
> > > > +             struct drm_i915_gem_object *obj;
> > > > +             bool has_pages = false;
> > > > +
> > > >               if (!blockable) {
> > > > -                     spin_unlock(&mn->lock);
> > > > -                     return -EAGAIN;
> > > > +                     ret = -EAGAIN;
> > > > +                     break;
> > > >               }
> > > > -             /* The mmu_object is released late when destroying the
> > > > +
> > > > +             /*
> > > > +              * The mmu_object is released late when destroying the
> > > >                * GEM object so it is entirely possible to gain a
> > > >                * reference on an object in the process of being freed
> > > >                * since our serialisation is via the spinlock and not
> > > > @@ -146,21 +156,44 @@ static int i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn,
> > > >                * use-after-free we only acquire a reference on the
> > > >                * object if it is not in the process of being destroyed.
> > > >                */
> > > > -             mo = container_of(it, struct i915_mmu_object, it);
> > > > -             if (kref_get_unless_zero(&mo->obj->base.refcount))
> > > > -                     queue_work(mn->wq, &mo->work);
> > > > +             obj = container_of(it, struct i915_mmu_object, it)->obj;
> > > > +             if (!kref_get_unless_zero(&obj->base.refcount)) {
> > > > +                     it = interval_tree_iter_next(it, start, end);
> > > > +                     continue;
> > > > +             }
> > > > +             spin_unlock(&mn->lock);
> > > >
> > > > -             list_add(&mo->link, &cancelled);
> > > > -             it = interval_tree_iter_next(it, start, end);
> > > > +             /* Cancel any pending worker and force us to re-evaluate gup */
> > > > +             mutex_lock_nested(&obj->mm.lock, I915_MM_SHRINKER);
> > >
> > > mutex_lock_nested is meant for static nesting. This here is rather
> > > dynamic, and I don't really see where the guarantee is that the classic
> > > deadlock isn't possible:
> >
> > It's meant to be after put_pages == 0 to ensure it only applied to a
> > different object. A bit more involved to do it exactly how I want, that
> > is to basically incorporate it into put_pages itself. Actually, if it is
> > in the put_pages it should just work...
>
> So for the shrinker I guess you can make this work. For my taste
> there's not enough GEM_WARN_ON to check the invariants of the nesting
> scheme (I assume it is list_empty(obj->mm.link) -> use I915_MM_NORMAL,
> and I915_MM_SHRINKER for anyone who discovers an object through the
> obj->mm.link lists, but would be nice to double-check that every time
> we take the obj->mm.lock with a GEM_WARN_ON).

After a bit more reading around I think this isn't enough, and we also
rely on list_del(obj->mm.link) being serialized by dev->struct_mutex.
But I'm not entirely sure. We definitely seem to rely on the magic
"holding dev->struct_mutex extends the lifetime of anything with a
non-empty obj->mm.link" on the shrinker side still.
-Daniel

> But for the mmu notifier I'm not seeing that same guarantee, and
> didn't spot any other, so not clear why this works. And assuming I'm
> reading the code correctly, we do insert the object into the rb tree
> already when gup worker is still doing it's thing.
>
> Or maybe there's another invariant that guarnatees the nesting, but I
> checked a few and they all don't seem to work (like pages_pin_count).
> -Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch



-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] drm/i915/userptr: Avoid struct_mutex recursion for mmu_invalidate_range_start
  2018-10-26 13:48       ` Daniel Vetter
@ 2018-10-29 10:56         ` Chris Wilson
  2018-11-06 10:48           ` Daniel Vetter
  0 siblings, 1 reply; 13+ messages in thread
From: Chris Wilson @ 2018-10-29 10:56 UTC (permalink / raw)
  To: Daniel Vetter; +Cc: intel-gfx

Quoting Daniel Vetter (2018-10-26 14:48:07)
> On Fri, Oct 26, 2018 at 11:02 AM Daniel Vetter <daniel@ffwll.ch> wrote:
> >
> > On Thu, Oct 25, 2018 at 9:39 PM Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > >
> > > Quoting Daniel Vetter (2018-10-25 20:16:50)
> > > > On Thu, Oct 25, 2018 at 01:45:42PM +0100, Chris Wilson wrote:
> > > > > Since commit 93065ac753e4 ("mm, oom: distinguish blockable mode for mmu
> > > > > notifiers") we have been able to report failure from
> > > > > mmu_invalidate_range_start which allows us to use a trylock on the
> > > > > struct_mutex to avoid potential recursion and report -EBUSY instead.
> > > > > Furthermore, this allows us to pull the work into the main callback and
> > > > > avoid the sleight-of-hand in using a workqueue to avoid lockdep.
> > > > >
> > > > > However, not all paths to mmu_invalidate_range_start are prepared to
> > > > > handle failure, so instead of reporting the recursion, deal with it.
> > > > >
> > > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=108375
> > > > > References: 93065ac753e4 ("mm, oom: distinguish blockable mode for mmu notifiers")
> > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > > > ---
> > > > >  drivers/gpu/drm/i915/i915_drv.h         |   4 +-
> > > > >  drivers/gpu/drm/i915/i915_gem.c         |  12 +-
> > > > >  drivers/gpu/drm/i915/i915_gem_userptr.c | 219 ++++++++++++------------
> > > > >  3 files changed, 115 insertions(+), 120 deletions(-)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > > > > index 2d7761b8ac07..c3b94c3f930f 100644
> > > > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > > > @@ -3076,8 +3076,8 @@ enum i915_mm_subclass { /* lockdep subclass for obj->mm.lock */
> > > > >       I915_MM_SHRINKER
> > > > >  };
> > > > >
> > > > > -void __i915_gem_object_put_pages(struct drm_i915_gem_object *obj,
> > > > > -                              enum i915_mm_subclass subclass);
> > > > > +int __i915_gem_object_put_pages(struct drm_i915_gem_object *obj,
> > > > > +                             enum i915_mm_subclass subclass);
> > > > >  void __i915_gem_object_invalidate(struct drm_i915_gem_object *obj);
> > > > >
> > > > >  enum i915_map_type {
> > > > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > > > > index 93d09282710d..de7ccb3eb7b8 100644
> > > > > --- a/drivers/gpu/drm/i915/i915_gem.c
> > > > > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > > > > @@ -2454,17 +2454,18 @@ __i915_gem_object_unset_pages(struct drm_i915_gem_object *obj)
> > > > >       return pages;
> > > > >  }
> > > > >
> > > > > -void __i915_gem_object_put_pages(struct drm_i915_gem_object *obj,
> > > > > -                              enum i915_mm_subclass subclass)
> > > > > +int __i915_gem_object_put_pages(struct drm_i915_gem_object *obj,
> > > > > +                             enum i915_mm_subclass subclass)
> > > > >  {
> > > > >       struct sg_table *pages;
> > > > > +     int ret = -EBUSY;
> > > > >
> > > > >       if (i915_gem_object_has_pinned_pages(obj))
> > > > > -             return;
> > > > > +             return -EBUSY;
> > > > >
> > > > >       GEM_BUG_ON(obj->bind_count);
> > > > >       if (!i915_gem_object_has_pages(obj))
> > > > > -             return;
> > > > > +             return 0;
> > > > >
> > > > >       /* May be called by shrinker from within get_pages() (on another bo) */
> > > > >       mutex_lock_nested(&obj->mm.lock, subclass);
> > > > > @@ -2480,8 +2481,11 @@ void __i915_gem_object_put_pages(struct drm_i915_gem_object *obj,
> > > > >       if (!IS_ERR(pages))
> > > > >               obj->ops->put_pages(obj, pages);
> > > > >
> > > > > +     ret = 0;
> > > > >  unlock:
> > > > >       mutex_unlock(&obj->mm.lock);
> > > > > +
> > > > > +     return ret;
> > > > >  }
> > > > >
> > > > >  bool i915_sg_trim(struct sg_table *orig_st)
> > > > > diff --git a/drivers/gpu/drm/i915/i915_gem_userptr.c b/drivers/gpu/drm/i915/i915_gem_userptr.c
> > > > > index 2c9b284036d1..a8f3c304db55 100644
> > > > > --- a/drivers/gpu/drm/i915/i915_gem_userptr.c
> > > > > +++ b/drivers/gpu/drm/i915/i915_gem_userptr.c
> > > > > @@ -50,79 +50,84 @@ struct i915_mmu_notifier {
> > > > >       struct hlist_node node;
> > > > >       struct mmu_notifier mn;
> > > > >       struct rb_root_cached objects;
> > > > > -     struct workqueue_struct *wq;
> > > > > +     struct i915_mm_struct *mm;
> > > > >  };
> > > > >
> > > > >  struct i915_mmu_object {
> > > > >       struct i915_mmu_notifier *mn;
> > > > >       struct drm_i915_gem_object *obj;
> > > > >       struct interval_tree_node it;
> > > > > -     struct list_head link;
> > > > > -     struct work_struct work;
> > > > > -     bool attached;
> > > > >  };
> > > > >
> > > > > -static void cancel_userptr(struct work_struct *work)
> > > > > -{
> > > > > -     struct i915_mmu_object *mo = container_of(work, typeof(*mo), work);
> > > > > -     struct drm_i915_gem_object *obj = mo->obj;
> > > > > -     struct work_struct *active;
> > > > > -
> > > > > -     /* Cancel any active worker and force us to re-evaluate gup */
> > > > > -     mutex_lock(&obj->mm.lock);
> > > > > -     active = fetch_and_zero(&obj->userptr.work);
> > > > > -     mutex_unlock(&obj->mm.lock);
> > > > > -     if (active)
> > > > > -             goto out;
> > > > > -
> > > > > -     i915_gem_object_wait(obj, I915_WAIT_ALL, MAX_SCHEDULE_TIMEOUT, NULL);
> > > > > -
> > > > > -     mutex_lock(&obj->base.dev->struct_mutex);
> > > > > -
> > > > > -     /* We are inside a kthread context and can't be interrupted */
> > > > > -     if (i915_gem_object_unbind(obj) == 0)
> > > > > -             __i915_gem_object_put_pages(obj, I915_MM_NORMAL);
> > > > > -     WARN_ONCE(i915_gem_object_has_pages(obj),
> > > > > -               "Failed to release pages: bind_count=%d, pages_pin_count=%d, pin_global=%d\n",
> > > > > -               obj->bind_count,
> > > > > -               atomic_read(&obj->mm.pages_pin_count),
> > > > > -               obj->pin_global);
> > > > > -
> > > > > -     mutex_unlock(&obj->base.dev->struct_mutex);
> > > > > -
> > > > > -out:
> > > > > -     i915_gem_object_put(obj);
> > > > > -}
> > > > > -
> > > > >  static void add_object(struct i915_mmu_object *mo)
> > > > >  {
> > > > > -     if (mo->attached)
> > > > > +     if (!RB_EMPTY_NODE(&mo->it.rb))
> > > > >               return;
> > > > >
> > > > >       interval_tree_insert(&mo->it, &mo->mn->objects);
> > > > > -     mo->attached = true;
> > > > >  }
> > > > >
> > > > >  static void del_object(struct i915_mmu_object *mo)
> > > > >  {
> > > > > -     if (!mo->attached)
> > > > > +     if (RB_EMPTY_NODE(&mo->it.rb))
> > > > >               return;
> > > > >
> > > > >       interval_tree_remove(&mo->it, &mo->mn->objects);
> > > > > -     mo->attached = false;
> > > > > +     RB_CLEAR_NODE(&mo->it.rb);
> > > > > +}
> > > > > +
> > > > > +static void
> > > > > +__i915_gem_userptr_set_active(struct drm_i915_gem_object *obj, bool value)
> > > > > +{
> > > > > +     struct i915_mmu_object *mo = obj->userptr.mmu_object;
> > > > > +
> > > > > +     /*
> > > > > +      * During mm_invalidate_range we need to cancel any userptr that
> > > > > +      * overlaps the range being invalidated. Doing so requires the
> > > > > +      * struct_mutex, and that risks recursion. In order to cause
> > > > > +      * recursion, the user must alias the userptr address space with
> > > > > +      * a GTT mmapping (possible with a MAP_FIXED) - then when we have
> > > > > +      * to invalidate that mmaping, mm_invalidate_range is called with
> > > > > +      * the userptr address *and* the struct_mutex held.  To prevent that
> > > > > +      * we set a flag under the i915_mmu_notifier spinlock to indicate
> > > > > +      * whether this object is valid.
> > > > > +      */
> > > > > +     if (!mo)
> > > > > +             return;
> > > > > +
> > > > > +     spin_lock(&mo->mn->lock);
> > > > > +     if (value)
> > > > > +             add_object(mo);
> > > > > +     else
> > > > > +             del_object(mo);
> > > > > +     spin_unlock(&mo->mn->lock);
> > > > > +}
> > > > > +
> > > > > +static struct mutex *__i915_mutex_lock_recursive(struct mutex *m)
> > > > > +{
> > > > > +     switch (mutex_trylock_recursive(m)) {
> > > > > +     default:
> > > > > +     case MUTEX_TRYLOCK_FAILED:
> > > > > +             mutex_lock(m);
> > > > > +     case MUTEX_TRYLOCK_SUCCESS:
> > > > > +             return m;
> > > > > +
> > > > > +     case MUTEX_TRYLOCK_RECURSIVE:
> > > > > +             return ERR_PTR(-EEXIST);
> > > > > +     }
> > > > >  }
> > > > >
> > > > >  static int i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn,
> > > > > -                                                    struct mm_struct *mm,
> > > > > -                                                    unsigned long start,
> > > > > -                                                    unsigned long end,
> > > > > -                                                    bool blockable)
> > > > > +                                                   struct mm_struct *mm,
> > > > > +                                                   unsigned long start,
> > > > > +                                                   unsigned long end,
> > > > > +                                                   bool blockable)
> > > > >  {
> > > > >       struct i915_mmu_notifier *mn =
> > > > >               container_of(_mn, struct i915_mmu_notifier, mn);
> > > > > -     struct i915_mmu_object *mo;
> > > > >       struct interval_tree_node *it;
> > > > > -     LIST_HEAD(cancelled);
> > > > > +     struct mutex *unlock = NULL;
> > > > > +     int ret = 0;
> > > > >
> > > > >       if (RB_EMPTY_ROOT(&mn->objects.rb_root))
> > > > >               return 0;
> > > > > @@ -133,11 +138,16 @@ static int i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn,
> > > > >       spin_lock(&mn->lock);
> > > > >       it = interval_tree_iter_first(&mn->objects, start, end);
> > > > >       while (it) {
> > > > > +             struct drm_i915_gem_object *obj;
> > > > > +             bool has_pages = false;
> > > > > +
> > > > >               if (!blockable) {
> > > > > -                     spin_unlock(&mn->lock);
> > > > > -                     return -EAGAIN;
> > > > > +                     ret = -EAGAIN;
> > > > > +                     break;
> > > > >               }
> > > > > -             /* The mmu_object is released late when destroying the
> > > > > +
> > > > > +             /*
> > > > > +              * The mmu_object is released late when destroying the
> > > > >                * GEM object so it is entirely possible to gain a
> > > > >                * reference on an object in the process of being freed
> > > > >                * since our serialisation is via the spinlock and not
> > > > > @@ -146,21 +156,44 @@ static int i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn,
> > > > >                * use-after-free we only acquire a reference on the
> > > > >                * object if it is not in the process of being destroyed.
> > > > >                */
> > > > > -             mo = container_of(it, struct i915_mmu_object, it);
> > > > > -             if (kref_get_unless_zero(&mo->obj->base.refcount))
> > > > > -                     queue_work(mn->wq, &mo->work);
> > > > > +             obj = container_of(it, struct i915_mmu_object, it)->obj;
> > > > > +             if (!kref_get_unless_zero(&obj->base.refcount)) {
> > > > > +                     it = interval_tree_iter_next(it, start, end);
> > > > > +                     continue;
> > > > > +             }
> > > > > +             spin_unlock(&mn->lock);
> > > > >
> > > > > -             list_add(&mo->link, &cancelled);
> > > > > -             it = interval_tree_iter_next(it, start, end);
> > > > > +             /* Cancel any pending worker and force us to re-evaluate gup */
> > > > > +             mutex_lock_nested(&obj->mm.lock, I915_MM_SHRINKER);
> > > >
> > > > mutex_lock_nested is meant for static nesting. This here is rather
> > > > dynamic, and I don't really see where the guarantee is that the classic
> > > > deadlock isn't possible:
> > >
> > > It's meant to be after put_pages == 0 to ensure it only applied to a
> > > different object. A bit more involved to do it exactly how I want, that
> > > is to basically incorporate it into put_pages itself. Actually, if it is
> > > in the put_pages it should just work...
> >
> > So for the shrinker I guess you can make this work. For my taste
> > there's not enough GEM_WARN_ON to check the invariants of the nesting
> > scheme (I assume it is list_empty(obj->mm.link) -> use I915_MM_NORMAL,
> > and I915_MM_SHRINKER for anyone who discovers an object through the
> > obj->mm.link lists, but would be nice to double-check that every time
> > we take the obj->mm.lock with a GEM_WARN_ON).
> 
> After a bit more reading around I think this isn't enough, and we also
> rely on list_del(obj->mm.link) being serialized by dev->struct_mutex.
> But I'm not entirely sure. We definitely seem to rely on the magic
> "holding dev->struct_mutex extends the lifetime of anything with a
> non-empty obj->mm.link" on the shrinker side still.

Fortunately not, mm.link is spinlock, it's the active ref where the
struct_mutex dance is still required.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] drm/i915/userptr: Avoid struct_mutex recursion for mmu_invalidate_range_start
  2018-10-29 10:56         ` Chris Wilson
@ 2018-11-06 10:48           ` Daniel Vetter
  0 siblings, 0 replies; 13+ messages in thread
From: Daniel Vetter @ 2018-11-06 10:48 UTC (permalink / raw)
  To: Chris Wilson; +Cc: intel-gfx

On Mon, Oct 29, 2018 at 10:56:47AM +0000, Chris Wilson wrote:
> Quoting Daniel Vetter (2018-10-26 14:48:07)
> > On Fri, Oct 26, 2018 at 11:02 AM Daniel Vetter <daniel@ffwll.ch> wrote:
> > >
> > > On Thu, Oct 25, 2018 at 9:39 PM Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > > >
> > > > Quoting Daniel Vetter (2018-10-25 20:16:50)
> > > > > On Thu, Oct 25, 2018 at 01:45:42PM +0100, Chris Wilson wrote:
> > > > > > Since commit 93065ac753e4 ("mm, oom: distinguish blockable mode for mmu
> > > > > > notifiers") we have been able to report failure from
> > > > > > mmu_invalidate_range_start which allows us to use a trylock on the
> > > > > > struct_mutex to avoid potential recursion and report -EBUSY instead.
> > > > > > Furthermore, this allows us to pull the work into the main callback and
> > > > > > avoid the sleight-of-hand in using a workqueue to avoid lockdep.
> > > > > >
> > > > > > However, not all paths to mmu_invalidate_range_start are prepared to
> > > > > > handle failure, so instead of reporting the recursion, deal with it.
> > > > > >
> > > > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=108375
> > > > > > References: 93065ac753e4 ("mm, oom: distinguish blockable mode for mmu notifiers")
> > > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > > > > ---
> > > > > >  drivers/gpu/drm/i915/i915_drv.h         |   4 +-
> > > > > >  drivers/gpu/drm/i915/i915_gem.c         |  12 +-
> > > > > >  drivers/gpu/drm/i915/i915_gem_userptr.c | 219 ++++++++++++------------
> > > > > >  3 files changed, 115 insertions(+), 120 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > > > > > index 2d7761b8ac07..c3b94c3f930f 100644
> > > > > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > > > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > > > > @@ -3076,8 +3076,8 @@ enum i915_mm_subclass { /* lockdep subclass for obj->mm.lock */
> > > > > >       I915_MM_SHRINKER
> > > > > >  };
> > > > > >
> > > > > > -void __i915_gem_object_put_pages(struct drm_i915_gem_object *obj,
> > > > > > -                              enum i915_mm_subclass subclass);
> > > > > > +int __i915_gem_object_put_pages(struct drm_i915_gem_object *obj,
> > > > > > +                             enum i915_mm_subclass subclass);
> > > > > >  void __i915_gem_object_invalidate(struct drm_i915_gem_object *obj);
> > > > > >
> > > > > >  enum i915_map_type {
> > > > > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > > > > > index 93d09282710d..de7ccb3eb7b8 100644
> > > > > > --- a/drivers/gpu/drm/i915/i915_gem.c
> > > > > > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > > > > > @@ -2454,17 +2454,18 @@ __i915_gem_object_unset_pages(struct drm_i915_gem_object *obj)
> > > > > >       return pages;
> > > > > >  }
> > > > > >
> > > > > > -void __i915_gem_object_put_pages(struct drm_i915_gem_object *obj,
> > > > > > -                              enum i915_mm_subclass subclass)
> > > > > > +int __i915_gem_object_put_pages(struct drm_i915_gem_object *obj,
> > > > > > +                             enum i915_mm_subclass subclass)
> > > > > >  {
> > > > > >       struct sg_table *pages;
> > > > > > +     int ret = -EBUSY;
> > > > > >
> > > > > >       if (i915_gem_object_has_pinned_pages(obj))
> > > > > > -             return;
> > > > > > +             return -EBUSY;
> > > > > >
> > > > > >       GEM_BUG_ON(obj->bind_count);
> > > > > >       if (!i915_gem_object_has_pages(obj))
> > > > > > -             return;
> > > > > > +             return 0;
> > > > > >
> > > > > >       /* May be called by shrinker from within get_pages() (on another bo) */
> > > > > >       mutex_lock_nested(&obj->mm.lock, subclass);
> > > > > > @@ -2480,8 +2481,11 @@ void __i915_gem_object_put_pages(struct drm_i915_gem_object *obj,
> > > > > >       if (!IS_ERR(pages))
> > > > > >               obj->ops->put_pages(obj, pages);
> > > > > >
> > > > > > +     ret = 0;
> > > > > >  unlock:
> > > > > >       mutex_unlock(&obj->mm.lock);
> > > > > > +
> > > > > > +     return ret;
> > > > > >  }
> > > > > >
> > > > > >  bool i915_sg_trim(struct sg_table *orig_st)
> > > > > > diff --git a/drivers/gpu/drm/i915/i915_gem_userptr.c b/drivers/gpu/drm/i915/i915_gem_userptr.c
> > > > > > index 2c9b284036d1..a8f3c304db55 100644
> > > > > > --- a/drivers/gpu/drm/i915/i915_gem_userptr.c
> > > > > > +++ b/drivers/gpu/drm/i915/i915_gem_userptr.c
> > > > > > @@ -50,79 +50,84 @@ struct i915_mmu_notifier {
> > > > > >       struct hlist_node node;
> > > > > >       struct mmu_notifier mn;
> > > > > >       struct rb_root_cached objects;
> > > > > > -     struct workqueue_struct *wq;
> > > > > > +     struct i915_mm_struct *mm;
> > > > > >  };
> > > > > >
> > > > > >  struct i915_mmu_object {
> > > > > >       struct i915_mmu_notifier *mn;
> > > > > >       struct drm_i915_gem_object *obj;
> > > > > >       struct interval_tree_node it;
> > > > > > -     struct list_head link;
> > > > > > -     struct work_struct work;
> > > > > > -     bool attached;
> > > > > >  };
> > > > > >
> > > > > > -static void cancel_userptr(struct work_struct *work)
> > > > > > -{
> > > > > > -     struct i915_mmu_object *mo = container_of(work, typeof(*mo), work);
> > > > > > -     struct drm_i915_gem_object *obj = mo->obj;
> > > > > > -     struct work_struct *active;
> > > > > > -
> > > > > > -     /* Cancel any active worker and force us to re-evaluate gup */
> > > > > > -     mutex_lock(&obj->mm.lock);
> > > > > > -     active = fetch_and_zero(&obj->userptr.work);
> > > > > > -     mutex_unlock(&obj->mm.lock);
> > > > > > -     if (active)
> > > > > > -             goto out;
> > > > > > -
> > > > > > -     i915_gem_object_wait(obj, I915_WAIT_ALL, MAX_SCHEDULE_TIMEOUT, NULL);
> > > > > > -
> > > > > > -     mutex_lock(&obj->base.dev->struct_mutex);
> > > > > > -
> > > > > > -     /* We are inside a kthread context and can't be interrupted */
> > > > > > -     if (i915_gem_object_unbind(obj) == 0)
> > > > > > -             __i915_gem_object_put_pages(obj, I915_MM_NORMAL);
> > > > > > -     WARN_ONCE(i915_gem_object_has_pages(obj),
> > > > > > -               "Failed to release pages: bind_count=%d, pages_pin_count=%d, pin_global=%d\n",
> > > > > > -               obj->bind_count,
> > > > > > -               atomic_read(&obj->mm.pages_pin_count),
> > > > > > -               obj->pin_global);
> > > > > > -
> > > > > > -     mutex_unlock(&obj->base.dev->struct_mutex);
> > > > > > -
> > > > > > -out:
> > > > > > -     i915_gem_object_put(obj);
> > > > > > -}
> > > > > > -
> > > > > >  static void add_object(struct i915_mmu_object *mo)
> > > > > >  {
> > > > > > -     if (mo->attached)
> > > > > > +     if (!RB_EMPTY_NODE(&mo->it.rb))
> > > > > >               return;
> > > > > >
> > > > > >       interval_tree_insert(&mo->it, &mo->mn->objects);
> > > > > > -     mo->attached = true;
> > > > > >  }
> > > > > >
> > > > > >  static void del_object(struct i915_mmu_object *mo)
> > > > > >  {
> > > > > > -     if (!mo->attached)
> > > > > > +     if (RB_EMPTY_NODE(&mo->it.rb))
> > > > > >               return;
> > > > > >
> > > > > >       interval_tree_remove(&mo->it, &mo->mn->objects);
> > > > > > -     mo->attached = false;
> > > > > > +     RB_CLEAR_NODE(&mo->it.rb);
> > > > > > +}
> > > > > > +
> > > > > > +static void
> > > > > > +__i915_gem_userptr_set_active(struct drm_i915_gem_object *obj, bool value)
> > > > > > +{
> > > > > > +     struct i915_mmu_object *mo = obj->userptr.mmu_object;
> > > > > > +
> > > > > > +     /*
> > > > > > +      * During mm_invalidate_range we need to cancel any userptr that
> > > > > > +      * overlaps the range being invalidated. Doing so requires the
> > > > > > +      * struct_mutex, and that risks recursion. In order to cause
> > > > > > +      * recursion, the user must alias the userptr address space with
> > > > > > +      * a GTT mmapping (possible with a MAP_FIXED) - then when we have
> > > > > > +      * to invalidate that mmaping, mm_invalidate_range is called with
> > > > > > +      * the userptr address *and* the struct_mutex held.  To prevent that
> > > > > > +      * we set a flag under the i915_mmu_notifier spinlock to indicate
> > > > > > +      * whether this object is valid.
> > > > > > +      */
> > > > > > +     if (!mo)
> > > > > > +             return;
> > > > > > +
> > > > > > +     spin_lock(&mo->mn->lock);
> > > > > > +     if (value)
> > > > > > +             add_object(mo);
> > > > > > +     else
> > > > > > +             del_object(mo);
> > > > > > +     spin_unlock(&mo->mn->lock);
> > > > > > +}
> > > > > > +
> > > > > > +static struct mutex *__i915_mutex_lock_recursive(struct mutex *m)
> > > > > > +{
> > > > > > +     switch (mutex_trylock_recursive(m)) {
> > > > > > +     default:
> > > > > > +     case MUTEX_TRYLOCK_FAILED:
> > > > > > +             mutex_lock(m);
> > > > > > +     case MUTEX_TRYLOCK_SUCCESS:
> > > > > > +             return m;
> > > > > > +
> > > > > > +     case MUTEX_TRYLOCK_RECURSIVE:
> > > > > > +             return ERR_PTR(-EEXIST);
> > > > > > +     }
> > > > > >  }
> > > > > >
> > > > > >  static int i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn,
> > > > > > -                                                    struct mm_struct *mm,
> > > > > > -                                                    unsigned long start,
> > > > > > -                                                    unsigned long end,
> > > > > > -                                                    bool blockable)
> > > > > > +                                                   struct mm_struct *mm,
> > > > > > +                                                   unsigned long start,
> > > > > > +                                                   unsigned long end,
> > > > > > +                                                   bool blockable)
> > > > > >  {
> > > > > >       struct i915_mmu_notifier *mn =
> > > > > >               container_of(_mn, struct i915_mmu_notifier, mn);
> > > > > > -     struct i915_mmu_object *mo;
> > > > > >       struct interval_tree_node *it;
> > > > > > -     LIST_HEAD(cancelled);
> > > > > > +     struct mutex *unlock = NULL;
> > > > > > +     int ret = 0;
> > > > > >
> > > > > >       if (RB_EMPTY_ROOT(&mn->objects.rb_root))
> > > > > >               return 0;
> > > > > > @@ -133,11 +138,16 @@ static int i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn,
> > > > > >       spin_lock(&mn->lock);
> > > > > >       it = interval_tree_iter_first(&mn->objects, start, end);
> > > > > >       while (it) {
> > > > > > +             struct drm_i915_gem_object *obj;
> > > > > > +             bool has_pages = false;
> > > > > > +
> > > > > >               if (!blockable) {
> > > > > > -                     spin_unlock(&mn->lock);
> > > > > > -                     return -EAGAIN;
> > > > > > +                     ret = -EAGAIN;
> > > > > > +                     break;
> > > > > >               }
> > > > > > -             /* The mmu_object is released late when destroying the
> > > > > > +
> > > > > > +             /*
> > > > > > +              * The mmu_object is released late when destroying the
> > > > > >                * GEM object so it is entirely possible to gain a
> > > > > >                * reference on an object in the process of being freed
> > > > > >                * since our serialisation is via the spinlock and not
> > > > > > @@ -146,21 +156,44 @@ static int i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn,
> > > > > >                * use-after-free we only acquire a reference on the
> > > > > >                * object if it is not in the process of being destroyed.
> > > > > >                */
> > > > > > -             mo = container_of(it, struct i915_mmu_object, it);
> > > > > > -             if (kref_get_unless_zero(&mo->obj->base.refcount))
> > > > > > -                     queue_work(mn->wq, &mo->work);
> > > > > > +             obj = container_of(it, struct i915_mmu_object, it)->obj;
> > > > > > +             if (!kref_get_unless_zero(&obj->base.refcount)) {
> > > > > > +                     it = interval_tree_iter_next(it, start, end);
> > > > > > +                     continue;
> > > > > > +             }
> > > > > > +             spin_unlock(&mn->lock);
> > > > > >
> > > > > > -             list_add(&mo->link, &cancelled);
> > > > > > -             it = interval_tree_iter_next(it, start, end);
> > > > > > +             /* Cancel any pending worker and force us to re-evaluate gup */
> > > > > > +             mutex_lock_nested(&obj->mm.lock, I915_MM_SHRINKER);
> > > > >
> > > > > mutex_lock_nested is meant for static nesting. This here is rather
> > > > > dynamic, and I don't really see where the guarantee is that the classic
> > > > > deadlock isn't possible:
> > > >
> > > > It's meant to be after put_pages == 0 to ensure it only applied to a
> > > > different object. A bit more involved to do it exactly how I want, that
> > > > is to basically incorporate it into put_pages itself. Actually, if it is
> > > > in the put_pages it should just work...
> > >
> > > So for the shrinker I guess you can make this work. For my taste
> > > there's not enough GEM_WARN_ON to check the invariants of the nesting
> > > scheme (I assume it is list_empty(obj->mm.link) -> use I915_MM_NORMAL,
> > > and I915_MM_SHRINKER for anyone who discovers an object through the
> > > obj->mm.link lists, but would be nice to double-check that every time
> > > we take the obj->mm.lock with a GEM_WARN_ON).
> > 
> > After a bit more reading around I think this isn't enough, and we also
> > rely on list_del(obj->mm.link) being serialized by dev->struct_mutex.
> > But I'm not entirely sure. We definitely seem to rely on the magic
> > "holding dev->struct_mutex extends the lifetime of anything with a
> > non-empty obj->mm.link" on the shrinker side still.
> 
> Fortunately not, mm.link is spinlock, it's the active ref where the
> struct_mutex dance is still required.

Yup, that's one side of the coin: struct_mutex guarantees that the weak
reference we have through mm.link doesn't result in an untimely demise of
the underlying object. This is easy to fix with a kref_get_unless_zero
while still holding the spinlock (and proceeding to the next obj if we
cannot get a full reference) in the shrinker.

But I think we also rely on struct_mutex making sure that no one else can
move the object off the mm.link list, so that list_del is always
meaningful. Otherwise someone might sneak in between the point where we
drop the spinlock and grab the backing storage lock and wreak some good
havoc. That's the part I'm not sure about though ... 
-Daneil
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH] drm/i915/userptr: Avoid struct_mutex recursion for mmu_invalidate_range_start
  2019-01-15 12:17 [CI] " Chris Wilson
@ 2019-01-15 12:44 ` Chris Wilson
  0 siblings, 0 replies; 13+ messages in thread
From: Chris Wilson @ 2019-01-15 12:44 UTC (permalink / raw)
  To: intel-gfx

Since commit 93065ac753e4 ("mm, oom: distinguish blockable mode for mmu
notifiers") we have been able to report failure from
mmu_invalidate_range_start which allows us to use a trylock on the
struct_mutex to avoid potential recursion and report -EBUSY instead.
Furthermore, this allows us to pull the work into the main callback and
avoid the sleight-of-hand in using a workqueue to avoid lockdep.

However, not all paths to mmu_invalidate_range_start are prepared to
handle failure, so instead of reporting the recursion, deal with it by
propagating the failure upwards, who can decide themselves to handle it
or report it.

v2: Mark up the recursive lock behaviour and comment on the various weak
points.

v3: Follow commit 3824e41975ae ("drm/i915: Use mutex_lock_killable() from
inside the shrinker") and also use mutex_lock_killeable().
v3.1: No leak on EINTR.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=108375
References: 93065ac753e4 ("mm, oom: distinguish blockable mode for mmu notifiers")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h         |   4 +-
 drivers/gpu/drm/i915/i915_gem.c         |  30 +++-
 drivers/gpu/drm/i915/i915_gem_object.h  |   7 +
 drivers/gpu/drm/i915/i915_gem_userptr.c | 224 ++++++++++++------------
 4 files changed, 139 insertions(+), 126 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 956c1c86f90d..da055a86db4d 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2935,8 +2935,8 @@ enum i915_mm_subclass { /* lockdep subclass for obj->mm.lock/struct_mutex */
 	I915_MM_SHRINKER /* called "recursively" from direct-reclaim-esque */
 };
 
-void __i915_gem_object_put_pages(struct drm_i915_gem_object *obj,
-				 enum i915_mm_subclass subclass);
+int __i915_gem_object_put_pages(struct drm_i915_gem_object *obj,
+				enum i915_mm_subclass subclass);
 void __i915_gem_object_invalidate(struct drm_i915_gem_object *obj);
 
 enum i915_map_type {
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 0bfed33178e1..90c167f71345 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2303,8 +2303,8 @@ __i915_gem_object_unset_pages(struct drm_i915_gem_object *obj)
 	struct sg_table *pages;
 
 	pages = fetch_and_zero(&obj->mm.pages);
-	if (!pages)
-		return NULL;
+	if (IS_ERR_OR_NULL(pages))
+		return pages;
 
 	spin_lock(&i915->mm.obj_lock);
 	list_del(&obj->mm.link);
@@ -2328,22 +2328,23 @@ __i915_gem_object_unset_pages(struct drm_i915_gem_object *obj)
 	return pages;
 }
 
-void __i915_gem_object_put_pages(struct drm_i915_gem_object *obj,
-				 enum i915_mm_subclass subclass)
+int __i915_gem_object_put_pages(struct drm_i915_gem_object *obj,
+				enum i915_mm_subclass subclass)
 {
 	struct sg_table *pages;
+	int ret;
 
 	if (i915_gem_object_has_pinned_pages(obj))
-		return;
+		return -EBUSY;
 
 	GEM_BUG_ON(obj->bind_count);
-	if (!i915_gem_object_has_pages(obj))
-		return;
 
 	/* May be called by shrinker from within get_pages() (on another bo) */
 	mutex_lock_nested(&obj->mm.lock, subclass);
-	if (unlikely(atomic_read(&obj->mm.pages_pin_count)))
+	if (unlikely(atomic_read(&obj->mm.pages_pin_count))) {
+		ret = -EBUSY;
 		goto unlock;
+	}
 
 	/*
 	 * ->put_pages might need to allocate memory for the bit17 swizzle
@@ -2351,11 +2352,24 @@ void __i915_gem_object_put_pages(struct drm_i915_gem_object *obj,
 	 * lists early.
 	 */
 	pages = __i915_gem_object_unset_pages(obj);
+
+	/*
+	 * XXX Temporary hijinx to avoid updating all backends to handle
+	 * NULL pages. In the future, when we have more asynchronous
+	 * get_pages backends we should be better able to handle the
+	 * cancellation of the async task in a more uniform manner.
+	 */
+	if (!pages && !i915_gem_object_needs_async_cancel(obj))
+		pages = ERR_PTR(-EINVAL);
+
 	if (!IS_ERR(pages))
 		obj->ops->put_pages(obj, pages);
 
+	ret = 0;
 unlock:
 	mutex_unlock(&obj->mm.lock);
+
+	return ret;
 }
 
 bool i915_sg_trim(struct sg_table *orig_st)
diff --git a/drivers/gpu/drm/i915/i915_gem_object.h b/drivers/gpu/drm/i915/i915_gem_object.h
index ff3da64470dd..cb1b0144d274 100644
--- a/drivers/gpu/drm/i915/i915_gem_object.h
+++ b/drivers/gpu/drm/i915/i915_gem_object.h
@@ -57,6 +57,7 @@ struct drm_i915_gem_object_ops {
 #define I915_GEM_OBJECT_HAS_STRUCT_PAGE	BIT(0)
 #define I915_GEM_OBJECT_IS_SHRINKABLE	BIT(1)
 #define I915_GEM_OBJECT_IS_PROXY	BIT(2)
+#define I915_GEM_OBJECT_ASYNC_CANCEL	BIT(3)
 
 	/* Interface between the GEM object and its backing storage.
 	 * get_pages() is called once prior to the use of the associated set
@@ -387,6 +388,12 @@ i915_gem_object_is_proxy(const struct drm_i915_gem_object *obj)
 	return obj->ops->flags & I915_GEM_OBJECT_IS_PROXY;
 }
 
+static inline bool
+i915_gem_object_needs_async_cancel(const struct drm_i915_gem_object *obj)
+{
+	return obj->ops->flags & I915_GEM_OBJECT_ASYNC_CANCEL;
+}
+
 static inline bool
 i915_gem_object_is_active(const struct drm_i915_gem_object *obj)
 {
diff --git a/drivers/gpu/drm/i915/i915_gem_userptr.c b/drivers/gpu/drm/i915/i915_gem_userptr.c
index 1fb6a7bb5054..38e19a42e0f4 100644
--- a/drivers/gpu/drm/i915/i915_gem_userptr.c
+++ b/drivers/gpu/drm/i915/i915_gem_userptr.c
@@ -49,77 +49,67 @@ struct i915_mmu_notifier {
 	struct hlist_node node;
 	struct mmu_notifier mn;
 	struct rb_root_cached objects;
-	struct workqueue_struct *wq;
+	struct i915_mm_struct *mm;
 };
 
 struct i915_mmu_object {
 	struct i915_mmu_notifier *mn;
 	struct drm_i915_gem_object *obj;
 	struct interval_tree_node it;
-	struct list_head link;
-	struct work_struct work;
-	bool attached;
 };
 
-static void cancel_userptr(struct work_struct *work)
+static void add_object(struct i915_mmu_object *mo)
 {
-	struct i915_mmu_object *mo = container_of(work, typeof(*mo), work);
-	struct drm_i915_gem_object *obj = mo->obj;
-	struct work_struct *active;
-
-	/* Cancel any active worker and force us to re-evaluate gup */
-	mutex_lock(&obj->mm.lock);
-	active = fetch_and_zero(&obj->userptr.work);
-	mutex_unlock(&obj->mm.lock);
-	if (active)
-		goto out;
-
-	i915_gem_object_wait(obj, I915_WAIT_ALL, MAX_SCHEDULE_TIMEOUT, NULL);
-
-	mutex_lock(&obj->base.dev->struct_mutex);
-
-	/* We are inside a kthread context and can't be interrupted */
-	if (i915_gem_object_unbind(obj) == 0)
-		__i915_gem_object_put_pages(obj, I915_MM_NORMAL);
-	WARN_ONCE(i915_gem_object_has_pages(obj),
-		  "Failed to release pages: bind_count=%d, pages_pin_count=%d, pin_global=%d\n",
-		  obj->bind_count,
-		  atomic_read(&obj->mm.pages_pin_count),
-		  obj->pin_global);
-
-	mutex_unlock(&obj->base.dev->struct_mutex);
-
-out:
-	i915_gem_object_put(obj);
+	GEM_BUG_ON(!RB_EMPTY_NODE(&mo->it.rb));
+	interval_tree_insert(&mo->it, &mo->mn->objects);
 }
 
-static void add_object(struct i915_mmu_object *mo)
+static void del_object(struct i915_mmu_object *mo)
 {
-	if (mo->attached)
+	if (RB_EMPTY_NODE(&mo->it.rb))
 		return;
 
-	interval_tree_insert(&mo->it, &mo->mn->objects);
-	mo->attached = true;
+	interval_tree_remove(&mo->it, &mo->mn->objects);
+	RB_CLEAR_NODE(&mo->it.rb);
 }
 
-static void del_object(struct i915_mmu_object *mo)
+static void
+__i915_gem_userptr_set_active(struct drm_i915_gem_object *obj, bool value)
 {
-	if (!mo->attached)
+	struct i915_mmu_object *mo = obj->userptr.mmu_object;
+
+	/*
+	 * During mm_invalidate_range we need to cancel any userptr that
+	 * overlaps the range being invalidated. Doing so requires the
+	 * struct_mutex, and that risks recursion. In order to cause
+	 * recursion, the user must alias the userptr address space with
+	 * a GTT mmapping (possible with a MAP_FIXED) - then when we have
+	 * to invalidate that mmaping, mm_invalidate_range is called with
+	 * the userptr address *and* the struct_mutex held.  To prevent that
+	 * we set a flag under the i915_mmu_notifier spinlock to indicate
+	 * whether this object is valid.
+	 */
+	if (!mo)
 		return;
 
-	interval_tree_remove(&mo->it, &mo->mn->objects);
-	mo->attached = false;
+	spin_lock(&mo->mn->lock);
+	if (value)
+		add_object(mo);
+	else
+		del_object(mo);
+	spin_unlock(&mo->mn->lock);
 }
 
-static int i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn,
-			const struct mmu_notifier_range *range)
+static int
+userptr_mn_invalidate_range_start(struct mmu_notifier *_mn,
+				  const struct mmu_notifier_range *range)
 {
 	struct i915_mmu_notifier *mn =
 		container_of(_mn, struct i915_mmu_notifier, mn);
-	struct i915_mmu_object *mo;
 	struct interval_tree_node *it;
-	LIST_HEAD(cancelled);
+	struct mutex *unlock = NULL;
 	unsigned long end;
+	int ret = 0;
 
 	if (RB_EMPTY_ROOT(&mn->objects.rb_root))
 		return 0;
@@ -130,11 +120,15 @@ static int i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn,
 	spin_lock(&mn->lock);
 	it = interval_tree_iter_first(&mn->objects, range->start, end);
 	while (it) {
+		struct drm_i915_gem_object *obj;
+
 		if (!range->blockable) {
-			spin_unlock(&mn->lock);
-			return -EAGAIN;
+			ret = -EAGAIN;
+			break;
 		}
-		/* The mmu_object is released late when destroying the
+
+		/*
+		 * The mmu_object is released late when destroying the
 		 * GEM object so it is entirely possible to gain a
 		 * reference on an object in the process of being freed
 		 * since our serialisation is via the spinlock and not
@@ -143,29 +137,65 @@ static int i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn,
 		 * use-after-free we only acquire a reference on the
 		 * object if it is not in the process of being destroyed.
 		 */
-		mo = container_of(it, struct i915_mmu_object, it);
-		if (kref_get_unless_zero(&mo->obj->base.refcount))
-			queue_work(mn->wq, &mo->work);
+		obj = container_of(it, struct i915_mmu_object, it)->obj;
+		if (!kref_get_unless_zero(&obj->base.refcount)) {
+			it = interval_tree_iter_next(it, range->start, end);
+			continue;
+		}
+		spin_unlock(&mn->lock);
+
+		if (!unlock) {
+			unlock = &mn->mm->i915->drm.struct_mutex;
+
+			switch (mutex_trylock_recursive(unlock)) {
+			default:
+			case MUTEX_TRYLOCK_FAILED:
+				if (!mutex_lock_killable_nested(unlock, I915_MM_SHRINKER)) {
+					i915_gem_object_put(obj);
+					return -EINTR;
+				}
+				/* fall through */
+			case MUTEX_TRYLOCK_SUCCESS:
+				break;
+
+			case MUTEX_TRYLOCK_RECURSIVE:
+				unlock = ERR_PTR(-EEXIST);
+				break;
+			}
+		}
+
+		ret = i915_gem_object_unbind(obj);
+		if (ret == 0)
+			ret = __i915_gem_object_put_pages(obj, I915_MM_SHRINKER);
+		i915_gem_object_put(obj);
+		if (ret)
+			goto unlock;
 
-		list_add(&mo->link, &cancelled);
-		it = interval_tree_iter_next(it, range->start, end);
+		spin_lock(&mn->lock);
+
+		/*
+		 * As we do not (yet) protect the mmu from concurrent insertion
+		 * over this range, there is no guarantee that this search will
+		 * terminate given a pathologic workload.
+		 */
+		it = interval_tree_iter_first(&mn->objects, range->start, end);
 	}
-	list_for_each_entry(mo, &cancelled, link)
-		del_object(mo);
 	spin_unlock(&mn->lock);
 
-	if (!list_empty(&cancelled))
-		flush_workqueue(mn->wq);
+unlock:
+	if (!IS_ERR_OR_NULL(unlock))
+		mutex_unlock(unlock);
+
+	return ret;
 
-	return 0;
 }
 
 static const struct mmu_notifier_ops i915_gem_userptr_notifier = {
-	.invalidate_range_start = i915_gem_userptr_mn_invalidate_range_start,
+	.invalidate_range_start = userptr_mn_invalidate_range_start,
 };
 
 static struct i915_mmu_notifier *
-i915_mmu_notifier_create(struct mm_struct *mm)
+i915_mmu_notifier_create(struct i915_mm_struct *mm)
 {
 	struct i915_mmu_notifier *mn;
 
@@ -176,13 +206,7 @@ i915_mmu_notifier_create(struct mm_struct *mm)
 	spin_lock_init(&mn->lock);
 	mn->mn.ops = &i915_gem_userptr_notifier;
 	mn->objects = RB_ROOT_CACHED;
-	mn->wq = alloc_workqueue("i915-userptr-release",
-				 WQ_UNBOUND | WQ_MEM_RECLAIM,
-				 0);
-	if (mn->wq == NULL) {
-		kfree(mn);
-		return ERR_PTR(-ENOMEM);
-	}
+	mn->mm = mm;
 
 	return mn;
 }
@@ -192,16 +216,14 @@ i915_gem_userptr_release__mmu_notifier(struct drm_i915_gem_object *obj)
 {
 	struct i915_mmu_object *mo;
 
-	mo = obj->userptr.mmu_object;
-	if (mo == NULL)
+	mo = fetch_and_zero(&obj->userptr.mmu_object);
+	if (!mo)
 		return;
 
 	spin_lock(&mo->mn->lock);
 	del_object(mo);
 	spin_unlock(&mo->mn->lock);
 	kfree(mo);
-
-	obj->userptr.mmu_object = NULL;
 }
 
 static struct i915_mmu_notifier *
@@ -214,7 +236,7 @@ i915_mmu_notifier_find(struct i915_mm_struct *mm)
 	if (mn)
 		return mn;
 
-	mn = i915_mmu_notifier_create(mm->mm);
+	mn = i915_mmu_notifier_create(mm);
 	if (IS_ERR(mn))
 		err = PTR_ERR(mn);
 
@@ -237,10 +259,8 @@ i915_mmu_notifier_find(struct i915_mm_struct *mm)
 	mutex_unlock(&mm->i915->mm_lock);
 	up_write(&mm->mm->mmap_sem);
 
-	if (mn && !IS_ERR(mn)) {
-		destroy_workqueue(mn->wq);
+	if (mn && !IS_ERR(mn))
 		kfree(mn);
-	}
 
 	return err ? ERR_PTR(err) : mm->mn;
 }
@@ -263,14 +283,14 @@ i915_gem_userptr_init__mmu_notifier(struct drm_i915_gem_object *obj,
 		return PTR_ERR(mn);
 
 	mo = kzalloc(sizeof(*mo), GFP_KERNEL);
-	if (mo == NULL)
+	if (!mo)
 		return -ENOMEM;
 
 	mo->mn = mn;
 	mo->obj = obj;
 	mo->it.start = obj->userptr.ptr;
 	mo->it.last = obj->userptr.ptr + obj->base.size - 1;
-	INIT_WORK(&mo->work, cancel_userptr);
+	RB_CLEAR_NODE(&mo->it.rb);
 
 	obj->userptr.mmu_object = mo;
 	return 0;
@@ -284,12 +304,16 @@ i915_mmu_notifier_free(struct i915_mmu_notifier *mn,
 		return;
 
 	mmu_notifier_unregister(&mn->mn, mm);
-	destroy_workqueue(mn->wq);
 	kfree(mn);
 }
 
 #else
 
+static void
+__i915_gem_userptr_set_active(struct drm_i915_gem_object *obj, bool value)
+{
+}
+
 static void
 i915_gem_userptr_release__mmu_notifier(struct drm_i915_gem_object *obj)
 {
@@ -458,42 +482,6 @@ __i915_gem_userptr_alloc_pages(struct drm_i915_gem_object *obj,
 	return st;
 }
 
-static int
-__i915_gem_userptr_set_active(struct drm_i915_gem_object *obj,
-			      bool value)
-{
-	int ret = 0;
-
-	/* During mm_invalidate_range we need to cancel any userptr that
-	 * overlaps the range being invalidated. Doing so requires the
-	 * struct_mutex, and that risks recursion. In order to cause
-	 * recursion, the user must alias the userptr address space with
-	 * a GTT mmapping (possible with a MAP_FIXED) - then when we have
-	 * to invalidate that mmaping, mm_invalidate_range is called with
-	 * the userptr address *and* the struct_mutex held.  To prevent that
-	 * we set a flag under the i915_mmu_notifier spinlock to indicate
-	 * whether this object is valid.
-	 */
-#if defined(CONFIG_MMU_NOTIFIER)
-	if (obj->userptr.mmu_object == NULL)
-		return 0;
-
-	spin_lock(&obj->userptr.mmu_object->mn->lock);
-	/* In order to serialise get_pages with an outstanding
-	 * cancel_userptr, we must drop the struct_mutex and try again.
-	 */
-	if (!value)
-		del_object(obj->userptr.mmu_object);
-	else if (!work_pending(&obj->userptr.mmu_object->work))
-		add_object(obj->userptr.mmu_object);
-	else
-		ret = -EAGAIN;
-	spin_unlock(&obj->userptr.mmu_object->mn->lock);
-#endif
-
-	return ret;
-}
-
 static void
 __i915_gem_userptr_get_pages_worker(struct work_struct *_work)
 {
@@ -679,8 +667,11 @@ i915_gem_userptr_put_pages(struct drm_i915_gem_object *obj,
 	struct sgt_iter sgt_iter;
 	struct page *page;
 
-	BUG_ON(obj->userptr.work != NULL);
+	/* Cancel any inflight work and force them to restart their gup */
+	obj->userptr.work = NULL;
 	__i915_gem_userptr_set_active(obj, false);
+	if (!pages)
+		return;
 
 	if (obj->mm.madv != I915_MADV_WILLNEED)
 		obj->mm.dirty = false;
@@ -718,7 +709,8 @@ i915_gem_userptr_dmabuf_export(struct drm_i915_gem_object *obj)
 
 static const struct drm_i915_gem_object_ops i915_gem_userptr_ops = {
 	.flags = I915_GEM_OBJECT_HAS_STRUCT_PAGE |
-		 I915_GEM_OBJECT_IS_SHRINKABLE,
+		 I915_GEM_OBJECT_IS_SHRINKABLE |
+		 I915_GEM_OBJECT_ASYNC_CANCEL,
 	.get_pages = i915_gem_userptr_get_pages,
 	.put_pages = i915_gem_userptr_put_pages,
 	.dmabuf_export = i915_gem_userptr_dmabuf_export,
-- 
2.20.1

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

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH] drm/i915/userptr: Avoid struct_mutex recursion for mmu_invalidate_range_start
@ 2018-10-23  7:27 Chris Wilson
  0 siblings, 0 replies; 13+ messages in thread
From: Chris Wilson @ 2018-10-23  7:27 UTC (permalink / raw)
  To: intel-gfx

Since commit 93065ac753e4 ("mm, oom: distinguish blockable mode for mmu
notifiers") we have been able to report failure from
mmu_invalidate_range_start which allows us to use a trylock on the
struct_mutex to avoid potential recursion and report -EBUSY instead.
Furthermore, this allows us to pull the work into the main callback and
avoid the sleight-of-hand in using a workqueue to avoid lockdep.

However, not all paths to mmu_invalidate_range_start are prepared to
handle failure, so instead of reporting the recursion, deal with it.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=108375
References: 93065ac753e4 ("mm, oom: distinguish blockable mode for mmu notifiers")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_gem_userptr.c | 144 +++++++++++++-----------
 1 file changed, 77 insertions(+), 67 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_userptr.c b/drivers/gpu/drm/i915/i915_gem_userptr.c
index 2c9b284036d1..64f2ed7e49ec 100644
--- a/drivers/gpu/drm/i915/i915_gem_userptr.c
+++ b/drivers/gpu/drm/i915/i915_gem_userptr.c
@@ -50,7 +50,7 @@ struct i915_mmu_notifier {
 	struct hlist_node node;
 	struct mmu_notifier mn;
 	struct rb_root_cached objects;
-	struct workqueue_struct *wq;
+	struct i915_mm_struct *mm;
 };
 
 struct i915_mmu_object {
@@ -58,42 +58,9 @@ struct i915_mmu_object {
 	struct drm_i915_gem_object *obj;
 	struct interval_tree_node it;
 	struct list_head link;
-	struct work_struct work;
 	bool attached;
 };
 
-static void cancel_userptr(struct work_struct *work)
-{
-	struct i915_mmu_object *mo = container_of(work, typeof(*mo), work);
-	struct drm_i915_gem_object *obj = mo->obj;
-	struct work_struct *active;
-
-	/* Cancel any active worker and force us to re-evaluate gup */
-	mutex_lock(&obj->mm.lock);
-	active = fetch_and_zero(&obj->userptr.work);
-	mutex_unlock(&obj->mm.lock);
-	if (active)
-		goto out;
-
-	i915_gem_object_wait(obj, I915_WAIT_ALL, MAX_SCHEDULE_TIMEOUT, NULL);
-
-	mutex_lock(&obj->base.dev->struct_mutex);
-
-	/* We are inside a kthread context and can't be interrupted */
-	if (i915_gem_object_unbind(obj) == 0)
-		__i915_gem_object_put_pages(obj, I915_MM_NORMAL);
-	WARN_ONCE(i915_gem_object_has_pages(obj),
-		  "Failed to release pages: bind_count=%d, pages_pin_count=%d, pin_global=%d\n",
-		  obj->bind_count,
-		  atomic_read(&obj->mm.pages_pin_count),
-		  obj->pin_global);
-
-	mutex_unlock(&obj->base.dev->struct_mutex);
-
-out:
-	i915_gem_object_put(obj);
-}
-
 static void add_object(struct i915_mmu_object *mo)
 {
 	if (mo->attached)
@@ -112,17 +79,33 @@ static void del_object(struct i915_mmu_object *mo)
 	mo->attached = false;
 }
 
+static struct mutex *__i915_mutex_lock_recursive(struct mutex *m)
+{
+	switch (mutex_trylock_recursive(m)) {
+	default:
+	case MUTEX_TRYLOCK_FAILED:
+		mutex_lock(m);
+	case MUTEX_TRYLOCK_SUCCESS:
+		return m;
+
+	case MUTEX_TRYLOCK_RECURSIVE:
+		return NULL;
+	}
+}
+
 static int i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn,
-						       struct mm_struct *mm,
-						       unsigned long start,
-						       unsigned long end,
-						       bool blockable)
+						      struct mm_struct *mm,
+						      unsigned long start,
+						      unsigned long end,
+						      bool blockable)
 {
 	struct i915_mmu_notifier *mn =
 		container_of(_mn, struct i915_mmu_notifier, mn);
-	struct i915_mmu_object *mo;
+	struct i915_mmu_object *mo, *next;
 	struct interval_tree_node *it;
 	LIST_HEAD(cancelled);
+	struct mutex *unlock;
+	int ret;
 
 	if (RB_EMPTY_ROOT(&mn->objects.rb_root))
 		return 0;
@@ -148,19 +131,61 @@ static int i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn,
 		 */
 		mo = container_of(it, struct i915_mmu_object, it);
 		if (kref_get_unless_zero(&mo->obj->base.refcount))
-			queue_work(mn->wq, &mo->work);
+			list_add(&mo->link, &cancelled);
 
-		list_add(&mo->link, &cancelled);
 		it = interval_tree_iter_next(it, start, end);
 	}
-	list_for_each_entry(mo, &cancelled, link)
-		del_object(mo);
 	spin_unlock(&mn->lock);
 
-	if (!list_empty(&cancelled))
-		flush_workqueue(mn->wq);
+	list_for_each_entry_safe(mo, next, &cancelled, link) {
+		struct drm_i915_gem_object *obj = mo->obj;
+		bool pending;
 
-	return 0;
+		/* Cancel any pending worker and force us to re-evaluate gup */
+		mutex_lock_nested(&obj->mm.lock, I915_MM_SHRINKER);
+		pending = fetch_and_zero(&obj->userptr.work);
+		mutex_unlock(&obj->mm.lock);
+		if (pending) {
+			list_del(&mo->link);
+
+			spin_lock(&mn->lock);
+			del_object(mo);
+			spin_unlock(&mn->lock);
+
+			i915_gem_object_put(obj);
+		}
+	}
+
+	if (list_empty(&cancelled))
+		return 0;
+
+	unlock = __i915_mutex_lock_recursive(&mn->mm->i915->drm.struct_mutex);
+
+	ret = 0;
+	list_for_each_entry_safe(mo, next, &cancelled, link) {
+		struct drm_i915_gem_object *obj = mo->obj;
+		int err;
+
+		err = i915_gem_object_unbind(obj);
+		if (err == 0) {
+			__i915_gem_object_put_pages(obj, I915_MM_SHRINKER);
+			GEM_BUG_ON(obj->mm.pages);
+
+			spin_lock(&mn->lock);
+			del_object(mo);
+			spin_unlock(&mn->lock);
+		} else {
+			if (ret == 0)
+				ret = err;
+		}
+
+		i915_gem_object_put(obj);
+	}
+
+	if (unlock)
+		mutex_unlock(unlock);
+
+	return ret;
 }
 
 static const struct mmu_notifier_ops i915_gem_userptr_notifier = {
@@ -168,7 +193,7 @@ static const struct mmu_notifier_ops i915_gem_userptr_notifier = {
 };
 
 static struct i915_mmu_notifier *
-i915_mmu_notifier_create(struct mm_struct *mm)
+i915_mmu_notifier_create(struct i915_mm_struct *mm)
 {
 	struct i915_mmu_notifier *mn;
 
@@ -179,13 +204,7 @@ i915_mmu_notifier_create(struct mm_struct *mm)
 	spin_lock_init(&mn->lock);
 	mn->mn.ops = &i915_gem_userptr_notifier;
 	mn->objects = RB_ROOT_CACHED;
-	mn->wq = alloc_workqueue("i915-userptr-release",
-				 WQ_UNBOUND | WQ_MEM_RECLAIM,
-				 0);
-	if (mn->wq == NULL) {
-		kfree(mn);
-		return ERR_PTR(-ENOMEM);
-	}
+	mn->mm = mm;
 
 	return mn;
 }
@@ -217,7 +236,7 @@ i915_mmu_notifier_find(struct i915_mm_struct *mm)
 	if (mn)
 		return mn;
 
-	mn = i915_mmu_notifier_create(mm->mm);
+	mn = i915_mmu_notifier_create(mm);
 	if (IS_ERR(mn))
 		err = PTR_ERR(mn);
 
@@ -240,10 +259,8 @@ i915_mmu_notifier_find(struct i915_mm_struct *mm)
 	mutex_unlock(&mm->i915->mm_lock);
 	up_write(&mm->mm->mmap_sem);
 
-	if (mn && !IS_ERR(mn)) {
-		destroy_workqueue(mn->wq);
+	if (mn && !IS_ERR(mn))
 		kfree(mn);
-	}
 
 	return err ? ERR_PTR(err) : mm->mn;
 }
@@ -273,7 +290,6 @@ i915_gem_userptr_init__mmu_notifier(struct drm_i915_gem_object *obj,
 	mo->obj = obj;
 	mo->it.start = obj->userptr.ptr;
 	mo->it.last = obj->userptr.ptr + obj->base.size - 1;
-	INIT_WORK(&mo->work, cancel_userptr);
 
 	obj->userptr.mmu_object = mo;
 	return 0;
@@ -287,7 +303,6 @@ i915_mmu_notifier_free(struct i915_mmu_notifier *mn,
 		return;
 
 	mmu_notifier_unregister(&mn->mn, mm);
-	destroy_workqueue(mn->wq);
 	kfree(mn);
 }
 
@@ -482,15 +497,10 @@ __i915_gem_userptr_set_active(struct drm_i915_gem_object *obj,
 		return 0;
 
 	spin_lock(&obj->userptr.mmu_object->mn->lock);
-	/* In order to serialise get_pages with an outstanding
-	 * cancel_userptr, we must drop the struct_mutex and try again.
-	 */
-	if (!value)
-		del_object(obj->userptr.mmu_object);
-	else if (!work_pending(&obj->userptr.mmu_object->work))
+	if (value)
 		add_object(obj->userptr.mmu_object);
 	else
-		ret = -EAGAIN;
+		del_object(obj->userptr.mmu_object);
 	spin_unlock(&obj->userptr.mmu_object->mn->lock);
 #endif
 
-- 
2.19.1

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

^ permalink raw reply related	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2019-01-15 12:44 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-10-25 12:45 [PATCH] drm/i915/userptr: Avoid struct_mutex recursion for mmu_invalidate_range_start Chris Wilson
2018-10-25 13:32 ` ✗ Fi.CI.CHECKPATCH: warning for drm/i915/userptr: Avoid struct_mutex recursion for mmu_invalidate_range_start (rev2) Patchwork
2018-10-25 13:50 ` ✓ Fi.CI.BAT: success " Patchwork
2018-10-25 18:57 ` ✗ Fi.CI.IGT: failure " Patchwork
2018-10-25 19:16 ` [PATCH] drm/i915/userptr: Avoid struct_mutex recursion for mmu_invalidate_range_start Daniel Vetter
2018-10-25 19:39   ` Chris Wilson
2018-10-26  9:02     ` Daniel Vetter
2018-10-26 13:48       ` Daniel Vetter
2018-10-29 10:56         ` Chris Wilson
2018-11-06 10:48           ` Daniel Vetter
2018-10-25 21:04 ` Mark Janes
  -- strict thread matches above, loose matches on Subject: below --
2019-01-15 12:17 [CI] " Chris Wilson
2019-01-15 12:44 ` [PATCH] " Chris Wilson
2018-10-23  7:27 Chris Wilson

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.