[Intel-gfx] [PATCH v3 1/3] drm/i915: Only update the current userptr worker

[Tvrtko Ursulin]

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 at 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 at intel.com>

Regards,

Tvrtko