[PATCH 1/3] drm/i915: Prevent concurrent GGTT update and use on Braswell (again)

[PATCH 1/3] drm/i915: Prevent concurrent GGTT update and use on Braswell (again)

[Chris Wilson]

On Braswell, under heavy stress, if we update the GGTT while
simultaneously accessing another region inside the GTT, we are returned
the wrong values. To prevent this we stop the machine to update the GGTT
entries so that no memory traffic can occur at the same time.

This was first spotted in

commit 5bab6f60cb4d1417ad7c599166bcfec87529c1a2
Author: Chris Wilson <chris@chris-wilson.co.uk>
Date:   Fri Oct 23 18:43:32 2015 +0100

    drm/i915: Serialise updates to GGTT with access through GGTT on Braswell

but removed again in forlorn hope with

commit 4509276ee824bb967885c095c610767e42345c36
Author: Chris Wilson <chris@chris-wilson.co.uk>
Date:   Mon Feb 20 12:47:18 2017 +0000

    drm/i915: Remove Braswell GGTT update w/a

However, gem_concurrent_blit is once again only stable with the patch
applied and CI is detecting the odd failure in forked gem_mmap_gtt tests
(which smell like the same issue). Fwiw, a wide variety of CPU memory
barriers (around GGTT flushing, fence updates, PTE updates) and GPU
flushes/invalidates (between requests, after PTE updates) were tried as
part of the investigation to find an alternate cause, nothing comes
close to serialised GGTT updates.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105591
Testcase: igt/gem_concurrent_blit
Testcase: igt/gem_mmap_gtt/*forked*
References: 5bab6f60cb4d ("drm/i915: Serialise updates to GGTT with access through GGTT on Braswell")
References: 4509276ee824 ("drm/i915: Remove Braswell GGTT update w/a")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_gem_gtt.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index dbea14bf67cc..f0d46366fb0b 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -3232,7 +3232,8 @@ static int gen8_gmch_probe(struct i915_ggtt *ggtt)
 	ggtt->vm.insert_entries = gen8_ggtt_insert_entries;
 
 	/* Serialize GTT updates with aperture access on BXT if VT-d is on. */
