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

[Chris Wilson]

Quoting Tvrtko Ursulin (2019-01-15 10:52:52)
> 
> On 15/01/2019 10:41, Chris Wilson wrote:
> > Quoting Tvrtko Ursulin (2019-01-15 10:27:02)
> >>
> >> On 14/01/2019 21:17, Chris Wilson wrote:
> >>> +     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.
> > 
> > Hmm, I thought we need to cover up to set-active (probe + queue + insert
> > into rbtree) as I thought the mmu-invalidate was under the mmap_sem wlock.
> 
> We have our own lock for set active so I don't see that we need mmap_sem 
> for it. Certainly wasn't needed before this patch so don't see that 
> would change now.

We had it before this patch, by ensuring that we added the rbtree
tracking before we queued the obj for gup, and then serialised inside
the mmu-invalidate.
 
> Btw, what you said regarding nested mmap_sem.. do we have lock inversion 
> with it and obj->mm.lock then? I mean both mmap_sem -> obj->mm.lock and 
> obj->mm.lock -> mmap_sem chains?

Hmm. Yes, there is a recursive read lock here. You want proof that it is
just limited to this single instance!
-Chris