[Intel-gfx] [PATCH v2] drm/i915/gtt: Pull global wc page stash under its own locking
Tvrtko Ursulin
tvrtko.ursulin at linux.intel.com
Wed Jul 4 15:10:45 UTC 2018
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 at chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin at 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
More information about the Intel-gfx
mailing list