[v3,1/3] drm/i915: Only update the current userptr worker
diff mbox

Message ID 1439196700-20045-1-git-send-email-chris@chris-wilson.co.uk
State New
Headers show

Commit Message

Chris Wilson Aug. 10, 2015, 8:51 a.m. UTC
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 | 32 ++++++++++++++++----------------
 1 file changed, 16 insertions(+), 16 deletions(-)

Comments

Tvrtko Ursulin Sept. 9, 2015, 10:39 a.m. UTC | #1
On 08/10/2015 09:51 AM, 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.

v2 changelog?

> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>   drivers/gpu/drm/i915/i915_gem_userptr.c | 32 ++++++++++++++++----------------
>   1 file changed, 16 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_userptr.c b/drivers/gpu/drm/i915/i915_gem_userptr.c
> index d11901d590ac..800a5394aa1e 100644
> --- a/drivers/gpu/drm/i915/i915_gem_userptr.c
> +++ b/drivers/gpu/drm/i915/i915_gem_userptr.c
> @@ -571,25 +571,25 @@ __i915_gem_userptr_get_pages_worker(struct work_struct *_work)
>   	struct get_pages_work *work = container_of(_work, typeof(*work), work);
>   	struct drm_i915_gem_object *obj = work->obj;
>   	struct drm_device *dev = obj->base.dev;
> -	const int num_pages = obj->base.size >> PAGE_SHIFT;
> +	const int npages = obj->base.size >> PAGE_SHIFT;
>   	struct page **pvec;
>   	int pinned, ret;
>
>   	ret = -ENOMEM;
>   	pinned = 0;
>
> -	pvec = kmalloc(num_pages*sizeof(struct page *),
> +	pvec = kmalloc(npages*sizeof(struct page *),
>   		       GFP_TEMPORARY | __GFP_NOWARN | __GFP_NORETRY);
>   	if (pvec == NULL)
> -		pvec = drm_malloc_ab(num_pages, sizeof(struct page *));
> +		pvec = drm_malloc_ab(npages, sizeof(struct page *));
>   	if (pvec != NULL) {
>   		struct mm_struct *mm = obj->userptr.mm->mm;
>
>   		down_read(&mm->mmap_sem);
> -		while (pinned < num_pages) {
> +		while (pinned < npages) {
>   			ret = get_user_pages(work->task, mm,
>   					     obj->userptr.ptr + pinned * PAGE_SIZE,
> -					     num_pages - pinned,
> +					     npages - pinned,

If you hadn't done this renaming you could have gotten away without a v2 
changelog request... :)

>   					     !obj->userptr.read_only, 0,
>   					     pvec + pinned, NULL);
>   			if (ret < 0)
> @@ -601,20 +601,20 @@ __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 = __i915_gem_userptr_set_pages(obj, pvec, num_pages);
> -		if (ret == 0) {
> -			list_add_tail(&obj->global_list, &to_i915(dev)->mm.unbound_list);
> -			obj->get_page.sg = obj->pages->sgl;
> -			obj->get_page.last = 0;
> -
> -			pinned = 0;
> +	if (obj->userptr.work == &work->work) {
> +		if (pinned == npages) {
> +			ret = __i915_gem_userptr_set_pages(obj, pvec, npages);
> +			if (ret == 0) {
> +				list_add_tail(&obj->global_list,
> +					      &to_i915(dev)->mm.unbound_list);
> +				obj->get_page.sg = obj->pages->sgl;
> +				obj->get_page.last = 0;

Wouldn't obj->get_page init fit better into 
__i915_gem_userptr_set_pages? Although that code is not from this patch. 
How come it is OK not to initialize them in the non-worker case?

With the v2 changelog, or dropped rename:

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

Regards,

Tvrtko
Chris Wilson Sept. 9, 2015, 10:44 a.m. UTC | #2
On Wed, Sep 09, 2015 at 11:39:01AM +0100, Tvrtko Ursulin wrote:
> 
> On 08/10/2015 09:51 AM, 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.
> 
> v2 changelog?
> 
> >Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> >---
> >  drivers/gpu/drm/i915/i915_gem_userptr.c | 32 ++++++++++++++++----------------
> >  1 file changed, 16 insertions(+), 16 deletions(-)
> >
> >diff --git a/drivers/gpu/drm/i915/i915_gem_userptr.c b/drivers/gpu/drm/i915/i915_gem_userptr.c
> >index d11901d590ac..800a5394aa1e 100644
> >--- a/drivers/gpu/drm/i915/i915_gem_userptr.c
> >+++ b/drivers/gpu/drm/i915/i915_gem_userptr.c
> >@@ -571,25 +571,25 @@ __i915_gem_userptr_get_pages_worker(struct work_struct *_work)
> >  	struct get_pages_work *work = container_of(_work, typeof(*work), work);
> >  	struct drm_i915_gem_object *obj = work->obj;
> >  	struct drm_device *dev = obj->base.dev;
> >-	const int num_pages = obj->base.size >> PAGE_SHIFT;
> >+	const int npages = obj->base.size >> PAGE_SHIFT;
> >  	struct page **pvec;
> >  	int pinned, ret;
> >
> >  	ret = -ENOMEM;
> >  	pinned = 0;
> >
> >-	pvec = kmalloc(num_pages*sizeof(struct page *),
> >+	pvec = kmalloc(npages*sizeof(struct page *),
> >  		       GFP_TEMPORARY | __GFP_NOWARN | __GFP_NORETRY);
> >  	if (pvec == NULL)
> >-		pvec = drm_malloc_ab(num_pages, sizeof(struct page *));
> >+		pvec = drm_malloc_ab(npages, sizeof(struct page *));
> >  	if (pvec != NULL) {
> >  		struct mm_struct *mm = obj->userptr.mm->mm;
> >
> >  		down_read(&mm->mmap_sem);
> >-		while (pinned < num_pages) {
> >+		while (pinned < npages) {
> >  			ret = get_user_pages(work->task, mm,
> >  					     obj->userptr.ptr + pinned * PAGE_SIZE,
> >-					     num_pages - pinned,
> >+					     npages - pinned,
> 
> If you hadn't done this renaming you could have gotten away without
> a v2 changelog request... :)

v2: rebase for some recent changes, rename to fix in 80 cols.

> >  					     !obj->userptr.read_only, 0,
> >  					     pvec + pinned, NULL);
> >  			if (ret < 0)
> >@@ -601,20 +601,20 @@ __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 = __i915_gem_userptr_set_pages(obj, pvec, num_pages);
> >-		if (ret == 0) {
> >-			list_add_tail(&obj->global_list, &to_i915(dev)->mm.unbound_list);
> >-			obj->get_page.sg = obj->pages->sgl;
> >-			obj->get_page.last = 0;
> >-
> >-			pinned = 0;
> >+	if (obj->userptr.work == &work->work) {
> >+		if (pinned == npages) {
> >+			ret = __i915_gem_userptr_set_pages(obj, pvec, npages);
> >+			if (ret == 0) {
> >+				list_add_tail(&obj->global_list,
> >+					      &to_i915(dev)->mm.unbound_list);
> >+				obj->get_page.sg = obj->pages->sgl;
> >+				obj->get_page.last = 0;
> 
> Wouldn't obj->get_page init fit better into
> __i915_gem_userptr_set_pages? Although that code is not from this
> patch. How come it is OK not to initialize them in the non-worker
> case?

It's done for us, the worker is the special case. I wanted to write the
set_pages initialiser differently so I could avoid this code, but I did
not prevail.
-Chris

Patch
diff mbox

diff --git a/drivers/gpu/drm/i915/i915_gem_userptr.c b/drivers/gpu/drm/i915/i915_gem_userptr.c
index d11901d590ac..800a5394aa1e 100644
--- a/drivers/gpu/drm/i915/i915_gem_userptr.c
+++ b/drivers/gpu/drm/i915/i915_gem_userptr.c
@@ -571,25 +571,25 @@  __i915_gem_userptr_get_pages_worker(struct work_struct *_work)
 	struct get_pages_work *work = container_of(_work, typeof(*work), work);
 	struct drm_i915_gem_object *obj = work->obj;
 	struct drm_device *dev = obj->base.dev;
-	const int num_pages = obj->base.size >> PAGE_SHIFT;
+	const int npages = obj->base.size >> PAGE_SHIFT;
 	struct page **pvec;
 	int pinned, ret;
 
 	ret = -ENOMEM;
 	pinned = 0;
 
-	pvec = kmalloc(num_pages*sizeof(struct page *),
+	pvec = kmalloc(npages*sizeof(struct page *),
 		       GFP_TEMPORARY | __GFP_NOWARN | __GFP_NORETRY);
 	if (pvec == NULL)
-		pvec = drm_malloc_ab(num_pages, sizeof(struct page *));
+		pvec = drm_malloc_ab(npages, sizeof(struct page *));
 	if (pvec != NULL) {
 		struct mm_struct *mm = obj->userptr.mm->mm;
 
 		down_read(&mm->mmap_sem);
-		while (pinned < num_pages) {
+		while (pinned < npages) {
 			ret = get_user_pages(work->task, mm,
 					     obj->userptr.ptr + pinned * PAGE_SIZE,
-					     num_pages - pinned,
+					     npages - pinned,
 					     !obj->userptr.read_only, 0,
 					     pvec + pinned, NULL);
 			if (ret < 0)
@@ -601,20 +601,20 @@  __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 = __i915_gem_userptr_set_pages(obj, pvec, num_pages);
-		if (ret == 0) {
-			list_add_tail(&obj->global_list, &to_i915(dev)->mm.unbound_list);
-			obj->get_page.sg = obj->pages->sgl;
-			obj->get_page.last = 0;
-
-			pinned = 0;
+	if (obj->userptr.work == &work->work) {
+		if (pinned == npages) {
+			ret = __i915_gem_userptr_set_pages(obj, pvec, npages);
+			if (ret == 0) {
+				list_add_tail(&obj->global_list,
+					      &to_i915(dev)->mm.unbound_list);
+				obj->get_page.sg = obj->pages->sgl;
+				obj->get_page.last = 0;
+				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);