Re: [PATCH 3/3] drm/i915/userptr: Probe vma range before gup

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

On 14/01/2019 21:17, Chris Wilson wrote:
> We want to exclude any GGTT objects from being present on our internal
> lists to avoid the deadlock we may run into with our requirement for
> struct_mutex during invalidate. However, if the gup_fast fails, we put
> the userptr onto the workqueue and mark it as active, so that we
> remember to serialise the worker upon mmu_invalidate.
> 
> Note that despite the previous fix, it is still better to avoid the
> struct_mutex recursion where possible, leaving the recursion only to
> handle the shrinker-esque paths.
> 
> v2: Hold mmap_sem to prevent modifications to the mm while we probe and
> add ourselves to the interval-tree for notificiation.
> v3: Rely on mmap_sem for a simpler patch.
> v4: Mark up the mmap_sem nesting
> v5: Don't deactivate on -EAGAIN as that means the worker is queued
> v6: Fight the indentation and chained if-else error handling
> v7: Fight again.
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=104209
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Michał Winiarski <michal.winiarski@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_gem_userptr.c | 150 ++++++++++++++++--------
>   1 file changed, 98 insertions(+), 52 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_userptr.c b/drivers/gpu/drm/i915/i915_gem_userptr.c
> index 9c2008a480e2..7b9e14bc3bc5 100644
> --- a/drivers/gpu/drm/i915/i915_gem_userptr.c
> +++ b/drivers/gpu/drm/i915/i915_gem_userptr.c
> @@ -435,7 +435,7 @@ struct get_pages_work {
>   	struct task_struct *task;
>   };
>   
> -static struct sg_table *
> +static int
>   __i915_gem_userptr_alloc_pages(struct drm_i915_gem_object *obj,
>   			       struct page **pvec, int num_pages)
>   {
> @@ -446,7 +446,7 @@ __i915_gem_userptr_alloc_pages(struct drm_i915_gem_object *obj,
>   
>   	st = kmalloc(sizeof(*st), GFP_KERNEL);
>   	if (!st)
> -		return ERR_PTR(-ENOMEM);
> +		return -ENOMEM;
>   
>   alloc_table:
>   	ret = __sg_alloc_table_from_pages(st, pvec, num_pages,
> @@ -455,7 +455,7 @@ __i915_gem_userptr_alloc_pages(struct drm_i915_gem_object *obj,
>   					  GFP_KERNEL);
>   	if (ret) {
>   		kfree(st);
> -		return ERR_PTR(ret);
> +		return ret;
>   	}
>   
>   	ret = i915_gem_gtt_prepare_pages(obj, st);
> @@ -468,14 +468,14 @@ __i915_gem_userptr_alloc_pages(struct drm_i915_gem_object *obj,
>   		}
>   
>   		kfree(st);
> -		return ERR_PTR(ret);
> +		return ret;
>   	}
>   
>   	sg_page_sizes = i915_sg_page_sizes(st->sgl);
>   
>   	__i915_gem_object_set_pages(obj, st, sg_page_sizes);
>   
> -	return st;
> +	return 0;
>   }
>   
>   static void
> @@ -520,19 +520,14 @@ __i915_gem_userptr_get_pages_worker(struct work_struct *_work)
>   
>   	mutex_lock(&obj->mm.lock);
>   	if (obj->userptr.work == &work->work) {
> -		struct sg_table *pages = ERR_PTR(ret);
> -
>   		if (pinned == npages) {
> -			pages = __i915_gem_userptr_alloc_pages(obj, pvec,
> -							       npages);
> -			if (!IS_ERR(pages)) {
> +			ret = __i915_gem_userptr_alloc_pages(obj, pvec, npages);
> +			if (!ret)
>   				pinned = 0;
> -				pages = NULL;
> -			}
>   		}
>   
> -		obj->userptr.work = ERR_CAST(pages);
> -		if (IS_ERR(pages))
> +		obj->userptr.work = ERR_PTR(ret);
> +		if (ret)
>   			__i915_gem_userptr_set_active(obj, false);
>   	}
>   	mutex_unlock(&obj->mm.lock);
> @@ -545,7 +540,7 @@ __i915_gem_userptr_get_pages_worker(struct work_struct *_work)
>   	kfree(work);
>   }
>   
> -static struct sg_table *
> +static int
>   __i915_gem_userptr_get_pages_schedule(struct drm_i915_gem_object *obj)
>   {
>   	struct get_pages_work *work;
> @@ -571,7 +566,7 @@ __i915_gem_userptr_get_pages_schedule(struct drm_i915_gem_object *obj)
>   	 */
>   	work = kmalloc(sizeof(*work), GFP_KERNEL);
>   	if (work == NULL)
> -		return ERR_PTR(-ENOMEM);
> +		return -ENOMEM;
>   
>   	obj->userptr.work = &work->work;
>   
> @@ -583,19 +578,86 @@ __i915_gem_userptr_get_pages_schedule(struct drm_i915_gem_object *obj)
>   	INIT_WORK(&work->work, __i915_gem_userptr_get_pages_worker);
>   	queue_work(to_i915(obj->base.dev)->mm.userptr_wq, &work->work);
>   
> -	return ERR_PTR(-EAGAIN);
> +	return -EAGAIN;
>   }
>   
> -static int i915_gem_userptr_get_pages(struct drm_i915_gem_object *obj)
> +static int
> +probe_range(struct mm_struct *mm, unsigned long addr, unsigned long len)
> +{
> +	const unsigned long end = addr + len;
> +	struct vm_area_struct *vma;
> +	int ret = -EFAULT;
> +
> +	for (vma = find_vma(mm, addr); vma; vma = vma->vm_next) {
> +		if (vma->vm_start > addr)
> +			break;
> +
> +		/*
> +		 * Exclude any VMA that is not backed only by struct_page, i.e.
> +		 * IO regions that include our own GGTT mmaps. We cannot handle
> +		 * such ranges, as we may encounter deadlocks around our
> +		 * struct_mutex on mmu_invalidate_range.
> +		 */
> +		if (vma->vm_flags & (VM_PFNMAP | VM_MIXEDMAP))
> +			break;
> +
> +		if (vma->vm_end >= end) {
> +			ret = 0;
> +			break;
> +		}
> +
> +		addr = vma->vm_end;
> +	}
> +
> +	return ret;
> +}
> +
> +static int try_fast_gup(struct drm_i915_gem_object *obj)
>   {
>   	const int num_pages = obj->base.size >> PAGE_SHIFT;
> -	struct mm_struct *mm = obj->userptr.mm->mm;
>   	struct page **pvec;
> -	struct sg_table *pages;
> -	bool active;
> -	int pinned;
> +	int pinned, err;
> +
> +	pvec = kvmalloc_array(num_pages, sizeof(struct page *),
> +			      GFP_KERNEL | __GFP_NORETRY | __GFP_NOWARN);
> +	if (!pvec) /* defer to worker if malloc fails */
> +		return -ENOMEM;
> +
> +	pinned = __get_user_pages_fast(obj->userptr.ptr,
> +				       num_pages,
> +				       !i915_gem_object_is_readonly(obj),
> +				       pvec);
> +	if (pinned < 0) {
> +		err = pinned;
> +		pinned = 0;
> +		goto out_pvec;
> +	}
>   
> -	/* If userspace should engineer that these pages are replaced in
> +	if (pinned < num_pages) {
> +		err = -EFAULT;
> +		goto out_pinned;
> +	}
> +
> +	err = __i915_gem_userptr_alloc_pages(obj, pvec, num_pages);
> +	if (err)
> +		goto out_pinned;
> +
> +	__i915_gem_userptr_set_active(obj, true);
> +	pinned = 0;
> +out_pinned:
> +	release_pages(pvec, pinned);
> +out_pvec:
> +	kvfree(pvec);
> +	return err;
> +}
> +
> +static int i915_gem_userptr_get_pages(struct drm_i915_gem_object *obj)
> +{
> +	struct mm_struct *mm = obj->userptr.mm->mm;
> +	int err;
> +
> +	/*
> +	 * 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
>   	 * pages they need to create a new bo to point to the new vma.
> @@ -620,40 +682,24 @@ static int i915_gem_userptr_get_pages(struct drm_i915_gem_object *obj)
>   			return -EAGAIN;
>   	}
>   
> -	pvec = NULL;
> -	pinned = 0;
> +	if (mm == current->mm && try_fast_gup(obj) == 0)
> +		return 0;
>   
> -	if (mm == current->mm) {
> -		pvec = kvmalloc_array(num_pages, sizeof(struct page *),
> -				      GFP_KERNEL |
> -				      __GFP_NORETRY |
> -				      __GFP_NOWARN);
> -		if (pvec) /* defer to worker if malloc fails */
> -			pinned = __get_user_pages_fast(obj->userptr.ptr,
> -						       num_pages,
> -						       !i915_gem_object_is_readonly(obj),
> -						       pvec);
> -	}
> +	/* lockdep doesn't yet automatically allow nesting of readers */
> +	down_read_nested(&mm->mmap_sem, SINGLE_DEPTH_NESTING);
>   
> -	active = false;
> -	if (pinned < 0) {
> -		pages = ERR_PTR(pinned);
> -		pinned = 0;
> -	} else if (pinned < num_pages) {
> -		pages = __i915_gem_userptr_get_pages_schedule(obj);
> -		active = pages == ERR_PTR(-EAGAIN);
> -	} else {
> -		pages = __i915_gem_userptr_alloc_pages(obj, pvec, num_pages);
> -		active = !IS_ERR(pages);
> -	}
> -	if (active)
> +	err = probe_range(mm, obj->userptr.ptr, obj->base.size);

Stick a comment here about how the probe is not a guarantee the mapping 
won't change by the time gup worker runs.

> +	if (err)
> +		goto err_unlock;
> +
> +	err = __i915_gem_userptr_get_pages_schedule(obj);
> +	if (err == -EAGAIN)
>   		__i915_gem_userptr_set_active(obj, true);
>   
> -	if (IS_ERR(pages))
> -		release_pages(pvec, pinned);
> -	kvfree(pvec);
> +err_unlock:
> +	up_read(&mm->mmap_sem);

May be safer to drop the lock earlier, immediately after probe. I don't 
see holding it while queuing the worker and doing internal book-keeping 
is useful and might just create more lock chain dependencies.

Regards,

Tvrtko

>   
> -	return PTR_ERR_OR_ZERO(pages);
> +	return err;
>   }
>   
>   static void
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx