Re: [PATCH] rt_mutex: correctly initialize lockdep in rt_mutex_init_proxy_locked
On Sun, Jun 11, 2017 at 02:51:09PM +, Levin, Alexander (Sasha Levin) wrote: > On Sat, Jun 10, 2017 at 04:02:12PM +0200, Peter Zijlstra wrote: > > On Sat, Jun 10, 2017 at 02:48:04AM +, Levin, Alexander (Sasha Levin) > > wrote: > > > lockdep can't deal with NULL name or key, and doesn't do anything > > > with the lock when that happens. > > > > Not doing anything is 'right', the proxy stuff won't be lockdep tracked > > anyway. But yeah, the first thing is a wee bit of a problem, for it will > > trigger DEBUG_LOCKS_WARN_ON() and fully kill lockdep. > > But don't we want pi_state->pi_mutex tracked by lockdep? Nope, we can't. That pi_mutex is owned by userspace and not all operations upon it are visible to the kernel. That is, a userspace thread can (conceptually) acquire the lock without the kernel ever knowing. We typically only create the pi_state when there's contention, at which point we create the pi_mutex as owned by someone else (hence proxy). Also, we 'obviously' hold the thing over the return to userspace, which is something lockdep very much doesn't like. > > Yeah, no need to do that; all we really need here is something like: > > > > --- > > kernel/locking/rtmutex-debug.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/kernel/locking/rtmutex-debug.c b/kernel/locking/rtmutex-debug.c > > index ac35e648b0e5..8dc647dc4b4b 100644 > > --- a/kernel/locking/rtmutex-debug.c > > +++ b/kernel/locking/rtmutex-debug.c > > @@ -175,7 +175,8 @@ void debug_rt_mutex_init(struct rt_mutex *lock, const > > char *name, struct lock_cl > > lock->name = name; > > > > #ifdef CONFIG_DEBUG_LOCK_ALLOC > > - lockdep_init_map(&lock->dep_map, name, key, 0); > > + if (name && key) > > + lockdep_init_map(&lock->dep_map, name, key, 0); > > #endif > > } > > I didn't want to do that because in later calls on that mutex we > will end up going into lockdep code, and I didn't think that doing > that without calling lockdep_init_map() initially was safe. So futex has its own private rt_mutex implementation and interface, none of which include lockdep hooks. So not initializing the field for that case should not be a problem.
Re: [PATCH] rt_mutex: correctly initialize lockdep in rt_mutex_init_proxy_locked
On Sat, Jun 10, 2017 at 04:02:12PM +0200, Peter Zijlstra wrote: > On Sat, Jun 10, 2017 at 02:48:04AM +, Levin, Alexander (Sasha Levin) > wrote: > > lockdep can't deal with NULL name or key, and doesn't do anything > > with the lock when that happens. > > Not doing anything is 'right', the proxy stuff won't be lockdep tracked > anyway. But yeah, the first thing is a wee bit of a problem, for it will > trigger DEBUG_LOCKS_WARN_ON() and fully kill lockdep. But don't we want pi_state->pi_mutex tracked by lockdep? > Yeah, no need to do that; all we really need here is something like: > > --- > kernel/locking/rtmutex-debug.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/kernel/locking/rtmutex-debug.c b/kernel/locking/rtmutex-debug.c > index ac35e648b0e5..8dc647dc4b4b 100644 > --- a/kernel/locking/rtmutex-debug.c > +++ b/kernel/locking/rtmutex-debug.c > @@ -175,7 +175,8 @@ void debug_rt_mutex_init(struct rt_mutex *lock, const > char *name, struct lock_cl > lock->name = name; > > #ifdef CONFIG_DEBUG_LOCK_ALLOC > - lockdep_init_map(&lock->dep_map, name, key, 0); > + if (name && key) > + lockdep_init_map(&lock->dep_map, name, key, 0); > #endif > } I didn't want to do that because in later calls on that mutex we will end up going into lockdep code, and I didn't think that doing that without calling lockdep_init_map() initially was safe. -- Thanks, Sasha
Re: [PATCH] rt_mutex: correctly initialize lockdep in rt_mutex_init_proxy_locked
On Sat, Jun 10, 2017 at 02:48:04AM +, Levin, Alexander (Sasha Levin) wrote:
> lockdep can't deal with NULL name or key, and doesn't do anything
> with the lock when that happens.
Not doing anything is 'right', the proxy stuff won't be lockdep tracked
anyway. But yeah, the first thing is a wee bit of a problem, for it will
trigger DEBUG_LOCKS_WARN_ON() and fully kill lockdep.
> Make rt_mutex_init_proxy_locked pass a name and a key for the lock.
>
> Fixes: f5694788ad8d ("rt_mutex: Add lockdep annotations")
> Cc: Linus Torvalds
> Cc: Peter Zijlstra
> Cc: Thomas Gleixner
> Cc: Ingo Molnar
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Sasha Levin
> ---
> kernel/locking/rtmutex.c| 6 --
> kernel/locking/rtmutex_common.h | 12 ++--
> 2 files changed, 14 insertions(+), 4 deletions(-)
>
> diff --git a/kernel/locking/rtmutex.c b/kernel/locking/rtmutex.c
> index 43123533e9b1..f540961cec30 100644
> --- a/kernel/locking/rtmutex.c
> +++ b/kernel/locking/rtmutex.c
> @@ -1679,10 +1679,12 @@ EXPORT_SYMBOL_GPL(__rt_mutex_init);
> * possible at this point because the pi_state which contains the rtmutex
> * is not yet visible to other tasks.
> */
> -void rt_mutex_init_proxy_locked(struct rt_mutex *lock,
> +void __rt_mutex_init_proxy_locked(struct rt_mutex *lock,
> + const char *name,
> + struct lock_class_key *key,
> struct task_struct *proxy_owner)
> {
> - __rt_mutex_init(lock, NULL, NULL);
> + __rt_mutex_init(lock, name, key);
> debug_rt_mutex_proxy_lock(lock, proxy_owner);
> rt_mutex_set_owner(lock, proxy_owner);
> }
Yeah, no need to do that; all we really need here is something like:
---
kernel/locking/rtmutex-debug.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/kernel/locking/rtmutex-debug.c b/kernel/locking/rtmutex-debug.c
index ac35e648b0e5..8dc647dc4b4b 100644
--- a/kernel/locking/rtmutex-debug.c
+++ b/kernel/locking/rtmutex-debug.c
@@ -175,7 +175,8 @@ void debug_rt_mutex_init(struct rt_mutex *lock, const char
*name, struct lock_cl
lock->name = name;
#ifdef CONFIG_DEBUG_LOCK_ALLOC
- lockdep_init_map(&lock->dep_map, name, key, 0);
+ if (name && key)
+ lockdep_init_map(&lock->dep_map, name, key, 0);
#endif
}
[PATCH] rt_mutex: correctly initialize lockdep in rt_mutex_init_proxy_locked
lockdep can't deal with NULL name or key, and doesn't do anything
with the lock when that happens.
Make rt_mutex_init_proxy_locked pass a name and a key for the lock.
Fixes: f5694788ad8d ("rt_mutex: Add lockdep annotations")
Cc: Linus Torvalds
Cc: Peter Zijlstra
Cc: Thomas Gleixner
Cc: Ingo Molnar
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Sasha Levin
---
kernel/locking/rtmutex.c| 6 --
kernel/locking/rtmutex_common.h | 12 ++--
2 files changed, 14 insertions(+), 4 deletions(-)
diff --git a/kernel/locking/rtmutex.c b/kernel/locking/rtmutex.c
index 43123533e9b1..f540961cec30 100644
--- a/kernel/locking/rtmutex.c
+++ b/kernel/locking/rtmutex.c
@@ -1679,10 +1679,12 @@ EXPORT_SYMBOL_GPL(__rt_mutex_init);
* possible at this point because the pi_state which contains the rtmutex
* is not yet visible to other tasks.
*/
-void rt_mutex_init_proxy_locked(struct rt_mutex *lock,
+void __rt_mutex_init_proxy_locked(struct rt_mutex *lock,
+ const char *name,
+ struct lock_class_key *key,
struct task_struct *proxy_owner)
{
- __rt_mutex_init(lock, NULL, NULL);
+ __rt_mutex_init(lock, name, key);
debug_rt_mutex_proxy_lock(lock, proxy_owner);
rt_mutex_set_owner(lock, proxy_owner);
}
diff --git a/kernel/locking/rtmutex_common.h b/kernel/locking/rtmutex_common.h
index 72ad45a9a794..110dc1ed1e89 100644
--- a/kernel/locking/rtmutex_common.h
+++ b/kernel/locking/rtmutex_common.h
@@ -100,8 +100,16 @@ enum rtmutex_chainwalk {
* PI-futex support (proxy locking functions, etc.):
*/
extern struct task_struct *rt_mutex_next_owner(struct rt_mutex *lock);
-extern void rt_mutex_init_proxy_locked(struct rt_mutex *lock,
- struct task_struct *proxy_owner);
+extern void __rt_mutex_init_proxy_locked(struct rt_mutex *lock,
+const char *name,
+struct lock_class_key *key,
+struct task_struct *proxy_owner);
+#define rt_mutex_init_proxy_locked(lock, proxy_owner) \
+do { \
+ static struct lock_class_key __key; \
+ __rt_mutex_init_proxy_locked(lock, #lock, &__key, proxy_owner); \
+} while (0)
+
extern void rt_mutex_proxy_unlock(struct rt_mutex *lock,
struct task_struct *proxy_owner);
extern void rt_mutex_init_waiter(struct rt_mutex_waiter *waiter);
--
2.11.0
