[PATCH] tests/gem_userptr_blits: subtests for MAP_FIXED mappings of regular bo

[PATCH] tests/gem_userptr_blits: subtests for MAP_FIXED mappings of regular bo

[Michał Winiarski]

When the the memory backing the userptr object is freed by the user, it's
possible to trigger recursive deadlock caused by operations done on
different BO mapped in that region, triggering invalidate.

Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
---
 tests/gem_userptr_blits.c | 83 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 83 insertions(+)

diff --git a/tests/gem_userptr_blits.c b/tests/gem_userptr_blits.c
index 1f2cc96..3fe8f90 100644
--- a/tests/gem_userptr_blits.c
+++ b/tests/gem_userptr_blits.c
@@ -640,6 +640,80 @@ static void test_forked_access(int fd)
 	free(ptr2);
 }
 
+static int test_map_fixed_invalidate(int fd, bool overlap)
+{
+	void *ptr;
+	void *map;
+	int i;
+	int num_handles = overlap ? 2 : 1;
+	uint32_t handle[num_handles];
+	uint32_t mmap_handle;
+	struct drm_i915_gem_mmap_gtt mmap_arg;
+
+	igt_assert(posix_memalign(&ptr, PAGE_SIZE, PAGE_SIZE) == 0);
+	for (i=0; i<num_handles; i++)
+		igt_assert(gem_userptr(fd, ptr, PAGE_SIZE, 0, &handle[i]) == 0);
+	free(ptr);
+
+	mmap_handle = gem_create(fd, PAGE_SIZE);
+
+	memset(&mmap_arg, 0, sizeof(mmap_arg));
+	mmap_arg.handle = mmap_handle;
+	do_ioctl(fd, DRM_IOCTL_I915_GEM_MMAP_GTT, &mmap_arg);
+	map = mmap(ptr, PAGE_SIZE, PROT_READ | PROT_WRITE, MAP_SHARED | MAP_FIXED,
+			fd, mmap_arg.offset);
+	igt_assert(map != MAP_FAILED);
+
+	*(uint32_t*)map = 0xdead;
+	gem_set_tiling(fd, mmap_handle, 2, 512 * 4);
+	munmap(map, PAGE_SIZE);
+
+	for (i=0; i<num_handles; i++)
+		gem_close(fd, handle[i]);
+
+	return 0;
+}
+
+static int test_map_fixed_partial_overlap(int fd)
+{
+	void *ptr;
+	void *map;
+	uint32_t handle;
+	uint32_t mmap_handle;
+	struct drm_i915_gem_mmap_gtt mmap_arg;
+	struct drm_i915_gem_set_domain set_domain;
+
+	igt_assert(posix_memalign(&ptr, PAGE_SIZE, sizeof(linear)) == 0);
+	handle = create_userptr(fd, 0, ptr);
+	copy(fd, handle, handle, 0);
+
+	mmap_handle = gem_create(fd, PAGE_SIZE);
+
+	memset(&mmap_arg, 0, sizeof(mmap_arg));
+	mmap_arg.handle = mmap_handle;
+	do_ioctl(fd, DRM_IOCTL_I915_GEM_MMAP_GTT, &mmap_arg);
+	map = mmap(ptr, PAGE_SIZE, PROT_READ | PROT_WRITE, MAP_SHARED | MAP_FIXED,
+			fd, mmap_arg.offset);
+	igt_assert(map != MAP_FAILED);
+
+	*(uint32_t*)map = 0xdead;
+
+	memset(&set_domain, 0, sizeof(set_domain));
+	set_domain.handle = handle;
+	set_domain.read_domains = I915_GEM_DOMAIN_GTT;
+	set_domain.write_domain = I915_GEM_DOMAIN_GTT;
+	igt_assert((drmIoctl(fd, DRM_IOCTL_I915_GEM_SET_DOMAIN, &set_domain) != 0) &&
+			errno == EFAULT);
+
+	free(ptr);
+	munmap(map, PAGE_SIZE);
+
+	gem_close(fd, handle);
+	gem_close(fd, mmap_handle);
+
+	return 0;
+}
+
 static int test_forbidden_ops(int fd)
 {
 	struct drm_i915_gem_pread gem_pread;
@@ -1489,6 +1563,15 @@ int main(int argc, char **argv)
 	igt_subtest("stress-mm-invalidate-close-overlap")
 		test_invalidate_close_race(fd, true);
 
+	igt_subtest("map-fixed-invalidate")
+		test_map_fixed_invalidate(fd, false);
+
+	igt_subtest("map-fixed-invalidate-overlap")
+		test_map_fixed_invalidate(fd, true);
+
+	igt_subtest("map-fixed-partial-overlap")
+		test_map_fixed_partial_overlap(fd);
+
 	igt_subtest("coherency-sync")
 		test_coherency(fd, count);
 
-- 
2.4.3

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

[PATCH] drm/i915: Fix userptr deadlock with MAP_FIXED

[Chris Wilson]

Michał Winiarski found a really evil way to trigger a struct_mutex
deadlock with userptr. He found that if he allocated a userptr bo and
then GTT mmaped another bo, or even itself, at the same address as the
userptr using MAP_FIXED, he could then cause a deadlock any time we then
had to invalidate the GTT mmappings (so at will).

To counter act the deadlock, we make the observation that when the
MAP_FIXED is made we would have an invalidate_range event for our
object. After that we should no longer alias with the rogue mmapping. If
we are then able to mark the object as no longer in use after the first
invalidate, we do not need to grab the struct_mutex for the subsequent
invalidations.

The patch makes one eye-catching change. That is the removal serial=0
after detecting a to-be-freed object inside the invalidate walker. I
felt setting serial=0 was a questionable pessismation: it denies us the
chance to reuse the current iterator for the next loop (before it is
freed) and being explicit makes the reader question the validity of the
locking (since the object-free race could occur elsewhere). The
serialisation of the iterator is through the spinlock, if the object is
freed before the next loop then the notifier.serial will be incremented
and we start the walk from the beginning as we detect the invalid cache.

Reported-by: Michał Winiarski <michal.winiarski@intel.com>
Testcase: igt/gem_userptr_blits/map-fixed*
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Michał Winiarski <michal.winiarski@intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: stable@vger.kernel.org
---
 drivers/gpu/drm/i915/i915_gem_userptr.c | 43 +++++++++++++++++++++++++++------
 1 file changed, 36 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_userptr.c b/drivers/gpu/drm/i915/i915_gem_userptr.c
index cb367d9f7909..8af2a01d726a 100644
--- a/drivers/gpu/drm/i915/i915_gem_userptr.c
+++ b/drivers/gpu/drm/i915/i915_gem_userptr.c
@@ -59,6 +59,7 @@ struct i915_mmu_object {
 	struct interval_tree_node it;
 	struct list_head link;
 	struct drm_i915_gem_object *obj;
+	bool active;
 	bool is_linear;
 };
 
@@ -114,7 +115,8 @@ restart:
 
 		obj = mo->obj;
 
-		if (!kref_get_unless_zero(&obj->base.refcount))
+		if (!mo->active ||
+		    !kref_get_unless_zero(&obj->base.refcount))
 			continue;
 
 		spin_unlock(&mn->lock);
@@ -151,7 +153,8 @@ static void i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn,
 		else
 			it = interval_tree_iter_first(&mn->objects, start, end);
 		if (it != NULL) {
-			obj = container_of(it, struct i915_mmu_object, it)->obj;
+			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
@@ -160,11 +163,9 @@ static void i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn,
 			 * the struct_mutex - and consequently use it after it
 			 * is freed and then double free it.
 			 */
-			if (!kref_get_unless_zero(&obj->base.refcount)) {
-				spin_unlock(&mn->lock);
-				serial = 0;
-				continue;
-			}
+			if (mo->active &&
+			    kref_get_unless_zero(&mo->obj->base.refcount))
+				obj = mo->obj;
 
 			serial = mn->serial;
 		}
@@ -606,6 +607,20 @@ __i915_gem_userptr_get_pages_worker(struct work_struct *_work)
 		wake_up_all(&to_i915(dev)->mm.queue);
 }
 
+static void
+__i915_gem_userptr_set_active(struct drm_i915_gem_object *obj,
+			      bool value)
+{
+#if defined(CONFIG_MMU_NOTIFIER)
+	if (obj->userptr.mmu_object == NULL)
+		return;
+
+	spin_lock(&obj->userptr.mmu_object->mn->lock);
+	obj->userptr.mmu_object->active = value;
+	spin_unlock(&obj->userptr.mmu_object->mn->lock);
+#endif
+}
+
 static int
 i915_gem_userptr_get_pages(struct drm_i915_gem_object *obj)
 {
@@ -613,6 +628,18 @@ i915_gem_userptr_get_pages(struct drm_i915_gem_object *obj)
 	struct page **pvec;
 	int pinned, ret;
 
+	/* 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 aliasing the userptr address space with a GTT
+	 * mmapping (possible with a MAP_FIXED) - then we 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.
+	 */
+	__i915_gem_userptr_set_active(obj, true);
+
 	/* 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
@@ -732,6 +759,8 @@ i915_gem_userptr_put_pages(struct drm_i915_gem_object *obj)
 
 	sg_free_table(obj->pages);
 	kfree(obj->pages);
+
+	__i915_gem_userptr_set_active(obj, false);
 }
 
 static void
-- 
2.1.4

[PATCH v2] drm/i915: Fix userptr deadlock with MAP_FIXED

[Chris Wilson]

Michał Winiarski found a really evil way to trigger a struct_mutex
deadlock with userptr. He found that if he allocated a userptr bo and
then GTT mmaped another bo, or even itself, at the same address as the
userptr using MAP_FIXED, he could then cause a deadlock any time we then
had to invalidate the GTT mmappings (so at will).

To counter act the deadlock, we make the observation that when the
MAP_FIXED is made we would have an invalidate_range event for our
object. After that we should no longer alias with the rogue mmapping. If
we are then able to mark the object as no longer in use after the first
invalidate, we do not need to grab the struct_mutex for the subsequent
invalidations.

The patch makes one eye-catching change. That is the removal serial=0
after detecting a to-be-freed object inside the invalidate walker. I
felt setting serial=0 was a questionable pessimisation: it denies us the
chance to reuse the current iterator for the next loop (before it is
freed) and being explicit makes the reader question the validity of the
locking (since the object-free race could occur elsewhere). The
serialisation of the iterator is through the spinlock, if the object is
freed before the next loop then the notifier.serial will be incremented
and we start the walk from the beginning as we detect the invalid cache.

v2: Grammar fixes

Reported-by: Michał Winiarski <michal.winiarski@intel.com>
Testcase: igt/gem_userptr_blits/map-fixed*
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Michał Winiarski <michal.winiarski@intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: stable@vger.kernel.org
---
 drivers/gpu/drm/i915/i915_gem_userptr.c | 43 +++++++++++++++++++++++++++------
 1 file changed, 36 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_userptr.c b/drivers/gpu/drm/i915/i915_gem_userptr.c
index cb367d9f7909..e1965d8c08c8 100644
--- a/drivers/gpu/drm/i915/i915_gem_userptr.c
+++ b/drivers/gpu/drm/i915/i915_gem_userptr.c
@@ -59,6 +59,7 @@ struct i915_mmu_object {
 	struct interval_tree_node it;
 	struct list_head link;
 	struct drm_i915_gem_object *obj;
+	bool active;
 	bool is_linear;
 };
 
@@ -114,7 +115,8 @@ restart:
 
 		obj = mo->obj;
 
-		if (!kref_get_unless_zero(&obj->base.refcount))
+		if (!mo->active ||
+		    !kref_get_unless_zero(&obj->base.refcount))
 			continue;
 
 		spin_unlock(&mn->lock);
@@ -151,7 +153,8 @@ static void i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn,
 		else
 			it = interval_tree_iter_first(&mn->objects, start, end);
 		if (it != NULL) {
-			obj = container_of(it, struct i915_mmu_object, it)->obj;
+			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
@@ -160,11 +163,9 @@ static void i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn,
 			 * the struct_mutex - and consequently use it after it
 			 * is freed and then double free it.
 			 */
-			if (!kref_get_unless_zero(&obj->base.refcount)) {
-				spin_unlock(&mn->lock);
-				serial = 0;
-				continue;
-			}
+			if (mo->active &&
+			    kref_get_unless_zero(&mo->obj->base.refcount))
+				obj = mo->obj;
 
 			serial = mn->serial;
 		}
