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

[Chris Wilson]

Quoting Chris Wilson (2017-12-15 09:27:27)
> 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.
> 
> 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 | 40 +++++++++++++++++++++++++++++++--
>  1 file changed, 38 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_userptr.c b/drivers/gpu/drm/i915/i915_gem_userptr.c
> index 382a77a1097e..562b869dc750 100644
> --- a/drivers/gpu/drm/i915/i915_gem_userptr.c
> +++ b/drivers/gpu/drm/i915/i915_gem_userptr.c
> @@ -598,6 +598,39 @@ __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;
> +
> +       down_read(&mm->mmap_sem);
> +       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;
> +       }
> +       up_read(&mm->mmap_sem);
> +
> +       return ret;
> +}
> +
>  static int i915_gem_userptr_get_pages(struct drm_i915_gem_object *obj)
>  {
>         const int num_pages = obj->base.size >> PAGE_SHIFT;
> @@ -632,9 +665,12 @@ static int i915_gem_userptr_get_pages(struct drm_i915_gem_object *obj)
>                         return -EAGAIN;
>         }
>  
> -       pvec = NULL;
> -       pinned = 0;
> +       /* Quickly exclude any invalid VMA */
> +       pinned = probe_range(mm, obj->userptr.ptr, obj->base.size);
> +       if (pinned)
> +               return pinned;

There's a nasty race here. Actually the same race we have with the
interval_tree anyway, if we schedule then there's a gap before we add it
to the interval_tree, the state of the mm may have changed with us
noticing or invalidating the pages.

To close that race, we will have to add to active first. Let's see how
that looks.
-Chris