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 <[email protected]>
Cc: Arnd Bergmann <[email protected]>
Signed-off-by: Peter Zijlstra <[email protected]>
Signed-off-by: Ming Lei <[email protected]>
---
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
Hi,
On Tue, May 22, 2012 at 9:58 AM, Ming Lei <[email protected]> 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
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?
> 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
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()?!
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.
On Fri, 25 May 2012 15:52:02 +0200
Peter Zijlstra <[email protected]> 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
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.