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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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@xxxxxxxxxxxxxxxxxx>
Cc: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
---
  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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux