* [PATCH] tty: tty_mutex: fix lockdep warning in tty_lock_pair(v1)
@ 2012-05-22 1:58 Ming Lei
2012-05-23 6:01 ` Ming Lei
0 siblings, 1 reply; 8+ messages in thread
From: Ming Lei @ 2012-05-22 1:58 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: linux-kernel, Ming Lei, Alan Cox, Arnd Bergmann, Peter Zijlstra
Commit d29f3ef39be4eec0362b985305fc526d9be318cf(tty_lock:
Localise the lock) introduces tty_lock_pair, in which
may cause lockdep warning because two locks with same lock
class are to be acquired one after another.
This patch uses mutex_lock_nested annotation to avoid
the warning as suggested by Peter.
Cc: Alan Cox <alan@linux.intel.com>
Cc: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Peter Zijlstra <peterz@infradead.org>
Signed-off-by: Ming Lei <ming.lei@canonical.com>
---
drivers/tty/tty_mutex.c | 19 ++++++++++++++-----
1 file changed, 14 insertions(+), 5 deletions(-)
diff --git a/drivers/tty/tty_mutex.c b/drivers/tty/tty_mutex.c
index 69adc80..fecf592 100644
--- a/drivers/tty/tty_mutex.c
+++ b/drivers/tty/tty_mutex.c
@@ -10,7 +10,8 @@
* Getting the big tty mutex.
*/
-void __lockfunc tty_lock(struct tty_struct *tty)
+static void __lockfunc tty_lock_nested(struct tty_struct *tty,
+ int subclass)
{
if (tty->magic != TTY_MAGIC) {
printk(KERN_ERR "L Bad %p\n", tty);
@@ -18,7 +19,12 @@ void __lockfunc tty_lock(struct tty_struct *tty)
return;
}
tty_kref_get(tty);
- mutex_lock(&tty->legacy_mutex);
+ mutex_lock_nested(&tty->legacy_mutex, subclass);
+}
+
+void __lockfunc tty_lock(struct tty_struct *tty)
+{
+ tty_lock_nested(tty, 0);
}
EXPORT_SYMBOL(tty_lock);
@@ -43,11 +49,14 @@ void __lockfunc tty_lock_pair(struct tty_struct *tty,
{
if (tty < tty2) {
tty_lock(tty);
- tty_lock(tty2);
+ tty_lock_nested(tty2, SINGLE_DEPTH_NESTING);
} else {
- if (tty2 && tty2 != tty)
+ int nested = 0;
+ if (tty2 && tty2 != tty) {
tty_lock(tty2);
- tty_lock(tty);
+ nested = SINGLE_DEPTH_NESTING;
+ }
+ tty_lock_nested(tty, nested);
}
}
EXPORT_SYMBOL(tty_lock_pair);
--
1.7.9.5
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] tty: tty_mutex: fix lockdep warning in tty_lock_pair(v1)
2012-05-22 1:58 [PATCH] tty: tty_mutex: fix lockdep warning in tty_lock_pair(v1) Ming Lei
@ 2012-05-23 6:01 ` Ming Lei
2012-05-25 13:28 ` Peter Zijlstra
0 siblings, 1 reply; 8+ messages in thread
From: Ming Lei @ 2012-05-23 6:01 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: linux-kernel, Ming Lei, Alan Cox, Arnd Bergmann, Peter Zijlstra
Hi,
On Tue, May 22, 2012 at 9:58 AM, Ming Lei <ming.lei@canonical.com> wrote:
> Commit d29f3ef39be4eec0362b985305fc526d9be318cf(tty_lock:
> Localise the lock) introduces tty_lock_pair, in which
> may cause lockdep warning because two locks with same lock
> class are to be acquired one after another.
>
> This patch uses mutex_lock_nested annotation to avoid
> the warning as suggested by Peter.
Sorry, please ignore the patch because it misses the change on
tty_unlock_pair, and the correct one should be [1].
Even though the patch is applied, there is still one related problem about
mixing tty_lock_pair with tty_unlock and tty_lock. If tty locks are
held by calling
tty_lock_pair, then deadlock warning between legacy_mutex/1 and legacy_mutex
may be triggered if tty_unlock(tty) and tty_lock(tty) are called later
when tty < tty2,
see tty_ldisc_release() in tty_release().
Thanks,
--
Ming Lei
[1],
---
drivers/tty/tty_mutex.c | 28 +++++++++++++++++++++-------
1 file changed, 21 insertions(+), 7 deletions(-)
diff --git a/drivers/tty/tty_mutex.c b/drivers/tty/tty_mutex.c
index 69adc80..c7f4523 100644
--- a/drivers/tty/tty_mutex.c
+++ b/drivers/tty/tty_mutex.c
@@ -10,7 +10,8 @@
* Getting the big tty mutex.
*/
-void __lockfunc tty_lock(struct tty_struct *tty)
+static void __lockfunc tty_lock_nested(struct tty_struct *tty,
+ int subclass)
{
if (tty->magic != TTY_MAGIC) {
printk(KERN_ERR "L Bad %p\n", tty);
@@ -18,7 +19,12 @@ void __lockfunc tty_lock(struct tty_struct *tty)
return;
}
tty_kref_get(tty);
- mutex_lock(&tty->legacy_mutex);
+ mutex_lock_nested(&tty->legacy_mutex, subclass);
+}
+
+void __lockfunc tty_lock(struct tty_struct *tty)
+{
+ tty_lock_nested(tty, 0);
}
EXPORT_SYMBOL(tty_lock);
@@ -43,11 +49,14 @@ void __lockfunc tty_lock_pair(struct tty_struct *tty,
{
if (tty < tty2) {
tty_lock(tty);
- tty_lock(tty2);
+ tty_lock_nested(tty2, SINGLE_DEPTH_NESTING);
} else {
- if (tty2 && tty2 != tty)
+ int nested = 0;
+ if (tty2 && tty2 != tty) {
tty_lock(tty2);
- tty_lock(tty);
+ nested = SINGLE_DEPTH_NESTING;
+ }
+ tty_lock_nested(tty, nested);
}
}
EXPORT_SYMBOL(tty_lock_pair);
@@ -55,8 +64,13 @@ EXPORT_SYMBOL(tty_lock_pair);
void __lockfunc tty_unlock_pair(struct tty_struct *tty,
struct tty_struct *tty2)
{
- tty_unlock(tty);
- if (tty2 && tty2 != tty)
+ if (tty < tty2) {
tty_unlock(tty2);
+ tty_unlock(tty);
+ } else {
+ tty_unlock(tty);
+ if (tty2 && tty2 != tty)
+ tty_unlock(tty2);
+ }
}
EXPORT_SYMBOL(tty_unlock_pair);
--
1.7.9.5
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] tty: tty_mutex: fix lockdep warning in tty_lock_pair(v1)
2012-05-23 6:01 ` Ming Lei
@ 2012-05-25 13:28 ` Peter Zijlstra
2012-05-25 13:39 ` Peter Zijlstra
2012-05-25 13:47 ` Alan Cox
0 siblings, 2 replies; 8+ messages in thread
From: Peter Zijlstra @ 2012-05-25 13:28 UTC (permalink / raw)
To: Ming Lei; +Cc: Greg Kroah-Hartman, linux-kernel, Alan Cox, Arnd Bergmann
On Wed, 2012-05-23 at 14:01 +0800, Ming Lei wrote:
> Even though the patch is applied, there is still one related problem about
> mixing tty_lock_pair with tty_unlock and tty_lock. If tty locks are
> held by calling
> tty_lock_pair, then deadlock warning between legacy_mutex/1 and legacy_mutex
> may be triggered if tty_unlock(tty) and tty_lock(tty) are called later
> when tty < tty2,
> see tty_ldisc_release() in tty_release().
This just gives me a head-ache instead of explaining anything.
Having looked at the source I still don't see how it could possibly
work,.. So the problem with tty_release() -> tty_ldisc_release() is that
tty_ldisc_release() does an unlock/lock of tty.
However your tty_lock_pair() can still result in tty being subclass 1,
see your else branch, nested case.
That said, how is this not a real deadlock? If you rely on tty pointer
ordering to avoid deadlocks, you always need to lock them in the same
order. The unlock+lock in ldisc_release violates that.
If we don't rely on the order, then why bother with the _pair()
primitive?
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] tty: tty_mutex: fix lockdep warning in tty_lock_pair(v1)
2012-05-25 13:28 ` Peter Zijlstra
@ 2012-05-25 13:39 ` Peter Zijlstra
2012-05-25 13:47 ` Alan Cox
1 sibling, 0 replies; 8+ messages in thread
From: Peter Zijlstra @ 2012-05-25 13:39 UTC (permalink / raw)
To: Ming Lei; +Cc: Greg Kroah-Hartman, linux-kernel, Alan Cox, Arnd Bergmann
On Fri, 2012-05-25 at 15:28 +0200, Peter Zijlstra wrote:
> On Wed, 2012-05-23 at 14:01 +0800, Ming Lei wrote:
>
> > Even though the patch is applied, there is still one related problem about
> > mixing tty_lock_pair with tty_unlock and tty_lock. If tty locks are
> > held by calling
> > tty_lock_pair, then deadlock warning between legacy_mutex/1 and legacy_mutex
> > may be triggered if tty_unlock(tty) and tty_lock(tty) are called later
> > when tty < tty2,
> > see tty_ldisc_release() in tty_release().
>
> This just gives me a head-ache instead of explaining anything.
>
> Having looked at the source I still don't see how it could possibly
> work,.. So the problem with tty_release() -> tty_ldisc_release() is that
> tty_ldisc_release() does an unlock/lock of tty.
>
> However your tty_lock_pair() can still result in tty being subclass 1,
> see your else branch, nested case.
>
> That said, how is this not a real deadlock? If you rely on tty pointer
> ordering to avoid deadlocks, you always need to lock them in the same
> order. The unlock+lock in ldisc_release violates that.
>
> If we don't rely on the order, then why bother with the _pair()
> primitive?
A git grep reveals tty_release() is the only user of tty_lock_pair() and
while we hold tty_mutex over the tty_lock_pair() its not held over
ldisc_release().
Thus afaict we can create the following deadlock:
cpu-A cpu-B
lock tty_mutex
lock tty
lock o_tty
unlock tty_mutex
unlock tty
lock tty_mutex
lock tty
lock o_tty -> block on A
lock tty -> block on B
Also, what is that plain call to schedule() doing in tty_release()?!
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] tty: tty_mutex: fix lockdep warning in tty_lock_pair(v1)
2012-05-25 13:28 ` Peter Zijlstra
2012-05-25 13:39 ` Peter Zijlstra
@ 2012-05-25 13:47 ` Alan Cox
2012-05-25 13:52 ` Peter Zijlstra
1 sibling, 1 reply; 8+ messages in thread
From: Alan Cox @ 2012-05-25 13:47 UTC (permalink / raw)
To: Peter Zijlstra; +Cc: Ming Lei, Greg Kroah-Hartman, linux-kernel, Arnd Bergmann
> Having looked at the source I still don't see how it could possibly
> work,.. So the problem with tty_release() -> tty_ldisc_release() is
> that tty_ldisc_release() does an unlock/lock of tty.
Yes it should do the pair, see the patch I posted restructing it, and
the second one restructing it right.
> However your tty_lock_pair() can still result in tty being subclass 1,
> see your else branch, nested case.
>
> That said, how is this not a real deadlock? If you rely on tty pointer
> ordering to avoid deadlocks, you always need to lock them in the same
> order. The unlock+lock in ldisc_release violates that.
Which was a bug.
Alan
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] tty: tty_mutex: fix lockdep warning in tty_lock_pair(v1)
2012-05-25 13:47 ` Alan Cox
@ 2012-05-25 13:52 ` Peter Zijlstra
2012-05-25 14:01 ` Alan Cox
0 siblings, 1 reply; 8+ messages in thread
From: Peter Zijlstra @ 2012-05-25 13:52 UTC (permalink / raw)
To: Alan Cox; +Cc: Ming Lei, Greg Kroah-Hartman, linux-kernel, Arnd Bergmann
On Fri, 2012-05-25 at 14:47 +0100, Alan Cox wrote:
> > Having looked at the source I still don't see how it could possibly
> > work,.. So the problem with tty_release() -> tty_ldisc_release() is
> > that tty_ldisc_release() does an unlock/lock of tty.
>
> Yes it should do the pair, see the patch I posted restructing it, and
> the second one restructing it right.
http://marc.info/?l=linux-kernel&m=133794355529930
That one? To what tree does one apply that? Because the tty_lock_pair()
in Linus is still missing a lockdep annotation afaict.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] tty: tty_mutex: fix lockdep warning in tty_lock_pair(v1)
2012-05-25 13:52 ` Peter Zijlstra
@ 2012-05-25 14:01 ` Alan Cox
2012-05-25 14:08 ` Peter Zijlstra
0 siblings, 1 reply; 8+ messages in thread
From: Alan Cox @ 2012-05-25 14:01 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Alan Cox, Ming Lei, Greg Kroah-Hartman, linux-kernel, Arnd Bergmann
On Fri, 25 May 2012 15:52:02 +0200
Peter Zijlstra <peterz@infradead.org> wrote:
> On Fri, 2012-05-25 at 14:47 +0100, Alan Cox wrote:
> > > Having looked at the source I still don't see how it could possibly
> > > work,.. So the problem with tty_release() -> tty_ldisc_release() is
> > > that tty_ldisc_release() does an unlock/lock of tty.
> >
> > Yes it should do the pair, see the patch I posted restructing it, and
> > the second one restructing it right.
>
> http://marc.info/?l=linux-kernel&m=133794355529930
>
> That one? To what tree does one apply that? Because the tty_lock_pair()
> in Linus is still missing a lockdep annotation afaict.
It applies on top of the other patches being tested in the thread on the
lockdep warning.
Alan
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] tty: tty_mutex: fix lockdep warning in tty_lock_pair(v1)
2012-05-25 14:01 ` Alan Cox
@ 2012-05-25 14:08 ` Peter Zijlstra
0 siblings, 0 replies; 8+ messages in thread
From: Peter Zijlstra @ 2012-05-25 14:08 UTC (permalink / raw)
To: Alan Cox
Cc: Alan Cox, Ming Lei, Greg Kroah-Hartman, linux-kernel, Arnd Bergmann
On Fri, 2012-05-25 at 15:01 +0100, Alan Cox wrote:
>
> It applies on top of the other patches being tested in the thread on the
> lockdep warning.
You're really going to make me hunt and peck patches from lkml?
/me looses interest and gets on with reducing his inbox.
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2012-05-25 14:08 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-05-22 1:58 [PATCH] tty: tty_mutex: fix lockdep warning in tty_lock_pair(v1) Ming Lei
2012-05-23 6:01 ` Ming Lei
2012-05-25 13:28 ` Peter Zijlstra
2012-05-25 13:39 ` Peter Zijlstra
2012-05-25 13:47 ` Alan Cox
2012-05-25 13:52 ` Peter Zijlstra
2012-05-25 14:01 ` Alan Cox
2012-05-25 14:08 ` Peter Zijlstra
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).