Re: [PATCH v2] drm/i915/gtt: Pull global wc page stash under its own locking

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx