All of lore.kernel.org
 help / color / mirror / Atom feed
From: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
To: Intel-gfx@lists.freedesktop.org
Subject: [PATCH 1/2] drm/i915: Reduce recursive mutex locking from the shrinker
Date: Fri,  4 Jan 2019 14:02:16 +0000	[thread overview]
Message-ID: <20190104140217.17822-1-tvrtko.ursulin@linux.intel.com> (raw)

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

In two codepaths internal to the shrinker we know we will end up taking
the resursive mutex path.

It instead feels more elegant to avoid this altogether and not call
mutex_trylock_recursive in those cases.

We achieve this by extracting the shrinking part to __i915_gem_shrink,
wrapped by struct mutex taking i915_gem_shrink.

At the same time move the runtime pm reference taking to always be in the
usual, before struct mutex, order.

v2:
 * Don't use flags but split i915_gem_shrink into locked and unlocked.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_shrinker.c | 152 +++++++++++++----------
 1 file changed, 83 insertions(+), 69 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_shrinker.c b/drivers/gpu/drm/i915/i915_gem_shrinker.c
index ea90d3a0d511..1ee5b08dab1b 100644
--- a/drivers/gpu/drm/i915/i915_gem_shrinker.c
+++ b/drivers/gpu/drm/i915/i915_gem_shrinker.c
@@ -117,36 +117,11 @@ static bool unsafe_drop_pages(struct drm_i915_gem_object *obj)
 	return !i915_gem_object_has_pages(obj);
 }
 
-/**
- * i915_gem_shrink - Shrink buffer object caches
- * @i915: i915 device
- * @target: amount of memory to make available, in pages
- * @nr_scanned: optional output for number of pages scanned (incremental)
- * @flags: control flags for selecting cache types
- *
- * This function is the main interface to the shrinker. It will try to release
- * up to @target pages of main memory backing storage from buffer objects.
- * Selection of the specific caches can be done with @flags. This is e.g. useful
- * when purgeable objects should be removed from caches preferentially.
- *
- * Note that it's not guaranteed that released amount is actually available as
- * free system memory - the pages might still be in-used to due to other reasons
- * (like cpu mmaps) or the mm core has reused them before we could grab them.
- * Therefore code that needs to explicitly shrink buffer objects caches (e.g. to
- * avoid deadlocks in memory reclaim) must fall back to i915_gem_shrink_all().
- *
- * Also note that any kind of pinning (both per-vma address space pins and
- * backing storage pins at the buffer object level) result in the shrinker code
- * having to skip the object.
- *
- * Returns:
- * The number of pages of backing storage actually released.
- */
 unsigned long
-i915_gem_shrink(struct drm_i915_private *i915,
-		unsigned long target,
-		unsigned long *nr_scanned,
-		unsigned flags)
+__i915_gem_shrink(struct drm_i915_private *i915,
+		  unsigned long target,
+		  unsigned long *nr_scanned,
+		  unsigned flags)
 {
 	const struct {
 		struct list_head *list;
@@ -158,10 +133,8 @@ i915_gem_shrink(struct drm_i915_private *i915,
 	}, *phase;
 	unsigned long count = 0;
 	unsigned long scanned = 0;
-	bool unlock;
 
-	if (!shrinker_lock(i915, &unlock))
-		return 0;
+	lockdep_assert_held(&i915->drm.struct_mutex);
 
 	/*
 	 * When shrinking the active list, also consider active contexts.
@@ -177,18 +150,8 @@ i915_gem_shrink(struct drm_i915_private *i915,
 				       I915_WAIT_LOCKED,
 				       MAX_SCHEDULE_TIMEOUT);
 
-	trace_i915_gem_shrink(i915, target, flags);
 	i915_retire_requests(i915);
 
-	/*
-	 * Unbinding of objects will require HW access; Let us not wake the
-	 * device just to recover a little memory. If absolutely necessary,
-	 * we will force the wake during oom-notifier.
-	 */
-	if ((flags & I915_SHRINK_BOUND) &&
-	    !intel_runtime_pm_get_if_in_use(i915))
-		flags &= ~I915_SHRINK_BOUND;
-
 	/*
 	 * As we may completely rewrite the (un)bound list whilst unbinding
 	 * (due to retiring requests) we have to strictly process only
@@ -267,15 +230,70 @@ i915_gem_shrink(struct drm_i915_private *i915,
 		spin_unlock(&i915->mm.obj_lock);
 	}
 
-	if (flags & I915_SHRINK_BOUND)
-		intel_runtime_pm_put(i915);
-
 	i915_retire_requests(i915);
 
-	shrinker_unlock(i915, unlock);
-
 	if (nr_scanned)
 		*nr_scanned += scanned;
+
+	return count;
+}
+
+/**
+ * i915_gem_shrink - Shrink buffer object caches
+ * @i915: i915 device
+ * @target: amount of memory to make available, in pages
+ * @nr_scanned: optional output for number of pages scanned (incremental)
+ * @flags: control flags for selecting cache types
+ *
+ * This function is the main interface to the shrinker. It will try to release
+ * up to @target pages of main memory backing storage from buffer objects.
+ * Selection of the specific caches can be done with @flags. This is e.g. useful
+ * when purgeable objects should be removed from caches preferentially.
+ *
+ * Note that it's not guaranteed that released amount is actually available as
+ * free system memory - the pages might still be in-used to due to other reasons
+ * (like cpu mmaps) or the mm core has reused them before we could grab them.
+ * Therefore code that needs to explicitly shrink buffer objects caches (e.g. to
+ * avoid deadlocks in memory reclaim) must fall back to i915_gem_shrink_all().
+ *
+ * Also note that any kind of pinning (both per-vma address space pins and
+ * backing storage pins at the buffer object level) result in the shrinker code
+ * having to skip the object.
+ *
+ * Returns:
+ * The number of pages of backing storage actually released.
+ */
+unsigned long
+i915_gem_shrink(struct drm_i915_private *i915,
+		unsigned long target,
+		unsigned long *nr_scanned,
+		unsigned flags)
+{
+	unsigned long count = 0;
+	bool unlock;
+
+	trace_i915_gem_shrink(i915, target, flags);
+
+	/*
+	 * Unbinding of objects will require HW access; Let us not wake the
+	 * device just to recover a little memory. If absolutely necessary,
+	 * we will force the wake during oom-notifier.
+	 */
+	if ((flags & I915_SHRINK_BOUND) &&
+	    !intel_runtime_pm_get_if_in_use(i915))
+		flags &= ~I915_SHRINK_BOUND;
+
+	if (!shrinker_lock(i915, &unlock))
+		goto out;
+
+	count = __i915_gem_shrink(i915, target, nr_scanned, flags);
+
+	shrinker_unlock(i915, unlock);
+
+out:
+	if (flags & I915_SHRINK_BOUND)
+		intel_runtime_pm_put(i915);
+
 	return count;
 }
 
@@ -352,6 +370,7 @@ i915_gem_shrinker_scan(struct shrinker *shrinker, struct shrink_control *sc)
 {
 	struct drm_i915_private *i915 =
 		container_of(shrinker, struct drm_i915_private, mm.shrinker);
+	const unsigned int flags = I915_SHRINK_BOUND | I915_SHRINK_UNBOUND;
 	unsigned long freed;
 	bool unlock;
 
@@ -360,26 +379,21 @@ i915_gem_shrinker_scan(struct shrinker *shrinker, struct shrink_control *sc)
 	if (!shrinker_lock(i915, &unlock))
 		return SHRINK_STOP;
 
-	freed = i915_gem_shrink(i915,
-				sc->nr_to_scan,
-				&sc->nr_scanned,
-				I915_SHRINK_BOUND |
-				I915_SHRINK_UNBOUND |
-				I915_SHRINK_PURGEABLE);
+	freed = __i915_gem_shrink(i915,
+				  sc->nr_to_scan,
+				  &sc->nr_scanned,
+				  flags | I915_SHRINK_PURGEABLE);
 	if (sc->nr_scanned < sc->nr_to_scan)
-		freed += i915_gem_shrink(i915,
-					 sc->nr_to_scan - sc->nr_scanned,
-					 &sc->nr_scanned,
-					 I915_SHRINK_BOUND |
-					 I915_SHRINK_UNBOUND);
+		freed += __i915_gem_shrink(i915,
+					   sc->nr_to_scan - sc->nr_scanned,
+					   &sc->nr_scanned,
+					   flags);
 	if (sc->nr_scanned < sc->nr_to_scan && current_is_kswapd()) {
 		intel_runtime_pm_get(i915);
-		freed += i915_gem_shrink(i915,
-					 sc->nr_to_scan - sc->nr_scanned,
-					 &sc->nr_scanned,
-					 I915_SHRINK_ACTIVE |
-					 I915_SHRINK_BOUND |
-					 I915_SHRINK_UNBOUND);
+		freed += __i915_gem_shrink(i915,
+					   sc->nr_to_scan - sc->nr_scanned,
+					   &sc->nr_scanned,
+					   flags | I915_SHRINK_ACTIVE);
 		intel_runtime_pm_put(i915);
 	}
 
@@ -477,11 +491,11 @@ i915_gem_shrinker_vmap(struct notifier_block *nb, unsigned long event, void *ptr
 		goto out;
 
 	intel_runtime_pm_get(i915);
-	freed_pages += i915_gem_shrink(i915, -1UL, NULL,
-				       I915_SHRINK_BOUND |
-				       I915_SHRINK_UNBOUND |
-				       I915_SHRINK_ACTIVE |
-				       I915_SHRINK_VMAPS);
+	freed_pages += __i915_gem_shrink(i915, -1UL, NULL,
+				         I915_SHRINK_BOUND |
+				         I915_SHRINK_UNBOUND |
+				         I915_SHRINK_ACTIVE |
+				         I915_SHRINK_VMAPS);
 	intel_runtime_pm_put(i915);
 
 	/* We also want to clear any cached iomaps as they wrap vmap */
-- 
2.19.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

             reply	other threads:[~2019-01-04 14:02 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-01-04 14:02 Tvrtko Ursulin [this message]
2019-01-04 14:02 ` [PATCH 2/2] drm/i915: Fix timeout handling in i915_gem_shrinker_vmap Tvrtko Ursulin
2019-01-04 14:10 ` ✗ Fi.CI.CHECKPATCH: warning for series starting with [1/2] drm/i915: Reduce recursive mutex locking from the shrinker Patchwork
2019-01-04 14:10 ` ✗ Fi.CI.SPARSE: " Patchwork
2019-01-04 14:27 ` ✓ Fi.CI.BAT: success " Patchwork
2019-01-04 14:54 ` [PATCH v3 1/2] " Tvrtko Ursulin
2019-01-04 15:30 ` ✓ Fi.CI.BAT: success for series starting with [v3,1/2] drm/i915: Reduce recursive mutex locking from the shrinker (rev2) Patchwork
2019-01-04 15:35 ` ✓ Fi.CI.IGT: success for series starting with [1/2] drm/i915: Reduce recursive mutex locking from the shrinker Patchwork
2019-01-04 16:33 ` ✓ Fi.CI.IGT: success for series starting with [v3,1/2] drm/i915: Reduce recursive mutex locking from the shrinker (rev2) Patchwork
2019-01-09 14:12 [PATCH 1/2] drm/i915: Reduce recursive mutex locking from the shrinker Tvrtko Ursulin
2019-01-09 20:54 ` Chris Wilson

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190104140217.17822-1-tvrtko.ursulin@linux.intel.com \
    --to=tvrtko.ursulin@linux.intel.com \
    --cc=Intel-gfx@lists.freedesktop.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.