-	if (intel_ggtt_update_needs_vtd_wa(dev_priv)) {
+	if (intel_ggtt_update_needs_vtd_wa(dev_priv) ||
+	    IS_CHERRYVIEW(dev_priv) /* fails with concurrent use/update */) {
 		ggtt->vm.insert_entries = bxt_vtd_ggtt_insert_entries__BKL;
 		ggtt->vm.insert_page    = bxt_vtd_ggtt_insert_page__BKL;
 		if (ggtt->vm.clear_range != nop_clear_range)
-- 
2.20.1

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

[PATCH 2/3] drm/i915/userptr: Avoid struct_mutex recursion for mmu_invalidate_range_start

[Chris Wilson]

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.

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>
---
 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 | 220 +++++++++++-------------
 4 files changed, 135 insertions(+), 126 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index fa99824f63b3..4b67037c0318 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2931,8 +2931,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..9c2008a480e2 100644
--- a/drivers/gpu/drm/i915/i915_gem_userptr.c
+++ b/drivers/gpu/drm/i915/i915_gem_userptr.c
@@ -49,77 +49,81 @@ 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 struct mutex *__i915_mutex_lock_recursive(struct mutex *m)
+{
+	switch (mutex_trylock_recursive(m)) {
+	default:
+	case MUTEX_TRYLOCK_FAILED:
+		mutex_lock_nested(m, I915_MM_SHRINKER);
+	case MUTEX_TRYLOCK_SUCCESS:
+		return m;
+
+	case MUTEX_TRYLOCK_RECURSIVE:
+		return ERR_PTR(-EEXIST);
+	}
+}
+
+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 +134,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 +151,47 @@ 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);
 
-		list_add(&mo->link, &cancelled);
-		it = interval_tree_iter_next(it, range->start, end);
+		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);
+
+		/*
+		 * 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 +202,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 +212,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 +232,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 +255,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 +279,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 +300,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 +478,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 +663,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 +705,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

Re: [PATCH 2/3] drm/i915/userptr: Avoid struct_mutex recursion for mmu_invalidate_range_start

[Tvrtko Ursulin]

On 14/01/2019 21:17, 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 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.
> 
> 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>
> ---
>   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 | 220 +++++++++++-------------
>   4 files changed, 135 insertions(+), 126 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index fa99824f63b3..4b67037c0318 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2931,8 +2931,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..9c2008a480e2 100644
> --- a/drivers/gpu/drm/i915/i915_gem_userptr.c
> +++ b/drivers/gpu/drm/i915/i915_gem_userptr.c
> @@ -49,77 +49,81 @@ 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 struct mutex *__i915_mutex_lock_recursive(struct mutex *m)
> +{
> +	switch (mutex_trylock_recursive(m)) {
> +	default:
> +	case MUTEX_TRYLOCK_FAILED:
> +		mutex_lock_nested(m, I915_MM_SHRINKER);

Have you found in the meantime that this is actually needed? Although it 
is since we use this subclass to put the pages..

Should i915_gem_shrinker_tainst_mutex taint obj->mm.lock with the 
shrinker subclass?

> +	case MUTEX_TRYLOCK_SUCCESS:
> +		return m;
> +
> +	case MUTEX_TRYLOCK_RECURSIVE:
> +		return ERR_PTR(-EEXIST);
> +	}
> +}
> +
> +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 +134,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 +151,47 @@ 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);
>   
> -		list_add(&mo->link, &cancelled);
> -		it = interval_tree_iter_next(it, range->start, end);
> +		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);
> +
> +		/*
> +		 * 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 +202,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 +212,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 +232,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 +255,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 +279,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 +300,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 +478,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 +663,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 +705,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,
> 

Regards,

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

Re: [PATCH 2/3] drm/i915/userptr: Avoid struct_mutex recursion for mmu_invalidate_range_start

[Chris Wilson]

Quoting Tvrtko Ursulin (2019-01-15 09:47:45)
> 
> On 14/01/2019 21:17, 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 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.
> > 
> > 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>
> > ---
> >   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 | 220 +++++++++++-------------
> >   4 files changed, 135 insertions(+), 126 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index fa99824f63b3..4b67037c0318 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -2931,8 +2931,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..9c2008a480e2 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_userptr.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_userptr.c
> > @@ -49,77 +49,81 @@ 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 struct mutex *__i915_mutex_lock_recursive(struct mutex *m)
> > +{
> > +     switch (mutex_trylock_recursive(m)) {
> > +     default:
> > +     case MUTEX_TRYLOCK_FAILED:
> > +             mutex_lock_nested(m, I915_MM_SHRINKER);
> 
> Have you found in the meantime that this is actually needed? Although it 
> is since we use this subclass to put the pages..

Yes, if we do not attempt to try a full mutex_lock() here, we fail
the invalidation subtests of gem_userptr_blits(). But you mean the
subclass? Given enough time I'll find the path to break it, at the
present time I feel more comfortable cargo-culting the rules devised for
the recursive locking from inside direct-reclaim to another
direct-reclaim path.

The end-goal here is that upon breaking up struct_mutex we replace it
various locks that we are not allowed to allocate under. No more direct
reclaim recursion.

> Should i915_gem_shrinker_tainst_mutex taint obj->mm.lock with the 
> shrinker subclass?

The shrinker uses the obj->mm.lock I915_MM_SHRINKER subclass, so it's a
little more faff to taint.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/3] drm/i915/userptr: Avoid struct_mutex recursion for mmu_invalidate_range_start

[Tvrtko Ursulin]

On 15/01/2019 10:00, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2019-01-15 09:47:45)
>>
>> On 14/01/2019 21:17, 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 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.
>>>
>>> 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>
>>> ---
>>>    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 | 220 +++++++++++-------------
>>>    4 files changed, 135 insertions(+), 126 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>>> index fa99824f63b3..4b67037c0318 100644
>>> --- a/drivers/gpu/drm/i915/i915_drv.h
>>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>>> @@ -2931,8 +2931,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..9c2008a480e2 100644
>>> --- a/drivers/gpu/drm/i915/i915_gem_userptr.c
>>> +++ b/drivers/gpu/drm/i915/i915_gem_userptr.c
>>> @@ -49,77 +49,81 @@ 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 struct mutex *__i915_mutex_lock_recursive(struct mutex *m)
>>> +{
>>> +     switch (mutex_trylock_recursive(m)) {
>>> +     default:
>>> +     case MUTEX_TRYLOCK_FAILED:
>>> +             mutex_lock_nested(m, I915_MM_SHRINKER);
>>
>> Have you found in the meantime that this is actually needed? Although it
>> is since we use this subclass to put the pages..
> 
> Yes, if we do not attempt to try a full mutex_lock() here, we fail
> the invalidation subtests of gem_userptr_blits(). But you mean the
> subclass? Given enough time I'll find the path to break it, at the
> present time I feel more comfortable cargo-culting the rules devised for
> the recursive locking from inside direct-reclaim to another
> direct-reclaim path.
> 
> The end-goal here is that upon breaking up struct_mutex we replace it
> various locks that we are not allowed to allocate under. No more direct
> reclaim recursion.
> 
>> Should i915_gem_shrinker_tainst_mutex taint obj->mm.lock with the
>> shrinker subclass?
> 
> The shrinker uses the obj->mm.lock I915_MM_SHRINKER subclass, so it's a
> little more faff to taint.

I am not 100% sure of all paths that could be affected by the struct 
mutex nested annotation but I guess it is worth a shot..

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

Regards,

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

Re: [PATCH 2/3] drm/i915/userptr: Avoid struct_mutex recursion for mmu_invalidate_range_start

[Tvrtko Ursulin]

On 14/01/2019 21:17, 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 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.
> 
> 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>
> ---
>   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 | 220 +++++++++++-------------
>   4 files changed, 135 insertions(+), 126 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index fa99824f63b3..4b67037c0318 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2931,8 +2931,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..9c2008a480e2 100644
> --- a/drivers/gpu/drm/i915/i915_gem_userptr.c
> +++ b/drivers/gpu/drm/i915/i915_gem_userptr.c
> @@ -49,77 +49,81 @@ 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 struct mutex *__i915_mutex_lock_recursive(struct mutex *m)
> +{
> +	switch (mutex_trylock_recursive(m)) {
> +	default:
> +	case MUTEX_TRYLOCK_FAILED:
> +		mutex_lock_nested(m, I915_MM_SHRINKER);

Worth using killable and refactoring the caller to handle the failure? 
If this path ends up deadlocking one day it would be slightly better 
that the process can be removed.

Regards,

Tvrtko

> +	case MUTEX_TRYLOCK_SUCCESS:
> +		return m;
> +
> +	case MUTEX_TRYLOCK_RECURSIVE:
> +		return ERR_PTR(-EEXIST);
> +	}
> +}
> +
> +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 +134,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 +151,47 @@ 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);
>   
> -		list_add(&mo->link, &cancelled);
> -		it = interval_tree_iter_next(it, range->start, end);
> +		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);
> +
> +		/*
> +		 * 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 +202,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 +212,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 +232,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 +255,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 +279,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 +300,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 +478,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 +663,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 +705,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,
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/3] drm/i915/userptr: Avoid struct_mutex recursion for mmu_invalidate_range_start

[Chris Wilson]

Quoting Tvrtko Ursulin (2019-01-15 11:54:15)
> 
> On 14/01/2019 21:17, Chris Wilson wrote:
> > -static int i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn,
> > -                     const struct mmu_notifier_range *range)
> > +static struct mutex *__i915_mutex_lock_recursive(struct mutex *m)
> > +{
> > +     switch (mutex_trylock_recursive(m)) {
> > +     default:
> > +     case MUTEX_TRYLOCK_FAILED:
> > +             mutex_lock_nested(m, I915_MM_SHRINKER);
> 
> Worth using killable and refactoring the caller to handle the failure? 
> If this path ends up deadlocking one day it would be slightly better 
> that the process can be removed.

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

[PATCH 3/3] drm/i915/userptr: Probe vma range before gup

[Chris Wilson]

We want to exclude any GGTT objects from being present on our internal
lists to avoid the deadlock we may run into with our requirement for
struct_mutex during invalidate. However, if the gup_fast fails, we put
the userptr onto the workqueue and mark it as active, so that we
remember to serialise the worker upon mmu_invalidate.

Note that despite the previous fix, it is still better to avoid the
struct_mutex recursion where possible, leaving the recursion only to
handle the shrinker-esque paths.

v2: Hold mmap_sem to prevent modifications to the mm while we probe and
add ourselves to the interval-tree for notificiation.
v3: Rely on mmap_sem for a simpler patch.
v4: Mark up the mmap_sem nesting
v5: Don't deactivate on -EAGAIN as that means the worker is queued
v6: Fight the indentation and chained if-else error handling
v7: Fight again.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=104209
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Michał Winiarski <michal.winiarski@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_userptr.c | 150 ++++++++++++++++--------
 1 file changed, 98 insertions(+), 52 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_userptr.c b/drivers/gpu/drm/i915/i915_gem_userptr.c
index 9c2008a480e2..7b9e14bc3bc5 100644
--- a/drivers/gpu/drm/i915/i915_gem_userptr.c
+++ b/drivers/gpu/drm/i915/i915_gem_userptr.c
@@ -435,7 +435,7 @@ struct get_pages_work {
 	struct task_struct *task;
 };
 
-static struct sg_table *
+static int
 __i915_gem_userptr_alloc_pages(struct drm_i915_gem_object *obj,
 			       struct page **pvec, int num_pages)
 {
@@ -446,7 +446,7 @@ __i915_gem_userptr_alloc_pages(struct drm_i915_gem_object *obj,
 
 	st = kmalloc(sizeof(*st), GFP_KERNEL);
 	if (!st)
-		return ERR_PTR(-ENOMEM);
+		return -ENOMEM;
 
 alloc_table:
 	ret = __sg_alloc_table_from_pages(st, pvec, num_pages,
@@ -455,7 +455,7 @@ __i915_gem_userptr_alloc_pages(struct drm_i915_gem_object *obj,
 					  GFP_KERNEL);
 	if (ret) {
 		kfree(st);
-		return ERR_PTR(ret);
+		return ret;
 	}
 
 	ret = i915_gem_gtt_prepare_pages(obj, st);
@@ -468,14 +468,14 @@ __i915_gem_userptr_alloc_pages(struct drm_i915_gem_object *obj,
 		}
 
 		kfree(st);
-		return ERR_PTR(ret);
+		return ret;
 	}
 
 	sg_page_sizes = i915_sg_page_sizes(st->sgl);
 
 	__i915_gem_object_set_pages(obj, st, sg_page_sizes);
 
-	return st;
+	return 0;
 }
 
 static void
@@ -520,19 +520,14 @@ __i915_gem_userptr_get_pages_worker(struct work_struct *_work)
 
 	mutex_lock(&obj->mm.lock);
 	if (obj->userptr.work == &work->work) {
-		struct sg_table *pages = ERR_PTR(ret);
-
 		if (pinned == npages) {
-			pages = __i915_gem_userptr_alloc_pages(obj, pvec,
-							       npages);
-			if (!IS_ERR(pages)) {
+			ret = __i915_gem_userptr_alloc_pages(obj, pvec, npages);
+			if (!ret)
 				pinned = 0;
-				pages = NULL;
-			}
 		}
 
-		obj->userptr.work = ERR_CAST(pages);
-		if (IS_ERR(pages))
+		obj->userptr.work = ERR_PTR(ret);
+		if (ret)
 			__i915_gem_userptr_set_active(obj, false);
 	}
 	mutex_unlock(&obj->mm.lock);
@@ -545,7 +540,7 @@ __i915_gem_userptr_get_pages_worker(struct work_struct *_work)
 	kfree(work);
 }
 
-static struct sg_table *
+static int
 __i915_gem_userptr_get_pages_schedule(struct drm_i915_gem_object *obj)
 {
 	struct get_pages_work *work;
@@ -571,7 +566,7 @@ __i915_gem_userptr_get_pages_schedule(struct drm_i915_gem_object *obj)
 	 */
 	work = kmalloc(sizeof(*work), GFP_KERNEL);
 	if (work == NULL)
-		return ERR_PTR(-ENOMEM);
+		return -ENOMEM;
 
 	obj->userptr.work = &work->work;
 
@@ -583,19 +578,86 @@ __i915_gem_userptr_get_pages_schedule(struct drm_i915_gem_object *obj)
 	INIT_WORK(&work->work, __i915_gem_userptr_get_pages_worker);
 	queue_work(to_i915(obj->base.dev)->mm.userptr_wq, &work->work);
 
-	return ERR_PTR(-EAGAIN);
+	return -EAGAIN;
 }
 
