[Intel-gfx] [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 at 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