@@ -606,6 +607,20 @@ __i915_gem_userptr_get_pages_worker(struct work_struct *_work)
 		wake_up_all(&to_i915(dev)->mm.queue);
 }
 
+static void
+__i915_gem_userptr_set_active(struct drm_i915_gem_object *obj,
+			      bool value)
+{
+#if defined(CONFIG_MMU_NOTIFIER)
+	if (obj->userptr.mmu_object == NULL)
+		return;
+
+	spin_lock(&obj->userptr.mmu_object->mn->lock);
+	obj->userptr.mmu_object->active = value;
+	spin_unlock(&obj->userptr.mmu_object->mn->lock);
+#endif
+}
+
 static int
 i915_gem_userptr_get_pages(struct drm_i915_gem_object *obj)
 {
@@ -613,6 +628,18 @@ i915_gem_userptr_get_pages(struct drm_i915_gem_object *obj)
 	struct page **pvec;
 	int pinned, ret;
 
+	/* 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.
+	 */
+	__i915_gem_userptr_set_active(obj, true);
+
 	/* 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
@@ -732,6 +759,8 @@ i915_gem_userptr_put_pages(struct drm_i915_gem_object *obj)
 
 	sg_free_table(obj->pages);
 	kfree(obj->pages);
+
+	__i915_gem_userptr_set_active(obj, false);
 }
 
 static void
-- 
2.1.4

Re: [PATCH v2] drm/i915: Fix userptr deadlock with MAP_FIXED

[Michał Winiarski]

On Mon, Jun 29, 2015 at 12:17:33PM +0100, Chris Wilson wrote:
> Michał Winiarski found a really evil way to trigger a struct_mutex
> deadlock with userptr. He found that if he allocated a userptr bo and
> then GTT mmaped another bo, or even itself, at the same address as the
> userptr using MAP_FIXED, he could then cause a deadlock any time we then
> had to invalidate the GTT mmappings (so at will).
> 
> To counter act the deadlock, we make the observation that when the
> MAP_FIXED is made we would have an invalidate_range event for our
> object. After that we should no longer alias with the rogue mmapping. If
> we are then able to mark the object as no longer in use after the first
> invalidate, we do not need to grab the struct_mutex for the subsequent
> invalidations.
> 
> The patch makes one eye-catching change. That is the removal serial=0
> after detecting a to-be-freed object inside the invalidate walker. I
> felt setting serial=0 was a questionable pessimisation: it denies us the
> chance to reuse the current iterator for the next loop (before it is
> freed) and being explicit makes the reader question the validity of the
> locking (since the object-free race could occur elsewhere). The
> serialisation of the iterator is through the spinlock, if the object is
> freed before the next loop then the notifier.serial will be incremented
> and we start the walk from the beginning as we detect the invalid cache.
> 
> v2: Grammar fixes
> 
> Reported-by: Michał Winiarski <michal.winiarski@intel.com>
> Testcase: igt/gem_userptr_blits/map-fixed*
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Michał Winiarski <michal.winiarski@intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: stable@vger.kernel.org
> ---
>  drivers/gpu/drm/i915/i915_gem_userptr.c | 43 +++++++++++++++++++++++++++------
>  1 file changed, 36 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_userptr.c b/drivers/gpu/drm/i915/i915_gem_userptr.c
> index cb367d9f7909..e1965d8c08c8 100644
> --- a/drivers/gpu/drm/i915/i915_gem_userptr.c
> +++ b/drivers/gpu/drm/i915/i915_gem_userptr.c
> @@ -59,6 +59,7 @@ struct i915_mmu_object {
>  	struct interval_tree_node it;
>  	struct list_head link;
>  	struct drm_i915_gem_object *obj;
> +	bool active;
>  	bool is_linear;
>  };
>  
> @@ -114,7 +115,8 @@ restart:
>  
>  		obj = mo->obj;
>  
> -		if (!kref_get_unless_zero(&obj->base.refcount))
> +		if (!mo->active ||
> +		    !kref_get_unless_zero(&obj->base.refcount))
>  			continue;
>  
>  		spin_unlock(&mn->lock);
> @@ -151,7 +153,8 @@ static void i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn,
>  		else
>  			it = interval_tree_iter_first(&mn->objects, start, end);
>  		if (it != NULL) {
> -			obj = container_of(it, struct i915_mmu_object, it)->obj;
> +			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
> @@ -160,11 +163,9 @@ static void i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn,
>  			 * the struct_mutex - and consequently use it after it
>  			 * is freed and then double free it.
>  			 */
> -			if (!kref_get_unless_zero(&obj->base.refcount)) {
> -				spin_unlock(&mn->lock);
> -				serial = 0;
> -				continue;
> -			}
> +			if (mo->active &&
> +			    kref_get_unless_zero(&mo->obj->base.refcount))
> +				obj = mo->obj;
>  
>  			serial = mn->serial;
>  		}
> @@ -606,6 +607,20 @@ __i915_gem_userptr_get_pages_worker(struct work_struct *_work)
>  		wake_up_all(&to_i915(dev)->mm.queue);
>  }
>  
> +static void
> +__i915_gem_userptr_set_active(struct drm_i915_gem_object *obj,
> +			      bool value)
> +{
> +#if defined(CONFIG_MMU_NOTIFIER)
> +	if (obj->userptr.mmu_object == NULL)
> +		return;
> +
> +	spin_lock(&obj->userptr.mmu_object->mn->lock);
> +	obj->userptr.mmu_object->active = value;
> +	spin_unlock(&obj->userptr.mmu_object->mn->lock);
> +#endif
> +}
> +
>  static int
>  i915_gem_userptr_get_pages(struct drm_i915_gem_object *obj)
>  {
> @@ -613,6 +628,18 @@ i915_gem_userptr_get_pages(struct drm_i915_gem_object *obj)
>  	struct page **pvec;
>  	int pinned, ret;
>  
> +	/* 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.
> +	 */
> +	__i915_gem_userptr_set_active(obj, true);
> +

This will set mmu_object to active even if we're exiting early from here
(because of error handling), creating another possibility for deadlock.

-Michał

>  	/* 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
> @@ -732,6 +759,8 @@ i915_gem_userptr_put_pages(struct drm_i915_gem_object *obj)
>  
>  	sg_free_table(obj->pages);
>  	kfree(obj->pages);
> +
> +	__i915_gem_userptr_set_active(obj, false);
>  }
>  
>  static void
> -- 
> 2.1.4
> 

Re: [PATCH v2] drm/i915: Fix userptr deadlock with MAP_FIXED

[Tvrtko Ursulin]

On 06/29/2015 04:57 PM, Michał Winiarski wrote:
> On Mon, Jun 29, 2015 at 12:17:33PM +0100, Chris Wilson wrote:
>> Michał Winiarski found a really evil way to trigger a struct_mutex
>> deadlock with userptr. He found that if he allocated a userptr bo and
>> then GTT mmaped another bo, or even itself, at the same address as the
>> userptr using MAP_FIXED, he could then cause a deadlock any time we then
>> had to invalidate the GTT mmappings (so at will).
>>
>> To counter act the deadlock, we make the observation that when the
>> MAP_FIXED is made we would have an invalidate_range event for our
>> object. After that we should no longer alias with the rogue mmapping. If
>> we are then able to mark the object as no longer in use after the first
>> invalidate, we do not need to grab the struct_mutex for the subsequent
>> invalidations.
>>
>> The patch makes one eye-catching change. That is the removal serial=0
>> after detecting a to-be-freed object inside the invalidate walker. I
>> felt setting serial=0 was a questionable pessimisation: it denies us the
>> chance to reuse the current iterator for the next loop (before it is
>> freed) and being explicit makes the reader question the validity of the
>> locking (since the object-free race could occur elsewhere). The
>> serialisation of the iterator is through the spinlock, if the object is
>> freed before the next loop then the notifier.serial will be incremented
>> and we start the walk from the beginning as we detect the invalid cache.
>>
>> v2: Grammar fixes
>>
>> Reported-by: Michał Winiarski <michal.winiarski@intel.com>
>> Testcase: igt/gem_userptr_blits/map-fixed*
>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Michał Winiarski <michal.winiarski@intel.com>
>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> Cc: stable@vger.kernel.org
>> ---
>>   drivers/gpu/drm/i915/i915_gem_userptr.c | 43 +++++++++++++++++++++++++++------
>>   1 file changed, 36 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_gem_userptr.c b/drivers/gpu/drm/i915/i915_gem_userptr.c
>> index cb367d9f7909..e1965d8c08c8 100644
>> --- a/drivers/gpu/drm/i915/i915_gem_userptr.c
>> +++ b/drivers/gpu/drm/i915/i915_gem_userptr.c
>> @@ -59,6 +59,7 @@ struct i915_mmu_object {
>>   	struct interval_tree_node it;
>>   	struct list_head link;
>>   	struct drm_i915_gem_object *obj;
>> +	bool active;
>>   	bool is_linear;
>>   };
>>
>> @@ -114,7 +115,8 @@ restart:
>>
>>   		obj = mo->obj;
>>
>> -		if (!kref_get_unless_zero(&obj->base.refcount))
>> +		if (!mo->active ||
>> +		    !kref_get_unless_zero(&obj->base.refcount))
>>   			continue;
>>
>>   		spin_unlock(&mn->lock);
>> @@ -151,7 +153,8 @@ static void i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn,
>>   		else
>>   			it = interval_tree_iter_first(&mn->objects, start, end);
>>   		if (it != NULL) {
>> -			obj = container_of(it, struct i915_mmu_object, it)->obj;
>> +			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
>> @@ -160,11 +163,9 @@ static void i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn,
>>   			 * the struct_mutex - and consequently use it after it
>>   			 * is freed and then double free it.
>>   			 */
>> -			if (!kref_get_unless_zero(&obj->base.refcount)) {
>> -				spin_unlock(&mn->lock);
>> -				serial = 0;
>> -				continue;
>> -			}
>> +			if (mo->active &&
>> +			    kref_get_unless_zero(&mo->obj->base.refcount))
>> +				obj = mo->obj;
>>
>>   			serial = mn->serial;
>>   		}
>> @@ -606,6 +607,20 @@ __i915_gem_userptr_get_pages_worker(struct work_struct *_work)
>>   		wake_up_all(&to_i915(dev)->mm.queue);
>>   }
>>
>> +static void
>> +__i915_gem_userptr_set_active(struct drm_i915_gem_object *obj,
>> +			      bool value)
>> +{
>> +#if defined(CONFIG_MMU_NOTIFIER)
>> +	if (obj->userptr.mmu_object == NULL)
>> +		return;
>> +
>> +	spin_lock(&obj->userptr.mmu_object->mn->lock);
>> +	obj->userptr.mmu_object->active = value;
>> +	spin_unlock(&obj->userptr.mmu_object->mn->lock);
>> +#endif
>> +}
>> +
>>   static int
>>   i915_gem_userptr_get_pages(struct drm_i915_gem_object *obj)
>>   {
>> @@ -613,6 +628,18 @@ i915_gem_userptr_get_pages(struct drm_i915_gem_object *obj)
>>   	struct page **pvec;
>>   	int pinned, ret;
>>
>> +	/* 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.
>> +	 */
>> +	__i915_gem_userptr_set_active(obj, true);
>> +
>
> This will set mmu_object to active even if we're exiting early from here
> (because of error handling), creating another possibility for deadlock.

I think this deadlock is reproducible without MAP_FIXED, so commit 
message should be probably reworded to allow for the more generic case.

I reproduced it like this:

1. mmap, gem_userptr, munmap
2. Create a normal bo.
3. Loop a bit mmapping the above until it hits the same address as userptr.
4. Write to the BO mmap to set fault_mappable.
5. set_tiling on normal bo handle.

I am still thinking about this active flag in the above scenario.

userptr->get_pages hasn't been called above so active == false. If 
between steps 4 and 5 we trigger get_pages, userptr transitions to 
active and set_tiling deadlocks. Or I still missing something?

Regards,

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

Re: [Intel-gfx] [PATCH v2] drm/i915: Fix userptr deadlock with MAP_FIXED

[Chris Wilson]

On Tue, Jun 30, 2015 at 03:52:52PM +0100, Tvrtko Ursulin wrote:
> I think this deadlock is reproducible without MAP_FIXED, so commit
> message should be probably reworded to allow for the more generic
> case.

Ok, but it is still aliasing the address range, MAP_FIXED is the
easiest way to demonstrate that.
 
> I reproduced it like this:
> 
> 1. mmap, gem_userptr, munmap
> 2. Create a normal bo.
> 3. Loop a bit mmapping the above until it hits the same address as userptr.
> 4. Write to the BO mmap to set fault_mappable.
> 5. set_tiling on normal bo handle.
> 
> I am still thinking about this active flag in the above scenario.
> 
> userptr->get_pages hasn't been called above so active == false. If
> between steps 4 and 5 we trigger get_pages, userptr transitions to
> active and set_tiling deadlocks. Or I still missing something?

That suggests MAP_FIXED is special in invalidating the range unlike a
normal mmap().  This is arguing that we must always make any access after
invalidate_range be EFAULT. The danger here is that I am not sure if there
are any circumstances where invalidate_range is called and the vma
survives. Looking again at mm/, I can't see any place where we can
legally be expecting to reuse the same address for a userptr after the
invalidate.

I'm sure you have a testcase waiting for igt :)
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [Intel-gfx] [PATCH v2] drm/i915: Fix userptr deadlock with MAP_FIXED

[Chris Wilson]

On Tue, Jun 30, 2015 at 04:31:15PM +0100, Chris Wilson wrote:
> That suggests MAP_FIXED is special in invalidating the range unlike a
> normal mmap().  This is arguing that we must always make any access after
> invalidate_range be EFAULT. The danger here is that I am not sure if there
> are any circumstances where invalidate_range is called and the vma
> survives. Looking again at mm/, I can't see any place where we can
> legally be expecting to reuse the same address for a userptr after the
> invalidate.

Sigh, changing page protections (e.g. after a fork) if I am not mistaken
also generates an invalidate_range. Back to the drawing board here I am
afraid, as I think I need a worker to do cancel_userptr with all the
complications of coordinating between the multiple workers.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] tests/gem_userptr_blits: subtests for MAP_FIXED mappings of regular bo