-static int i915_gem_userptr_get_pages(struct drm_i915_gem_object *obj)
+static int
+probe_range(struct mm_struct *mm, unsigned long addr, unsigned long len)
+{
+	const unsigned long end = addr + len;
+	struct vm_area_struct *vma;
+	int ret = -EFAULT;
+
+	for (vma = find_vma(mm, addr); vma; vma = vma->vm_next) {
+		if (vma->vm_start > addr)
+			break;
+
+		/*
+		 * Exclude any VMA that is not backed only by struct_page, i.e.
+		 * IO regions that include our own GGTT mmaps. We cannot handle
+		 * such ranges, as we may encounter deadlocks around our
+		 * struct_mutex on mmu_invalidate_range.
+		 */
+		if (vma->vm_flags & (VM_PFNMAP | VM_MIXEDMAP))
+			break;
+
+		if (vma->vm_end >= end) {
+			ret = 0;
+			break;
+		}
+
+		addr = vma->vm_end;
+	}
+
+	return ret;
+}
+
+static int try_fast_gup(struct drm_i915_gem_object *obj)
 {
 	const int num_pages = obj->base.size >> PAGE_SHIFT;
-	struct mm_struct *mm = obj->userptr.mm->mm;
 	struct page **pvec;
-	struct sg_table *pages;
-	bool active;
-	int pinned;
+	int pinned, err;
+
+	pvec = kvmalloc_array(num_pages, sizeof(struct page *),
+			      GFP_KERNEL | __GFP_NORETRY | __GFP_NOWARN);
+	if (!pvec) /* defer to worker if malloc fails */
+		return -ENOMEM;
+
+	pinned = __get_user_pages_fast(obj->userptr.ptr,
+				       num_pages,
+				       !i915_gem_object_is_readonly(obj),
+				       pvec);
+	if (pinned < 0) {
+		err = pinned;
+		pinned = 0;
+		goto out_pvec;
+	}
 
-	/* If userspace should engineer that these pages are replaced in
+	if (pinned < num_pages) {
+		err = -EFAULT;
+		goto out_pinned;
+	}
+
+	err = __i915_gem_userptr_alloc_pages(obj, pvec, num_pages);
+	if (err)
+		goto out_pinned;
+
+	__i915_gem_userptr_set_active(obj, true);
+	pinned = 0;
+out_pinned:
+	release_pages(pvec, pinned);
+out_pvec:
+	kvfree(pvec);
+	return err;
+}
+
+static int i915_gem_userptr_get_pages(struct drm_i915_gem_object *obj)
+{
+	struct mm_struct *mm = obj->userptr.mm->mm;
+	int err;
+
+	/*
+	 * If userspace should engineer that these pages are replaced in
 	 * the vma between us binding this page into the GTT and completion
 	 * of rendering... Their loss. If they change the mapping of their
 	 * pages they need to create a new bo to point to the new vma.
@@ -620,40 +682,24 @@ static int i915_gem_userptr_get_pages(struct drm_i915_gem_object *obj)
 			return -EAGAIN;
 	}
 
-	pvec = NULL;
-	pinned = 0;
+	if (mm == current->mm && try_fast_gup(obj) == 0)
+		return 0;
 
-	if (mm == current->mm) {
-		pvec = kvmalloc_array(num_pages, sizeof(struct page *),
-				      GFP_KERNEL |
-				      __GFP_NORETRY |
-				      __GFP_NOWARN);
-		if (pvec) /* defer to worker if malloc fails */
-			pinned = __get_user_pages_fast(obj->userptr.ptr,
-						       num_pages,
-						       !i915_gem_object_is_readonly(obj),
-						       pvec);
-	}
+	/* lockdep doesn't yet automatically allow nesting of readers */
+	down_read_nested(&mm->mmap_sem, SINGLE_DEPTH_NESTING);
 
-	active = false;
-	if (pinned < 0) {
-		pages = ERR_PTR(pinned);
-		pinned = 0;
-	} else if (pinned < num_pages) {
-		pages = __i915_gem_userptr_get_pages_schedule(obj);
-		active = pages == ERR_PTR(-EAGAIN);
-	} else {
-		pages = __i915_gem_userptr_alloc_pages(obj, pvec, num_pages);
-		active = !IS_ERR(pages);
-	}
-	if (active)
+	err = probe_range(mm, obj->userptr.ptr, obj->base.size);
+	if (err)
+		goto err_unlock;
+
+	err = __i915_gem_userptr_get_pages_schedule(obj);
+	if (err == -EAGAIN)
 		__i915_gem_userptr_set_active(obj, true);
 
-	if (IS_ERR(pages))
-		release_pages(pvec, pinned);
-	kvfree(pvec);
+err_unlock:
+	up_read(&mm->mmap_sem);
 
-	return PTR_ERR_OR_ZERO(pages);
+	return err;
 }
 
 static void
-- 
2.20.1

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

Re: [PATCH 3/3] drm/i915/userptr: Probe vma range before gup

[Tvrtko Ursulin]

On 14/01/2019 21:17, Chris Wilson wrote:
> We want to exclude any GGTT objects from being present on our internal
> lists to avoid the deadlock we may run into with our requirement for
> struct_mutex during invalidate. However, if the gup_fast fails, we put
> the userptr onto the workqueue and mark it as active, so that we
> remember to serialise the worker upon mmu_invalidate.
> 
> Note that despite the previous fix, it is still better to avoid the
> struct_mutex recursion where possible, leaving the recursion only to
> handle the shrinker-esque paths.
> 
> v2: Hold mmap_sem to prevent modifications to the mm while we probe and
> add ourselves to the interval-tree for notificiation.
> v3: Rely on mmap_sem for a simpler patch.
> v4: Mark up the mmap_sem nesting
> v5: Don't deactivate on -EAGAIN as that means the worker is queued
> v6: Fight the indentation and chained if-else error handling
> v7: Fight again.
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=104209
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Michał Winiarski <michal.winiarski@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_gem_userptr.c | 150 ++++++++++++++++--------
>   1 file changed, 98 insertions(+), 52 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_userptr.c b/drivers/gpu/drm/i915/i915_gem_userptr.c
> index 9c2008a480e2..7b9e14bc3bc5 100644
> --- a/drivers/gpu/drm/i915/i915_gem_userptr.c
> +++ b/drivers/gpu/drm/i915/i915_gem_userptr.c
> @@ -435,7 +435,7 @@ struct get_pages_work {
>   	struct task_struct *task;
>   };
>   
> -static struct sg_table *
> +static int
>   __i915_gem_userptr_alloc_pages(struct drm_i915_gem_object *obj,
>   			       struct page **pvec, int num_pages)
>   {
> @@ -446,7 +446,7 @@ __i915_gem_userptr_alloc_pages(struct drm_i915_gem_object *obj,
>   
>   	st = kmalloc(sizeof(*st), GFP_KERNEL);
>   	if (!st)
> -		return ERR_PTR(-ENOMEM);
> +		return -ENOMEM;
>   
>   alloc_table:
>   	ret = __sg_alloc_table_from_pages(st, pvec, num_pages,
> @@ -455,7 +455,7 @@ __i915_gem_userptr_alloc_pages(struct drm_i915_gem_object *obj,
>   					  GFP_KERNEL);
>   	if (ret) {
>   		kfree(st);
> -		return ERR_PTR(ret);
> +		return ret;
>   	}
>   
>   	ret = i915_gem_gtt_prepare_pages(obj, st);
> @@ -468,14 +468,14 @@ __i915_gem_userptr_alloc_pages(struct drm_i915_gem_object *obj,
>   		}
>   
>   		kfree(st);
> -		return ERR_PTR(ret);
> +		return ret;
>   	}
>   
>   	sg_page_sizes = i915_sg_page_sizes(st->sgl);
>   
>   	__i915_gem_object_set_pages(obj, st, sg_page_sizes);
>   
> -	return st;
> +	return 0;
>   }
>   
>   static void
> @@ -520,19 +520,14 @@ __i915_gem_userptr_get_pages_worker(struct work_struct *_work)
>   
>   	mutex_lock(&obj->mm.lock);
>   	if (obj->userptr.work == &work->work) {
> -		struct sg_table *pages = ERR_PTR(ret);
> -
>   		if (pinned == npages) {
> -			pages = __i915_gem_userptr_alloc_pages(obj, pvec,
> -							       npages);
> -			if (!IS_ERR(pages)) {
> +			ret = __i915_gem_userptr_alloc_pages(obj, pvec, npages);
> +			if (!ret)
>   				pinned = 0;
> -				pages = NULL;
> -			}
>   		}
>   
> -		obj->userptr.work = ERR_CAST(pages);
> -		if (IS_ERR(pages))
> +		obj->userptr.work = ERR_PTR(ret);
> +		if (ret)
>   			__i915_gem_userptr_set_active(obj, false);
>   	}
>   	mutex_unlock(&obj->mm.lock);
> @@ -545,7 +540,7 @@ __i915_gem_userptr_get_pages_worker(struct work_struct *_work)
>   	kfree(work);
>   }
>   
> -static struct sg_table *
> +static int
>   __i915_gem_userptr_get_pages_schedule(struct drm_i915_gem_object *obj)
>   {
>   	struct get_pages_work *work;
> @@ -571,7 +566,7 @@ __i915_gem_userptr_get_pages_schedule(struct drm_i915_gem_object *obj)
>   	 */
>   	work = kmalloc(sizeof(*work), GFP_KERNEL);
>   	if (work == NULL)
> -		return ERR_PTR(-ENOMEM);
> +		return -ENOMEM;
>   
>   	obj->userptr.work = &work->work;
>   
> @@ -583,19 +578,86 @@ __i915_gem_userptr_get_pages_schedule(struct drm_i915_gem_object *obj)
>   	INIT_WORK(&work->work, __i915_gem_userptr_get_pages_worker);
>   	queue_work(to_i915(obj->base.dev)->mm.userptr_wq, &work->work);
>   
> -	return ERR_PTR(-EAGAIN);
> +	return -EAGAIN;
>   }
>   
> -static int i915_gem_userptr_get_pages(struct drm_i915_gem_object *obj)
> +static int
> +probe_range(struct mm_struct *mm, unsigned long addr, unsigned long len)
> +{
> +	const unsigned long end = addr + len;
> +	struct vm_area_struct *vma;
> +	int ret = -EFAULT;
> +
> +	for (vma = find_vma(mm, addr); vma; vma = vma->vm_next) {
> +		if (vma->vm_start > addr)
> +			break;
> +
> +		/*
> +		 * Exclude any VMA that is not backed only by struct_page, i.e.
> +		 * IO regions that include our own GGTT mmaps. We cannot handle
> +		 * such ranges, as we may encounter deadlocks around our
> +		 * struct_mutex on mmu_invalidate_range.
> +		 */
> +		if (vma->vm_flags & (VM_PFNMAP | VM_MIXEDMAP))
> +			break;
> +
> +		if (vma->vm_end >= end) {
> +			ret = 0;
> +			break;
> +		}
> +
> +		addr = vma->vm_end;
> +	}
> +
> +	return ret;
> +}
> +
> +static int try_fast_gup(struct drm_i915_gem_object *obj)
>   {
>   	const int num_pages = obj->base.size >> PAGE_SHIFT;
> -	struct mm_struct *mm = obj->userptr.mm->mm;
>   	struct page **pvec;
> -	struct sg_table *pages;
> -	bool active;
> -	int pinned;
> +	int pinned, err;
> +
> +	pvec = kvmalloc_array(num_pages, sizeof(struct page *),
> +			      GFP_KERNEL | __GFP_NORETRY | __GFP_NOWARN);
> +	if (!pvec) /* defer to worker if malloc fails */
> +		return -ENOMEM;
> +
> +	pinned = __get_user_pages_fast(obj->userptr.ptr,
> +				       num_pages,
> +				       !i915_gem_object_is_readonly(obj),
> +				       pvec);
> +	if (pinned < 0) {
> +		err = pinned;
> +		pinned = 0;

pinned is unused on the exit branch.

> +		goto out_pvec;
> +	}
>   
> -	/* If userspace should engineer that these pages are replaced in
> +	if (pinned < num_pages) {
> +		err = -EFAULT;
> +		goto out_pinned;
> +	}
> +
> +	err = __i915_gem_userptr_alloc_pages(obj, pvec, num_pages);
> +	if (err)
> +		goto out_pinned;
> +
> +	__i915_gem_userptr_set_active(obj, true);
> +	pinned = 0;
> +out_pinned:
> +	release_pages(pvec, pinned);
> +out_pvec:
> +	kvfree(pvec);
> +	return err;
> +}
> +
> +static int i915_gem_userptr_get_pages(struct drm_i915_gem_object *obj)
> +{
> +	struct mm_struct *mm = obj->userptr.mm->mm;
> +	int err;
> +
> +	/*
> +	 * If userspace should engineer that these pages are replaced in
>   	 * the vma between us binding this page into the GTT and completion
>   	 * of rendering... Their loss. If they change the mapping of their
>   	 * pages they need to create a new bo to point to the new vma.
> @@ -620,40 +682,24 @@ static int i915_gem_userptr_get_pages(struct drm_i915_gem_object *obj)
>   			return -EAGAIN;
>   	}
>   
> -	pvec = NULL;
> -	pinned = 0;
> +	if (mm == current->mm && try_fast_gup(obj) == 0)
> +		return 0;

Is try_fast_gup guaranteed to fail on ranges probe_range would have 
rejected?

>   
> -	if (mm == current->mm) {
> -		pvec = kvmalloc_array(num_pages, sizeof(struct page *),
> -				      GFP_KERNEL |
> -				      __GFP_NORETRY |
> -				      __GFP_NOWARN);
> -		if (pvec) /* defer to worker if malloc fails */
> -			pinned = __get_user_pages_fast(obj->userptr.ptr,
> -						       num_pages,
> -						       !i915_gem_object_is_readonly(obj),
> -						       pvec);
> -	}
> +	/* lockdep doesn't yet automatically allow nesting of readers */
> +	down_read_nested(&mm->mmap_sem, SINGLE_DEPTH_NESTING);

