[Intel-gfx] [PATCH v5] drm/i915/userptr: Probe vma range before gup

[Tvrtko Ursulin]

On 15/12/2017 14:48, 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.
> 
> 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
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=104209
> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> Cc: Michał Winiarski <michal.winiarski at intel.com>
> ---
>   drivers/gpu/drm/i915/i915_gem_userptr.c | 55 +++++++++++++++++++++++++++------
>   1 file changed, 45 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_userptr.c b/drivers/gpu/drm/i915/i915_gem_userptr.c
> index 382a77a1097e..b16b8e226e40 100644
> --- a/drivers/gpu/drm/i915/i915_gem_userptr.c
> +++ b/drivers/gpu/drm/i915/i915_gem_userptr.c
> @@ -598,13 +598,43 @@ __i915_gem_userptr_get_pages_schedule(struct drm_i915_gem_object *obj)
>   	return ERR_PTR(-EAGAIN);
>   }
>   
> +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 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 i915_gem_userptr_get_pages(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;
>   
>   	/* If userspace should engineer that these pages are replaced in
> @@ -634,7 +664,6 @@ static int i915_gem_userptr_get_pages(struct drm_i915_gem_object *obj)
>   
>   	pvec = NULL;
>   	pinned = 0;
> -
>   	if (mm == current->mm) {
>   		pvec = kvmalloc_array(num_pages, sizeof(struct page *),
>   				      GFP_KERNEL |
> @@ -647,19 +676,25 @@ static int i915_gem_userptr_get_pages(struct drm_i915_gem_object *obj)
>   						       pvec);
>   	}
>   
> -	active = false;
> +	down_read_nested(&mm->mmap_sem, SINGLE_DEPTH_NESTING);

Big fat comment here? Something about the gem fault path?

>   	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 if (pinned < num_pages &&
> +		   probe_range(mm, obj->userptr.ptr, obj->base.size)) {

Keep going back to remind on the return value semantics. Rename to 
range_invalid or something?

> +		pages = ERR_PTR(-EFAULT);
> +	} else  if (__i915_gem_userptr_set_active(obj, true)) {

Isn't this the normal path now, I mean the path where pinned == 
num_pages? So it sets the active to true, which is a bit evil to do in 
the condition check, and then fall through to the else block to finish 
the job.

I'll see if I can come up with something perhaps more readable in the 
background.

> +		pages = ERR_PTR(-EAGAI >   	} else {
> -		pages = __i915_gem_userptr_alloc_pages(obj, pvec, num_pages);
> -		active = !IS_ERR(pages);
> +		if (pinned < num_pages)
> +			pages = __i915_gem_userptr_get_pages_schedule(obj);
> +		else
> +			pages = __i915_gem_userptr_alloc_pages(obj,
> +							       pvec, num_pages);
> +		if (IS_ERR(pages) && pages != ERR_PTR(-EAGAIN))
> +			__i915_gem_userptr_set_active(obj, false);
>   	}
> -	if (active)
> -		__i915_gem_userptr_set_active(obj, true);
> +	up_read(&mm->mmap_sem);
>   
>   	if (IS_ERR(pages))
>   		release_pages(pvec, pinned);
> 

Regards,

Tvrtko