[Tvrtko Ursulin]

On 06/29/2015 11:59 AM, Michał Winiarski wrote:
> When the the memory backing the userptr object is freed by the user, it's
> possible to trigger recursive deadlock caused by operations done on
> different BO mapped in that region, triggering invalidate.
>
> Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
> ---
>   tests/gem_userptr_blits.c | 83 +++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 83 insertions(+)
>
> diff --git a/tests/gem_userptr_blits.c b/tests/gem_userptr_blits.c
> index 1f2cc96..3fe8f90 100644
> --- a/tests/gem_userptr_blits.c
> +++ b/tests/gem_userptr_blits.c
> @@ -640,6 +640,80 @@ static void test_forked_access(int fd)
>   	free(ptr2);
>   }
>
> +static int test_map_fixed_invalidate(int fd, bool overlap)
> +{
> +	void *ptr;
> +	void *map;
> +	int i;
> +	int num_handles = overlap ? 2 : 1;
> +	uint32_t handle[num_handles];
> +	uint32_t mmap_handle;
> +	struct drm_i915_gem_mmap_gtt mmap_arg;
> +
> +	igt_assert(posix_memalign(&ptr, PAGE_SIZE, PAGE_SIZE) == 0);
> +	for (i=0; i<num_handles; i++)
> +		igt_assert(gem_userptr(fd, ptr, PAGE_SIZE, 0, &handle[i]) == 0);
> +	free(ptr);

I am not sure we can rely on free triggering munmap(2) here, I think 
this is just glibc implementation detail. So I would suggest allocating 
with mmap and freeing with munmap.

> +
> +	mmap_handle = gem_create(fd, PAGE_SIZE);
> +
> +	memset(&mmap_arg, 0, sizeof(mmap_arg));
> +	mmap_arg.handle = mmap_handle;
> +	do_ioctl(fd, DRM_IOCTL_I915_GEM_MMAP_GTT, &mmap_arg);
> +	map = mmap(ptr, PAGE_SIZE, PROT_READ | PROT_WRITE, MAP_SHARED | MAP_FIXED,
> +			fd, mmap_arg.offset);
> +	igt_assert(map != MAP_FAILED);
> +
> +	*(uint32_t*)map = 0xdead;
> +	gem_set_tiling(fd, mmap_handle, 2, 512 * 4);
> +	munmap(map, PAGE_SIZE);
> +
> +	for (i=0; i<num_handles; i++)
> +		gem_close(fd, handle[i]);
> +
> +	return 0;
> +}
> +
> +static int test_map_fixed_partial_overlap(int fd)
> +{
> +	void *ptr;
> +	void *map;
> +	uint32_t handle;
> +	uint32_t mmap_handle;
> +	struct drm_i915_gem_mmap_gtt mmap_arg;
> +	struct drm_i915_gem_set_domain set_domain;
> +
> +	igt_assert(posix_memalign(&ptr, PAGE_SIZE, sizeof(linear)) == 0);
> +	handle = create_userptr(fd, 0, ptr);
> +	copy(fd, handle, handle, 0);
> +
> +	mmap_handle = gem_create(fd, PAGE_SIZE);
> +
> +	memset(&mmap_arg, 0, sizeof(mmap_arg));
> +	mmap_arg.handle = mmap_handle;
> +	do_ioctl(fd, DRM_IOCTL_I915_GEM_MMAP_GTT, &mmap_arg);
> +	map = mmap(ptr, PAGE_SIZE, PROT_READ | PROT_WRITE, MAP_SHARED | MAP_FIXED,
> +			fd, mmap_arg.offset);
> +	igt_assert(map != MAP_FAILED);
> +
> +	*(uint32_t*)map = 0xdead;
> +
> +	memset(&set_domain, 0, sizeof(set_domain));
> +	set_domain.handle = handle;
> +	set_domain.read_domains = I915_GEM_DOMAIN_GTT;
> +	set_domain.write_domain = I915_GEM_DOMAIN_GTT;
> +	igt_assert((drmIoctl(fd, DRM_IOCTL_I915_GEM_SET_DOMAIN, &set_domain) != 0) &&
> +			errno == EFAULT);
> +
> +	free(ptr);
> +	munmap(map, PAGE_SIZE);
> +
> +	gem_close(fd, handle);
> +	gem_close(fd, mmap_handle);
> +
> +	return 0;
> +}
> +
>   static int test_forbidden_ops(int fd)
>   {
>   	struct drm_i915_gem_pread gem_pread;
> @@ -1489,6 +1563,15 @@ int main(int argc, char **argv)
>   	igt_subtest("stress-mm-invalidate-close-overlap")
>   		test_invalidate_close_race(fd, true);
>
> +	igt_subtest("map-fixed-invalidate")
> +		test_map_fixed_invalidate(fd, false);
> +
> +	igt_subtest("map-fixed-invalidate-overlap")
> +		test_map_fixed_invalidate(fd, true);
> +
> +	igt_subtest("map-fixed-partial-overlap")
> +		test_map_fixed_partial_overlap(fd);
> +

It is a bit confusing how overlap means two different things between 
subtests. In one it is two userptr objects that are overlapping and in 
another it is the mmap of a normal GEM bo overlapping the userptr range. 
I mean, in all subtests mmap always overlap the userptr.

Also, should there be a subtest which mmaps the userptr bo itself with 
MAP_FIXED, to the same or overlapping range?


Regards,

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

Re: [PATCH] tests/gem_userptr_blits: subtests for MAP_FIXED mappings of regular bo

[Chris Wilson]

On Mon, Jun 29, 2015 at 03:01:12PM +0100, Tvrtko Ursulin wrote:
> 
> On 06/29/2015 11:59 AM, Michał Winiarski wrote:
> >When the the memory backing the userptr object is freed by the user, it's
> >possible to trigger recursive deadlock caused by operations done on
> >different BO mapped in that region, triggering invalidate.
> >
> >Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
> >---
> >  tests/gem_userptr_blits.c | 83 +++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 83 insertions(+)
> >
> >diff --git a/tests/gem_userptr_blits.c b/tests/gem_userptr_blits.c
> >index 1f2cc96..3fe8f90 100644
> >--- a/tests/gem_userptr_blits.c
> >+++ b/tests/gem_userptr_blits.c
> >@@ -640,6 +640,80 @@ static void test_forked_access(int fd)
> >  	free(ptr2);
> >  }
> >
> >+static int test_map_fixed_invalidate(int fd, bool overlap)
> >+{
> >+	void *ptr;
> >+	void *map;
> >+	int i;
> >+	int num_handles = overlap ? 2 : 1;
> >+	uint32_t handle[num_handles];
> >+	uint32_t mmap_handle;
> >+	struct drm_i915_gem_mmap_gtt mmap_arg;
> >+
> >+	igt_assert(posix_memalign(&ptr, PAGE_SIZE, PAGE_SIZE) == 0);
> >+	for (i=0; i<num_handles; i++)
> >+		igt_assert(gem_userptr(fd, ptr, PAGE_SIZE, 0, &handle[i]) == 0);
> >+	free(ptr);
> 
> I am not sure we can rely on free triggering munmap(2) here, I think
> this is just glibc implementation detail. So I would suggest
> allocating with mmap and freeing with munmap.

The MAP_FIXED itself should be sufficient to invalidate any prior vma at
that address, so we don't depend upon the free() here to zap
userptr->pages. In this instance userptr->pages doesn't get populated
before the MAP_FIXED anyway.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] tests/gem_userptr_blits: subtests for MAP_FIXED mappings of regular bo

