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

[Chris Wilson <chris@chris-wilson.co.uk>]

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 | 158 ++++++++++++++++--------
 1 file changed, 106 insertions(+), 52 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_userptr.c b/drivers/gpu/drm/i915/i915_gem_userptr.c
index 8b07fd44731f..744aa538d5db 100644
--- a/drivers/gpu/drm/i915/i915_gem_userptr.c
+++ b/drivers/gpu/drm/i915/i915_gem_userptr.c
@@ -439,7 +439,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)
 {
@@ -450,7 +450,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,
@@ -459,7 +459,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);
@@ -472,14 +472,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
@@ -524,19 +524,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);
@@ -549,7 +544,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;
@@ -575,7 +570,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;
 
@@ -587,19 +582,89 @@ __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;
 
-	/* If userspace should engineer that these pages are replaced in
+	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 (pinned < num_pages) {
+		err = -EFAULT;
+		goto out_pinned;
+	}
+
+	__i915_gem_userptr_set_active(obj, true);
+
+	err = __i915_gem_userptr_alloc_pages(obj, pvec, num_pages);
+	if (err) {
+		__i915_gem_userptr_set_active(obj, false);
+		goto out_pinned;
+	}
+
+	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.
@@ -624,40 +689,29 @@ static int i915_gem_userptr_get_pages(struct drm_i915_gem_object *obj)
 			return -EAGAIN;
 	}
 
-	pvec = NULL;
-	pinned = 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);
+		err = try_fast_gup(obj);
+		if (!err)
+			return 0;
 	}
 
-	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)
-		__i915_gem_userptr_set_active(obj, true);
+	/* lockdep doesn't yet automatically allow nesting of readers */
+	down_read_nested(&mm->mmap_sem, SINGLE_DEPTH_NESTING);
 
-	if (IS_ERR(pages))
-		release_pages(pvec, pinned);
-	kvfree(pvec);
+	err = probe_range(mm, obj->userptr.ptr, obj->base.size);
+	if (err)
+		goto err_unlock;
+
+	__i915_gem_userptr_set_active(obj, true);
+
+	err = __i915_gem_userptr_get_pages_schedule(obj);
+	if (err != -EAGAIN)
+		__i915_gem_userptr_set_active(obj, false);
+
+err_unlock:
+	up_read(&mm->mmap_sem);
 
-	return PTR_ERR_OR_ZERO(pages);
+	return err;
 }
 
 static void
-- 
2.20.0.rc2

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