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@xxxxxxxxxxxxxxxxxx>
> >---
> > 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
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