[Tvrtko Ursulin]

On 06/29/2015 03:07 PM, Chris Wilson wrote:
> On Mon, Jun 29, 2015 at 03:01:12PM +0100, Tvrtko Ursulin wrote:
>>
>> On 06/29/2015 11:59 AM, Michał Winiarski wrote:
>>> When the the memory backing the userptr object is freed by the user, it's
>>> possible to trigger recursive deadlock caused by operations done on
>>> different BO mapped in that region, triggering invalidate.
>>>
>>> Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
>>> ---
>>>   tests/gem_userptr_blits.c | 83 +++++++++++++++++++++++++++++++++++++++++++++++
>>>   1 file changed, 83 insertions(+)
>>>
>>> diff --git a/tests/gem_userptr_blits.c b/tests/gem_userptr_blits.c
>>> index 1f2cc96..3fe8f90 100644
>>> --- a/tests/gem_userptr_blits.c
>>> +++ b/tests/gem_userptr_blits.c
>>> @@ -640,6 +640,80 @@ static void test_forked_access(int fd)
>>>   	free(ptr2);
>>>   }
>>>
>>> +static int test_map_fixed_invalidate(int fd, bool overlap)
>>> +{
>>> +	void *ptr;
>>> +	void *map;
>>> +	int i;
>>> +	int num_handles = overlap ? 2 : 1;
>>> +	uint32_t handle[num_handles];
>>> +	uint32_t mmap_handle;
>>> +	struct drm_i915_gem_mmap_gtt mmap_arg;
>>> +
>>> +	igt_assert(posix_memalign(&ptr, PAGE_SIZE, PAGE_SIZE) == 0);
>>> +	for (i=0; i<num_handles; i++)
>>> +		igt_assert(gem_userptr(fd, ptr, PAGE_SIZE, 0, &handle[i]) == 0);
>>> +	free(ptr);
>>
>> I am not sure we can rely on free triggering munmap(2) here, I think
>> this is just glibc implementation detail. So I would suggest
>> allocating with mmap and freeing with munmap.
>
> The MAP_FIXED itself should be sufficient to invalidate any prior vma at
> that address, so we don't depend upon the free() here to zap

Yeah, but does the free trigger an invalidate, or does it not? Or in 
other words, is the one from MAP_FIXED the first one, or not? Better to 
be explicit than undefined in the test case, that was my point.

> userptr->pages. In this instance userptr->pages doesn't get populated
> before the MAP_FIXED anyway.

Hmmm... did not even think about that. Why does MAP_FIXED trigger get_pages?

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

Re: [PATCH] tests/gem_userptr_blits: subtests for MAP_FIXED mappings of regular bo

[Chris Wilson]

On Mon, Jun 29, 2015 at 03:15:12PM +0100, Tvrtko Ursulin wrote:
> 
> On 06/29/2015 03:07 PM, Chris Wilson wrote:
> >On Mon, Jun 29, 2015 at 03:01:12PM +0100, Tvrtko Ursulin wrote:
> >>
> >>On 06/29/2015 11:59 AM, Michał Winiarski wrote:
> >>>When the the memory backing the userptr object is freed by the user, it's
> >>>possible to trigger recursive deadlock caused by operations done on
> >>>different BO mapped in that region, triggering invalidate.
> >>>
> >>>Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
> >>>---
> >>>  tests/gem_userptr_blits.c | 83 +++++++++++++++++++++++++++++++++++++++++++++++
> >>>  1 file changed, 83 insertions(+)
> >>>
> >>>diff --git a/tests/gem_userptr_blits.c b/tests/gem_userptr_blits.c
> >>>index 1f2cc96..3fe8f90 100644
> >>>--- a/tests/gem_userptr_blits.c
> >>>+++ b/tests/gem_userptr_blits.c
> >>>@@ -640,6 +640,80 @@ static void test_forked_access(int fd)
> >>>  	free(ptr2);
> >>>  }
> >>>
> >>>+static int test_map_fixed_invalidate(int fd, bool overlap)
> >>>+{
> >>>+	void *ptr;
> >>>+	void *map;
> >>>+	int i;
> >>>+	int num_handles = overlap ? 2 : 1;
> >>>+	uint32_t handle[num_handles];
> >>>+	uint32_t mmap_handle;
> >>>+	struct drm_i915_gem_mmap_gtt mmap_arg;
> >>>+
> >>>+	igt_assert(posix_memalign(&ptr, PAGE_SIZE, PAGE_SIZE) == 0);
> >>>+	for (i=0; i<num_handles; i++)
> >>>+		igt_assert(gem_userptr(fd, ptr, PAGE_SIZE, 0, &handle[i]) == 0);
> >>>+	free(ptr);
> >>
> >>I am not sure we can rely on free triggering munmap(2) here, I think
> >>this is just glibc implementation detail. So I would suggest
> >>allocating with mmap and freeing with munmap.
> >
> >The MAP_FIXED itself should be sufficient to invalidate any prior vma at
> >that address, so we don't depend upon the free() here to zap
> 
> Yeah, but does the free trigger an invalidate, or does it not? Or in
> other words, is the one from MAP_FIXED the first one, or not? Better
> to be explicit than undefined in the test case, that was my point.

Yes. But I would prefer MAP_FIXED to be the first invalidate, but that
looks like we then need to leak the ptr.
 
> >userptr->pages. In this instance userptr->pages doesn't get populated
> >before the MAP_FIXED anyway.
> 
> Hmmm... did not even think about that. Why does MAP_FIXED trigger get_pages?

MAP_FIXED doesn't. Part of the tests is to make sure that MAP_FIXED
places a GTT object over top of the userptr is that the userptr then
reports EFAULT when used. My worrying about obj->pages prior to the
invalidation is that there are few paths through cancel_userptr() that
depend upon whether the object has been used (i.e. has pages) or whether
the object is active on the GPU. That's the purpose of doing a copy or
set-domain or nothing prior to the MAP_FIXED.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] tests/gem_userptr_blits: subtests for MAP_FIXED mappings of regular bo

[Tvrtko Ursulin]

