* [PATCH] rt_mutex: correctly initialize lockdep in rt_mutex_init_proxy_locked
@ 2017-06-10 2:48 Levin, Alexander (Sasha Levin)
2017-06-10 14:02 ` Peter Zijlstra
0 siblings, 1 reply; 4+ messages in thread
From: Levin, Alexander (Sasha Levin) @ 2017-06-10 2:48 UTC (permalink / raw)
To: peterz
Cc: Levin, Alexander (Sasha Levin),
Linus Torvalds, Thomas Gleixner, Ingo Molnar, linux-kernel
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 <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Sasha Levin <alexander.levin@verizon.com>
---
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
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] rt_mutex: correctly initialize lockdep in rt_mutex_init_proxy_locked
2017-06-10 2:48 [PATCH] rt_mutex: correctly initialize lockdep in rt_mutex_init_proxy_locked Levin, Alexander (Sasha Levin)
@ 2017-06-10 14:02 ` Peter Zijlstra
2017-06-11 14:51 ` Levin, Alexander (Sasha Levin)
0 siblings, 1 reply; 4+ messages in thread
From: Peter Zijlstra @ 2017-06-10 14:02 UTC (permalink / raw)
To: Levin, Alexander (Sasha Levin)
Cc: Linus Torvalds, Thomas Gleixner, Ingo Molnar, linux-kernel
On Sat, Jun 10, 2017 at 02:48:04AM +0000, 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 <torvalds@linux-foundation.org>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Ingo Molnar <mingo@kernel.org>
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Sasha Levin <alexander.levin@verizon.com>
> ---
> 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
}
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] rt_mutex: correctly initialize lockdep in rt_mutex_init_proxy_locked
2017-06-10 14:02 ` Peter Zijlstra
@ 2017-06-11 14:51 ` Levin, Alexander (Sasha Levin)
2017-06-12 12:34 ` Peter Zijlstra
0 siblings, 1 reply; 4+ messages in thread
From: Levin, Alexander (Sasha Levin) @ 2017-06-11 14:51 UTC (permalink / raw)
To: Peter Zijlstra; +Cc: Linus Torvalds, Thomas Gleixner, Ingo Molnar, linux-kernel
On Sat, Jun 10, 2017 at 04:02:12PM +0200, Peter Zijlstra wrote:
> On Sat, Jun 10, 2017 at 02:48:04AM +0000, 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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] rt_mutex: correctly initialize lockdep in rt_mutex_init_proxy_locked
2017-06-11 14:51 ` Levin, Alexander (Sasha Levin)
@ 2017-06-12 12:34 ` Peter Zijlstra
0 siblings, 0 replies; 4+ messages in thread
From: Peter Zijlstra @ 2017-06-12 12:34 UTC (permalink / raw)
To: Levin, Alexander (Sasha Levin)
Cc: Linus Torvalds, Thomas Gleixner, Ingo Molnar, linux-kernel
On Sun, Jun 11, 2017 at 02:51:09PM +0000, 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 +0000, 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.
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2017-06-12 12:34 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-06-10 2:48 [PATCH] rt_mutex: correctly initialize lockdep in rt_mutex_init_proxy_locked Levin, Alexander (Sasha Levin)
2017-06-10 14:02 ` Peter Zijlstra
2017-06-11 14:51 ` Levin, Alexander (Sasha Levin)
2017-06-12 12:34 ` Peter Zijlstra
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.