Re: [Intel-gfx] [PATCH 1/5] mutex: Export an interface to wrap a mutex lock
Hi,
On ti, 2015-04-07 at 17:31 +0100, Chris Wilson wrote:
In i915, we have a big mutex around our device struct - every time before
we attempt to communicate with the GPU, we acquire the mutex. This makes
it a convenient juncture to place our GPU error handling - before we take
the mutex we first check whether the GPU is hung or whether we are in
the process of recovering from a GPU hang. So we wrap the call to
mutex_lock() alongside our additional error handling routines.
The downside of using a wrapper around mutex_lock() is that lockdep and
lockstat cannot discriminate the true callers of mutex_lock(). Unless we
provide a means for the wrapper to pass that information down.
It also appears that i915 is almost unique in this manner of wrapping
mutex_lock(), with only one or two other potential candidates for using
this interface.
Signed-off-by: Chris Wilson ch...@chris-wilson.co.uk
Cc: Ben Widawsky benjamin.widaw...@intel.com
Seems like reasonable thing to do, I'd split this to two completely
separate parts, the kernel change and the gem side.
You might want to CC the kernel side with a little bigger audience, as
this could be called invasive change. Not by changing existing stuff,
but maybe somebody with much experience in the mutex subsystem might
want to object exposing such functionality (maybe to keep people from
using mutexes the way we do it) for reasons beyond me.
Regards, joonas
---
drivers/gpu/drm/i915/i915_gem.c | 4 +++-
include/linux/mutex.h | 9 +
kernel/locking/mutex.c | 36
3 files changed, 48 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 267fdf0f46ae..7ab8e0039790 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -135,7 +135,9 @@ int i915_mutex_lock_interruptible(struct drm_device *dev)
if (ret)
return ret;
- ret = mutex_lock_interruptible(dev-struct_mutex);
+ ret = mutex_lock_wrapper(dev-struct_mutex,
+ TASK_INTERRUPTIBLE,
+ _RET_IP_);
if (ret)
return ret;
diff --git a/include/linux/mutex.h b/include/linux/mutex.h
index 2cb7531e7d7a..3f6030b3f5aa 100644
--- a/include/linux/mutex.h
+++ b/include/linux/mutex.h
@@ -142,10 +142,15 @@ extern int __must_check
mutex_lock_interruptible_nested(struct mutex *lock,
unsigned int subclass);
extern int __must_check mutex_lock_killable_nested(struct mutex *lock,
unsigned int subclass);
+extern int __must_check mutex_lock_wrapper_nested(struct mutex *lock,
+ unsigned int subclass,
+ long state,
+ unsigned long ip);
#define mutex_lock(lock) mutex_lock_nested(lock, 0)
#define mutex_lock_interruptible(lock) mutex_lock_interruptible_nested(lock,
0)
#define mutex_lock_killable(lock) mutex_lock_killable_nested(lock, 0)
+#define mutex_lock_wrapper(lock, state, ip) mutex_lock_wrapper_nested(lock,
0, state, ip)
#define mutex_lock_nest_lock(lock, nest_lock)
\
do { \
@@ -157,10 +162,14 @@ do {
\
extern void mutex_lock(struct mutex *lock);
extern int __must_check mutex_lock_interruptible(struct mutex *lock);
extern int __must_check mutex_lock_killable(struct mutex *lock);
+extern int __must_check mutex_lock_wrapper(struct mutex *lock,
+long state,
+unsigned long ip);
# define mutex_lock_nested(lock, subclass) mutex_lock(lock)
# define mutex_lock_interruptible_nested(lock, subclass)
mutex_lock_interruptible(lock)
# define mutex_lock_killable_nested(lock, subclass) mutex_lock_killable(lock)
+# define mutex_lock_wrapper_nested(lock, subclass, state, ip)
mutex_lock_wrapper(lock, state, ip)
# define mutex_lock_nest_lock(lock, nest_lock) mutex_lock(lock)
#endif
diff --git a/kernel/locking/mutex.c b/kernel/locking/mutex.c
index 94674e5919cb..098b9e71ada1 100644
--- a/kernel/locking/mutex.c
+++ b/kernel/locking/mutex.c
@@ -658,6 +658,17 @@ mutex_lock_interruptible_nested(struct mutex *lock,
unsigned int subclass)
EXPORT_SYMBOL_GPL(mutex_lock_interruptible_nested);
+int __sched
+mutex_lock_wrapper_nested(struct mutex *lock, unsigned int subclass,
+ long state, unsigned long ip)
+{
+ might_sleep();
+ return __mutex_lock_common(lock, state,
+subclass, NULL, ip, NULL, 0);
+}
+
+EXPORT_SYMBOL_GPL(mutex_lock_wrapper_nested);
+
static inline int
[Intel-gfx] [PATCH 1/5] mutex: Export an interface to wrap a mutex lock
In i915, we have a big mutex around our device struct - every time before
we attempt to communicate with the GPU, we acquire the mutex. This makes
it a convenient juncture to place our GPU error handling - before we take
the mutex we first check whether the GPU is hung or whether we are in
the process of recovering from a GPU hang. So we wrap the call to
mutex_lock() alongside our additional error handling routines.
The downside of using a wrapper around mutex_lock() is that lockdep and
lockstat cannot discriminate the true callers of mutex_lock(). Unless we
provide a means for the wrapper to pass that information down.
It also appears that i915 is almost unique in this manner of wrapping
mutex_lock(), with only one or two other potential candidates for using
this interface.
Signed-off-by: Chris Wilson ch...@chris-wilson.co.uk
Cc: Ben Widawsky benjamin.widaw...@intel.com
---
drivers/gpu/drm/i915/i915_gem.c | 4 +++-
include/linux/mutex.h | 9 +
kernel/locking/mutex.c | 36
3 files changed, 48 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 267fdf0f46ae..7ab8e0039790 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -135,7 +135,9 @@ int i915_mutex_lock_interruptible(struct drm_device *dev)
if (ret)
return ret;
- ret = mutex_lock_interruptible(dev-struct_mutex);
+ ret = mutex_lock_wrapper(dev-struct_mutex,
+TASK_INTERRUPTIBLE,
+_RET_IP_);
if (ret)
return ret;
diff --git a/include/linux/mutex.h b/include/linux/mutex.h
index 2cb7531e7d7a..3f6030b3f5aa 100644
--- a/include/linux/mutex.h
+++ b/include/linux/mutex.h
@@ -142,10 +142,15 @@ extern int __must_check
mutex_lock_interruptible_nested(struct mutex *lock,
unsigned int subclass);
extern int __must_check mutex_lock_killable_nested(struct mutex *lock,
unsigned int subclass);
+extern int __must_check mutex_lock_wrapper_nested(struct mutex *lock,
+ unsigned int subclass,
+ long state,
+ unsigned long ip);
#define mutex_lock(lock) mutex_lock_nested(lock, 0)
#define mutex_lock_interruptible(lock) mutex_lock_interruptible_nested(lock, 0)
#define mutex_lock_killable(lock) mutex_lock_killable_nested(lock, 0)
+#define mutex_lock_wrapper(lock, state, ip) mutex_lock_wrapper_nested(lock, 0,
state, ip)
#define mutex_lock_nest_lock(lock, nest_lock) \
do { \
@@ -157,10 +162,14 @@ do {
\
extern void mutex_lock(struct mutex *lock);
extern int __must_check mutex_lock_interruptible(struct mutex *lock);
extern int __must_check mutex_lock_killable(struct mutex *lock);
+extern int __must_check mutex_lock_wrapper(struct mutex *lock,
+ long state,
+ unsigned long ip);
# define mutex_lock_nested(lock, subclass) mutex_lock(lock)
# define mutex_lock_interruptible_nested(lock, subclass)
mutex_lock_interruptible(lock)
# define mutex_lock_killable_nested(lock, subclass) mutex_lock_killable(lock)
+# define mutex_lock_wrapper_nested(lock, subclass, state, ip)
mutex_lock_wrapper(lock, state, ip)
# define mutex_lock_nest_lock(lock, nest_lock) mutex_lock(lock)
#endif
diff --git a/kernel/locking/mutex.c b/kernel/locking/mutex.c
index 94674e5919cb..098b9e71ada1 100644
--- a/kernel/locking/mutex.c
+++ b/kernel/locking/mutex.c
@@ -658,6 +658,17 @@ mutex_lock_interruptible_nested(struct mutex *lock,
unsigned int subclass)
EXPORT_SYMBOL_GPL(mutex_lock_interruptible_nested);
+int __sched
+mutex_lock_wrapper_nested(struct mutex *lock, unsigned int subclass,
+ long state, unsigned long ip)
+{
+ might_sleep();
+ return __mutex_lock_common(lock, state,
+ subclass, NULL, ip, NULL, 0);
+}
+
+EXPORT_SYMBOL_GPL(mutex_lock_wrapper_nested);
+
static inline int
ww_mutex_deadlock_injection(struct ww_mutex *lock, struct ww_acquire_ctx *ctx)
{
@@ -780,6 +791,9 @@ __mutex_lock_killable_slowpath(struct mutex *lock);
static noinline int __sched
__mutex_lock_interruptible_slowpath(struct mutex *lock);
+static noinline int __sched
+__mutex_lock_wrapper_slowpath(struct mutex *lock, long state, unsigned long
ip);
+
/**
* mutex_lock_interruptible - acquire the mutex, interruptible
* @lock: the mutex to be acquired
@@ -806,6 +820,21 @@ int __sched mutex_lock_interruptible(struct mutex *lock)
EXPORT_SYMBOL(mutex_lock_interruptible);
+int __sched