On 06/29/2015 03:25 PM, Chris Wilson wrote:
> On Mon, Jun 29, 2015 at 03:15:12PM +0100, Tvrtko Ursulin wrote:
>>
>> On 06/29/2015 03:07 PM, Chris Wilson wrote:
>>> On Mon, Jun 29, 2015 at 03:01:12PM +0100, Tvrtko Ursulin wrote:
>>>>
>>>> On 06/29/2015 11:59 AM, Michał Winiarski wrote:
>>>>> When the the memory backing the userptr object is freed by the user, it's
>>>>> possible to trigger recursive deadlock caused by operations done on
>>>>> different BO mapped in that region, triggering invalidate.
>>>>>
>>>>> Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
>>>>> ---
>>>>>   tests/gem_userptr_blits.c | 83 +++++++++++++++++++++++++++++++++++++++++++++++
>>>>>   1 file changed, 83 insertions(+)
>>>>>
>>>>> diff --git a/tests/gem_userptr_blits.c b/tests/gem_userptr_blits.c
>>>>> index 1f2cc96..3fe8f90 100644
>>>>> --- a/tests/gem_userptr_blits.c
>>>>> +++ b/tests/gem_userptr_blits.c
>>>>> @@ -640,6 +640,80 @@ static void test_forked_access(int fd)
>>>>>   	free(ptr2);
>>>>>   }
>>>>>
>>>>> +static int test_map_fixed_invalidate(int fd, bool overlap)
>>>>> +{
>>>>> +	void *ptr;
>>>>> +	void *map;
>>>>> +	int i;
>>>>> +	int num_handles = overlap ? 2 : 1;
>>>>> +	uint32_t handle[num_handles];
>>>>> +	uint32_t mmap_handle;
>>>>> +	struct drm_i915_gem_mmap_gtt mmap_arg;
>>>>> +
>>>>> +	igt_assert(posix_memalign(&ptr, PAGE_SIZE, PAGE_SIZE) == 0);
>>>>> +	for (i=0; i<num_handles; i++)
>>>>> +		igt_assert(gem_userptr(fd, ptr, PAGE_SIZE, 0, &handle[i]) == 0);
>>>>> +	free(ptr);
>>>>
>>>> I am not sure we can rely on free triggering munmap(2) here, I think
>>>> this is just glibc implementation detail. So I would suggest
>>>> allocating with mmap and freeing with munmap.
>>>
>>> The MAP_FIXED itself should be sufficient to invalidate any prior vma at
>>> that address, so we don't depend upon the free() here to zap
>>
>> Yeah, but does the free trigger an invalidate, or does it not? Or in
>> other words, is the one from MAP_FIXED the first one, or not? Better
>> to be explicit than undefined in the test case, that was my point.
>
> Yes. But I would prefer MAP_FIXED to be the first invalidate, but that
> looks like we then need to leak the ptr.

If it used mmap instead of posix_memalign and no free then what would it 
leak? MAP_FIXED would be guaranteed first invalidate and it would 
discard the mapping.

Tvrtko



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

Re: [PATCH] tests/gem_userptr_blits: subtests for MAP_FIXED mappings of regular bo

[Chris Wilson]

On Mon, Jun 29, 2015 at 03:56:07PM +0100, Tvrtko Ursulin wrote:
> >Yes. But I would prefer MAP_FIXED to be the first invalidate, but that
> >looks like we then need to leak the ptr.
> 
> If it used mmap instead of posix_memalign and no free then what
> would it leak? MAP_FIXED would be guaranteed first invalidate and it
> would discard the mapping.

Nothing, good idea.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2] tests/gem_userptr_blits: subtests for MAP_FIXED mappings of regular BO

[Michał Winiarski]

When the memory backing the userptr object is freed by the user, but the
BO itself is not closed, it's possible to trigger recursive deadlock
caused by operations done on different BO mapped in that region.
Testcases are simulating such behaviour by using MAP_FIXED mmap flag.

v2: Grammar/naming fixes, s/posix_memalign/mmap (Tvrtko), merge tests into
single function, call set_tiling after get_pages, comments, GUP slowpath

Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 tests/gem_userptr_blits.c | 97 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 97 insertions(+)

diff --git a/tests/gem_userptr_blits.c b/tests/gem_userptr_blits.c
index 1f2cc96..04f6fa9 100644
--- a/tests/gem_userptr_blits.c
+++ b/tests/gem_userptr_blits.c
@@ -640,6 +640,93 @@ static void test_forked_access(int fd)
 	free(ptr2);
 }
 
+#define MAP_FIXED_INVALIDATE_OVERLAP		(1<<0)
+#define MAP_FIXED_INVALIDATE_BUSY			(1<<1)
+#define MAP_FIXED_INVALIDATE_GET_PAGES		(1<<2)
+#define MAP_FIXED_INVALIDATE_FILE_BACKED	(1<<3)
+#define ALL_MAP_FIXED_INVALIDATE (MAP_FIXED_INVALIDATE_OVERLAP | \
+		MAP_FIXED_INVALIDATE_BUSY | \
+		MAP_FIXED_INVALIDATE_GET_PAGES | \
+		MAP_FIXED_INVALIDATE_FILE_BACKED)
+
+static void map_fixed_invalidate_drop_caches(int fd)
+{
+	if (fd != -1) {
+		igt_assert(fsync(fd) == 0);
+		intel_purge_vm_caches();
+	}
+}
+
+static int test_map_fixed_invalidate(int fd, uint32_t flags)
+{
+	void *ptr;
+	size_t ptr_size = sizeof(linear);
+	void *map;
+	int i;
+	int num_handles = (flags & MAP_FIXED_INVALIDATE_OVERLAP) ? 2 : 1;
+	int userptr_fd = -1;
+	char userptr_fd_name[32];
+	uint32_t handle[num_handles];
+	uint32_t mmap_handle;
+	struct drm_i915_gem_mmap_gtt mmap_arg;
+	struct drm_i915_gem_set_domain set_domain;
+
+	//We can excercise mmap with fd to trigger slowpath on GUP
+	if (flags & MAP_FIXED_INVALIDATE_FILE_BACKED) {
+		snprintf(userptr_fd_name, 32, "igt.XXXXXX");
+		userptr_fd = mkstemp(userptr_fd_name);
+		igt_assert(userptr_fd != -1);
+		igt_assert(unlink(userptr_fd_name) == 0);
+		igt_assert(ftruncate(userptr_fd, ptr_size) == 0);
+		ptr = mmap(NULL, ptr_size, PROT_READ | PROT_WRITE, MAP_SHARED,
+				userptr_fd, 0);
+	}
+	else
+		ptr = mmap(NULL, ptr_size, PROT_READ | PROT_WRITE, MAP_SHARED |
+				MAP_ANONYMOUS, -1, 0);
+	igt_assert(ptr != MAP_FAILED);
+
+	for (i=0; i<num_handles; i++)
+		handle[i] = create_userptr(fd, 0, ptr);
+	mmap_handle = gem_create(fd, PAGE_SIZE);
+
+	if (flags & MAP_FIXED_INVALIDATE_BUSY) {
+		map_fixed_invalidate_drop_caches(userptr_fd);
+		copy(fd, handle[0], handle[num_handles-1], 0);
+		map_fixed_invalidate_drop_caches(userptr_fd);
+	}
+
+	//Map regular buffer in userptr range, triggering invalidate
+	memset(&mmap_arg, 0, sizeof(mmap_arg));
+	mmap_arg.handle = mmap_handle;
+	do_ioctl(fd, DRM_IOCTL_I915_GEM_MMAP_GTT, &mmap_arg);
+	map = mmap(ptr, PAGE_SIZE, PROT_READ | PROT_WRITE, MAP_SHARED | MAP_FIXED,
+			fd, mmap_arg.offset);
+	igt_assert(map != MAP_FAILED);
+
+	//Make sure the obj is fault_mappable
+	*(uint32_t*)map = 0xdead;
+
+	//Trigger get_pages on stale userptr
+	if (flags & MAP_FIXED_INVALIDATE_GET_PAGES) {
+		memset(&set_domain, 0, sizeof(set_domain));
+		set_domain.handle = handle[0];
+		set_domain.read_domains = I915_GEM_DOMAIN_GTT;
+		set_domain.write_domain = I915_GEM_DOMAIN_GTT;
+		igt_assert((drmIoctl(fd, DRM_IOCTL_I915_GEM_SET_DOMAIN, &set_domain) != 0) &&
+				errno == EFAULT);
+	}
+
+	gem_set_tiling(fd, mmap_handle, 2, 512 * 4);
+
+	munmap(ptr, ptr_size);
+	for (i=0; i<num_handles; i++)
+		gem_close(fd, handle[i]);
+	gem_close(fd, mmap_handle);
+
+	return 0;
+}
+
 static int test_forbidden_ops(int fd)
 {
 	struct drm_i915_gem_pread gem_pread;
@@ -1489,6 +1576,16 @@ int main(int argc, char **argv)
 	igt_subtest("stress-mm-invalidate-close-overlap")
 		test_invalidate_close_race(fd, true);
 
+	for (unsigned flags = 0; flags < ALL_MAP_FIXED_INVALIDATE + 1; flags++) {
+		igt_subtest_f("map-fixed-invalidate%s%s%s%s",
+			flags & MAP_FIXED_INVALIDATE_OVERLAP ? "-overlap" : "",
+			flags & MAP_FIXED_INVALIDATE_BUSY ? "-busy" : "",
+			flags & MAP_FIXED_INVALIDATE_GET_PAGES ? "-gup" : "",
+			flags & MAP_FIXED_INVALIDATE_FILE_BACKED ? "-file" : "") {
+			test_map_fixed_invalidate(fd, flags);
+		}
+	}
+
 	igt_subtest("coherency-sync")
 		test_coherency(fd, count);
 
-- 
2.4.3

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

[PATCH 1/3] drm/i915: Only update the current userptr worker

[Chris Wilson]

The userptr worker allows for a slight race condition where upon there
may two or more threads calling get_user_pages for the same object. When
we have the array of pages, then we serialise the update of the object.
However, the worker should only overwrite the obj->userptr.work pointer
if and only if it is the active one. Currently we clear it for a
secondary worker with the effect that we may rarely force a second
lookup.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_gem_userptr.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_userptr.c b/drivers/gpu/drm/i915/i915_gem_userptr.c
index 7a5242cd5ea5..cb367d9f7909 100644
--- a/drivers/gpu/drm/i915/i915_gem_userptr.c
+++ b/drivers/gpu/drm/i915/i915_gem_userptr.c
@@ -581,17 +581,17 @@ __i915_gem_userptr_get_pages_worker(struct work_struct *_work)
 	}
 
 	mutex_lock(&dev->struct_mutex);
