| Message ID | 20180704142514.26997-1-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 04/07/2018 15:25, Chris Wilson wrote: > Currently, the wc-stash used for providing flushed WC pages ready for > constructing the page directories is assumed to be protected by the > struct_mutex. However, we want to remove this global lock and so must > install a replacement global lock for accessing the global wc-stash (the > per-vm stash continues to be guarded by the vm). > > We need to push ahead on this patch due to an oversight in hastily > removing the struct_mutex guard around the igt_ppgtt_alloc selftest. No > matter, it will prove very useful (i.e. will be required) in the near > future. > > v2: Restore the onstack stash so that we can drop the vm->mutex in > future across the allocation. > > Fixes: 1f6f00238abf ("drm/i915/selftests: Drop struct_mutex around lowlevel pggtt allocation") > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > --- > drivers/gpu/drm/i915/i915_drv.h | 2 +- > drivers/gpu/drm/i915/i915_gem_gtt.c | 152 ++++++++++++++++++---------- > drivers/gpu/drm/i915/i915_gem_gtt.h | 7 +- > 3 files changed, 107 insertions(+), 54 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 2cefe4c30f88..e5a0a65ec2e9 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -952,7 +952,7 @@ struct i915_gem_mm { > /** > * Small stash of WC pages > */ > - struct pagevec wc_stash; > + struct pagestash wc_stash; > > /** > * tmpfs instance used for shmem backed objects > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c > index c6aa761ca085..7048bfb282dc 100644 > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c > @@ -375,27 +375,60 @@ static gen6_pte_t iris_pte_encode(dma_addr_t addr, > return pte; > } > > +static void stash_init(struct pagestash *stash) > +{ > + pagevec_init(&stash->pvec); > + spin_lock_init(&stash->lock); > +} > + > +static struct page *stash_get(struct pagestash *stash) stash_get_page? > +{ > + struct page *page = NULL; > + > + spin_lock(&stash->lock); > + if (likely(stash->pvec.nr)) > + page = stash->pvec.pages[--stash->pvec.nr]; > + spin_unlock(&stash->lock); > + > + return page; > +} > + > +static void stash_push(struct pagestash *stash, struct pagevec *pvec) stash_append_pagevec? > +{ > + int nr; > + > + spin_lock(&stash->lock); > + > + nr = min_t(int, pvec->nr, pagevec_space(&stash->pvec)); > + memcpy(stash->pvec.pages + stash->pvec.nr, > + pvec->pages + pvec->nr - nr, > + sizeof(pvec->pages[0]) * nr); > + stash->pvec.nr += nr; > + > + spin_unlock(&stash->lock); > + > + pvec->nr -= nr; > +} > + > static struct page *vm_alloc_page(struct i915_address_space *vm, gfp_t gfp) > { > - struct pagevec *pvec = &vm->free_pages; > - struct pagevec stash; > + struct pagevec stack; > + struct page *page; > > if (I915_SELFTEST_ONLY(should_fail(&vm->fault_attr, 1))) > i915_gem_shrink_all(vm->i915); > > - if (likely(pvec->nr)) > - return pvec->pages[--pvec->nr]; > + page = stash_get(&vm->free_pages); > + if (page) > + return page; > > if (!vm->pt_kmap_wc) > return alloc_page(gfp); > > - /* A placeholder for a specific mutex to guard the WC stash */ > - lockdep_assert_held(&vm->i915->drm.struct_mutex); > - > /* Look in our global stash of WC pages... */ > - pvec = &vm->i915->mm.wc_stash; > - if (likely(pvec->nr)) > - return pvec->pages[--pvec->nr]; > + page = stash_get(&vm->i915->mm.wc_stash); > + if (page) > + return page; > > /* > * Otherwise batch allocate pages to amoritize cost of set_pages_wc. > @@ -405,7 +438,6 @@ static struct page *vm_alloc_page(struct i915_address_space *vm, gfp_t gfp) > * So we add our WB pages into a temporary pvec on the stack and merge > * them into the WC stash after all the allocations are complete. > */ > - pagevec_init(&stash); Stack look uninitialized. > do { > struct page *page; > > @@ -413,59 +445,67 @@ static struct page *vm_alloc_page(struct i915_address_space *vm, gfp_t gfp) > if (unlikely(!page)) > break; > > - stash.pages[stash.nr++] = page; > - } while (stash.nr < pagevec_space(pvec)); > + stack.pages[stack.nr++] = page; > + } while (pagevec_space(&stack)); > > - if (stash.nr) { > - int nr = min_t(int, stash.nr, pagevec_space(pvec)); > - struct page **pages = stash.pages + stash.nr - nr; > + if (stack.nr && !set_pages_array_wc(stack.pages, stack.nr)) { > + page = stack.pages[--stack.nr]; > > - if (nr && !set_pages_array_wc(pages, nr)) { > - memcpy(pvec->pages + pvec->nr, > - pages, sizeof(pages[0]) * nr); > - pvec->nr += nr; > - stash.nr -= nr; > - } > + /* Merge spare WC pages to the global stash */ > + stash_push(&vm->i915->mm.wc_stash, &stack); > + > + /* Push any surplus WC pages onto the local VM stash */ > + if (stack.nr) > + stash_push(&vm->free_pages, &stack); > + } > > - pagevec_release(&stash); > + /* Return unwanted leftovers */ > + if (unlikely(stack.nr)) { > + WARN_ON_ONCE(set_pages_array_wb(stack.pages, stack.nr)); > + __pagevec_release(&stack); Hmm.. I thought there can't be any. But in multi-threaded cases it can be. > } > > - return likely(pvec->nr) ? pvec->pages[--pvec->nr] : NULL; > + return page; > } > > static void vm_free_pages_release(struct i915_address_space *vm, > - bool immediate) > + unsigned int immediate) Change the name at least if you cannot resist changing the semantics. > { > - struct pagevec *pvec = &vm->free_pages; > + struct pagevec *pvec = &vm->free_pages.pvec; > + struct pagevec stack; > > + lockdep_assert_held(&vm->free_pages.lock); > GEM_BUG_ON(!pagevec_count(pvec)); > > if (vm->pt_kmap_wc) { > - struct pagevec *stash = &vm->i915->mm.wc_stash; > - > - /* When we use WC, first fill up the global stash and then > + /* > + * When we use WC, first fill up the global stash and then > * only if full immediately free the overflow. > */ > + stash_push(&vm->i915->mm.wc_stash, pvec); > > - lockdep_assert_held(&vm->i915->drm.struct_mutex); > - if (pagevec_space(stash)) { > - do { > - stash->pages[stash->nr++] = > - pvec->pages[--pvec->nr]; > - if (!pvec->nr) > - return; > - } while (pagevec_space(stash)); > - > - /* As we have made some room in the VM's free_pages, > - * we can wait for it to fill again. Unless we are > - * inside i915_address_space_fini() and must > - * immediately release the pages! > - */ > - if (!immediate) > - return; > - } > + /* > + * As we have made some room in the VM's free_pages, > + * we can wait for it to fill again. Unless we are > + * inside i915_address_space_fini() and must > + * immediately release the pages! > + */ > + if (pvec->nr <= immediate) > + return; > + > + /* > + * We have to drop the lock to allow ourselves to sleep, > + * so take a copy of the pvec and clear the stash for > + * others to use it as we sleep. > + */ > + stack = *pvec; > + pagevec_init(pvec); > + spin_unlock(&vm->free_pages.lock); > > + pvec = &stack; > set_pages_array_wb(pvec->pages, pvec->nr); > + > + spin_lock(&vm->free_pages.lock); > } > > __pagevec_release(pvec); > @@ -481,8 +521,10 @@ static void vm_free_page(struct i915_address_space *vm, struct page *page) > * unconditional might_sleep() for everybody. > */ > might_sleep(); > - if (!pagevec_add(&vm->free_pages, page)) > - vm_free_pages_release(vm, false); > + spin_lock(&vm->free_pages.lock); > + if (!pagevec_add(&vm->free_pages.pvec, page)) > + vm_free_pages_release(vm, PAGEVEC_SIZE - 1); > + spin_unlock(&vm->free_pages.lock); > } > > static int __setup_page_dma(struct i915_address_space *vm, > @@ -2112,18 +2154,22 @@ static void i915_address_space_init(struct i915_address_space *vm, > drm_mm_init(&vm->mm, 0, vm->total); > vm->mm.head_node.color = I915_COLOR_UNEVICTABLE; > > + stash_init(&vm->free_pages); > + > INIT_LIST_HEAD(&vm->active_list); > INIT_LIST_HEAD(&vm->inactive_list); > INIT_LIST_HEAD(&vm->unbound_list); > > list_add_tail(&vm->global_link, &dev_priv->vm_list); > - pagevec_init(&vm->free_pages); > } > > static void i915_address_space_fini(struct i915_address_space *vm) > { > - if (pagevec_count(&vm->free_pages)) > - vm_free_pages_release(vm, true); > + spin_lock(&vm->free_pages.lock); > + if (pagevec_count(&vm->free_pages.pvec)) > + vm_free_pages_release(vm, 0); > + GEM_BUG_ON(pagevec_count(&vm->free_pages.pvec)); > + spin_unlock(&vm->free_pages.lock); > > drm_mm_takedown(&vm->mm); > list_del(&vm->global_link); > @@ -2918,7 +2964,7 @@ void i915_ggtt_cleanup_hw(struct drm_i915_private *dev_priv) > > ggtt->vm.cleanup(&ggtt->vm); > > - pvec = &dev_priv->mm.wc_stash; > + pvec = &dev_priv->mm.wc_stash.pvec; > if (pvec->nr) { > set_pages_array_wb(pvec->pages, pvec->nr); > __pagevec_release(pvec); > @@ -3518,6 +3564,8 @@ int i915_ggtt_init_hw(struct drm_i915_private *dev_priv) > struct i915_ggtt *ggtt = &dev_priv->ggtt; > int ret; > > + stash_init(&dev_priv->mm.wc_stash); > + > INIT_LIST_HEAD(&dev_priv->vm_list); > > /* Note that we use page colouring to enforce a guard page at the > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h > index 9a4824cae68d..9c2089c270f8 100644 > --- a/drivers/gpu/drm/i915/i915_gem_gtt.h > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h > @@ -270,6 +270,11 @@ struct i915_vma_ops { > void (*clear_pages)(struct i915_vma *vma); > }; > > +struct pagestash { > + spinlock_t lock; > + struct pagevec pvec; > +}; > + > struct i915_address_space { > struct drm_mm mm; > struct drm_i915_private *i915; > @@ -324,7 +329,7 @@ struct i915_address_space { > */ > struct list_head unbound_list; > > - struct pagevec free_pages; > + struct pagestash free_pages; > bool pt_kmap_wc; > > /* FIXME: Need a more generic return type */ > Regards, Tvrtko
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 2cefe4c30f88..e5a0a65ec2e9 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -952,7 +952,7 @@ struct i915_gem_mm { /** * Small stash of WC pages */ - struct pagevec wc_stash; + struct pagestash wc_stash; /** * tmpfs instance used for shmem backed objects diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c index c6aa761ca085..7048bfb282dc 100644 --- a/drivers/gpu/drm/i915/i915_gem_gtt.c +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c @@ -375,27 +375,60 @@ static gen6_pte_t iris_pte_encode(dma_addr_t addr, return pte; } +static void stash_init(struct pagestash *stash) +{ + pagevec_init(&stash->pvec); + spin_lock_init(&stash->lock); +} + +static struct page *stash_get(struct pagestash *stash) +{ + struct page *page = NULL; + + spin_lock(&stash->lock); + if (likely(stash->pvec.nr)) + page = stash->pvec.pages[--stash->pvec.nr]; + spin_unlock(&stash->lock); + + return page; +} + +static void stash_push(struct pagestash *stash, struct pagevec *pvec) +{ + int nr; + + spin_lock(&stash->lock); + + nr = min_t(int, pvec->nr, pagevec_space(&stash->pvec)); + memcpy(stash->pvec.pages + stash->pvec.nr, + pvec->pages + pvec->nr - nr, + sizeof(pvec->pages[0]) * nr); + stash->pvec.nr += nr; + + spin_unlock(&stash->lock); + + pvec->nr -= nr; +} + static struct page *vm_alloc_page(struct i915_address_space *vm, gfp_t gfp) { - struct pagevec *pvec = &vm->free_pages; - struct pagevec stash; + struct pagevec stack; + struct page *page; if (I915_SELFTEST_ONLY(should_fail(&vm->fault_attr, 1))) i915_gem_shrink_all(vm->i915); - if (likely(pvec->nr)) - return pvec->pages[--pvec->nr]; + page = stash_get(&vm->free_pages); + if (page) + return page; if (!vm->pt_kmap_wc) return alloc_page(gfp); - /* A placeholder for a specific mutex to guard the WC stash */ - lockdep_assert_held(&vm->i915->drm.struct_mutex); - /* Look in our global stash of WC pages... */ - pvec = &vm->i915->mm.wc_stash; - if (likely(pvec->nr)) - return pvec->pages[--pvec->nr]; + page = stash_get(&vm->i915->mm.wc_stash); + if (page) + return page; /* * Otherwise batch allocate pages to amoritize cost of set_pages_wc. @@ -405,7 +438,6 @@ static struct page *vm_alloc_page(struct i915_address_space *vm, gfp_t gfp) * So we add our WB pages into a temporary pvec on the stack and merge * them into the WC stash after all the allocations are complete. */ - pagevec_init(&stash); do { struct page *page; @@ -413,59 +445,67 @@ static struct page *vm_alloc_page(struct i915_address_space *vm, gfp_t gfp) if (unlikely(!page)) break; - stash.pages[stash.nr++] = page; - } while (stash.nr < pagevec_space(pvec)); + stack.pages[stack.nr++] = page; + } while (pagevec_space(&stack)); - if (stash.nr) { - int nr = min_t(int, stash.nr, pagevec_space(pvec)); - struct page **pages = stash.pages + stash.nr - nr; + if (stack.nr && !set_pages_array_wc(stack.pages, stack.nr)) { + page = stack.pages[--stack.nr]; - if (nr && !set_pages_array_wc(pages, nr)) { - memcpy(pvec->pages + pvec->nr, - pages, sizeof(pages[0]) * nr); - pvec->nr += nr; - stash.nr -= nr; - } + /* Merge spare WC pages to the global stash */ + stash_push(&vm->i915->mm.wc_stash, &stack); + + /* Push any surplus WC pages onto the local VM stash */ + if (stack.nr) + stash_push(&vm->free_pages, &stack); + } - pagevec_release(&stash); + /* Return unwanted leftovers */ + if (unlikely(stack.nr)) { + WARN_ON_ONCE(set_pages_array_wb(stack.pages, stack.nr)); + __pagevec_release(&stack); } - return likely(pvec->nr) ? pvec->pages[--pvec->nr] : NULL; + return page; } static void vm_free_pages_release(struct i915_address_space *vm, - bool immediate) + unsigned int immediate) { - struct pagevec *pvec = &vm->free_pages; + struct pagevec *pvec = &vm->free_pages.pvec; + struct pagevec stack; + lockdep_assert_held(&vm->free_pages.lock); GEM_BUG_ON(!pagevec_count(pvec)); if (vm->pt_kmap_wc) { - struct pagevec *stash = &vm->i915->mm.wc_stash; - - /* When we use WC, first fill up the global stash and then + /* + * When we use WC, first fill up the global stash and then * only if full immediately free the overflow. */ + stash_push(&vm->i915->mm.wc_stash, pvec); - lockdep_assert_held(&vm->i915->drm.struct_mutex); - if (pagevec_space(stash)) { - do { - stash->pages[stash->nr++] = - pvec->pages[--pvec->nr]; - if (!pvec->nr) - return; - } while (pagevec_space(stash)); - - /* As we have made some room in the VM's free_pages, - * we can wait for it to fill again. Unless we are - * inside i915_address_space_fini() and must - * immediately release the pages! - */ - if (!immediate) - return; - } + /* + * As we have made some room in the VM's free_pages, + * we can wait for it to fill again. Unless we are + * inside i915_address_space_fini() and must + * immediately release the pages! + */ + if (pvec->nr <= immediate) + return; + + /* + * We have to drop the lock to allow ourselves to sleep, + * so take a copy of the pvec and clear the stash for + * others to use it as we sleep. + */ + stack = *pvec; + pagevec_init(pvec); + spin_unlock(&vm->free_pages.lock); + pvec = &stack; set_pages_array_wb(pvec->pages, pvec->nr); + + spin_lock(&vm->free_pages.lock); } __pagevec_release(pvec); @@ -481,8 +521,10 @@ static void vm_free_page(struct i915_address_space *vm, struct page *page) * unconditional might_sleep() for everybody. */ might_sleep(); - if (!pagevec_add(&vm->free_pages, page)) - vm_free_pages_release(vm, false); + spin_lock(&vm->free_pages.lock); + if (!pagevec_add(&vm->free_pages.pvec, page)) + vm_free_pages_release(vm, PAGEVEC_SIZE - 1); + spin_unlock(&vm->free_pages.lock); } static int __setup_page_dma(struct i915_address_space *vm, @@ -2112,18 +2154,22 @@ static void i915_address_space_init(struct i915_address_space *vm, drm_mm_init(&vm->mm, 0, vm->total); vm->mm.head_node.color = I915_COLOR_UNEVICTABLE; + stash_init(&vm->free_pages); + INIT_LIST_HEAD(&vm->active_list); INIT_LIST_HEAD(&vm->inactive_list); INIT_LIST_HEAD(&vm->unbound_list); list_add_tail(&vm->global_link, &dev_priv->vm_list); - pagevec_init(&vm->free_pages); } static void i915_address_space_fini(struct i915_address_space *vm) { - if (pagevec_count(&vm->free_pages)) - vm_free_pages_release(vm, true); + spin_lock(&vm->free_pages.lock); + if (pagevec_count(&vm->free_pages.pvec)) + vm_free_pages_release(vm, 0); + GEM_BUG_ON(pagevec_count(&vm->free_pages.pvec)); + spin_unlock(&vm->free_pages.lock); drm_mm_takedown(&vm->mm); list_del(&vm->global_link); @@ -2918,7 +2964,7 @@ void i915_ggtt_cleanup_hw(struct drm_i915_private *dev_priv) ggtt->vm.cleanup(&ggtt->vm); - pvec = &dev_priv->mm.wc_stash; + pvec = &dev_priv->mm.wc_stash.pvec; if (pvec->nr) { set_pages_array_wb(pvec->pages, pvec->nr); __pagevec_release(pvec); @@ -3518,6 +3564,8 @@ int i915_ggtt_init_hw(struct drm_i915_private *dev_priv) struct i915_ggtt *ggtt = &dev_priv->ggtt; int ret; + stash_init(&dev_priv->mm.wc_stash); + INIT_LIST_HEAD(&dev_priv->vm_list); /* Note that we use page colouring to enforce a guard page at the diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h index 9a4824cae68d..9c2089c270f8 100644 --- a/drivers/gpu/drm/i915/i915_gem_gtt.h +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h @@ -270,6 +270,11 @@ struct i915_vma_ops { void (*clear_pages)(struct i915_vma *vma); }; +struct pagestash { + spinlock_t lock; + struct pagevec pvec; +}; + struct i915_address_space { struct drm_mm mm; struct drm_i915_private *i915; @@ -324,7 +329,7 @@ struct i915_address_space { */ struct list_head unbound_list; - struct pagevec free_pages; + struct pagestash free_pages; bool pt_kmap_wc; /* FIXME: Need a more generic return type */
Currently, the wc-stash used for providing flushed WC pages ready for constructing the page directories is assumed to be protected by the struct_mutex. However, we want to remove this global lock and so must install a replacement global lock for accessing the global wc-stash (the per-vm stash continues to be guarded by the vm). We need to push ahead on this patch due to an oversight in hastily removing the struct_mutex guard around the igt_ppgtt_alloc selftest. No matter, it will prove very useful (i.e. will be required) in the near future. v2: Restore the onstack stash so that we can drop the vm->mutex in future across the allocation. Fixes: 1f6f00238abf ("drm/i915/selftests: Drop struct_mutex around lowlevel pggtt allocation") Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> --- drivers/gpu/drm/i915/i915_drv.h | 2 +- drivers/gpu/drm/i915/i915_gem_gtt.c | 152 ++++++++++++++++++---------- drivers/gpu/drm/i915/i915_gem_gtt.h | 7 +- 3 files changed, 107 insertions(+), 54 deletions(-)