Why is nesting needed - comment needs to explain.

>   
> -	active = false;
> -	if (pinned < 0) {
> -		pages = ERR_PTR(pinned);
> -		pinned = 0;
> -	} else if (pinned < num_pages) {
> -		pages = __i915_gem_userptr_get_pages_schedule(obj);
> -		active = pages == ERR_PTR(-EAGAIN);
> -	} else {
> -		pages = __i915_gem_userptr_alloc_pages(obj, pvec, num_pages);
> -		active = !IS_ERR(pages);
> -	}
> -	if (active)
> +	err = probe_range(mm, obj->userptr.ptr, obj->base.size);
> +	if (err)
> +		goto err_unlock;
> +
> +	err = __i915_gem_userptr_get_pages_schedule(obj);
> +	if (err == -EAGAIN)
>   		__i915_gem_userptr_set_active(obj, true);
>   
> -	if (IS_ERR(pages))
> -		release_pages(pvec, pinned);
> -	kvfree(pvec);
> +err_unlock:
> +	up_read(&mm->mmap_sem);
>   
> -	return PTR_ERR_OR_ZERO(pages);
> +	return err;
>   }
>   
>   static void
> 

Regards,

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

Re: [PATCH 3/3] drm/i915/userptr: Probe vma range before gup

[Chris Wilson]

Quoting Tvrtko Ursulin (2019-01-15 10:19:11)
> 
> On 14/01/2019 21:17, Chris Wilson wrote:
> > @@ -620,40 +682,24 @@ static int i915_gem_userptr_get_pages(struct drm_i915_gem_object *obj)
> >                       return -EAGAIN;
> >       }
> >   
> > -     pvec = NULL;
> > -     pinned = 0;
> > +     if (mm == current->mm && try_fast_gup(obj) == 0)
> > +             return 0;
> 
> Is try_fast_gup guaranteed to fail on ranges probe_range would have 
> rejected?

If a page isn't present, it will fail. For our purposes that means
fast_gup fails for GGTT mmaps, as well as other ranges.

> > -     if (mm == current->mm) {
> > -             pvec = kvmalloc_array(num_pages, sizeof(struct page *),
> > -                                   GFP_KERNEL |
> > -                                   __GFP_NORETRY |
> > -                                   __GFP_NOWARN);
> > -             if (pvec) /* defer to worker if malloc fails */
> > -                     pinned = __get_user_pages_fast(obj->userptr.ptr,
> > -                                                    num_pages,
> > -                                                    !i915_gem_object_is_readonly(obj),
> > -                                                    pvec);
> > -     }
> > +     /* lockdep doesn't yet automatically allow nesting of readers */
> > +     down_read_nested(&mm->mmap_sem, SINGLE_DEPTH_NESTING);
> 
> Why is nesting needed - comment needs to explain.

We get called with mm->mmap_sem held for reading on a fault path, and
without mm->mmap_sem held otherwise. lockdep doesn't allow nesting of
readers, hence the comment.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/3] drm/i915/userptr: Probe vma range before gup

[Tvrtko Ursulin]

On 15/01/2019 10:30, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2019-01-15 10:19:11)
>>
>> On 14/01/2019 21:17, Chris Wilson wrote:
>>> @@ -620,40 +682,24 @@ static int i915_gem_userptr_get_pages(struct drm_i915_gem_object *obj)
>>>                        return -EAGAIN;
>>>        }
>>>    
>>> -     pvec = NULL;
>>> -     pinned = 0;
>>> +     if (mm == current->mm && try_fast_gup(obj) == 0)
>>> +             return 0;
>>
>> Is try_fast_gup guaranteed to fail on ranges probe_range would have
>> rejected?
> 
> If a page isn't present, it will fail. For our purposes that means
> fast_gup fails for GGTT mmaps, as well as other ranges.

What if it has been pre-faulted?

>>> -     if (mm == current->mm) {
>>> -             pvec = kvmalloc_array(num_pages, sizeof(struct page *),
>>> -                                   GFP_KERNEL |
>>> -                                   __GFP_NORETRY |
>>> -                                   __GFP_NOWARN);
>>> -             if (pvec) /* defer to worker if malloc fails */
>>> -                     pinned = __get_user_pages_fast(obj->userptr.ptr,
>>> -                                                    num_pages,
>>> -                                                    !i915_gem_object_is_readonly(obj),
>>> -                                                    pvec);
>>> -     }
>>> +     /* lockdep doesn't yet automatically allow nesting of readers */
>>> +     down_read_nested(&mm->mmap_sem, SINGLE_DEPTH_NESTING);
>>
>> Why is nesting needed - comment needs to explain.
> 
> We get called with mm->mmap_sem held for reading on a fault path, and
> without mm->mmap_sem held otherwise. lockdep doesn't allow nesting of
> readers, hence the comment.

Something like that in the comment yes please. :)

Regards,

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

Re: [PATCH 3/3] drm/i915/userptr: Probe vma range before gup

[Chris Wilson]

Quoting Tvrtko Ursulin (2019-01-15 10:40:24)
> 
> On 15/01/2019 10:30, Chris Wilson wrote:
> > Quoting Tvrtko Ursulin (2019-01-15 10:19:11)
> >>
> >> On 14/01/2019 21:17, Chris Wilson wrote:
> >>> @@ -620,40 +682,24 @@ static int i915_gem_userptr_get_pages(struct drm_i915_gem_object *obj)
> >>>                        return -EAGAIN;
> >>>        }
> >>>    
> >>> -     pvec = NULL;
> >>> -     pinned = 0;
> >>> +     if (mm == current->mm && try_fast_gup(obj) == 0)
> >>> +             return 0;
> >>
> >> Is try_fast_gup guaranteed to fail on ranges probe_range would have
> >> rejected?
> > 
> > If a page isn't present, it will fail. For our purposes that means
> > fast_gup fails for GGTT mmaps, as well as other ranges.
> 
> What if it has been pre-faulted?

It will still not have a struct page :)
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/3] drm/i915/userptr: Probe vma range before gup

[Tvrtko Ursulin]

On 14/01/2019 21:17, Chris Wilson wrote:
> We want to exclude any GGTT objects from being present on our internal
> lists to avoid the deadlock we may run into with our requirement for
> struct_mutex during invalidate. However, if the gup_fast fails, we put
> the userptr onto the workqueue and mark it as active, so that we
> remember to serialise the worker upon mmu_invalidate.
> 
> Note that despite the previous fix, it is still better to avoid the
> struct_mutex recursion where possible, leaving the recursion only to
> handle the shrinker-esque paths.
> 
> v2: Hold mmap_sem to prevent modifications to the mm while we probe and
> add ourselves to the interval-tree for notificiation.
> v3: Rely on mmap_sem for a simpler patch.
> v4: Mark up the mmap_sem nesting
> v5: Don't deactivate on -EAGAIN as that means the worker is queued
> v6: Fight the indentation and chained if-else error handling
> v7: Fight again.
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=104209
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Michał Winiarski <michal.winiarski@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_gem_userptr.c | 150 ++++++++++++++++--------
>   1 file changed, 98 insertions(+), 52 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_userptr.c b/drivers/gpu/drm/i915/i915_gem_userptr.c
> index 9c2008a480e2..7b9e14bc3bc5 100644
> --- a/drivers/gpu/drm/i915/i915_gem_userptr.c
> +++ b/drivers/gpu/drm/i915/i915_gem_userptr.c
> @@ -435,7 +435,7 @@ struct get_pages_work {
>   	struct task_struct *task;
>   };
>   
> -static struct sg_table *
> +static int
>   __i915_gem_userptr_alloc_pages(struct drm_i915_gem_object *obj,
>   			       struct page **pvec, int num_pages)
>   {
> @@ -446,7 +446,7 @@ __i915_gem_userptr_alloc_pages(struct drm_i915_gem_object *obj,
>   
>   	st = kmalloc(sizeof(*st), GFP_KERNEL);
>   	if (!st)
> -		return ERR_PTR(-ENOMEM);
> +		return -ENOMEM;
>   
>   alloc_table:
>   	ret = __sg_alloc_table_from_pages(st, pvec, num_pages,
> @@ -455,7 +455,7 @@ __i915_gem_userptr_alloc_pages(struct drm_i915_gem_object *obj,
>   					  GFP_KERNEL);
>   	if (ret) {
>   		kfree(st);
> -		return ERR_PTR(ret);
> +		return ret;
>   	}
>   
>   	ret = i915_gem_gtt_prepare_pages(obj, st);
> @@ -468,14 +468,14 @@ __i915_gem_userptr_alloc_pages(struct drm_i915_gem_object *obj,
>   		}
>   
>   		kfree(st);
> -		return ERR_PTR(ret);
> +		return ret;
>   	}
>   
>   	sg_page_sizes = i915_sg_page_sizes(st->sgl);
>   
>   	__i915_gem_object_set_pages(obj, st, sg_page_sizes);
>   
> -	return st;
> +	return 0;
>   }
>   
>   static void
> @@ -520,19 +520,14 @@ __i915_gem_userptr_get_pages_worker(struct work_struct *_work)
>   
>   	mutex_lock(&obj->mm.lock);
>   	if (obj->userptr.work == &work->work) {
> -		struct sg_table *pages = ERR_PTR(ret);
> -
>   		if (pinned == npages) {
> -			pages = __i915_gem_userptr_alloc_pages(obj, pvec,
> -							       npages);
> -			if (!IS_ERR(pages)) {
> +			ret = __i915_gem_userptr_alloc_pages(obj, pvec, npages);
> +			if (!ret)
>   				pinned = 0;
> -				pages = NULL;
> -			}
>   		}
>   
> -		obj->userptr.work = ERR_CAST(pages);
> -		if (IS_ERR(pages))
> +		obj->userptr.work = ERR_PTR(ret);
> +		if (ret)
>   			__i915_gem_userptr_set_active(obj, false);
>   	}
>   	mutex_unlock(&obj->mm.lock);
> @@ -545,7 +540,7 @@ __i915_gem_userptr_get_pages_worker(struct work_struct *_work)
>   	kfree(work);
>   }
>   
> -static struct sg_table *
> +static int
>   __i915_gem_userptr_get_pages_schedule(struct drm_i915_gem_object *obj)
>   {
>   	struct get_pages_work *work;
> @@ -571,7 +566,7 @@ __i915_gem_userptr_get_pages_schedule(struct drm_i915_gem_object *obj)
>   	 */
>   	work = kmalloc(sizeof(*work), GFP_KERNEL);
>   	if (work == NULL)
> -		return ERR_PTR(-ENOMEM);
> +		return -ENOMEM;
>   
>   	obj->userptr.work = &work->work;
>   
> @@ -583,19 +578,86 @@ __i915_gem_userptr_get_pages_schedule(struct drm_i915_gem_object *obj)
>   	INIT_WORK(&work->work, __i915_gem_userptr_get_pages_worker);
>   	queue_work(to_i915(obj->base.dev)->mm.userptr_wq, &work->work);
>   
> -	return ERR_PTR(-EAGAIN);
> +	return -EAGAIN;
>   }
>   
> -static int i915_gem_userptr_get_pages(struct drm_i915_gem_object *obj)
> +static int
> +probe_range(struct mm_struct *mm, unsigned long addr, unsigned long len)
> +{
> +	const unsigned long end = addr + len;
> +	struct vm_area_struct *vma;
> +	int ret = -EFAULT;
> +
> +	for (vma = find_vma(mm, addr); vma; vma = vma->vm_next) {
> +		if (vma->vm_start > addr)
> +			break;
> +
> +		/*
> +		 * Exclude any VMA that is not backed only by struct_page, i.e.
> +		 * IO regions that include our own GGTT mmaps. We cannot handle
> +		 * such ranges, as we may encounter deadlocks around our
> +		 * struct_mutex on mmu_invalidate_range.
> +		 */
> +		if (vma->vm_flags & (VM_PFNMAP | VM_MIXEDMAP))
> +			break;
> +
> +		if (vma->vm_end >= end) {
> +			ret = 0;
> +			break;
> +		}
> +
> +		addr = vma->vm_end;
> +	}
> +
> +	return ret;
> +}
> +
> +static int try_fast_gup(struct drm_i915_gem_object *obj)
>   {
>   	const int num_pages = obj->base.size >> PAGE_SHIFT;
> -	struct mm_struct *mm = obj->userptr.mm->mm;
>   	struct page **pvec;
> -	struct sg_table *pages;
> -	bool active;
> -	int pinned;
> +	int pinned, err;
> +
> +	pvec = kvmalloc_array(num_pages, sizeof(struct page *),
> +			      GFP_KERNEL | __GFP_NORETRY | __GFP_NOWARN);
> +	if (!pvec) /* defer to worker if malloc fails */
> +		return -ENOMEM;
> +
> +	pinned = __get_user_pages_fast(obj->userptr.ptr,
> +				       num_pages,
> +				       !i915_gem_object_is_readonly(obj),
> +				       pvec);
> +	if (pinned < 0) {
> +		err = pinned;
> +		pinned = 0;
> +		goto out_pvec;
> +	}
>   
> -	/* If userspace should engineer that these pages are replaced in
> +	if (pinned < num_pages) {
> +		err = -EFAULT;
> +		goto out_pinned;
> +	}
> +
> +	err = __i915_gem_userptr_alloc_pages(obj, pvec, num_pages);
> +	if (err)
> +		goto out_pinned;
> +
> +	__i915_gem_userptr_set_active(obj, true);
> +	pinned = 0;
> +out_pinned:
> +	release_pages(pvec, pinned);
> +out_pvec:
> +	kvfree(pvec);
> +	return err;
> +}
> +
> +static int i915_gem_userptr_get_pages(struct drm_i915_gem_object *obj)
> +{
> +	struct mm_struct *mm = obj->userptr.mm->mm;
> +	int err;
> +
> +	/*
> +	 * If userspace should engineer that these pages are replaced in
>   	 * the vma between us binding this page into the GTT and completion
>   	 * of rendering... Their loss. If they change the mapping of their
>   	 * pages they need to create a new bo to point to the new vma.
> @@ -620,40 +682,24 @@ static int i915_gem_userptr_get_pages(struct drm_i915_gem_object *obj)
>   			return -EAGAIN;
>   	}
>   
> -	pvec = NULL;
> -	pinned = 0;
> +	if (mm == current->mm && try_fast_gup(obj) == 0)
> +		return 0;
>   
> -	if (mm == current->mm) {
> -		pvec = kvmalloc_array(num_pages, sizeof(struct page *),
> -				      GFP_KERNEL |
> -				      __GFP_NORETRY |
> -				      __GFP_NOWARN);
> -		if (pvec) /* defer to worker if malloc fails */
> -			pinned = __get_user_pages_fast(obj->userptr.ptr,
> -						       num_pages,
> -						       !i915_gem_object_is_readonly(obj),
> -						       pvec);
> -	}
> +	/* lockdep doesn't yet automatically allow nesting of readers */
> +	down_read_nested(&mm->mmap_sem, SINGLE_DEPTH_NESTING);
>   
> -	active = false;
> -	if (pinned < 0) {
> -		pages = ERR_PTR(pinned);
> -		pinned = 0;
> -	} else if (pinned < num_pages) {
> -		pages = __i915_gem_userptr_get_pages_schedule(obj);
> -		active = pages == ERR_PTR(-EAGAIN);
> -	} else {
> -		pages = __i915_gem_userptr_alloc_pages(obj, pvec, num_pages);
> -		active = !IS_ERR(pages);
> -	}
> -	if (active)
> +	err = probe_range(mm, obj->userptr.ptr, obj->base.size);