-	if (obj->userptr.work != &work->work) {
-		ret = 0;
-	} else if (pinned == num_pages) {
-		ret = st_set_pages(&obj->pages, pvec, num_pages);
-		if (ret == 0) {
-			list_add_tail(&obj->global_list, &to_i915(dev)->mm.unbound_list);
-			pinned = 0;
+	if (obj->userptr.work == &work->work) {
+		if (pinned == num_pages) {
+			ret = st_set_pages(&obj->pages, pvec, num_pages);
+			if (ret == 0) {
+				list_add_tail(&obj->global_list, &to_i915(dev)->mm.unbound_list);
+				pinned = 0;
+			}
 		}
+		obj->userptr.work = ERR_PTR(ret);
 	}
 
-	obj->userptr.work = ERR_PTR(ret);
 	obj->userptr.workers--;
 	drm_gem_object_unreference(&obj->base);
 	mutex_unlock(&dev->struct_mutex);
-- 
2.1.4

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

[PATCH 2/3] drm/i915: Fix userptr deadlock with MAP_FIXED

[Chris Wilson]

Michał Winiarski found a really evil way to trigger a struct_mutex
deadlock with userptr. He found that if he allocated a userptr bo and
then GTT mmaped another bo, or even itself, at the same address as the
userptr using MAP_FIXED, he could then cause a deadlock any time we then
had to invalidate the GTT mmappings (so at will).

To counter act the deadlock, we make the observation that when the
MAP_FIXED is made we would have an invalidate_range event for our
object. After that we should no longer alias with the rogue mmapping. If
we are then able to mark the object as no longer in use after the first
invalidate, we do not need to grab the struct_mutex for the subsequent
invalidations.

The patch makes one eye-catching change. That is the removal serial=0
after detecting a to-be-freed object inside the invalidate walker. I
felt setting serial=0 was a questionable pessimisation: it denies us the
chance to reuse the current iterator for the next loop (before it is
freed) and being explicit makes the reader question the validity of the
locking (since the object-free race could occur elsewhere). The
serialisation of the iterator is through the spinlock, if the object is
freed before the next loop then the notifier.serial will be incremented
and we start the walk from the beginning as we detect the invalid cache.

v2: Grammar fixes
v3: Reorder set-active so that it is only set when obj->pages is set
(and so needs cancellation). Only the order of setting obj->pages and
the active-flag is crucial. Calling gup after invalidate-range begin
means the userptr sees the new set of backing storage (and so will not
need to invalidate its new pages), but we have to be careful not to set
the active-flag prior to successfully establishing obj->pages.

Reported-by: Michał Winiarski <michal.winiarski@intel.com>
Testcase: igt/gem_userptr_blits/map-fixed*
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Michał Winiarski <michal.winiarski@intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: stable@vger.kernel.org
---
 drivers/gpu/drm/i915/i915_gem_userptr.c | 43 +++++++++++++++++++++++++++------
 1 file changed, 36 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_userptr.c b/drivers/gpu/drm/i915/i915_gem_userptr.c
index cb367d9f7909..d3213fdefafc 100644
--- a/drivers/gpu/drm/i915/i915_gem_userptr.c
+++ b/drivers/gpu/drm/i915/i915_gem_userptr.c
@@ -59,6 +59,7 @@ struct i915_mmu_object {
 	struct interval_tree_node it;
 	struct list_head link;
 	struct drm_i915_gem_object *obj;
+	bool active;
 	bool is_linear;
 };
 
@@ -114,7 +115,8 @@ restart:
 
 		obj = mo->obj;
 
-		if (!kref_get_unless_zero(&obj->base.refcount))
+		if (!mo->active ||
+		    !kref_get_unless_zero(&obj->base.refcount))
 			continue;
 
 		spin_unlock(&mn->lock);
@@ -151,7 +153,8 @@ static void i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn,
 		else
 			it = interval_tree_iter_first(&mn->objects, start, end);
 		if (it != NULL) {
-			obj = container_of(it, struct i915_mmu_object, it)->obj;
+			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
@@ -160,11 +163,9 @@ static void i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn,
 			 * the struct_mutex - and consequently use it after it
 			 * is freed and then double free it.
 			 */
-			if (!kref_get_unless_zero(&obj->base.refcount)) {
-				spin_unlock(&mn->lock);
-				serial = 0;
-				continue;
-			}
+			if (mo->active &&
+			    kref_get_unless_zero(&mo->obj->base.refcount))
+				obj = mo->obj;
 
 			serial = mn->serial;
 		}
@@ -546,6 +547,30 @@ err:
 }
 
 static void
+__i915_gem_userptr_set_active(struct drm_i915_gem_object *obj,
+			      bool value)
+{
+	/* 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;
+
+	spin_lock(&obj->userptr.mmu_object->mn->lock);
+	obj->userptr.mmu_object->active = value;
+	spin_unlock(&obj->userptr.mmu_object->mn->lock);
+#endif
+}
+
+static void
 __i915_gem_userptr_get_pages_worker(struct work_struct *_work)
 {
 	struct get_pages_work *work = container_of(_work, typeof(*work), work);
@@ -585,6 +610,7 @@ __i915_gem_userptr_get_pages_worker(struct work_struct *_work)
 		if (pinned == num_pages) {
 			ret = st_set_pages(&obj->pages, pvec, num_pages);
 			if (ret == 0) {
+				__i915_gem_userptr_set_active(obj, true);
 				list_add_tail(&obj->global_list, &to_i915(dev)->mm.unbound_list);
 				pinned = 0;
 			}
@@ -699,6 +725,7 @@ i915_gem_userptr_get_pages(struct drm_i915_gem_object *obj)
 	} else {
 		ret = st_set_pages(&obj->pages, pvec, num_pages);
 		if (ret == 0) {
+			__i915_gem_userptr_set_active(obj, true);
 			obj->userptr.work = NULL;
 			pinned = 0;
 		}
@@ -732,6 +759,8 @@ i915_gem_userptr_put_pages(struct drm_i915_gem_object *obj)
 
 	sg_free_table(obj->pages);
 	kfree(obj->pages);
+
+	__i915_gem_userptr_set_active(obj, false);
 }
 
 static void
-- 
2.1.4

Re: [PATCH 2/3] drm/i915: Fix userptr deadlock with MAP_FIXED

[Tvrtko Ursulin]

On 06/30/2015 05:55 PM, Chris Wilson wrote:
> Michał Winiarski found a really evil way to trigger a struct_mutex
> deadlock with userptr. He found that if he allocated a userptr bo and
> then GTT mmaped another bo, or even itself, at the same address as the
> userptr using MAP_FIXED, he could then cause a deadlock any time we then
> had to invalidate the GTT mmappings (so at will).
>
> To counter act the deadlock, we make the observation that when the
> MAP_FIXED is made we would have an invalidate_range event for our
> object. After that we should no longer alias with the rogue mmapping. If
> we are then able to mark the object as no longer in use after the first
> invalidate, we do not need to grab the struct_mutex for the subsequent
> invalidations.
>
> The patch makes one eye-catching change. That is the removal serial=0
> after detecting a to-be-freed object inside the invalidate walker. I
> felt setting serial=0 was a questionable pessimisation: it denies us the
> chance to reuse the current iterator for the next loop (before it is
> freed) and being explicit makes the reader question the validity of the
> locking (since the object-free race could occur elsewhere). The
> serialisation of the iterator is through the spinlock, if the object is
> freed before the next loop then the notifier.serial will be incremented
> and we start the walk from the beginning as we detect the invalid cache.
>
> v2: Grammar fixes
> v3: Reorder set-active so that it is only set when obj->pages is set
> (and so needs cancellation). Only the order of setting obj->pages and
> the active-flag is crucial. Calling gup after invalidate-range begin
> means the userptr sees the new set of backing storage (and so will not
> need to invalidate its new pages), but we have to be careful not to set
> the active-flag prior to successfully establishing obj->pages.
>
> Reported-by: Michał Winiarski <michal.winiarski@intel.com>
> Testcase: igt/gem_userptr_blits/map-fixed*
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Michał Winiarski <michal.winiarski@intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: stable@vger.kernel.org
> ---
>   drivers/gpu/drm/i915/i915_gem_userptr.c | 43 +++++++++++++++++++++++++++------
>   1 file changed, 36 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_userptr.c b/drivers/gpu/drm/i915/i915_gem_userptr.c
> index cb367d9f7909..d3213fdefafc 100644
> --- a/drivers/gpu/drm/i915/i915_gem_userptr.c
> +++ b/drivers/gpu/drm/i915/i915_gem_userptr.c
> @@ -59,6 +59,7 @@ struct i915_mmu_object {
>   	struct interval_tree_node it;
>   	struct list_head link;
>   	struct drm_i915_gem_object *obj;
> +	bool active;
>   	bool is_linear;
>   };
>
> @@ -114,7 +115,8 @@ restart:
>
>   		obj = mo->obj;
>
> -		if (!kref_get_unless_zero(&obj->base.refcount))
> +		if (!mo->active ||
> +		    !kref_get_unless_zero(&obj->base.refcount))
>   			continue;
>
>   		spin_unlock(&mn->lock);
> @@ -151,7 +153,8 @@ static void i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn,
>   		else
>   			it = interval_tree_iter_first(&mn->objects, start, end);
>   		if (it != NULL) {
> -			obj = container_of(it, struct i915_mmu_object, it)->obj;
> +			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
> @@ -160,11 +163,9 @@ static void i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn,
>   			 * the struct_mutex - and consequently use it after it
>   			 * is freed and then double free it.
>   			 */
> -			if (!kref_get_unless_zero(&obj->base.refcount)) {
> -				spin_unlock(&mn->lock);
> -				serial = 0;
> -				continue;
> -			}
> +			if (mo->active &&
> +			    kref_get_unless_zero(&mo->obj->base.refcount))
> +				obj = mo->obj;
>
>   			serial = mn->serial;
>   		}
> @@ -546,6 +547,30 @@ err:
>   }
>
>   static void
> +__i915_gem_userptr_set_active(struct drm_i915_gem_object *obj,
> +			      bool value)
> +{
> +	/* 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;
> +
> +	spin_lock(&obj->userptr.mmu_object->mn->lock);
> +	obj->userptr.mmu_object->active = value;
> +	spin_unlock(&obj->userptr.mmu_object->mn->lock);
> +#endif

Would this be more obvious as atomic_t? Since I suspect spinlock is just 
for the memory barrier, if that. Hm.. What if we get invalidate while 
get_pages is running, before it set active but after gup has succeeded?

> +
> +static void
>   __i915_gem_userptr_get_pages_worker(struct work_struct *_work)
>   {
>   	struct get_pages_work *work = container_of(_work, typeof(*work), work);
> @@ -585,6 +610,7 @@ __i915_gem_userptr_get_pages_worker(struct work_struct *_work)
>   		if (pinned == num_pages) {
>   			ret = st_set_pages(&obj->pages, pvec, num_pages);
>   			if (ret == 0) {
> +				__i915_gem_userptr_set_active(obj, true);
>   				list_add_tail(&obj->global_list, &to_i915(dev)->mm.unbound_list);
>   				pinned = 0;
>   			}
> @@ -699,6 +725,7 @@ i915_gem_userptr_get_pages(struct drm_i915_gem_object *obj)
>   	} else {
>   		ret = st_set_pages(&obj->pages, pvec, num_pages);
>   		if (ret == 0) {
> +			__i915_gem_userptr_set_active(obj, true);
>   			obj->userptr.work = NULL;
>   			pinned = 0;
>   		}
> @@ -732,6 +759,8 @@ i915_gem_userptr_put_pages(struct drm_i915_gem_object *obj)
>
>   	sg_free_table(obj->pages);
>   	kfree(obj->pages);
> +
> +	__i915_gem_userptr_set_active(obj, false);
>   }
>
>   static void
>

Regards,

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

Re: [Intel-gfx] [PATCH 2/3] drm/i915: Fix userptr deadlock with MAP_FIXED

[Chris Wilson]

On Wed, Jul 01, 2015 at 12:14:34PM +0100, Tvrtko Ursulin wrote:
> >  static void
> >+__i915_gem_userptr_set_active(struct drm_i915_gem_object *obj,
> >+			      bool value)
> >+{
> >+	/* 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;
> >+
> >+	spin_lock(&obj->userptr.mmu_object->mn->lock);
> >+	obj->userptr.mmu_object->active = value;
> >+	spin_unlock(&obj->userptr.mmu_object->mn->lock);
> >+#endif
> 
> Would this be more obvious as atomic_t? Since I suspect spinlock is
> just for the memory barrier, if that.

Yes, you could probably get away with a little thought and just a memory
barrier. But since one side is guarded by the spinlock, I think it is
easier to reuse that. Especially when the expression becomes a little
more complicated.

> Hm.. What if we get invalidate
> while get_pages is running, before it set active but after gup has
> succeeded?

Yeah, that actually is a probably. Especially thinking about the
gup_worker which we need to cancel.

Back to setting active earlier and clearing it along error paths.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

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

[Chris Wilson]

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(-)

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
-- 
2.1.4

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

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

[Tvrtko Ursulin]

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Chris Wilson]

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

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[shuang.he]

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 '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH] drm/i915: Use a task to cancel the userptr on invalidate_range

[Chris Wilson]

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's page 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. The only race condition that this worsens is remapping
an userptr active on the GPU where fresh work may still reference the
old pages due to struct_mutex contention. Given that userspace is racing
with the GPU, it is fair to say that the results are undefined.

v2: Only queue (and importantly only take one refcnt) the worker once.

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, 57 insertions(+), 85 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_userptr.c b/drivers/gpu/drm/i915/i915_gem_userptr.c
index d3213fdefafc..6f55c874e65b 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,28 @@ 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);
+		/* only schedule one work packet to avoid the refleak */
+		mo->active = false;
 	}
 
