Fixing the i915 lock ordering

Fixing the i915 lock ordering

[Eric Anholt]

Here's a new patch series with a proposed fix for two of the paths
with lock ordering problems in i915.  If these look OK (in particular GTT
pwrite, which is the most important performance path), I'll work on
resolving the non-GEM issue and figuring out what we want to do about
execbuffers (either this plan, or krh's patch, or something with
get_user_pages and kmap).

[PATCH] drm/i915: Fix lock order reversal in GTT pwrite path.

[Eric Anholt]

Since the pagefault path determines that the lock order we use has to be
mmap_sem -> struct_mutex, we can't allow page faults to occur while the
struct_mutex is held.  To fix this in pwrite, we first try optimistically to
see if we can copy from user without faulting.  If it fails, fall back to
allocating memory, copying, then taking the lock and copying it to the GPU.

Thanks to Linus for suggesting this (in retrospect) obvious way of doing it.

Signed-off-by: Eric Anholt <eric@anholt.net>
---
 drivers/gpu/drm/i915/i915_gem.c |  131 +++++++++++++++++++++++++++++++--------
 1 files changed, 105 insertions(+), 26 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index c94069b..2c85249 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -223,29 +223,30 @@ fast_user_write(struct io_mapping *mapping,
  */
 
 static inline int
-slow_user_write(struct io_mapping *mapping,
-		loff_t page_base, int page_offset,
-		char __user *user_data,
-		int length)
+slow_kernel_write(struct io_mapping *mapping,
+		  loff_t page_base, int page_offset,
+		  char *data,
+		  int length)
 {
 	char __iomem *vaddr;
-	unsigned long unwritten;
 
 	vaddr = io_mapping_map_wc(mapping, page_base);
 	if (vaddr == NULL)
-		return -EFAULT;
-	unwritten = __copy_from_user(vaddr + page_offset,
-				     user_data, length);
+		return -ENOMEM;
+	memcpy(vaddr + page_offset, data, length);
 	io_mapping_unmap(vaddr);
-	if (unwritten)
-		return -EFAULT;
+
 	return 0;
 }
 
+/**
+ * This is the fast pwrite path, where we copy the data directly from the
+ * user into the GTT, uncached.
+ */
 static int
-i915_gem_gtt_pwrite(struct drm_device *dev, struct drm_gem_object *obj,
-		    struct drm_i915_gem_pwrite *args,
-		    struct drm_file *file_priv)
+i915_gem_gtt_pwrite_fast(struct drm_device *dev, struct drm_gem_object *obj,
+			 struct drm_i915_gem_pwrite *args,
+			 struct drm_file *file_priv)
 {
 	struct drm_i915_gem_object *obj_priv = obj->driver_private;
 	drm_i915_private_t *dev_priv = dev->dev_private;
@@ -273,8 +274,8 @@ i915_gem_gtt_pwrite(struct drm_device *dev, struct drm_gem_object *obj,
 
 	obj_priv = obj->driver_private;
 	offset = obj_priv->gtt_offset + args->offset;
-	obj_priv->dirty = 1;
 
+	pagefault_disable();
 	while (remain > 0) {
 		/* Operation in this page
 		 *
@@ -292,22 +293,19 @@ i915_gem_gtt_pwrite(struct drm_device *dev, struct drm_gem_object *obj,
 				       page_offset, user_data, page_length);
 
 		/* If we get a fault while copying data, then (presumably) our
-		 * source page isn't available. In this case, use the
-		 * non-atomic function
+		 * source page isn't available.  Return the error and we'll
+		 * retry in the slow path.
 		 */
-		if (ret) {
-			ret = slow_user_write (dev_priv->mm.gtt_mapping,
-					       page_base, page_offset,
-					       user_data, page_length);
-			if (ret)
-				goto fail;
-		}
+		if (ret)
+			goto fail_pagefault;
 
 		remain -= page_length;
 		user_data += page_length;
 		offset += page_length;
 	}
 
+fail_pagefault:
+	pagefault_enable();
 fail:
 	i915_gem_object_unpin(obj);
 	mutex_unlock(&dev->struct_mutex);
@@ -315,6 +313,83 @@ fail:
 	return ret;
 }
 
+/**
+ * This is the fallback GTT pwrite path, which allocates temporary storage
+ * in kernel space to copy_from_user, so we can take pagefaults outside of the
+ * struct_mutex.
+ */
+static int
+i915_gem_gtt_pwrite_slow(struct drm_device *dev, struct drm_gem_object *obj,
+			 struct drm_i915_gem_pwrite *args,
+			 struct drm_file *file_priv)
+{
+	struct drm_i915_gem_object *obj_priv = obj->driver_private;
+	drm_i915_private_t *dev_priv = dev->dev_private;
+	ssize_t remain;
+	loff_t offset, page_base;
+	char __user *user_data;
+	char *data;
+	int page_offset, page_length;
+	int ret;
+
+	user_data = (char __user *) (uintptr_t) args->data_ptr;
+	remain = args->size;
+
+	data = drm_alloc(args->size, DRM_MEM_DRIVER);
+	if (data == NULL)
+		return -ENOMEM;
+
+	ret = copy_from_user(data, user_data, args->size);
+	if (ret != 0)
+		goto fail_free;
+
+	mutex_lock(&dev->struct_mutex);
+	ret = i915_gem_object_pin(obj, 0);
+	if (ret)
+		goto fail_unlock;
+
+	ret = i915_gem_object_set_to_gtt_domain(obj, 1);
+	if (ret)
+		goto fail_unpin;
+
+	obj_priv = obj->driver_private;
+	offset = obj_priv->gtt_offset + args->offset;
+	obj_priv->dirty = 1;
+
+	while (remain > 0) {
+		/* Operation in this page
+		 *
+		 * page_base = page offset within aperture
+		 * page_offset = offset within page
+		 * page_length = bytes to copy for this page
+		 */
+		page_base = (offset & ~(PAGE_SIZE-1));
+		page_offset = offset & (PAGE_SIZE-1);
+		page_length = remain;
+		if ((page_offset + remain) > PAGE_SIZE)
+			page_length = PAGE_SIZE - page_offset;
+
+		ret = slow_kernel_write(dev_priv->mm.gtt_mapping,
+					page_base, page_offset,
+					user_data, page_length);
+		if (ret)
+			goto fail_unpin;
+
+		remain -= page_length;
+		user_data += page_length;
+		offset += page_length;
+	}
+
+fail_unpin:
+	i915_gem_object_unpin(obj);
+fail_unlock:
+	mutex_unlock(&dev->struct_mutex);
+fail_free:
+	drm_free(data, args->size, DRM_MEM_DRIVER);
+
+	return ret;
+}
+
 static int
 i915_gem_shmem_pwrite(struct drm_device *dev, struct drm_gem_object *obj,
 		      struct drm_i915_gem_pwrite *args,
@@ -388,9 +463,13 @@ i915_gem_pwrite_ioctl(struct drm_device *dev, void *data,
 	if (obj_priv->phys_obj)
 		ret = i915_gem_phys_pwrite(dev, obj, args, file_priv);
 	else if (obj_priv->tiling_mode == I915_TILING_NONE &&
-		 dev->gtt_total != 0)
-		ret = i915_gem_gtt_pwrite(dev, obj, args, file_priv);
-	else
+		 dev->gtt_total != 0) {
+		ret = i915_gem_gtt_pwrite_fast(dev, obj, args, file_priv);
+		if (ret == -EFAULT) {
+			ret = i915_gem_gtt_pwrite_slow(dev, obj, args,
+						       file_priv);
+		}
+	} else
 		ret = i915_gem_shmem_pwrite(dev, obj, args, file_priv);
 
 #if WATCH_PWRITE
-- 
1.5.6.5

[PATCH] drm/i915: Fix lock order reversal in shmem pwrite path.

[Eric Anholt]

Like the GTT pwrite path fix, this uses pagefault_enable/disable and a
fallback to allocating and copying the memory in outside of the lock.

Signed-off-by: Eric Anholt <eric@anholt.net>
---
 drivers/gpu/drm/i915/i915_drv.h |    1 +
 drivers/gpu/drm/i915/i915_gem.c |  117 ++++++++++++++++++++++++++++++++++++--
 2 files changed, 111 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index d6cc986..385ca8d 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -405,6 +405,7 @@ struct drm_i915_gem_object {
 	DRM_AGP_MEM *agp_mem;
 
 	struct page **page_list;
+	int page_list_refcount;
 
 	/**
 	 * Current offset of the object in GTT space.
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 2c85249..3a96e0c 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -239,6 +239,23 @@ slow_kernel_write(struct io_mapping *mapping,
 	return 0;
 }
 
+static inline int
+slow_shmem_write(struct page **page_list,
+		 loff_t page_base, int page_offset,
+		 char *data,
+		 int length)
+{
+	char __iomem *vaddr;
+
+	vaddr = kmap_atomic(page_list[page_base >> PAGE_SHIFT], KM_USER0);
+	if (vaddr == NULL)
+		return -ENOMEM;
+	memcpy(vaddr + page_offset, data, length);
+	kunmap_atomic(vaddr, KM_USER0);
+
+	return 0;
+}
+
 /**
  * This is the fast pwrite path, where we copy the data directly from the
  * user into the GTT, uncached.
@@ -391,9 +408,9 @@ fail_free:
 }
 
 static int
-i915_gem_shmem_pwrite(struct drm_device *dev, struct drm_gem_object *obj,
-		      struct drm_i915_gem_pwrite *args,
-		      struct drm_file *file_priv)
+i915_gem_shmem_pwrite_fast(struct drm_device *dev, struct drm_gem_object *obj,
+			   struct drm_i915_gem_pwrite *args,
+			   struct drm_file *file_priv)
 {
 	int ret;
 	loff_t offset;
@@ -409,9 +426,11 @@ i915_gem_shmem_pwrite(struct drm_device *dev, struct drm_gem_object *obj,
 
 	offset = args->offset;
 
+	pagefault_disable();
 	written = vfs_write(obj->filp,
 			    (char __user *)(uintptr_t) args->data_ptr,
 			    args->size, &offset);
+	pagefault_enable();
 	if (written != args->size) {
 		mutex_unlock(&dev->struct_mutex);
 		if (written < 0)
@@ -426,6 +445,83 @@ i915_gem_shmem_pwrite(struct drm_device *dev, struct drm_gem_object *obj,
 }
 
 /**
+ * This is the fallback GTT pwrite path, which allocates temporary storage
+ * in kernel space to copy_from_user, so we can take pagefaults outside of the
+ * struct_mutex.
+ */
+static int
+i915_gem_shmem_pwrite_slow(struct drm_device *dev, struct drm_gem_object *obj,
+			   struct drm_i915_gem_pwrite *args,
+			   struct drm_file *file_priv)
+{
+	struct drm_i915_gem_object *obj_priv = obj->driver_private;
+	ssize_t remain;
+	loff_t offset, page_base;
+	char __user *user_data;
+	char *data;
+	int page_offset, page_length;
+	int ret;
+
+	user_data = (char __user *) (uintptr_t) args->data_ptr;
+	remain = args->size;
+
+	data = drm_alloc(args->size, DRM_MEM_DRIVER);
+	if (data == NULL)
+		return -ENOMEM;
+
+	ret = copy_from_user(data, user_data, args->size);
+	if (ret != 0)
+		goto fail_free;
+
+	mutex_lock(&dev->struct_mutex);
+
+	ret = i915_gem_object_get_page_list(obj);
+	if (ret != 0)
+		goto fail_unlock;
+
+	ret = i915_gem_object_set_to_cpu_domain(obj, 1);
+	if (ret != 0)
+		goto fail_put_pages;
+
+	obj_priv = obj->driver_private;
+	offset = obj_priv->gtt_offset + args->offset;
+	obj_priv->dirty = 1;
+
+	while (remain > 0) {
+		/* Operation in this page
+		 *
+		 * page_base = page offset within aperture
+		 * page_offset = offset within page
+		 * page_length = bytes to copy for this page
+		 */
+		page_base = (offset & ~(PAGE_SIZE-1));
+		page_offset = offset & (PAGE_SIZE-1);
+		page_length = remain;
+		if ((page_offset + remain) > PAGE_SIZE)
+			page_length = PAGE_SIZE - page_offset;
+
+		ret = slow_shmem_write(obj_priv->page_list,
+				       page_base, page_offset,
+				       user_data, page_length);
+		if (ret)
+			goto fail_put_pages;
+
+		remain -= page_length;
+		user_data += page_length;
+		offset += page_length;
+	}
+
+fail_put_pages:
+	i915_gem_object_free_page_list(obj);
+fail_unlock:
+	mutex_unlock(&dev->struct_mutex);
+fail_free:
+	drm_free(data, args->size, DRM_MEM_DRIVER);
+
+	return ret;
+}
+
+/**
  * Writes data to the object referenced by handle.
  *
  * On error, the contents of the buffer that were to be modified are undefined.
@@ -469,8 +565,13 @@ i915_gem_pwrite_ioctl(struct drm_device *dev, void *data,
 			ret = i915_gem_gtt_pwrite_slow(dev, obj, args,
 						       file_priv);
 		}
-	} else
-		ret = i915_gem_shmem_pwrite(dev, obj, args, file_priv);
+	} else {
+		ret = i915_gem_shmem_pwrite_fast(dev, obj, args, file_priv);
+		if (ret == -EFAULT) {
+			ret = i915_gem_shmem_pwrite_slow(dev, obj, args,
+							 file_priv);
+		}
+	}
 
 #if WATCH_PWRITE
 	if (ret)
@@ -901,9 +1002,10 @@ i915_gem_object_free_page_list(struct drm_gem_object *obj)
 	int page_count = obj->size / PAGE_SIZE;
 	int i;
 
-	if (obj_priv->page_list == NULL)
+	if (--obj_priv->page_list_refcount != 0)
 		return;
 
+	BUG_ON(obj_priv->page_list_refcount < 0);
 
 	for (i = 0; i < page_count; i++)
 		if (obj_priv->page_list[i] != NULL) {
@@ -1497,7 +1599,7 @@ i915_gem_object_get_page_list(struct drm_gem_object *obj)
 	struct page *page;
 	int ret;
 
-	if (obj_priv->page_list)
+	if (obj_priv->page_list_refcount++ != 0)
 		return 0;
 
 	/* Get the list of pages out of our struct file.  They'll be pinned
@@ -1509,6 +1611,7 @@ i915_gem_object_get_page_list(struct drm_gem_object *obj)
 					 DRM_MEM_DRIVER);
 	if (obj_priv->page_list == NULL) {
 		DRM_ERROR("Faled to allocate page list\n");
+		obj_priv->page_list_refcount--;
 		return -ENOMEM;
 	}
 
-- 
1.5.6.5

Re: [PATCH] drm/i915: Fix lock order reversal in GTT pwrite path.

[Eric Anholt]

[-- Attachment #1: Type: text/plain, Size: 2652 bytes --]

On Mon, 2009-03-09 at 18:33 -0700, Eric Anholt wrote:
> Since the pagefault path determines that the lock order we use has to be
> mmap_sem -> struct_mutex, we can't allow page faults to occur while the
> struct_mutex is held.  To fix this in pwrite, we first try optimistically to
> see if we can copy from user without faulting.  If it fails, fall back to
> allocating memory, copying, then taking the lock and copying it to the GPU.
> 
> Thanks to Linus for suggesting this (in retrospect) obvious way of doing it.

Simple, obvious, and wrong, I guess:

[  114.016013] BUG: scheduling while atomic: oglconform/4555/0x10000001
[  114.016015] 1 lock held by oglconform/4555:
[  114.016017]  #0:  (&dev->struct_mutex){--..}, at: [<f8a99cad>] i915_gem_pwrite_
ioctl+0x3be/0x5f5 [i915]
[  114.016033] Modules linked in: i915 drm i2c_algo_bit cfbcopyarea cfbimgblt cfbfillrect 8250_pnp
[  114.016042] Pid: 4555, comm: oglconform Not tainted 2.6.29-rc6drm-intel-next #175
[  114.016044] Call Trace:
[  114.016048]  [<c0128e02>] __schedule_bug+0x5e/0x65
[  114.016051]  [<c07ccbe8>] schedule+0x9f/0x9e4
[  114.016055]  [<c012904f>] __cond_resched+0x25/0x3b
[  114.016058]  [<c07cd5bf>] _cond_resched+0x24/0x2f
[  114.016062]  [<c07cdfa9>] mutex_lock_nested+0x22/0x254
[  114.016065]  [<c014a631>] ? mark_held_locks+0x53/0x6a
[  114.016068]  [<c07cf66d>] ? _spin_unlock_irq+0x22/0x26
[  114.016072]  [<c014a7c8>] ? trace_hardirqs_on_caller+0xf3/0x12d
[  114.016075]  [<c0167ced>] generic_file_aio_write+0x54/0xbd
[  114.016079]  [<c0184cc5>] do_sync_write+0xab/0xe9
[  114.016082]  [<c0130ce9>] ? __do_softirq+0x135/0x13d
[  114.016086]  [<c013d1b3>] ? autoremove_wake_function+0x0/0x33
[  114.016089]  [<c0102f1c>] ? restore_nocheck_notrace+0x0/0xe
[  114.016093]  [<c014007b>] ? hrtimer_interrupt+0x53/0x146
[  114.016097]  [<c02a5662>] ? security_file_permission+0xf/0x11
[  114.016100]  [<c0184c1a>] ? do_sync_write+0x0/0xe9
[  114.016103]  [<c018549b>] vfs_write+0x8a/0x104
[  114.016116]  [<f8a99cfb>] i915_gem_pwrite_ioctl+0x40c/0x5f5 [i915]

Peter, it seems that pagefault_disable() implies not just disabling
pagefaults, but also scheduling.  I'm just trying to avoid taking
mmap_sem through the use of pagefault_disable(), while I'd like to still
be able to schedule if that's what it takes to get the work done.
Should I be able to do this, or should I just rewrite things to not use
vfs_write() and do my sleeping outside the lock?  I'm not opposed to
that, I just want to know if you needed an excuse to fix this.

-- 
Eric Anholt
eric@anholt.net                         eric.anholt@intel.com



[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 197 bytes --]