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 _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