-	return NULL;
+	return end;
 }
 
 static void i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn,
@@ -136,45 +119,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;
 
-			serial = mn->serial;
+	/* interval ranges are inclusive, but invalidate range is exclusive */
+	end--;
+
+	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 +164,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 +177,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 +223,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 +253,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 +319,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 +508,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 +526,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
-- 
2.1.4

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

Re: [PATCH 1/3] drm/i915: Only update the current userptr worker

[Tvrtko Ursulin]

On 06/30/2015 05:55 PM, Chris Wilson wrote:
> The userptr worker allows for a slight race condition where upon there
> may two or more threads calling get_user_pages for the same object. When
> we have the array of pages, then we serialise the update of the object.
> However, the worker should only overwrite the obj->userptr.work pointer
> if and only if it is the active one. Currently we clear it for a
> secondary worker with the effect that we may rarely force a second
> lookup.

Secondary worker can fire only if invalidate clears the current one, no? 
(if (obj->userptr.work == NULL && ...))

It then "cancels" the worker so that the st_set_pages path is avoided.

> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>   drivers/gpu/drm/i915/i915_gem_userptr.c | 16 ++++++++--------
>   1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_userptr.c b/drivers/gpu/drm/i915/i915_gem_userptr.c
> index 7a5242cd5ea5..cb367d9f7909 100644
> --- a/drivers/gpu/drm/i915/i915_gem_userptr.c
> +++ b/drivers/gpu/drm/i915/i915_gem_userptr.c
> @@ -581,17 +581,17 @@ __i915_gem_userptr_get_pages_worker(struct work_struct *_work)
>   	}
>
>   	mutex_lock(&dev->struct_mutex);
> -	if (obj->userptr.work != &work->work) {
> -		ret = 0;
> -	} else if (pinned == num_pages) {
> -		ret = st_set_pages(&obj->pages, pvec, num_pages);
> -		if (ret == 0) {
> -			list_add_tail(&obj->global_list, &to_i915(dev)->mm.unbound_list);
> -			pinned = 0;
> +	if (obj->userptr.work == &work->work) {
> +		if (pinned == num_pages) {
> +			ret = st_set_pages(&obj->pages, pvec, num_pages);
> +			if (ret == 0) {
> +				list_add_tail(&obj->global_list, &to_i915(dev)->mm.unbound_list);
> +				pinned = 0;
> +			}
>   		}
> +		obj->userptr.work = ERR_PTR(ret);
>   	}
>
> -	obj->userptr.work = ERR_PTR(ret);
>   	obj->userptr.workers--;
>   	drm_gem_object_unreference(&obj->base);
>   	mutex_unlock(&dev->struct_mutex);

Previously the canceled worker would allow another worker to be created 
in case it failed (obj->userptr.work != &work->work; ret = 0;) and now 
it still does since obj->userptr.work remains at NULL from cancellation.

Both seem wrong, am I missing the change?

Regards,

Tvrtko




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

Re: [PATCH 1/3] drm/i915: Only update the current userptr worker

[Chris Wilson]

On Wed, Jul 01, 2015 at 10:48:59AM +0100, Tvrtko Ursulin wrote:
> 
> On 06/30/2015 05:55 PM, Chris Wilson wrote:
> >The userptr worker allows for a slight race condition where upon there
> >may two or more threads calling get_user_pages for the same object. When
> >we have the array of pages, then we serialise the update of the object.
> >However, the worker should only overwrite the obj->userptr.work pointer
> >if and only if it is the active one. Currently we clear it for a
> >secondary worker with the effect that we may rarely force a second
> >lookup.
> 
> Secondary worker can fire only if invalidate clears the current one,
> no? (if (obj->userptr.work == NULL && ...))
> 
> It then "cancels" the worker so that the st_set_pages path is avoided.

I may have overegged the changelog, but what I did not like here was
that we would touch obj->userptr.work when we clearly had lost ownership
of that field.
 
> >Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> >---
> >  drivers/gpu/drm/i915/i915_gem_userptr.c | 16 ++++++++--------
> >  1 file changed, 8 insertions(+), 8 deletions(-)
> >
> >diff --git a/drivers/gpu/drm/i915/i915_gem_userptr.c b/drivers/gpu/drm/i915/i915_gem_userptr.c
> >index 7a5242cd5ea5..cb367d9f7909 100644
> >--- a/drivers/gpu/drm/i915/i915_gem_userptr.c
> >+++ b/drivers/gpu/drm/i915/i915_gem_userptr.c
> >@@ -581,17 +581,17 @@ __i915_gem_userptr_get_pages_worker(struct work_struct *_work)
> >  	}
> >
> >  	mutex_lock(&dev->struct_mutex);
> >-	if (obj->userptr.work != &work->work) {
> >-		ret = 0;
> >-	} else if (pinned == num_pages) {
> >-		ret = st_set_pages(&obj->pages, pvec, num_pages);
> >-		if (ret == 0) {
> >-			list_add_tail(&obj->global_list, &to_i915(dev)->mm.unbound_list);
> >-			pinned = 0;
> >+	if (obj->userptr.work == &work->work) {
> >+		if (pinned == num_pages) {
> >+			ret = st_set_pages(&obj->pages, pvec, num_pages);
> >+			if (ret == 0) {
> >+				list_add_tail(&obj->global_list, &to_i915(dev)->mm.unbound_list);
> >+				pinned = 0;
> >+			}
> >  		}
> >+		obj->userptr.work = ERR_PTR(ret);
> >  	}
> >
> >-	obj->userptr.work = ERR_PTR(ret);
> >  	obj->userptr.workers--;
> >  	drm_gem_object_unreference(&obj->base);
> >  	mutex_unlock(&dev->struct_mutex);
> 
> Previously the canceled worker would allow another worker to be
> created in case it failed (obj->userptr.work != &work->work; ret =
> 0;) and now it still does since obj->userptr.work remains at NULL
> from cancellation.
> 
> Both seem wrong, am I missing the change?

No, the obj->userptr.work must remain NULL until a new get_pages()
because we don't actually know if this worker's gup was before or after
the cancellation  - mmap_sem vs struct_mutex ordering.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/3] drm/i915: Only update the current userptr worker

[Tvrtko Ursulin]

On 07/01/2015 10:59 AM, Chris Wilson wrote:
> On Wed, Jul 01, 2015 at 10:48:59AM +0100, Tvrtko Ursulin wrote:
>>
>> On 06/30/2015 05:55 PM, Chris Wilson wrote:
>>> The userptr worker allows for a slight race condition where upon there
>>> may two or more threads calling get_user_pages for the same object. When
>>> we have the array of pages, then we serialise the update of the object.
>>> However, the worker should only overwrite the obj->userptr.work pointer
>>> if and only if it is the active one. Currently we clear it for a
>>> secondary worker with the effect that we may rarely force a second
>>> lookup.
>>
>> Secondary worker can fire only if invalidate clears the current one,
>> no? (if (obj->userptr.work == NULL && ...))
>>
>> It then "cancels" the worker so that the st_set_pages path is avoided.
>
> I may have overegged the changelog, but what I did not like here was
> that we would touch obj->userptr.work when we clearly had lost ownership
> of that field.

Yes that part makes sense.

>>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>>> ---
>>>   drivers/gpu/drm/i915/i915_gem_userptr.c | 16 ++++++++--------
>>>   1 file changed, 8 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_gem_userptr.c b/drivers/gpu/drm/i915/i915_gem_userptr.c
>>> index 7a5242cd5ea5..cb367d9f7909 100644
>>> --- a/drivers/gpu/drm/i915/i915_gem_userptr.c
>>> +++ b/drivers/gpu/drm/i915/i915_gem_userptr.c
>>> @@ -581,17 +581,17 @@ __i915_gem_userptr_get_pages_worker(struct work_struct *_work)
>>>   	}
>>>
>>>   	mutex_lock(&dev->struct_mutex);
>>> -	if (obj->userptr.work != &work->work) {
>>> -		ret = 0;
>>> -	} else if (pinned == num_pages) {
>>> -		ret = st_set_pages(&obj->pages, pvec, num_pages);
>>> -		if (ret == 0) {
>>> -			list_add_tail(&obj->global_list, &to_i915(dev)->mm.unbound_list);
>>> -			pinned = 0;
>>> +	if (obj->userptr.work == &work->work) {
>>> +		if (pinned == num_pages) {
>>> +			ret = st_set_pages(&obj->pages, pvec, num_pages);
>>> +			if (ret == 0) {
>>> +				list_add_tail(&obj->global_list, &to_i915(dev)->mm.unbound_list);
>>> +				pinned = 0;
>>> +			}
>>>   		}
>>> +		obj->userptr.work = ERR_PTR(ret);
>>>   	}
>>>
>>> -	obj->userptr.work = ERR_PTR(ret);
>>>   	obj->userptr.workers--;
>>>   	drm_gem_object_unreference(&obj->base);
>>>   	mutex_unlock(&dev->struct_mutex);
>>
>> Previously the canceled worker would allow another worker to be
>> created in case it failed (obj->userptr.work != &work->work; ret =
>> 0;) and now it still does since obj->userptr.work remains at NULL
>> from cancellation.
>>
>> Both seem wrong, am I missing the change?
>
> No, the obj->userptr.work must remain NULL until a new get_pages()
> because we don't actually know if this worker's gup was before or after
> the cancellation  - mmap_sem vs struct_mutex ordering.

No one is not wrong, or no I was not missing the change?

I am thinking more and more that we should just mark it canceled forever 
and not allow get_pages to succeed ever since.

Regards,

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

Re: [PATCH 1/3] drm/i915: Only update the current userptr worker

[Chris Wilson]

On Wed, Jul 01, 2015 at 11:58:46AM +0100, Tvrtko Ursulin wrote:
> On 07/01/2015 10:59 AM, Chris Wilson wrote:
> >On Wed, Jul 01, 2015 at 10:48:59AM +0100, Tvrtko Ursulin wrote:
> >>Previously the canceled worker would allow another worker to be
> >>created in case it failed (obj->userptr.work != &work->work; ret =
> >>0;) and now it still does since obj->userptr.work remains at NULL
> >>from cancellation.
> >>
> >>Both seem wrong, am I missing the change?
> >
> >No, the obj->userptr.work must remain NULL until a new get_pages()
> >because we don't actually know if this worker's gup was before or after
> >the cancellation  - mmap_sem vs struct_mutex ordering.
> 
> No one is not wrong, or no I was not missing the change?

The only change is that we don't change the value of userptr.work if it
is set to something else. The only time it should be different was if it
had been cancelled and so NULL. The patch just makes it so that a coding
error is less damaging - and I think easier to read because of that.
 
> I am thinking more and more that we should just mark it canceled
> forever and not allow get_pages to succeed ever since.

Yes, I toyed with that yesterday in response to you being able to alias
a GTT mmap address with the userptr after munmap(userptr.ptr). The
problem is that cancel_userptr() is caller for any change in the CPU
PTE's, including mprotect() or cow after forking. Both of those are
valid situations where we want to keep the userptr around, but with a
new gup.

It's tricky to know what the right thing to do is. For example, another
quirk is that we can recover a failed get_pages() by repeatedly invoking
it after a new aliasing. Again, I'm not sure if the current behaviour is
a little too lax.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/3] drm/i915: Only update the current userptr worker

[Tvrtko Ursulin]

On 07/01/2015 12:09 PM, Chris Wilson wrote:
> On Wed, Jul 01, 2015 at 11:58:46AM +0100, Tvrtko Ursulin wrote:
>> On 07/01/2015 10:59 AM, Chris Wilson wrote:
>>> On Wed, Jul 01, 2015 at 10:48:59AM +0100, Tvrtko Ursulin wrote:
>>>> Previously the canceled worker would allow another worker to be
>>>> created in case it failed (obj->userptr.work != &work->work; ret =
>>>> 0;) and now it still does since obj->userptr.work remains at NULL
>>> >from cancellation.
>>>>
>>>> Both seem wrong, am I missing the change?
>>>
>>> No, the obj->userptr.work must remain NULL until a new get_pages()
>>> because we don't actually know if this worker's gup was before or after
>>> the cancellation  - mmap_sem vs struct_mutex ordering.
>>
>> No one is not wrong, or no I was not missing the change?
>
> The only change is that we don't change the value of userptr.work if it
> is set to something else. The only time it should be different was if it
> had been cancelled and so NULL. The patch just makes it so that a coding
> error is less damaging - and I think easier to read because of that.
>
>> I am thinking more and more that we should just mark it canceled
>> forever and not allow get_pages to succeed ever since.
>
> Yes, I toyed with that yesterday in response to you being able to alias
> a GTT mmap address with the userptr after munmap(userptr.ptr). The
> problem is that cancel_userptr() is caller for any change in the CPU
> PTE's, including mprotect() or cow after forking. Both of those are
> valid situations where we want to keep the userptr around, but with a
> new gup.

Why do we want that? I would be surprised if someone is using it like 
that. How would it be defined on the GEM handle level even?

Regards,

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

Re: [PATCH 1/3] drm/i915: Only update the current userptr worker

[Chris Wilson]

On Wed, Jul 01, 2015 at 01:26:59PM +0100, Tvrtko Ursulin wrote:
> 
> On 07/01/2015 12:09 PM, Chris Wilson wrote:
> >On Wed, Jul 01, 2015 at 11:58:46AM +0100, Tvrtko Ursulin wrote:
> >>On 07/01/2015 10:59 AM, Chris Wilson wrote:
> >>>On Wed, Jul 01, 2015 at 10:48:59AM +0100, Tvrtko Ursulin wrote:
> >>>>Previously the canceled worker would allow another worker to be
> >>>>created in case it failed (obj->userptr.work != &work->work; ret =
> >>>>0;) and now it still does since obj->userptr.work remains at NULL
> >>>>from cancellation.
> >>>>
> >>>>Both seem wrong, am I missing the change?
> >>>
> >>>No, the obj->userptr.work must remain NULL until a new get_pages()
> >>>because we don't actually know if this worker's gup was before or after
> >>>the cancellation  - mmap_sem vs struct_mutex ordering.
> >>
> >>No one is not wrong, or no I was not missing the change?
> >
> >The only change is that we don't change the value of userptr.work if it
> >is set to something else. The only time it should be different was if it
> >had been cancelled and so NULL. The patch just makes it so that a coding
> >error is less damaging - and I think easier to read because of that.
> >
> >>I am thinking more and more that we should just mark it canceled
> >>forever and not allow get_pages to succeed ever since.
> >
> >Yes, I toyed with that yesterday in response to you being able to alias
> >a GTT mmap address with the userptr after munmap(userptr.ptr). The
> >problem is that cancel_userptr() is caller for any change in the CPU
> >PTE's, including mprotect() or cow after forking. Both of those are
> >valid situations where we want to keep the userptr around, but with a
> >new gup.
> 
> Why do we want that? I would be surprised if someone is using it
> like that. How would it be defined on the GEM handle level even?

I would be surprised as well, but it is a race condition we can handle
correctly and succinctly.

The race is just
	bo = userptr(ptr, size);
	set-to-domain(bo);
	mremap(ptr, newptr, size);
	set-to-domain(bo); // or exec(bo);
-Chris


-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/3] drm/i915: Only update the current userptr worker

[Michał Winiarski]

On Tue, Jun 30, 2015 at 05:55:31PM +0100, Chris Wilson wrote:
> The userptr worker allows for a slight race condition where upon there
> may two or more threads calling get_user_pages for the same object. When
> we have the array of pages, then we serialise the update of the object.
> However, the worker should only overwrite the obj->userptr.work pointer
> if and only if it is the active one. Currently we clear it for a
> secondary worker with the effect that we may rarely force a second
> lookup.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

Whole series:
Tested-by: Michał Winiarski <michal.winiarski@intel.com>

> ---
>  drivers/gpu/drm/i915/i915_gem_userptr.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_userptr.c b/drivers/gpu/drm/i915/i915_gem_userptr.c
> index 7a5242cd5ea5..cb367d9f7909 100644
> --- a/drivers/gpu/drm/i915/i915_gem_userptr.c
> +++ b/drivers/gpu/drm/i915/i915_gem_userptr.c
> @@ -581,17 +581,17 @@ __i915_gem_userptr_get_pages_worker(struct work_struct *_work)
>  	}
>  
>  	mutex_lock(&dev->struct_mutex);
> -	if (obj->userptr.work != &work->work) {
> -		ret = 0;
> -	} else if (pinned == num_pages) {
> -		ret = st_set_pages(&obj->pages, pvec, num_pages);
> -		if (ret == 0) {
> -			list_add_tail(&obj->global_list, &to_i915(dev)->mm.unbound_list);
> -			pinned = 0;
> +	if (obj->userptr.work == &work->work) {
> +		if (pinned == num_pages) {
> +			ret = st_set_pages(&obj->pages, pvec, num_pages);
> +			if (ret == 0) {
> +				list_add_tail(&obj->global_list, &to_i915(dev)->mm.unbound_list);
> +				pinned = 0;
> +			}
>  		}
> +		obj->userptr.work = ERR_PTR(ret);
>  	}
>  
> -	obj->userptr.work = ERR_PTR(ret);
>  	obj->userptr.workers--;
>  	drm_gem_object_unreference(&obj->base);
>  	mutex_unlock(&dev->struct_mutex);
> -- 
> 2.1.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/3] drm/i915: Only update the current userptr worker

[Chris Wilson]

On Fri, Jul 03, 2015 at 12:48:03PM +0200, Michał Winiarski wrote:
> On Tue, Jun 30, 2015 at 05:55:31PM +0100, Chris Wilson wrote:
> > The userptr worker allows for a slight race condition where upon there
> > may two or more threads calling get_user_pages for the same object. When
> > we have the array of pages, then we serialise the update of the object.
> > However, the worker should only overwrite the obj->userptr.work pointer
> > if and only if it is the active one. Currently we clear it for a
> > secondary worker with the effect that we may rarely force a second
> > lookup.
> > 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> 
> Whole series:
> Tested-by: Michał Winiarski <michal.winiarski@intel.com>

That reminds me there was a refleak in patch 3 if a second
invalidate-range notification before the first's worker had run (we
would take the ref for the active mo, but since the worker was queued,
it would still only run once and not drop our new ref.)
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx