* [PATCH] tty: tty_mutex: fix lockdep warning in tty_lock_pair @ 2012-05-17 5:58 Ming Lei 2012-05-17 18:28 ` Greg Kroah-Hartman 0 siblings, 1 reply; 5+ messages in thread From: Ming Lei @ 2012-05-17 5: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_nest_lock annotation to avoid the warning. Cc: Alan Cox <alan@linux.intel.com> Cc: Arnd Bergmann <arnd@arndb.de> Cc: Peter Zijlstra <peterz@infradead.org> Signed-off-by: Ming Lei <ming.lei@canonical.com> --- drivers/tty/tty_mutex.c | 21 ++++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-) diff --git a/drivers/tty/tty_mutex.c b/drivers/tty/tty_mutex.c index 69adc80..079f9d7 100644 --- a/drivers/tty/tty_mutex.c +++ b/drivers/tty/tty_mutex.c @@ -10,6 +10,18 @@ * Getting the big tty mutex. */ +static void __lockfunc tty_lock_nest_lock(struct tty_struct *tty, + struct tty_struct *tty2) +{ + if (tty->magic != TTY_MAGIC) { + printk(KERN_ERR "L Bad %p\n", tty); + WARN_ON(1); + return; + } + tty_kref_get(tty); + mutex_lock_nest_lock(&tty->legacy_mutex, &tty2->legacy_mutex); +} + void __lockfunc tty_lock(struct tty_struct *tty) { if (tty->magic != TTY_MAGIC) { @@ -43,11 +55,14 @@ void __lockfunc tty_lock_pair(struct tty_struct *tty, { if (tty < tty2) { tty_lock(tty); - tty_lock(tty2); + tty_lock_nest_lock(tty2, tty); } else { - if (tty2 && tty2 != tty) + if (tty2 && tty2 != tty) { tty_lock(tty2); - tty_lock(tty); + tty_lock_nest_lock(tty, tty2); + } else { + tty_lock(tty); + } } } EXPORT_SYMBOL(tty_lock_pair); -- 1.7.9.5 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] tty: tty_mutex: fix lockdep warning in tty_lock_pair 2012-05-17 5:58 [PATCH] tty: tty_mutex: fix lockdep warning in tty_lock_pair Ming Lei @ 2012-05-17 18:28 ` Greg Kroah-Hartman 2012-05-17 18:48 ` Peter Zijlstra 0 siblings, 1 reply; 5+ messages in thread From: Greg Kroah-Hartman @ 2012-05-17 18:28 UTC (permalink / raw) To: Ming Lei; +Cc: linux-kernel, Alan Cox, Arnd Bergmann, Peter Zijlstra On Thu, May 17, 2012 at 01:58:16PM +0800, Ming Lei 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_nest_lock annotation to avoid > the warning. > > Cc: Alan Cox <alan@linux.intel.com> > Cc: Arnd Bergmann <arnd@arndb.de> > Cc: Peter Zijlstra <peterz@infradead.org> > Signed-off-by: Ming Lei <ming.lei@canonical.com> > --- > drivers/tty/tty_mutex.c | 21 ++++++++++++++++++--- > 1 file changed, 18 insertions(+), 3 deletions(-) > > diff --git a/drivers/tty/tty_mutex.c b/drivers/tty/tty_mutex.c > index 69adc80..079f9d7 100644 > --- a/drivers/tty/tty_mutex.c > +++ b/drivers/tty/tty_mutex.c > @@ -10,6 +10,18 @@ > * Getting the big tty mutex. > */ > > +static void __lockfunc tty_lock_nest_lock(struct tty_struct *tty, > + struct tty_struct *tty2) Duplicating tty_lock() just for this one issue seems wrong and prone to error, don't you think? > +{ > + if (tty->magic != TTY_MAGIC) { > + printk(KERN_ERR "L Bad %p\n", tty); > + WARN_ON(1); > + return; > + } > + tty_kref_get(tty); > + mutex_lock_nest_lock(&tty->legacy_mutex, &tty2->legacy_mutex); > +} > + > void __lockfunc tty_lock(struct tty_struct *tty) > { > if (tty->magic != TTY_MAGIC) { > @@ -43,11 +55,14 @@ void __lockfunc tty_lock_pair(struct tty_struct *tty, > { > if (tty < tty2) { > tty_lock(tty); > - tty_lock(tty2); > + tty_lock_nest_lock(tty2, tty); > } else { > - if (tty2 && tty2 != tty) > + if (tty2 && tty2 != tty) { > tty_lock(tty2); > - tty_lock(tty); > + tty_lock_nest_lock(tty, tty2); This is wonky, and confusing, don't you think? I don't like it, surely there's a better way to solve this? thanks, greg k-h ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] tty: tty_mutex: fix lockdep warning in tty_lock_pair 2012-05-17 18:28 ` Greg Kroah-Hartman @ 2012-05-17 18:48 ` Peter Zijlstra 2012-05-18 1:57 ` Ming Lei 0 siblings, 1 reply; 5+ messages in thread From: Peter Zijlstra @ 2012-05-17 18:48 UTC (permalink / raw) To: Greg Kroah-Hartman; +Cc: Ming Lei, linux-kernel, Alan Cox, Arnd Bergmann On Thu, 2012-05-17 at 11:28 -0700, Greg Kroah-Hartman wrote: > > +static void __lockfunc tty_lock_nest_lock(struct tty_struct *tty, > > + struct tty_struct *tty2) > > Duplicating tty_lock() just for this one issue seems wrong and prone to > error, don't you think? > > > +{ > > + if (tty->magic != TTY_MAGIC) { > > + printk(KERN_ERR "L Bad %p\n", tty); > > + WARN_ON(1); > > + return; > > + } > > + tty_kref_get(tty); > > + mutex_lock_nest_lock(&tty->legacy_mutex, &tty2->legacy_mutex); Yeah, its completely broken, even the lockdep annotation is the wrong one. Something like the (completely untested) below patch is the 'right' way. --- drivers/tty/tty_mutex.c | 35 ++++++++++++++++++++--------------- 1 file changed, 20 insertions(+), 15 deletions(-) diff --git a/drivers/tty/tty_mutex.c b/drivers/tty/tty_mutex.c index 69adc80..587330b 100644 --- a/drivers/tty/tty_mutex.c +++ b/drivers/tty/tty_mutex.c @@ -10,7 +10,7 @@ * 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 +18,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); @@ -38,25 +43,25 @@ EXPORT_SYMBOL(tty_unlock); * Getting the big tty mutex for a pair of ttys with lock ordering * On a non pty/tty pair tty2 can be NULL which is just fine. */ -void __lockfunc tty_lock_pair(struct tty_struct *tty, - struct tty_struct *tty2) +void __lockfunc tty_lock_pair(struct tty_struct *tty1, struct tty_struct *tty2) { - if (tty < tty2) { - tty_lock(tty); - tty_lock(tty2); - } else { - if (tty2 && tty2 != tty) - tty_lock(tty2); - tty_lock(tty); + if (!tty2 || tty1 == tty2) { + tty_lock(tty1); + return; } + + if (tty2 < tty1) + swap(tty1, tty2); + + tty_lock(tty1); + tty_lock_nested(tty2, SINGLE_DEPTH_NESTING); } EXPORT_SYMBOL(tty_lock_pair); -void __lockfunc tty_unlock_pair(struct tty_struct *tty, - struct tty_struct *tty2) +void __lockfunc tty_unlock_pair(struct tty_struct *tty1, struct tty_struct *tty2) { - tty_unlock(tty); - if (tty2 && tty2 != tty) + tty_unlock(tty1); + if (tty2 && tty2 != tty1) tty_unlock(tty2); } EXPORT_SYMBOL(tty_unlock_pair); ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] tty: tty_mutex: fix lockdep warning in tty_lock_pair 2012-05-17 18:48 ` Peter Zijlstra @ 2012-05-18 1:57 ` Ming Lei 2012-05-21 11:31 ` Ming Lei 0 siblings, 1 reply; 5+ messages in thread From: Ming Lei @ 2012-05-18 1:57 UTC (permalink / raw) To: Peter Zijlstra; +Cc: Greg Kroah-Hartman, linux-kernel, Alan Cox, Arnd Bergmann On Fri, May 18, 2012 at 2:48 AM, Peter Zijlstra <peterz@infradead.org> wrote: > On Thu, 2012-05-17 at 11:28 -0700, Greg Kroah-Hartman wrote: >> > +static void __lockfunc tty_lock_nest_lock(struct tty_struct *tty, >> > + struct tty_struct *tty2) >> >> Duplicating tty_lock() just for this one issue seems wrong and prone to >> error, don't you think? >> >> > +{ >> > + if (tty->magic != TTY_MAGIC) { >> > + printk(KERN_ERR "L Bad %p\n", tty); >> > + WARN_ON(1); >> > + return; >> > + } >> > + tty_kref_get(tty); >> > + mutex_lock_nest_lock(&tty->legacy_mutex, &tty2->legacy_mutex); > > Yeah, its completely broken, even the lockdep annotation is the wrong > one. > > Something like the (completely untested) below patch is the 'right' way. Yes, it should be, but the warning is still triggered with the patch, also it is worse, kernel hanged during boot. > --- > drivers/tty/tty_mutex.c | 35 ++++++++++++++++++++--------------- > 1 file changed, 20 insertions(+), 15 deletions(-) > > diff --git a/drivers/tty/tty_mutex.c b/drivers/tty/tty_mutex.c > index 69adc80..587330b 100644 > --- a/drivers/tty/tty_mutex.c > +++ b/drivers/tty/tty_mutex.c > @@ -10,7 +10,7 @@ > * 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 +18,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); > > @@ -38,25 +43,25 @@ EXPORT_SYMBOL(tty_unlock); > * Getting the big tty mutex for a pair of ttys with lock ordering > * On a non pty/tty pair tty2 can be NULL which is just fine. > */ > -void __lockfunc tty_lock_pair(struct tty_struct *tty, > - struct tty_struct *tty2) > +void __lockfunc tty_lock_pair(struct tty_struct *tty1, struct tty_struct *tty2) > { > - if (tty < tty2) { > - tty_lock(tty); > - tty_lock(tty2); > - } else { > - if (tty2 && tty2 != tty) > - tty_lock(tty2); > - tty_lock(tty); > + if (!tty2 || tty1 == tty2) { > + tty_lock(tty1); > + return; > } > + > + if (tty2 < tty1) > + swap(tty1, tty2); That is too crazy. > + > + tty_lock(tty1); > + tty_lock_nested(tty2, SINGLE_DEPTH_NESTING); > } > EXPORT_SYMBOL(tty_lock_pair); > > -void __lockfunc tty_unlock_pair(struct tty_struct *tty, > - struct tty_struct *tty2) > +void __lockfunc tty_unlock_pair(struct tty_struct *tty1, struct tty_struct *tty2) > { > - tty_unlock(tty); > - if (tty2 && tty2 != tty) > + tty_unlock(tty1); > + if (tty2 && tty2 != tty1) > tty_unlock(tty2); > } > EXPORT_SYMBOL(tty_unlock_pair); > So how about the below one(tested OK)? 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); Thanks, -- Ming Lei ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] tty: tty_mutex: fix lockdep warning in tty_lock_pair 2012-05-18 1:57 ` Ming Lei @ 2012-05-21 11:31 ` Ming Lei 0 siblings, 0 replies; 5+ messages in thread From: Ming Lei @ 2012-05-21 11:31 UTC (permalink / raw) To: Peter Zijlstra; +Cc: Greg Kroah-Hartman, linux-kernel, Alan Cox, Arnd Bergmann On Fri, May 18, 2012 at 9:57 AM, Ming Lei <ming.lei@canonical.com> wrote: > On Fri, May 18, 2012 at 2:48 AM, Peter Zijlstra <peterz@infradead.org> wrote: >> On Thu, 2012-05-17 at 11:28 -0700, Greg Kroah-Hartman wrote: >>> > +static void __lockfunc tty_lock_nest_lock(struct tty_struct *tty, >>> > + struct tty_struct *tty2) >>> >>> Duplicating tty_lock() just for this one issue seems wrong and prone to >>> error, don't you think? >>> >>> > +{ >>> > + if (tty->magic != TTY_MAGIC) { >>> > + printk(KERN_ERR "L Bad %p\n", tty); >>> > + WARN_ON(1); >>> > + return; >>> > + } >>> > + tty_kref_get(tty); >>> > + mutex_lock_nest_lock(&tty->legacy_mutex, &tty2->legacy_mutex); >> >> Yeah, its completely broken, even the lockdep annotation is the wrong >> one. >> >> Something like the (completely untested) below patch is the 'right' way. > > Yes, it should be, but the warning is still triggered with the patch, also > it is worse, kernel hanged during boot. > >> --- >> drivers/tty/tty_mutex.c | 35 ++++++++++++++++++++--------------- >> 1 file changed, 20 insertions(+), 15 deletions(-) >> >> diff --git a/drivers/tty/tty_mutex.c b/drivers/tty/tty_mutex.c >> index 69adc80..587330b 100644 >> --- a/drivers/tty/tty_mutex.c >> +++ b/drivers/tty/tty_mutex.c >> @@ -10,7 +10,7 @@ >> * 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 +18,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); >> >> @@ -38,25 +43,25 @@ EXPORT_SYMBOL(tty_unlock); >> * Getting the big tty mutex for a pair of ttys with lock ordering >> * On a non pty/tty pair tty2 can be NULL which is just fine. >> */ >> -void __lockfunc tty_lock_pair(struct tty_struct *tty, >> - struct tty_struct *tty2) >> +void __lockfunc tty_lock_pair(struct tty_struct *tty1, struct tty_struct *tty2) >> { >> - if (tty < tty2) { >> - tty_lock(tty); >> - tty_lock(tty2); >> - } else { >> - if (tty2 && tty2 != tty) >> - tty_lock(tty2); >> - tty_lock(tty); >> + if (!tty2 || tty1 == tty2) { >> + tty_lock(tty1); >> + return; >> } >> + >> + if (tty2 < tty1) >> + swap(tty1, tty2); > > That is too crazy. > >> + >> + tty_lock(tty1); >> + tty_lock_nested(tty2, SINGLE_DEPTH_NESTING); >> } >> EXPORT_SYMBOL(tty_lock_pair); >> >> -void __lockfunc tty_unlock_pair(struct tty_struct *tty, >> - struct tty_struct *tty2) >> +void __lockfunc tty_unlock_pair(struct tty_struct *tty1, struct tty_struct *tty2) >> { >> - tty_unlock(tty); >> - if (tty2 && tty2 != tty) >> + tty_unlock(tty1); >> + if (tty2 && tty2 != tty1) >> tty_unlock(tty2); >> } >> EXPORT_SYMBOL(tty_unlock_pair); >> > > So how about the below one(tested OK)? Looks no objections, I will prepare a formal one later, :-) > > 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); Thanks, -- Ming Lei ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2012-05-21 11:31 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-05-17 5:58 [PATCH] tty: tty_mutex: fix lockdep warning in tty_lock_pair Ming Lei 2012-05-17 18:28 ` Greg Kroah-Hartman 2012-05-17 18:48 ` Peter Zijlstra 2012-05-18 1:57 ` Ming Lei 2012-05-21 11:31 ` Ming Lei
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox