| Message ID | 20191111133205.11590-2-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [FIXES,1/3] drm/i915/userptr: Try to acquire the page lock around set_page_dirty() | expand |
On 11/11/2019 13:32, Chris Wilson wrote: > Enable gup to retry and fault the pages outside of the mmap_sem lock in > our worker. As we are inside our worker, outside of any critical path, > we can allow the mmap_sem lock to be dropped in order to service a page > fault; this in turn allows the mm to populate the page using a slow > fault handler. > > Testcase: igt/gem_userptr/userfault There are no references or explanation on what does this fix? Regards, Tvrtko > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > --- > drivers/gpu/drm/i915/gem/i915_gem_userptr.c | 11 ++++++++--- > 1 file changed, 8 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c > index dd104b0e2071..54ebc7ab71bc 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c > +++ b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c > @@ -459,26 +459,31 @@ __i915_gem_userptr_get_pages_worker(struct work_struct *_work) > if (pvec != NULL) { > struct mm_struct *mm = obj->userptr.mm->mm; > unsigned int flags = 0; > + int locked = 0; > > if (!i915_gem_object_is_readonly(obj)) > flags |= FOLL_WRITE; > > ret = -EFAULT; > if (mmget_not_zero(mm)) { > - down_read(&mm->mmap_sem); > while (pinned < npages) { > + if (!locked) { > + down_read(&mm->mmap_sem); > + locked = 1; > + } > ret = get_user_pages_remote > (work->task, mm, > obj->userptr.ptr + pinned * PAGE_SIZE, > npages - pinned, > flags, > - pvec + pinned, NULL, NULL); > + pvec + pinned, NULL, &locked); > if (ret < 0) > break; > > pinned += ret; > } > - up_read(&mm->mmap_sem); > + if (locked) > + up_read(&mm->mmap_sem); > mmput(mm); > } > } >
Quoting Tvrtko Ursulin (2019-11-11 14:19:31) > > On 11/11/2019 13:32, Chris Wilson wrote: > > Enable gup to retry and fault the pages outside of the mmap_sem lock in > > our worker. As we are inside our worker, outside of any critical path, > > we can allow the mmap_sem lock to be dropped in order to service a page > > fault; this in turn allows the mm to populate the page using a slow > > fault handler. > > > > Testcase: igt/gem_userptr/userfault > > There are no references or explanation on what does this fix? gup simply fails if it is not allowed to drop the lock for some faults, __get_user_pages_locked: ret = __get_user_pages(tsk, mm, start, nr_pages, flags, pages, vmas, locked); if (!locked) /* VM_FAULT_RETRY couldn't trigger, bypass */ return ret; userfault being the first time I discovered this even existed. Since we are only holding the mmap_sem for the gup (and not protecting anything else) we can simply allow gup to drop the lock if it needs to. -Chris
Quoting Chris Wilson (2019-11-11 14:27:16) > Quoting Tvrtko Ursulin (2019-11-11 14:19:31) > > > > On 11/11/2019 13:32, Chris Wilson wrote: > > > Enable gup to retry and fault the pages outside of the mmap_sem lock in > > > our worker. As we are inside our worker, outside of any critical path, > > > we can allow the mmap_sem lock to be dropped in order to service a page > > > fault; this in turn allows the mm to populate the page using a slow > > > fault handler. > > > > > > Testcase: igt/gem_userptr/userfault > > > > There are no references or explanation on what does this fix? > > gup simply fails if it is not allowed to drop the lock for some faults, > > __get_user_pages_locked: > ret = __get_user_pages(tsk, mm, start, nr_pages, flags, pages, > vmas, locked); > if (!locked) > /* VM_FAULT_RETRY couldn't trigger, bypass */ > return ret; > > userfault being the first time I discovered this even existed. Since we > are only holding the mmap_sem for the gup (and not protecting anything > else) we can simply allow gup to drop the lock if it needs to. Fixes: 5b56d49fc31d ("mm: add locked parameter to get_user_pages_remote()") -Chris
On 11/11/2019 14:32, Chris Wilson wrote: > Quoting Chris Wilson (2019-11-11 14:27:16) >> Quoting Tvrtko Ursulin (2019-11-11 14:19:31) >>> >>> On 11/11/2019 13:32, Chris Wilson wrote: >>>> Enable gup to retry and fault the pages outside of the mmap_sem lock in >>>> our worker. As we are inside our worker, outside of any critical path, >>>> we can allow the mmap_sem lock to be dropped in order to service a page >>>> fault; this in turn allows the mm to populate the page using a slow >>>> fault handler. >>>> >>>> Testcase: igt/gem_userptr/userfault >>> >>> There are no references or explanation on what does this fix? >> >> gup simply fails if it is not allowed to drop the lock for some faults, >> >> __get_user_pages_locked: >> ret = __get_user_pages(tsk, mm, start, nr_pages, flags, pages, >> vmas, locked); >> if (!locked) >> /* VM_FAULT_RETRY couldn't trigger, bypass */ >> return ret; >> >> userfault being the first time I discovered this even existed. Since we >> are only holding the mmap_sem for the gup (and not protecting anything >> else) we can simply allow gup to drop the lock if it needs to. > > Fixes: 5b56d49fc31d ("mm: add locked parameter to get_user_pages_remote()") s/Fixes/Reference/ I guess. Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Regards, Tvrtko
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c index dd104b0e2071..54ebc7ab71bc 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c @@ -459,26 +459,31 @@ __i915_gem_userptr_get_pages_worker(struct work_struct *_work) if (pvec != NULL) { struct mm_struct *mm = obj->userptr.mm->mm; unsigned int flags = 0; + int locked = 0; if (!i915_gem_object_is_readonly(obj)) flags |= FOLL_WRITE; ret = -EFAULT; if (mmget_not_zero(mm)) { - down_read(&mm->mmap_sem); while (pinned < npages) { + if (!locked) { + down_read(&mm->mmap_sem); + locked = 1; + } ret = get_user_pages_remote (work->task, mm, obj->userptr.ptr + pinned * PAGE_SIZE, npages - pinned, flags, - pvec + pinned, NULL, NULL); + pvec + pinned, NULL, &locked); if (ret < 0) break; pinned += ret; } - up_read(&mm->mmap_sem); + if (locked) + up_read(&mm->mmap_sem); mmput(mm); } }
Enable gup to retry and fault the pages outside of the mmap_sem lock in our worker. As we are inside our worker, outside of any critical path, we can allow the mmap_sem lock to be dropped in order to service a page fault; this in turn allows the mm to populate the page using a slow fault handler. Testcase: igt/gem_userptr/userfault Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> --- drivers/gpu/drm/i915/gem/i915_gem_userptr.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)