[3/3] drm/i915: Use a task to cancel the userptr on invalidate_range
diff mbox

Message ID 1435683333-17844-3-git-send-email-chris@chris-wilson.co.uk
State New
Headers show

Commit Message

Chris Wilson June 30, 2015, 4:55 p.m. UTC
Whilst discussing possible ways to trigger an invalidate_range on a
userptr with an aliased GGTT mmapping (and so cause a struct_mutex
deadlock), the conclusion is that we can, and we must, prevent any
possible deadlock by avoiding taking the mutex at all during
invalidate_range. This has numerous advantages all of which stem from
avoid the sleeping function from inside the unknown context. In
particular, it simplifies the invalidate_range because we no longer
have to juggle the spinlock/mutex and can just hold the spinlock
for the entire walk. To compensate, we have to make get_pages a bit more
complicated in order to serialise with a pending cancel_userptr worker.
As we hold the struct_mutex, we have no choice but to return EAGAIN and
hope that the worker is then flushed before we retry after reacquiring
the struct_mutex.

The important caveat is that the invalidate_range itself is no longer
synchronous. There exists a small but definite period in time in which
the old PTE remain accessible via the GPU. Note however that the
physical pages themselves are not invalidated by the mmu_notifier, just
the CPU view of the address space. The impact should be limited to a
delay in pages being flushed, rather than a possibility of writing to
the wrong pages.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Micha? Winiarski <michal.winiarski@intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_userptr.c | 142 +++++++++++++-------------------
 1 file changed, 56 insertions(+), 86 deletions(-)

Comments

Tvrtko Ursulin July 1, 2015, 12:56 p.m. UTC | #1
On 06/30/2015 05:55 PM, Chris Wilson wrote:
> Whilst discussing possible ways to trigger an invalidate_range on a
> userptr with an aliased GGTT mmapping (and so cause a struct_mutex
> deadlock), the conclusion is that we can, and we must, prevent any
> possible deadlock by avoiding taking the mutex at all during
> invalidate_range. This has numerous advantages all of which stem from
> avoid the sleeping function from inside the unknown context. In
> particular, it simplifies the invalidate_range because we no longer
> have to juggle the spinlock/mutex and can just hold the spinlock
> for the entire walk. To compensate, we have to make get_pages a bit more
> complicated in order to serialise with a pending cancel_userptr worker.
> As we hold the struct_mutex, we have no choice but to return EAGAIN and
> hope that the worker is then flushed before we retry after reacquiring
> the struct_mutex.
>
> The important caveat is that the invalidate_range itself is no longer
> synchronous. There exists a small but definite period in time in which
> the old PTE remain accessible via the GPU. Note however that the
> physical pages themselves are not invalidated by the mmu_notifier, just
> the CPU view of the address space. The impact should be limited to a
> delay in pages being flushed, rather than a possibility of writing to
> the wrong pages.

I915_USERPTR_SOMEWHAT_SYNCHRONIZED :/

Because "small but definite" might be not that small in some 
circumstances. Although.. it is not like current synchronous behavior 
really is so since a) GPU activity doesn't stop right away on 
cancellation and b) it is a "don't do that" situation anyway. So from 
that angle maybe it is not that bad, and advantages from your first 
commit paragraph are definitely real.

> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

Ok ok :)

> Cc: Micha? Winiarski <michal.winiarski@intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_gem_userptr.c | 142 +++++++++++++-------------------
>   1 file changed, 56 insertions(+), 86 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_userptr.c b/drivers/gpu/drm/i915/i915_gem_userptr.c
> index d3213fdefafc..9056aa5b00f3 100644
> --- a/drivers/gpu/drm/i915/i915_gem_userptr.c
> +++ b/drivers/gpu/drm/i915/i915_gem_userptr.c
> @@ -50,7 +50,6 @@ struct i915_mmu_notifier {
>   	struct mmu_notifier mn;
>   	struct rb_root objects;
>   	struct list_head linear;
> -	unsigned long serial;
>   	bool has_linear;
>   };
>
> @@ -59,14 +58,16 @@ struct i915_mmu_object {
>   	struct interval_tree_node it;
>   	struct list_head link;
>   	struct drm_i915_gem_object *obj;
> +	struct work_struct work;
>   	bool active;
>   	bool is_linear;
>   };
>
> -static unsigned long cancel_userptr(struct drm_i915_gem_object *obj)
> +static void __cancel_userptr__worker(struct work_struct *work)
>   {
> +	struct i915_mmu_object *mo = container_of(work, typeof(*mo), work);
> +	struct drm_i915_gem_object *obj = mo->obj;
>   	struct drm_device *dev = obj->base.dev;
> -	unsigned long end;
>
>   	mutex_lock(&dev->struct_mutex);
>   	/* Cancel any active worker and force us to re-evaluate gup */
> @@ -89,46 +90,26 @@ static unsigned long cancel_userptr(struct drm_i915_gem_object *obj)
>   		dev_priv->mm.interruptible = was_interruptible;
>   	}
>
> -	end = obj->userptr.ptr + obj->base.size;
> -
>   	drm_gem_object_unreference(&obj->base);
>   	mutex_unlock(&dev->struct_mutex);
> -
> -	return end;
>   }
>
> -static void *invalidate_range__linear(struct i915_mmu_notifier *mn,
> -				      struct mm_struct *mm,
> -				      unsigned long start,
> -				      unsigned long end)
> +static unsigned long cancel_userptr(struct i915_mmu_object *mo)
>   {
> -	struct i915_mmu_object *mo;
> -	unsigned long serial;
> -
> -restart:
> -	serial = mn->serial;
> -	list_for_each_entry(mo, &mn->linear, link) {
> -		struct drm_i915_gem_object *obj;
> -
> -		if (mo->it.last < start || mo->it.start > end)
> -			continue;
> -
> -		obj = mo->obj;
> -
> -		if (!mo->active ||
> -		    !kref_get_unless_zero(&obj->base.refcount))
> -			continue;
> -
> -		spin_unlock(&mn->lock);
> -
> -		cancel_userptr(obj);
> -
> -		spin_lock(&mn->lock);
> -		if (serial != mn->serial)
> -			goto restart;
> -	}
> +	unsigned long end = mo->obj->userptr.ptr + mo->obj->base.size;
> +
> +	/* 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
> +	 * the struct_mutex - and consequently use it after it
> +	 * is freed and then double free it.
> +	 */
> +	if (mo->active &&
> +	    kref_get_unless_zero(&mo->obj->base.refcount))
> +		schedule_work(&mo->work);
>
> -	return NULL;
> +	return end;
>   }
>
>   static void i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn,
> @@ -136,45 +117,32 @@ static void i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn,
>   						       unsigned long start,
>   						       unsigned long end)
>   {
> -	struct i915_mmu_notifier *mn = container_of(_mn, struct i915_mmu_notifier, mn);
> -	struct interval_tree_node *it = NULL;
> -	unsigned long next = start;
> -	unsigned long serial = 0;
> -
> -	end--; /* interval ranges are inclusive, but invalidate range is exclusive */
> -	while (next < end) {
> -		struct drm_i915_gem_object *obj = NULL;
> -
> -		spin_lock(&mn->lock);
> -		if (mn->has_linear)
> -			it = invalidate_range__linear(mn, mm, start, end);
> -		else if (serial == mn->serial)
> -			it = interval_tree_iter_next(it, next, end);
> -		else
> -			it = interval_tree_iter_first(&mn->objects, start, end);
> -		if (it != NULL) {
> -			struct i915_mmu_object *mo =
> -				container_of(it, struct i915_mmu_object, it);
> -
> -			/* 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
> -			 * the struct_mutex - and consequently use it after it
> -			 * is freed and then double free it.
> -			 */
> -			if (mo->active &&
> -			    kref_get_unless_zero(&mo->obj->base.refcount))
> -				obj = mo->obj;
> +	struct i915_mmu_notifier *mn =
> +		container_of(_mn, struct i915_mmu_notifier, mn);
> +	struct i915_mmu_object *mo;
> +
> +	/* interval ranges are inclusive, but invalidate range is exclusive */
> +	end--;
>
> -			serial = mn->serial;
> +	spin_lock(&mn->lock);
> +	if (mn->has_linear) {
> +		list_for_each_entry(mo, &mn->linear, link) {
> +			if (mo->it.last < start || mo->it.start > end)
> +				continue;
> +
> +			cancel_userptr(mo);
>   		}
> -		spin_unlock(&mn->lock);
> -		if (obj == NULL)
> -			return;
> +	} else {
> +		struct interval_tree_node *it;
>
> -		next = cancel_userptr(obj);
> +		it = interval_tree_iter_first(&mn->objects, start, end);
> +		while (it) {
> +			mo = container_of(it, struct i915_mmu_object, it);
> +			start = cancel_userptr(mo);
> +			it = interval_tree_iter_next(it, start, end);
> +		}
>   	}
> +	spin_unlock(&mn->lock);
>   }
>
>   static const struct mmu_notifier_ops i915_gem_userptr_notifier = {
> @@ -194,7 +162,6 @@ i915_mmu_notifier_create(struct mm_struct *mm)
>   	spin_lock_init(&mn->lock);
>   	mn->mn.ops = &i915_gem_userptr_notifier;
>   	mn->objects = RB_ROOT;
> -	mn->serial = 1;
>   	INIT_LIST_HEAD(&mn->linear);
>   	mn->has_linear = false;
>
> @@ -208,12 +175,6 @@ i915_mmu_notifier_create(struct mm_struct *mm)
>   	return mn;
>   }
>
> -static void __i915_mmu_notifier_update_serial(struct i915_mmu_notifier *mn)
> -{
> -	if (++mn->serial == 0)
> -		mn->serial = 1;
> -}
> -
>   static int
>   i915_mmu_notifier_add(struct drm_device *dev,
>   		      struct i915_mmu_notifier *mn,
> @@ -260,10 +221,9 @@ i915_mmu_notifier_add(struct drm_device *dev,
>   	} else
>   		interval_tree_insert(&mo->it, &mn->objects);
>
> -	if (ret == 0) {
> +	if (ret == 0)
>   		list_add(&mo->link, &mn->linear);
> -		__i915_mmu_notifier_update_serial(mn);
> -	}
> +
>   	spin_unlock(&mn->lock);
>   	mutex_unlock(&dev->struct_mutex);
>
> @@ -291,7 +251,6 @@ i915_mmu_notifier_del(struct i915_mmu_notifier *mn,
>   		mn->has_linear = i915_mmu_notifier_has_linear(mn);
>   	else
>   		interval_tree_remove(&mo->it, &mn->objects);
> -	__i915_mmu_notifier_update_serial(mn);
>   	spin_unlock(&mn->lock);
>   }
>
> @@ -358,6 +317,7 @@ i915_gem_userptr_init__mmu_notifier(struct drm_i915_gem_object *obj,
>   	mo->it.start = obj->userptr.ptr;
>   	mo->it.last = mo->it.start + obj->base.size - 1;
>   	mo->obj = obj;
> +	INIT_WORK(&mo->work, __cancel_userptr__worker);
>
>   	ret = i915_mmu_notifier_add(obj->base.dev, mn, mo);
>   	if (ret) {
> @@ -546,10 +506,12 @@ err:
>   	return ret;
>   }
>
> -static void
> +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
> @@ -562,12 +524,20 @@ __i915_gem_userptr_set_active(struct drm_i915_gem_object *obj,
>   	 */
>   #if defined(CONFIG_MMU_NOTIFIER)
>   	if (obj->userptr.mmu_object == NULL)
> -		return;
> +		return 0;
>
>   	spin_lock(&obj->userptr.mmu_object->mn->lock);
> -	obj->userptr.mmu_object->active = value;
> +	/* In order to serialise get_pages with an outstanding
> +	 * cancel_userptr, we must drop the struct_mutex and try again.
> +	 */
> +	if (!value || !work_pending(&obj->userptr.mmu_object->work))
> +		obj->userptr.mmu_object->active = value;
> +	else
> +		ret = -EAGAIN;
>   	spin_unlock(&obj->userptr.mmu_object->mn->lock);
>   #endif
> +
> +	return ret;
>   }

I think it would be a lot more efficient if we dropped the mutex here 
and waited for the worker to complete in kernel, rather than letting 
userspace hammer it (the mutex). Especially having experienced one 
worker-structmutex starvation yesterday.

Regards,

Tvrtko
Shuang He July 2, 2015, 4:40 p.m. UTC | #2
Tested-By: Intel Graphics QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 6689
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
ILK                                  302/302              302/302
SNB                                  312/316              312/316
IVB                                  343/343              343/343
BYT                 -1              287/287              286/287
HSW                                  380/380              380/380
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
*BYT  igt@gem_userptr_blits@sync-unmap-cycles      PASS(1)      FAIL(1)
Note: You need to pay more attention to line start with '*'
Chris Wilson July 3, 2015, 11 a.m. UTC | #3
On Wed, Jul 01, 2015 at 01:56:30PM +0100, Tvrtko Ursulin wrote:
> >@@ -562,12 +524,20 @@ __i915_gem_userptr_set_active(struct drm_i915_gem_object *obj,
> >  	 */
> >  #if defined(CONFIG_MMU_NOTIFIER)
> >  	if (obj->userptr.mmu_object == NULL)
> >-		return;
> >+		return 0;
> >
> >  	spin_lock(&obj->userptr.mmu_object->mn->lock);
> >-	obj->userptr.mmu_object->active = value;
> >+	/* In order to serialise get_pages with an outstanding
> >+	 * cancel_userptr, we must drop the struct_mutex and try again.
> >+	 */
> >+	if (!value || !work_pending(&obj->userptr.mmu_object->work))
> >+		obj->userptr.mmu_object->active = value;
> >+	else
> >+		ret = -EAGAIN;
> >  	spin_unlock(&obj->userptr.mmu_object->mn->lock);
> >  #endif
> >+
> >+	return ret;
> >  }
> 
> I think it would be a lot more efficient if we dropped the mutex
> here and waited for the worker to complete in kernel, rather than
> letting userspace hammer it (the mutex). Especially having
> experienced one worker-structmutex starvation yesterday.

Yes, it would be much easier and simpler. It also goes bang very quickly
- as we cannot drop the mutex here in the middle of an execbuffer as
that quickly corrupts the state believed reserved by the execbuffer.

If you look at other patches in my tree, we can hide the starvation
issue in the kernel - but we still have no way to solve the priority
inversion problem of using a worker for a high priority task, except to
move them to the high priority queue. Worth it? Not sure. But flushing a
single queue is easier than doing semaphore completion.
-Chris

Patch
diff mbox

diff --git a/drivers/gpu/drm/i915/i915_gem_userptr.c b/drivers/gpu/drm/i915/i915_gem_userptr.c
index d3213fdefafc..9056aa5b00f3 100644
--- a/drivers/gpu/drm/i915/i915_gem_userptr.c
+++ b/drivers/gpu/drm/i915/i915_gem_userptr.c
@@ -50,7 +50,6 @@  struct i915_mmu_notifier {
 	struct mmu_notifier mn;
 	struct rb_root objects;
 	struct list_head linear;
-	unsigned long serial;
 	bool has_linear;
 };
 
@@ -59,14 +58,16 @@  struct i915_mmu_object {
 	struct interval_tree_node it;
 	struct list_head link;
 	struct drm_i915_gem_object *obj;
+	struct work_struct work;
 	bool active;
 	bool is_linear;
 };
 
-static unsigned long cancel_userptr(struct drm_i915_gem_object *obj)
+static void __cancel_userptr__worker(struct work_struct *work)
 {
+	struct i915_mmu_object *mo = container_of(work, typeof(*mo), work);
+	struct drm_i915_gem_object *obj = mo->obj;
 	struct drm_device *dev = obj->base.dev;
-	unsigned long end;
 
 	mutex_lock(&dev->struct_mutex);
 	/* Cancel any active worker and force us to re-evaluate gup */
@@ -89,46 +90,26 @@  static unsigned long cancel_userptr(struct drm_i915_gem_object *obj)
 		dev_priv->mm.interruptible = was_interruptible;
 	}
 
-	end = obj->userptr.ptr + obj->base.size;
-
 	drm_gem_object_unreference(&obj->base);
 	mutex_unlock(&dev->struct_mutex);
-
-	return end;
 }
 
-static void *invalidate_range__linear(struct i915_mmu_notifier *mn,
-				      struct mm_struct *mm,
-				      unsigned long start,
-				      unsigned long end)
+static unsigned long cancel_userptr(struct i915_mmu_object *mo)
 {
-	struct i915_mmu_object *mo;
-	unsigned long serial;
-
-restart:
-	serial = mn->serial;
-	list_for_each_entry(mo, &mn->linear, link) {
-		struct drm_i915_gem_object *obj;
-
-		if (mo->it.last < start || mo->it.start > end)
-			continue;
-
-		obj = mo->obj;
-
-		if (!mo->active ||
-		    !kref_get_unless_zero(&obj->base.refcount))
-			continue;
-
-		spin_unlock(&mn->lock);
-
-		cancel_userptr(obj);
-
-		spin_lock(&mn->lock);
-		if (serial != mn->serial)
-			goto restart;
-	}
+	unsigned long end = mo->obj->userptr.ptr + mo->obj->base.size;
+
+	/* 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
+	 * the struct_mutex - and consequently use it after it
+	 * is freed and then double free it.
+	 */
+	if (mo->active &&
+	    kref_get_unless_zero(&mo->obj->base.refcount))
+		schedule_work(&mo->work);
 
-	return NULL;
+	return end;
 }
 
 static void i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn,
@@ -136,45 +117,32 @@  static void i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn,
 						       unsigned long start,
 						       unsigned long end)
 {
-	struct i915_mmu_notifier *mn = container_of(_mn, struct i915_mmu_notifier, mn);
-	struct interval_tree_node *it = NULL;
-	unsigned long next = start;
-	unsigned long serial = 0;
-
-	end--; /* interval ranges are inclusive, but invalidate range is exclusive */
-	while (next < end) {
-		struct drm_i915_gem_object *obj = NULL;
-
-		spin_lock(&mn->lock);
-		if (mn->has_linear)
-			it = invalidate_range__linear(mn, mm, start, end);
-		else if (serial == mn->serial)
-			it = interval_tree_iter_next(it, next, end);
-		else
-			it = interval_tree_iter_first(&mn->objects, start, end);
-		if (it != NULL) {
-			struct i915_mmu_object *mo =
-				container_of(it, struct i915_mmu_object, it);
-
-			/* 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
-			 * the struct_mutex - and consequently use it after it
-			 * is freed and then double free it.
-			 */
-			if (mo->active &&
-			    kref_get_unless_zero(&mo->obj->base.refcount))
-				obj = mo->obj;
+	struct i915_mmu_notifier *mn =
+		container_of(_mn, struct i915_mmu_notifier, mn);
+	struct i915_mmu_object *mo;
+
+	/* interval ranges are inclusive, but invalidate range is exclusive */
+	end--;
 
-			serial = mn->serial;
+	spin_lock(&mn->lock);
+	if (mn->has_linear) {
+		list_for_each_entry(mo, &mn->linear, link) {
+			if (mo->it.last < start || mo->it.start > end)
+				continue;
+
+			cancel_userptr(mo);
 		}
-		spin_unlock(&mn->lock);
-		if (obj == NULL)
-			return;
+	} else {
+		struct interval_tree_node *it;
 
-		next = cancel_userptr(obj);
+		it = interval_tree_iter_first(&mn->objects, start, end);
+		while (it) {
+			mo = container_of(it, struct i915_mmu_object, it);
+			start = cancel_userptr(mo);
+			it = interval_tree_iter_next(it, start, end);
+		}
 	}
+	spin_unlock(&mn->lock);
 }
 
 static const struct mmu_notifier_ops i915_gem_userptr_notifier = {
@@ -194,7 +162,6 @@  i915_mmu_notifier_create(struct mm_struct *mm)
 	spin_lock_init(&mn->lock);
 	mn->mn.ops = &i915_gem_userptr_notifier;
 	mn->objects = RB_ROOT;
-	mn->serial = 1;
 	INIT_LIST_HEAD(&mn->linear);
 	mn->has_linear = false;
 
@@ -208,12 +175,6 @@  i915_mmu_notifier_create(struct mm_struct *mm)
 	return mn;
 }
 
-static void __i915_mmu_notifier_update_serial(struct i915_mmu_notifier *mn)
-{
-	if (++mn->serial == 0)
-		mn->serial = 1;
-}
-
 static int
 i915_mmu_notifier_add(struct drm_device *dev,
 		      struct i915_mmu_notifier *mn,
@@ -260,10 +221,9 @@  i915_mmu_notifier_add(struct drm_device *dev,
 	} else
 		interval_tree_insert(&mo->it, &mn->objects);
 
-	if (ret == 0) {
+	if (ret == 0)
 		list_add(&mo->link, &mn->linear);
-		__i915_mmu_notifier_update_serial(mn);
-	}
+
 	spin_unlock(&mn->lock);
 	mutex_unlock(&dev->struct_mutex);
 
@@ -291,7 +251,6 @@  i915_mmu_notifier_del(struct i915_mmu_notifier *mn,
 		mn->has_linear = i915_mmu_notifier_has_linear(mn);
 	else
 		interval_tree_remove(&mo->it, &mn->objects);
-	__i915_mmu_notifier_update_serial(mn);
 	spin_unlock(&mn->lock);
 }
 
@@ -358,6 +317,7 @@  i915_gem_userptr_init__mmu_notifier(struct drm_i915_gem_object *obj,
 	mo->it.start = obj->userptr.ptr;
 	mo->it.last = mo->it.start + obj->base.size - 1;
 	mo->obj = obj;
+	INIT_WORK(&mo->work, __cancel_userptr__worker);
 
 	ret = i915_mmu_notifier_add(obj->base.dev, mn, mo);
 	if (ret) {
@@ -546,10 +506,12 @@  err:
 	return ret;
 }
 
-static void
+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
@@ -562,12 +524,20 @@  __i915_gem_userptr_set_active(struct drm_i915_gem_object *obj,
 	 */
 #if defined(CONFIG_MMU_NOTIFIER)
 	if (obj->userptr.mmu_object == NULL)
-		return;
+		return 0;
 
 	spin_lock(&obj->userptr.mmu_object->mn->lock);
-	obj->userptr.mmu_object->active = value;
+	/* In order to serialise get_pages with an outstanding
+	 * cancel_userptr, we must drop the struct_mutex and try again.
+	 */
+	if (!value || !work_pending(&obj->userptr.mmu_object->work))
+		obj->userptr.mmu_object->active = value;
+	else
+		ret = -EAGAIN;
 	spin_unlock(&obj->userptr.mmu_object->mn->lock);
 #endif
+
+	return ret;
 }
 
 static void