[rfc PATCH] drm/i915: Simplify shrinker_lock

* [rfc PATCH] drm/i915: Simplify shrinker_lock
@ 2016-07-17 18:45 Davidlohr Bueso
  2016-07-17 21:54   ` Chris Wilson
  2016-07-19  7:23 ` ✗ Ro.CI.BAT: failure for " Patchwork
  0 siblings, 2 replies; 5+ messages in thread
From: Davidlohr Bueso @ 2016-07-17 18:45 UTC (permalink / raw)
  To: chris, daniel.vetter, jani.nikula; +Cc: intel-gfx, linux-kernel, dave

Currently i915_gem_shrinker_lock() suffers from a few issues:

(1) In the attempt to avoid recursive locking, the mutex owner is
consulted. This is obviously nasty and extends the standard api
with artifacts like mutex_is_locked_by() deep in driver code.

(2) If the lock is taken by another task, the shrinking callbacks
are immediately aborted or can block until taken. This seems harsh
and we could wait on for the lock. Of course if it is highly contended
then it could be a while, but that would still allow the operation to
take place or block less for the uninterruptable cases and help memory
pressure.

While the right way of doing things would probably be to rework the
locking rules to completely avoid (1), which goes against our explicit
mutex rules, this seems to be the this way since forever -- and i915
locking looks very nasty overall. As such, this patch moves the owner
logic into mutex.h (which yes, is terrible to expose that logic :/),
and prevents possible tearing issues.

In addition, we can simplify the overall function wrt (2), by first
checking if we are the lock owner, then address the trylock and
deal with (2) if locked/contended by a traditional mutex_lock().
This should be safe considering that if current is the lock owner,
then we are guaranteed not to race with the counter->owner updates
(the counter is updated first which sets the mutex to be visibly locked).

Finally, also get rid of the mutex_is_locked() call, which is completely
racy and makes things even messier.

Not-yet-signed-off-by: Davidlohr Bueso <dbueso@suse.de>
---

Hi,

I ran into this because I noticed the mutex owner being used outside of
core mutex code. Given that I have no idea about i915 (much less your
locking semantics), this patch could be completely bogus for both (1) and
(2). Ultimately if someone knows a better way of dealing with this and
avoiding using recursive locking that would be great :)

Compile-tested only.

Thanks.

 drivers/gpu/drm/i915/i915_gem_shrinker.c | 27 +++++++++------------------
 include/linux/mutex.h                    | 12 ++++++++++++
 2 files changed, 21 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_shrinker.c b/drivers/gpu/drm/i915/i915_gem_shrinker.c
index 66571466e9a8..d28d1a84787c 100644
--- a/drivers/gpu/drm/i915/i915_gem_shrinker.c
+++ b/drivers/gpu/drm/i915/i915_gem_shrinker.c
@@ -35,19 +35,6 @@
 #include "i915_drv.h"
 #include "i915_trace.h"
 
-static bool mutex_is_locked_by(struct mutex *mutex, struct task_struct *task)
-{
-	if (!mutex_is_locked(mutex))
-		return false;
-
-#if defined(CONFIG_DEBUG_MUTEXES) || defined(CONFIG_MUTEX_SPIN_ON_OWNER)
-	return mutex->owner == task;
-#else
-	/* Since UP may be pre-empted, we cannot assume that we own the lock */
-	return false;
-#endif
-}
-
 static int num_vma_bound(struct drm_i915_gem_object *obj)
 {
 	struct i915_vma *vma;
@@ -226,16 +213,20 @@ unsigned long i915_gem_shrink_all(struct drm_i915_private *dev_priv)
 
 static bool i915_gem_shrinker_lock(struct drm_device *dev, bool *unlock)
 {
-	if (!mutex_trylock(&dev->struct_mutex)) {
-		if (!mutex_is_locked_by(&dev->struct_mutex, current))
-			return false;
-
+	/*
+	 * Recursion detection: do we already hold the lock?
+	 * If so, then only we can drop it, hence no racing,
+	 * with the actual lock counter and owner fields.
+	 */
+	if (mutex_owner(&dev->struct_mutex) == current) {
 		if (to_i915(dev)->mm.shrinker_no_lock_stealing)
 			return false;
 
 		*unlock = false;
-	} else
+	} else {
 		*unlock = true;
+		mutex_lock(&dev->struct_mutex);
+	}
 
 	return true;
 }
diff --git a/include/linux/mutex.h b/include/linux/mutex.h
index 2cb7531e7d7a..c4363a01c48a 100644
--- a/include/linux/mutex.h
+++ b/include/linux/mutex.h
@@ -130,6 +130,18 @@ static inline int mutex_is_locked(struct mutex *lock)
 	return atomic_read(&lock->count) != 1;
 }
 
+#if defined(CONFIG_DEBUG_MUTEXES) || defined(CONFIG_MUTEX_SPIN_ON_OWNER)
+static inline struct task_struct *mutex_owner(struct mutex *lock)
+{
+	return READ_ONCE(lock->owner);
+}
+#else
+static inline struct task_struct *mutex_owner(struct mutex *lock)
+{
+	return NULL;
+}
+#endif
+
 /*
  * See kernel/locking/mutex.c for detailed documentation of these APIs.
  * Also see Documentation/locking/mutex-design.txt.
-- 
2.6.6

^ permalink raw reply related	[flat|nested] 5+ messages in thread