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

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

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