Stick a comment here about how the probe is not a guarantee the mapping 
won't change by the time gup worker runs.

> +	if (err)
> +		goto err_unlock;
> +
> +	err = __i915_gem_userptr_get_pages_schedule(obj);
> +	if (err == -EAGAIN)
>   		__i915_gem_userptr_set_active(obj, true);
>   
> -	if (IS_ERR(pages))
> -		release_pages(pvec, pinned);
> -	kvfree(pvec);
> +err_unlock:
> +	up_read(&mm->mmap_sem);

May be safer to drop the lock earlier, immediately after probe. I don't 
see holding it while queuing the worker and doing internal book-keeping 
is useful and might just create more lock chain dependencies.

Regards,

Tvrtko

>   
> -	return PTR_ERR_OR_ZERO(pages);
> +	return err;
>   }
>   
>   static void
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/3] drm/i915/userptr: Probe vma range before gup

[Chris Wilson]

Quoting Tvrtko Ursulin (2019-01-15 10:27:02)
> 
> On 14/01/2019 21:17, Chris Wilson wrote:
> > +     if (err)
> > +             goto err_unlock;
> > +
> > +     err = __i915_gem_userptr_get_pages_schedule(obj);
> > +     if (err == -EAGAIN)
> >               __i915_gem_userptr_set_active(obj, true);
> >   
> > -     if (IS_ERR(pages))
> > -             release_pages(pvec, pinned);
> > -     kvfree(pvec);
> > +err_unlock:
> > +     up_read(&mm->mmap_sem);
> 
> May be safer to drop the lock earlier, immediately after probe. I don't 
> see holding it while queuing the worker and doing internal book-keeping 
> is useful and might just create more lock chain dependencies.

Hmm, I thought we need to cover up to set-active (probe + queue + insert
into rbtree) as I thought the mmu-invalidate was under the mmap_sem wlock.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/3] drm/i915/userptr: Probe vma range before gup

[Tvrtko Ursulin]

On 15/01/2019 10:41, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2019-01-15 10:27:02)
>>
>> On 14/01/2019 21:17, Chris Wilson wrote:
>>> +     if (err)
>>> +             goto err_unlock;
>>> +
>>> +     err = __i915_gem_userptr_get_pages_schedule(obj);
>>> +     if (err == -EAGAIN)
>>>                __i915_gem_userptr_set_active(obj, true);
>>>    
>>> -     if (IS_ERR(pages))
>>> -             release_pages(pvec, pinned);
>>> -     kvfree(pvec);
>>> +err_unlock:
>>> +     up_read(&mm->mmap_sem);
>>
>> May be safer to drop the lock earlier, immediately after probe. I don't
>> see holding it while queuing the worker and doing internal book-keeping
>> is useful and might just create more lock chain dependencies.
> 
> Hmm, I thought we need to cover up to set-active (probe + queue + insert
> into rbtree) as I thought the mmu-invalidate was under the mmap_sem wlock.

We have our own lock for set active so I don't see that we need mmap_sem 
for it. Certainly wasn't needed before this patch so don't see that 
would change now.

Btw, what you said regarding nested mmap_sem.. do we have lock inversion 
with it and obj->mm.lock then? I mean both mmap_sem -> obj->mm.lock and 
obj->mm.lock -> mmap_sem chains?

Regards,

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

Re: [PATCH 3/3] drm/i915/userptr: Probe vma range before gup

[Chris Wilson]

Quoting Tvrtko Ursulin (2019-01-15 10:52:52)
> 
> On 15/01/2019 10:41, Chris Wilson wrote:
> > Quoting Tvrtko Ursulin (2019-01-15 10:27:02)
> >>
> >> On 14/01/2019 21:17, Chris Wilson wrote:
> >>> +     if (err)
> >>> +             goto err_unlock;
> >>> +
> >>> +     err = __i915_gem_userptr_get_pages_schedule(obj);
> >>> +     if (err == -EAGAIN)
> >>>                __i915_gem_userptr_set_active(obj, true);
> >>>    
> >>> -     if (IS_ERR(pages))
> >>> -             release_pages(pvec, pinned);
> >>> -     kvfree(pvec);
> >>> +err_unlock:
> >>> +     up_read(&mm->mmap_sem);
> >>
> >> May be safer to drop the lock earlier, immediately after probe. I don't
> >> see holding it while queuing the worker and doing internal book-keeping
> >> is useful and might just create more lock chain dependencies.
> > 
> > Hmm, I thought we need to cover up to set-active (probe + queue + insert
> > into rbtree) as I thought the mmu-invalidate was under the mmap_sem wlock.
> 
> We have our own lock for set active so I don't see that we need mmap_sem 
> for it. Certainly wasn't needed before this patch so don't see that 
> would change now.

We had it before this patch, by ensuring that we added the rbtree
tracking before we queued the obj for gup, and then serialised inside
the mmu-invalidate.
 
> Btw, what you said regarding nested mmap_sem.. do we have lock inversion 
> with it and obj->mm.lock then? I mean both mmap_sem -> obj->mm.lock and 
> obj->mm.lock -> mmap_sem chains?

Hmm. Yes, there is a recursive read lock here. You want proof that it is
just limited to this single instance!
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.CHECKPATCH: warning for series starting with [1/3] drm/i915: Prevent concurrent GGTT update and use on Braswell (again)

[Patchwork]

== Series Details ==

Series: series starting with [1/3] drm/i915: Prevent concurrent GGTT update and use on Braswell (again)
URL   : https://patchwork.freedesktop.org/series/55201/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
071bbd0707df drm/i915: Prevent concurrent GGTT update and use on Braswell (again)
-:14: ERROR:GIT_COMMIT_ID: Please use git commit description style 'commit <12+ chars of sha1> ("<title line>")' - ie: 'commit 5bab6f60cb4d ("drm/i915: Serialise updates to GGTT with access through GGTT on Braswell")'
#14: 
commit 5bab6f60cb4d1417ad7c599166bcfec87529c1a2

-:22: ERROR:GIT_COMMIT_ID: Please use git commit description style 'commit <12+ chars of sha1> ("<title line>")' - ie: 'commit 4509276ee824 ("drm/i915: Remove Braswell GGTT update w/a")'
#22: 
commit 4509276ee824bb967885c095c610767e42345c36

-:39: WARNING:COMMIT_LOG_LONG_LINE: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#39: 
References: 5bab6f60cb4d ("drm/i915: Serialise updates to GGTT with access through GGTT on Braswell")

-:39: ERROR:GIT_COMMIT_ID: Please use git commit description style 'commit <12+ chars of sha1> ("<title line>")' - ie: 'commit 5bab6f60cb4d ("drm/i915: Serialise updates to GGTT with access through GGTT on Braswell")'
#39: 
References: 5bab6f60cb4d ("drm/i915: Serialise updates to GGTT with access through GGTT on Braswell")

-:40: ERROR:GIT_COMMIT_ID: Please use git commit description style 'commit <12+ chars of sha1> ("<title line>")' - ie: 'commit 4509276ee824 ("drm/i915: Remove Braswell GGTT update w/a")'
#40: 
References: 4509276ee824 ("drm/i915: Remove Braswell GGTT update w/a")

total: 4 errors, 1 warnings, 0 checks, 9 lines checked
b69127d2d857 drm/i915/userptr: Avoid struct_mutex recursion for mmu_invalidate_range_start
-:23: WARNING:COMMIT_LOG_LONG_LINE: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#23: 
References: 93065ac753e4 ("mm, oom: distinguish blockable mode for mmu notifiers")

-:23: 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")'
#23: 
References: 93065ac753e4 ("mm, oom: distinguish blockable mode for mmu notifiers")

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

total: 2 errors, 1 warnings, 0 checks, 442 lines checked
14241cdbf4bd drm/i915/userptr: Probe vma range before gup

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

✓ Fi.CI.BAT: success for series starting with [1/3] drm/i915: Prevent concurrent GGTT update and use on Braswell (again)

[Patchwork]

== Series Details ==

Series: series starting with [1/3] drm/i915: Prevent concurrent GGTT update and use on Braswell (again)
URL   : https://patchwork.freedesktop.org/series/55201/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_5418 -> Patchwork_11293
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/55201/revisions/1/mbox/

Known issues
------------

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

### IGT changes ###

#### Issues hit ####

  * igt@debugfs_test@read_all_entries:
    - fi-ilk-650:         PASS -> DMESG-WARN [fdo#106387]

  * igt@gem_exec_suspend@basic-s4-devices:
    - fi-blb-e6850:       PASS -> INCOMPLETE [fdo#107718]

  * igt@kms_frontbuffer_tracking@basic:
    - fi-hsw-peppy:       NOTRUN -> DMESG-WARN [fdo#102614]

  * igt@pm_rpm@basic-rte:
    - fi-bsw-kefka:       PASS -> FAIL [fdo#108800]

  
#### Possible fixes ####

  * igt@debugfs_test@read_all_entries:
    - fi-kbl-7567u:       DMESG-WARN [fdo#103558] / [fdo#105602] -> PASS

  * igt@i915_selftest@live_hangcheck:
    - fi-skl-iommu:       INCOMPLETE [fdo#108602] / [fdo#108744] -> PASS

  * igt@kms_chamelium@hdmi-hpd-fast:
    - fi-kbl-7500u:       FAIL [fdo#108767] -> PASS

  * igt@prime_vgem@basic-fence-flip:
    - fi-ilk-650:         FAIL [fdo#104008] -> PASS

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#102614]: https://bugs.freedesktop.org/show_bug.cgi?id=102614
  [fdo#103558]: https://bugs.freedesktop.org/show_bug.cgi?id=103558
  [fdo#104008]: https://bugs.freedesktop.org/show_bug.cgi?id=104008
  [fdo#105602]: https://bugs.freedesktop.org/show_bug.cgi?id=105602
  [fdo#106387]: https://bugs.freedesktop.org/show_bug.cgi?id=106387
  [fdo#107718]: https://bugs.freedesktop.org/show_bug.cgi?id=107718
  [fdo#108602]: https://bugs.freedesktop.org/show_bug.cgi?id=108602
  [fdo#108744]: https://bugs.freedesktop.org/show_bug.cgi?id=108744
  [fdo#108767]: https://bugs.freedesktop.org/show_bug.cgi?id=108767
  [fdo#108800]: https://bugs.freedesktop.org/show_bug.cgi?id=108800
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109278]: https://bugs.freedesktop.org/show_bug.cgi?id=109278


Participating hosts (41 -> 40)
------------------------------

  Additional (4): fi-skl-6700k2 fi-hsw-peppy fi-apl-guc fi-pnv-d510 
  Missing    (5): fi-ilk-m540 fi-byt-squawks fi-bsw-cyan fi-bxt-j4205 fi-bdw-samus 


Build changes
-------------

    * Linux: CI_DRM_5418 -> Patchwork_11293

  CI_DRM_5418: 845c8d109d2e0d6bb66b25fc859c5b931baf6da7 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4770: 8cb8fd13d592d9821c59eb321b1766725f9d1b37 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_11293: 14241cdbf4bd095e7c322eba7572a2038d72843e @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

14241cdbf4bd drm/i915/userptr: Probe vma range before gup
b69127d2d857 drm/i915/userptr: Avoid struct_mutex recursion for mmu_invalidate_range_start
071bbd0707df drm/i915: Prevent concurrent GGTT update and use on Braswell (again)

== Logs ==

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

✓ Fi.CI.IGT: success for series starting with [1/3] drm/i915: Prevent concurrent GGTT update and use on Braswell (again)

[Patchwork]

== Series Details ==

Series: series starting with [1/3] drm/i915: Prevent concurrent GGTT update and use on Braswell (again)
URL   : https://patchwork.freedesktop.org/series/55201/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_5418_full -> Patchwork_11293_full
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  

Known issues
------------

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

### IGT changes ###

#### Issues hit ####

  * igt@i915_suspend@shrink:
    - shard-skl:          NOTRUN -> DMESG-WARN [fdo#107886] / [fdo#109244]

  * igt@kms_busy@extended-pageflip-modeset-hang-oldfb-render-a:
    - shard-glk:          PASS -> DMESG-WARN [fdo#107956]
    - shard-apl:          NOTRUN -> DMESG-WARN [fdo#107956]

  * igt@kms_busy@extended-pageflip-modeset-hang-oldfb-render-c:
    - shard-skl:          NOTRUN -> DMESG-WARN [fdo#107956] +2

  * igt@kms_cursor_crc@cursor-128x128-onscreen:
    - shard-iclb:         NOTRUN -> FAIL [fdo#103232] +1

  * igt@kms_cursor_crc@cursor-256x85-onscreen:
    - shard-apl:          PASS -> FAIL [fdo#103232] +1

  * igt@kms_cursor_crc@cursor-64x64-sliding:
    - shard-glk:          PASS -> FAIL [fdo#103232] +1

  * igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-draw-pwrite:
    - shard-apl:          PASS -> FAIL [fdo#103167] +1

  * igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-draw-render:
    - shard-glk:          PASS -> FAIL [fdo#103167]

  * igt@kms_frontbuffer_tracking@psr-1p-primscrn-indfb-msflip-blt:
    - shard-skl:          PASS -> FAIL [fdo#103167] +1

  * igt@kms_hdmi_inject@inject-audio:
    - shard-iclb:         NOTRUN -> FAIL [fdo#102370]

  * igt@kms_plane@pixel-format-pipe-a-planes-source-clamping:
    - shard-apl:          PASS -> FAIL [fdo#108948]

  * igt@kms_plane_alpha_blend@pipe-a-coverage-7efc:
    - shard-skl:          PASS -> FAIL [fdo#107815] / [fdo#108145]

  * igt@kms_plane_alpha_blend@pipe-c-alpha-opaque-fb:
    - shard-glk:          PASS -> FAIL [fdo#108145]

  * igt@kms_plane_alpha_blend@pipe-c-alpha-transparant-fb:
    - shard-skl:          NOTRUN -> FAIL [fdo#108145]

  * igt@kms_plane_multiple@atomic-pipe-a-tiling-x:
    - shard-apl:          PASS -> FAIL [fdo#103166] +2

  * igt@kms_plane_multiple@atomic-pipe-a-tiling-y:
    - shard-glk:          PASS -> FAIL [fdo#103166]

  * igt@pm_rpm@gem-mmap-cpu:
    - shard-skl:          PASS -> INCOMPLETE [fdo#107807]

  
#### Possible fixes ####

  * igt@kms_chv_cursor_fail@pipe-a-256x256-right-edge:
    - shard-skl:          FAIL [fdo#104671] -> PASS +1

  * igt@kms_cursor_crc@cursor-256x85-onscreen:
    - shard-glk:          FAIL [fdo#103232] -> PASS +2

  * igt@kms_draw_crc@draw-method-xrgb2101010-blt-untiled:
    - shard-skl:          FAIL [fdo#103184] / [fdo#108472] -> PASS

  * igt@kms_flip@flip-vs-absolute-wf_vblank-interruptible:
    - shard-apl:          INCOMPLETE [fdo#103927] -> PASS

  * igt@kms_frontbuffer_tracking@fbc-1p-primscrn-cur-indfb-draw-mmap-gtt:
    - shard-apl:          FAIL [fdo#103167] -> PASS

  * igt@kms_frontbuffer_tracking@fbc-2p-primscrn-spr-indfb-onoff:
    - shard-glk:          FAIL [fdo#103167] -> PASS +4

  * igt@kms_frontbuffer_tracking@fbcpsr-suspend:
    - shard-skl:          FAIL [fdo#105682] -> PASS +1

  * igt@kms_frontbuffer_tracking@psr-rgb565-draw-mmap-wc:
    - shard-skl:          FAIL [fdo#103167] -> PASS

  * igt@kms_plane_alpha_blend@pipe-c-constant-alpha-max:
    - shard-glk:          FAIL [fdo#108145] -> PASS

  * igt@kms_plane_multiple@atomic-pipe-a-tiling-y:
    - shard-apl:          FAIL [fdo#103166] -> PASS +1

  * igt@kms_setmode@basic:
    - shard-apl:          FAIL [fdo#99912] -> PASS

  * igt@pm_rc6_residency@rc6-accuracy:
    - shard-snb:          {SKIP} [fdo#109271] -> PASS

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#102370]: https://bugs.freedesktop.org/show_bug.cgi?id=102370
  [fdo#103166]: https://bugs.freedesktop.org/show_bug.cgi?id=103166
  [fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
  [fdo#103184]: https://bugs.freedesktop.org/show_bug.cgi?id=103184
  [fdo#103232]: https://bugs.freedesktop.org/show_bug.cgi?id=103232
  [fdo#103927]: https://bugs.freedesktop.org/show_bug.cgi?id=103927
  [fdo#104671]: https://bugs.freedesktop.org/show_bug.cgi?id=104671
  [fdo#105682]: https://bugs.freedesktop.org/show_bug.cgi?id=105682
  [fdo#107807]: https://bugs.freedesktop.org/show_bug.cgi?id=107807
  [fdo#107815]: https://bugs.freedesktop.org/show_bug.cgi?id=107815
  [fdo#107886]: https://bugs.freedesktop.org/show_bug.cgi?id=107886
  [fdo#107956]: https://bugs.freedesktop.org/show_bug.cgi?id=107956
  [fdo#108145]: https://bugs.freedesktop.org/show_bug.cgi?id=108145
  [fdo#108472]: https://bugs.freedesktop.org/show_bug.cgi?id=108472
  [fdo#108948]: https://bugs.freedesktop.org/show_bug.cgi?id=108948
  [fdo#109244]: https://bugs.freedesktop.org/show_bug.cgi?id=109244
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109274]: https://bugs.freedesktop.org/show_bug.cgi?id=109274
  [fdo#109275]: https://bugs.freedesktop.org/show_bug.cgi?id=109275
  [fdo#109276]: https://bugs.freedesktop.org/show_bug.cgi?id=109276
  [fdo#109278]: https://bugs.freedesktop.org/show_bug.cgi?id=109278
  [fdo#109279]: https://bugs.freedesktop.org/show_bug.cgi?id=109279
  [fdo#109280]: https://bugs.freedesktop.org/show_bug.cgi?id=109280
  [fdo#109281]: https://bugs.freedesktop.org/show_bug.cgi?id=109281
  [fdo#109286]: https://bugs.freedesktop.org/show_bug.cgi?id=109286
  [fdo#109289]: https://bugs.freedesktop.org/show_bug.cgi?id=109289
  [fdo#109290]: https://bugs.freedesktop.org/show_bug.cgi?id=109290
  [fdo#109291]: https://bugs.freedesktop.org/show_bug.cgi?id=109291
  [fdo#109312]: https://bugs.freedesktop.org/show_bug.cgi?id=109312
  [fdo#109350]: https://bugs.freedesktop.org/show_bug.cgi?id=109350
  [fdo#99912]: https://bugs.freedesktop.org/show_bug.cgi?id=99912


Participating hosts (7 -> 7)
------------------------------

  No changes in participating hosts


Build changes
-------------

    * Linux: CI_DRM_5418 -> Patchwork_11293

  CI_DRM_5418: 845c8d109d2e0d6bb66b25fc859c5b931baf6da7 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4770: 8cb8fd13d592d9821c59eb321b1766725f9d1b37 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_11293: 14241cdbf4bd095e7c322eba7572a2038d72843e @ 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_11293/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/3] drm/i915: Prevent concurrent GGTT update and use on Braswell (again)

[Tvrtko Ursulin]

On 14/01/2019 21:17, Chris Wilson wrote:
> On Braswell, under heavy stress, if we update the GGTT while
> simultaneously accessing another region inside the GTT, we are returned
> the wrong values. To prevent this we stop the machine to update the GGTT
> entries so that no memory traffic can occur at the same time.
> 
> This was first spotted in
> 
> commit 5bab6f60cb4d1417ad7c599166bcfec87529c1a2
> Author: Chris Wilson <chris@chris-wilson.co.uk>
> Date:   Fri Oct 23 18:43:32 2015 +0100
> 
>      drm/i915: Serialise updates to GGTT with access through GGTT on Braswell
> 
> but removed again in forlorn hope with
> 
> commit 4509276ee824bb967885c095c610767e42345c36
> Author: Chris Wilson <chris@chris-wilson.co.uk>
> Date:   Mon Feb 20 12:47:18 2017 +0000
> 
>      drm/i915: Remove Braswell GGTT update w/a
> 
> However, gem_concurrent_blit is once again only stable with the patch
> applied and CI is detecting the odd failure in forked gem_mmap_gtt tests
> (which smell like the same issue). Fwiw, a wide variety of CPU memory
> barriers (around GGTT flushing, fence updates, PTE updates) and GPU
> flushes/invalidates (between requests, after PTE updates) were tried as
> part of the investigation to find an alternate cause, nothing comes
> close to serialised GGTT updates.
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105591
> Testcase: igt/gem_concurrent_blit
> Testcase: igt/gem_mmap_gtt/*forked*
> References: 5bab6f60cb4d ("drm/i915: Serialise updates to GGTT with access through GGTT on Braswell")
> References: 4509276ee824 ("drm/i915: Remove Braswell GGTT update w/a")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>   drivers/gpu/drm/i915/i915_gem_gtt.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index dbea14bf67cc..f0d46366fb0b 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -3232,7 +3232,8 @@ static int gen8_gmch_probe(struct i915_ggtt *ggtt)
>   	ggtt->vm.insert_entries = gen8_ggtt_insert_entries;
>   
>   	/* Serialize GTT updates with aperture access on BXT if VT-d is on. */
> -	if (intel_ggtt_update_needs_vtd_wa(dev_priv)) {
> +	if (intel_ggtt_update_needs_vtd_wa(dev_priv) ||
> +	    IS_CHERRYVIEW(dev_priv) /* fails with concurrent use/update */) {
>   		ggtt->vm.insert_entries = bxt_vtd_ggtt_insert_entries__BKL;
>   		ggtt->vm.insert_page    = bxt_vtd_ggtt_insert_page__BKL;
>   		if (ggtt->vm.clear_range != nop_clear_range)
> 
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

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

Re: [PATCH 1/3] drm/i915: Prevent concurrent GGTT update and use on Braswell (again)

[Chris Wilson]

Quoting Tvrtko Ursulin (2019-01-15 09:04:47)
> 
> On 14/01/2019 21:17, Chris Wilson wrote:
> > On Braswell, under heavy stress, if we update the GGTT while
> > simultaneously accessing another region inside the GTT, we are returned
> > the wrong values. To prevent this we stop the machine to update the GGTT
> > entries so that no memory traffic can occur at the same time.
> > 
> > This was first spotted in
> > 
> > commit 5bab6f60cb4d1417ad7c599166bcfec87529c1a2
> > Author: Chris Wilson <chris@chris-wilson.co.uk>
> > Date:   Fri Oct 23 18:43:32 2015 +0100
> > 
> >      drm/i915: Serialise updates to GGTT with access through GGTT on Braswell
> > 
> > but removed again in forlorn hope with
> > 
> > commit 4509276ee824bb967885c095c610767e42345c36
> > Author: Chris Wilson <chris@chris-wilson.co.uk>
> > Date:   Mon Feb 20 12:47:18 2017 +0000
> > 
> >      drm/i915: Remove Braswell GGTT update w/a
> > 
> > However, gem_concurrent_blit is once again only stable with the patch
> > applied and CI is detecting the odd failure in forked gem_mmap_gtt tests
> > (which smell like the same issue). Fwiw, a wide variety of CPU memory
> > barriers (around GGTT flushing, fence updates, PTE updates) and GPU
> > flushes/invalidates (between requests, after PTE updates) were tried as
> > part of the investigation to find an alternate cause, nothing comes
> > close to serialised GGTT updates.
> > 
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105591
> > Testcase: igt/gem_concurrent_blit
> > Testcase: igt/gem_mmap_gtt/*forked*
> > References: 5bab6f60cb4d ("drm/i915: Serialise updates to GGTT with access through GGTT on Braswell")
> > References: 4509276ee824 ("drm/i915: Remove Braswell GGTT update w/a")
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > ---
> >   drivers/gpu/drm/i915/i915_gem_gtt.c | 3 ++-
> >   1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> > index dbea14bf67cc..f0d46366fb0b 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> > @@ -3232,7 +3232,8 @@ static int gen8_gmch_probe(struct i915_ggtt *ggtt)
> >       ggtt->vm.insert_entries = gen8_ggtt_insert_entries;
> >   
> >       /* Serialize GTT updates with aperture access on BXT if VT-d is on. */
> > -     if (intel_ggtt_update_needs_vtd_wa(dev_priv)) {
> > +     if (intel_ggtt_update_needs_vtd_wa(dev_priv) ||
> > +         IS_CHERRYVIEW(dev_priv) /* fails with concurrent use/update */) {
> >               ggtt->vm.insert_entries = bxt_vtd_ggtt_insert_entries__BKL;
> >               ggtt->vm.insert_page    = bxt_vtd_ggtt_insert_page__BKL;
> >               if (ggtt->vm.clear_range != nop_clear_range)
> > 
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Thanks, pushed and closed the bugzilla. If I am allowed to be
optimistic, maybe this will be the cause of the fail-on-boot as well?
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx