* [PATCH] mutex: Export an interface to wrap a mutex lock
@ 2014-07-26 8:01 Chris Wilson
2014-07-28 22:07 ` Ben Widawsky
0 siblings, 1 reply; 2+ messages in thread
From: Chris Wilson @ 2014-07-26 8:01 UTC (permalink / raw)
To: intel-gfx; +Cc: Ben Widawsky
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 unique in this manner of wrapping
mutex_lock().
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Ben Widawsky <benjamin.widawsky@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 ae3c7b6162f5..888acd01db44 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -233,7 +233,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 42aa9b9ecd5f..b88255a390a2 100644
--- a/include/linux/mutex.h
+++ b/include/linux/mutex.h
@@ -143,10 +143,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_killable_nested(lock, 0, state, ip)
#define mutex_lock_nest_lock(lock, nest_lock) \
do { \
@@ -158,10 +163,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 acca2c1a3c5e..243ee5fc5dc3 100644
--- a/kernel/locking/mutex.c
+++ b/kernel/locking/mutex.c
@@ -619,6 +619,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)
{
@@ -733,6 +744,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
@@ -759,6 +773,21 @@ int __sched mutex_lock_interruptible(struct mutex *lock)
EXPORT_SYMBOL(mutex_lock_interruptible);
+int __sched mutex_lock_wrapper(struct mutex *lock, long state, unsigned long ip)
+{
+ int ret;
+
+ might_sleep();
+ ret = __mutex_fastpath_lock_retval(&lock->count);
+ if (likely(!ret)) {
+ mutex_set_owner(lock);
+ return 0;
+ } else
+ return __mutex_lock_wrapper_slowpath(lock, state, ip);
+}
+
+EXPORT_SYMBOL(mutex_lock_wrapper);
+
int __sched mutex_lock_killable(struct mutex *lock)
{
int ret;
@@ -797,6 +826,13 @@ __mutex_lock_interruptible_slowpath(struct mutex *lock)
}
static noinline int __sched
+__mutex_lock_wrapper_slowpath(struct mutex *lock, long state, unsigned long ip)
+{
+ return __mutex_lock_common(lock, state, 0,
+ NULL, ip, NULL, 0);
+}
+
+static noinline int __sched
__ww_mutex_lock_slowpath(struct ww_mutex *lock, struct ww_acquire_ctx *ctx)
{
return __mutex_lock_common(&lock->base, TASK_UNINTERRUPTIBLE, 0,
--
2.0.1
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [PATCH] mutex: Export an interface to wrap a mutex lock
2014-07-26 8:01 [PATCH] mutex: Export an interface to wrap a mutex lock Chris Wilson
@ 2014-07-28 22:07 ` Ben Widawsky
0 siblings, 0 replies; 2+ messages in thread
From: Ben Widawsky @ 2014-07-28 22:07 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx
On Sat, Jul 26, 2014 at 09:01:55AM +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 unique in this manner of wrapping
> mutex_lock().
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Ben Widawsky <benjamin.widawsky@intel.com>
With the one fix inline, take your pick:
Reported-and-tested-by: Ben Widawsky <ben@bwidawsk.net>
Reviewed-by: Ben Widawsky <ben@bwidawsk.net>
I find it very strange that i915 is unique here. It makes me feel like
we're doin' in wrong. BTW, a cleaned up version of the macro version I
sent you might meet with less resistance given that no other driver
needs it [currently].
Also, looking at this code now, we could probably replace a bunch of
mutex_lock()'s in i915 with mutex_lock_wrapper(..., TASK_KILLABLE, ...)
> ---
> 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 ae3c7b6162f5..888acd01db44 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -233,7 +233,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 42aa9b9ecd5f..b88255a390a2 100644
> --- a/include/linux/mutex.h
> +++ b/include/linux/mutex.h
> @@ -143,10 +143,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_killable_nested(lock, 0, state, ip)
s/mutex_lock_killable_nested/mutex_lock_wrapper_nested/
>
> #define mutex_lock_nest_lock(lock, nest_lock) \
> do { \
> @@ -158,10 +163,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 acca2c1a3c5e..243ee5fc5dc3 100644
> --- a/kernel/locking/mutex.c
> +++ b/kernel/locking/mutex.c
> @@ -619,6 +619,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)
> {
> @@ -733,6 +744,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
> @@ -759,6 +773,21 @@ int __sched mutex_lock_interruptible(struct mutex *lock)
>
> EXPORT_SYMBOL(mutex_lock_interruptible);
>
> +int __sched mutex_lock_wrapper(struct mutex *lock, long state, unsigned long ip)
> +{
> + int ret;
> +
> + might_sleep();
> + ret = __mutex_fastpath_lock_retval(&lock->count);
> + if (likely(!ret)) {
> + mutex_set_owner(lock);
> + return 0;
> + } else
> + return __mutex_lock_wrapper_slowpath(lock, state, ip);
> +}
> +
> +EXPORT_SYMBOL(mutex_lock_wrapper);
> +
> int __sched mutex_lock_killable(struct mutex *lock)
> {
> int ret;
> @@ -797,6 +826,13 @@ __mutex_lock_interruptible_slowpath(struct mutex *lock)
> }
>
> static noinline int __sched
> +__mutex_lock_wrapper_slowpath(struct mutex *lock, long state, unsigned long ip)
> +{
> + return __mutex_lock_common(lock, state, 0,
> + NULL, ip, NULL, 0);
> +}
> +
> +static noinline int __sched
> __ww_mutex_lock_slowpath(struct ww_mutex *lock, struct ww_acquire_ctx *ctx)
> {
> return __mutex_lock_common(&lock->base, TASK_UNINTERRUPTIBLE, 0,
> --
> 2.0.1
>
--
Ben Widawsky, Intel Open Source Technology Center
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2014-07-28 22:05 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-07-26 8:01 [PATCH] mutex: Export an interface to wrap a mutex lock Chris Wilson
2014-07-28 22:07 ` Ben Widawsky
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.