| Message ID | 1439196700-20045-1-git-send-email-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 08/10/2015 09:51 AM, Chris Wilson wrote: > The userptr worker allows for a slight race condition where upon there > may two or more threads calling get_user_pages for the same object. When > we have the array of pages, then we serialise the update of the object. > However, the worker should only overwrite the obj->userptr.work pointer > if and only if it is the active one. Currently we clear it for a > secondary worker with the effect that we may rarely force a second > lookup. v2 changelog? > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > --- > drivers/gpu/drm/i915/i915_gem_userptr.c | 32 ++++++++++++++++---------------- > 1 file changed, 16 insertions(+), 16 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem_userptr.c b/drivers/gpu/drm/i915/i915_gem_userptr.c > index d11901d590ac..800a5394aa1e 100644 > --- a/drivers/gpu/drm/i915/i915_gem_userptr.c > +++ b/drivers/gpu/drm/i915/i915_gem_userptr.c > @@ -571,25 +571,25 @@ __i915_gem_userptr_get_pages_worker(struct work_struct *_work) > struct get_pages_work *work = container_of(_work, typeof(*work), work); > struct drm_i915_gem_object *obj = work->obj; > struct drm_device *dev = obj->base.dev; > - const int num_pages = obj->base.size >> PAGE_SHIFT; > + const int npages = obj->base.size >> PAGE_SHIFT; > struct page **pvec; > int pinned, ret; > > ret = -ENOMEM; > pinned = 0; > > - pvec = kmalloc(num_pages*sizeof(struct page *), > + pvec = kmalloc(npages*sizeof(struct page *), > GFP_TEMPORARY | __GFP_NOWARN | __GFP_NORETRY); > if (pvec == NULL) > - pvec = drm_malloc_ab(num_pages, sizeof(struct page *)); > + pvec = drm_malloc_ab(npages, sizeof(struct page *)); > if (pvec != NULL) { > struct mm_struct *mm = obj->userptr.mm->mm; > > down_read(&mm->mmap_sem); > - while (pinned < num_pages) { > + while (pinned < npages) { > ret = get_user_pages(work->task, mm, > obj->userptr.ptr + pinned * PAGE_SIZE, > - num_pages - pinned, > + npages - pinned, If you hadn't done this renaming you could have gotten away without a v2 changelog request... :) > !obj->userptr.read_only, 0, > pvec + pinned, NULL); > if (ret < 0) > @@ -601,20 +601,20 @@ __i915_gem_userptr_get_pages_worker(struct work_struct *_work) > } > > mutex_lock(&dev->struct_mutex); > - if (obj->userptr.work != &work->work) { > - ret = 0; > - } else if (pinned == num_pages) { > - ret = __i915_gem_userptr_set_pages(obj, pvec, num_pages); > - if (ret == 0) { > - list_add_tail(&obj->global_list, &to_i915(dev)->mm.unbound_list); > - obj->get_page.sg = obj->pages->sgl; > - obj->get_page.last = 0; > - > - pinned = 0; > + if (obj->userptr.work == &work->work) { > + if (pinned == npages) { > + ret = __i915_gem_userptr_set_pages(obj, pvec, npages); > + if (ret == 0) { > + list_add_tail(&obj->global_list, > + &to_i915(dev)->mm.unbound_list); > + obj->get_page.sg = obj->pages->sgl; > + obj->get_page.last = 0; Wouldn't obj->get_page init fit better into __i915_gem_userptr_set_pages? Although that code is not from this patch. How come it is OK not to initialize them in the non-worker case? With the v2 changelog, or dropped rename: Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Regards, Tvrtko
On Wed, Sep 09, 2015 at 11:39:01AM +0100, Tvrtko Ursulin wrote: > > On 08/10/2015 09:51 AM, Chris Wilson wrote: > >The userptr worker allows for a slight race condition where upon there > >may two or more threads calling get_user_pages for the same object. When > >we have the array of pages, then we serialise the update of the object. > >However, the worker should only overwrite the obj->userptr.work pointer > >if and only if it is the active one. Currently we clear it for a > >secondary worker with the effect that we may rarely force a second > >lookup. > > v2 changelog? > > >Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > >--- > > drivers/gpu/drm/i915/i915_gem_userptr.c | 32 ++++++++++++++++---------------- > > 1 file changed, 16 insertions(+), 16 deletions(-) > > > >diff --git a/drivers/gpu/drm/i915/i915_gem_userptr.c b/drivers/gpu/drm/i915/i915_gem_userptr.c > >index d11901d590ac..800a5394aa1e 100644 > >--- a/drivers/gpu/drm/i915/i915_gem_userptr.c > >+++ b/drivers/gpu/drm/i915/i915_gem_userptr.c > >@@ -571,25 +571,25 @@ __i915_gem_userptr_get_pages_worker(struct work_struct *_work) > > struct get_pages_work *work = container_of(_work, typeof(*work), work); > > struct drm_i915_gem_object *obj = work->obj; > > struct drm_device *dev = obj->base.dev; > >- const int num_pages = obj->base.size >> PAGE_SHIFT; > >+ const int npages = obj->base.size >> PAGE_SHIFT; > > struct page **pvec; > > int pinned, ret; > > > > ret = -ENOMEM; > > pinned = 0; > > > >- pvec = kmalloc(num_pages*sizeof(struct page *), > >+ pvec = kmalloc(npages*sizeof(struct page *), > > GFP_TEMPORARY | __GFP_NOWARN | __GFP_NORETRY); > > if (pvec == NULL) > >- pvec = drm_malloc_ab(num_pages, sizeof(struct page *)); > >+ pvec = drm_malloc_ab(npages, sizeof(struct page *)); > > if (pvec != NULL) { > > struct mm_struct *mm = obj->userptr.mm->mm; > > > > down_read(&mm->mmap_sem); > >- while (pinned < num_pages) { > >+ while (pinned < npages) { > > ret = get_user_pages(work->task, mm, > > obj->userptr.ptr + pinned * PAGE_SIZE, > >- num_pages - pinned, > >+ npages - pinned, > > If you hadn't done this renaming you could have gotten away without > a v2 changelog request... :) v2: rebase for some recent changes, rename to fix in 80 cols. > > !obj->userptr.read_only, 0, > > pvec + pinned, NULL); > > if (ret < 0) > >@@ -601,20 +601,20 @@ __i915_gem_userptr_get_pages_worker(struct work_struct *_work) > > } > > > > mutex_lock(&dev->struct_mutex); > >- if (obj->userptr.work != &work->work) { > >- ret = 0; > >- } else if (pinned == num_pages) { > >- ret = __i915_gem_userptr_set_pages(obj, pvec, num_pages); > >- if (ret == 0) { > >- list_add_tail(&obj->global_list, &to_i915(dev)->mm.unbound_list); > >- obj->get_page.sg = obj->pages->sgl; > >- obj->get_page.last = 0; > >- > >- pinned = 0; > >+ if (obj->userptr.work == &work->work) { > >+ if (pinned == npages) { > >+ ret = __i915_gem_userptr_set_pages(obj, pvec, npages); > >+ if (ret == 0) { > >+ list_add_tail(&obj->global_list, > >+ &to_i915(dev)->mm.unbound_list); > >+ obj->get_page.sg = obj->pages->sgl; > >+ obj->get_page.last = 0; > > Wouldn't obj->get_page init fit better into > __i915_gem_userptr_set_pages? Although that code is not from this > patch. How come it is OK not to initialize them in the non-worker > case? It's done for us, the worker is the special case. I wanted to write the set_pages initialiser differently so I could avoid this code, but I did not prevail. -Chris
diff --git a/drivers/gpu/drm/i915/i915_gem_userptr.c b/drivers/gpu/drm/i915/i915_gem_userptr.c index d11901d590ac..800a5394aa1e 100644 --- a/drivers/gpu/drm/i915/i915_gem_userptr.c +++ b/drivers/gpu/drm/i915/i915_gem_userptr.c @@ -571,25 +571,25 @@ __i915_gem_userptr_get_pages_worker(struct work_struct *_work) struct get_pages_work *work = container_of(_work, typeof(*work), work); struct drm_i915_gem_object *obj = work->obj; struct drm_device *dev = obj->base.dev; - const int num_pages = obj->base.size >> PAGE_SHIFT; + const int npages = obj->base.size >> PAGE_SHIFT; struct page **pvec; int pinned, ret; ret = -ENOMEM; pinned = 0; - pvec = kmalloc(num_pages*sizeof(struct page *), + pvec = kmalloc(npages*sizeof(struct page *), GFP_TEMPORARY | __GFP_NOWARN | __GFP_NORETRY); if (pvec == NULL) - pvec = drm_malloc_ab(num_pages, sizeof(struct page *)); + pvec = drm_malloc_ab(npages, sizeof(struct page *)); if (pvec != NULL) { struct mm_struct *mm = obj->userptr.mm->mm; down_read(&mm->mmap_sem); - while (pinned < num_pages) { + while (pinned < npages) { ret = get_user_pages(work->task, mm, obj->userptr.ptr + pinned * PAGE_SIZE, - num_pages - pinned, + npages - pinned, !obj->userptr.read_only, 0, pvec + pinned, NULL); if (ret < 0) @@ -601,20 +601,20 @@ __i915_gem_userptr_get_pages_worker(struct work_struct *_work) } mutex_lock(&dev->struct_mutex); - if (obj->userptr.work != &work->work) { - ret = 0; - } else if (pinned == num_pages) { - ret = __i915_gem_userptr_set_pages(obj, pvec, num_pages); - if (ret == 0) { - list_add_tail(&obj->global_list, &to_i915(dev)->mm.unbound_list); - obj->get_page.sg = obj->pages->sgl; - obj->get_page.last = 0; - - pinned = 0; + if (obj->userptr.work == &work->work) { + if (pinned == npages) { + ret = __i915_gem_userptr_set_pages(obj, pvec, npages); + if (ret == 0) { + list_add_tail(&obj->global_list, + &to_i915(dev)->mm.unbound_list); + obj->get_page.sg = obj->pages->sgl; + obj->get_page.last = 0; + pinned = 0; + } } + obj->userptr.work = ERR_PTR(ret); } - obj->userptr.work = ERR_PTR(ret); obj->userptr.workers--; drm_gem_object_unreference(&obj->base); mutex_unlock(&dev->struct_mutex);
The userptr worker allows for a slight race condition where upon there may two or more threads calling get_user_pages for the same object. When we have the array of pages, then we serialise the update of the object. However, the worker should only overwrite the obj->userptr.work pointer if and only if it is the active one. Currently we clear it for a secondary worker with the effect that we may rarely force a second lookup. Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> --- drivers/gpu/drm/i915/i915_gem_userptr.c | 32 ++++++++++++++++---------------- 1 file changed, 16 insertions(+), 16 deletions(-)