lkml.org 
[lkml]   [2017]   [Jun]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [bisected] Re: tty lockdep trace
From
Date
On 06/04/17 11:02, Mike Galbraith wrote:
> On Sun, 2017-06-04 at 10:32 +0200, Greg Kroah-Hartman wrote:
>> On Sat, Jun 03, 2017 at 08:33:52AM +0200, Mike Galbraith wrote:
>>> On Wed, 2017-05-31 at 13:21 -0400, Dave Jones wrote:
>>>> Just hit this during a trinity run.
>>>
>>> 925bb1ce47f429f69aad35876df7ecd8c53deb7e is the first bad commit
>>> commit 925bb1ce47f429f69aad35876df7ecd8c53deb7e
>>> Author: Vegard Nossum <vegard.nossum@oracle.com>
>>> Date: Thu May 11 12:18:52 2017 +0200
>>>
>>> tty: fix port buffer locking
>>
>> Now reverting this. Oops, sorry, forgot to add Dave and your names to
>> the patch revert. The list of people who reported this was really long,
>> many thanks for this.
>
> If flush_to_ldisc() is the problem, and taking atomic_write_lock in
> that path an acceptable solution, how about do that a bit differently
> instead. Lockdep stopped grumbling, vbox seems happy.
>
> 925bb1ce47f4 (tty: fix port buffer locking) upset lockdep by holding buf->lock
> while acquiring tty->atomic_write_lock. Move acquisition to flush_to_ldisc(),
> taking it prior to taking buf->lock. Costs a reference, but appeases lockdep.
>
> Not-so-signed-off-by: /me
> ---
> drivers/tty/tty_buffer.c | 10 ++++++++++
> drivers/tty/tty_port.c | 2 --
> 2 files changed, 10 insertions(+), 2 deletions(-)
>
> --- a/drivers/tty/tty_buffer.c
> +++ b/drivers/tty/tty_buffer.c
> @@ -465,7 +465,13 @@ static void flush_to_ldisc(struct work_s
> {
> struct tty_port *port = container_of(work, struct tty_port, buf.work);
> struct tty_bufhead *buf = &port->buf;
> + struct tty_struct *tty = READ_ONCE(port->itty);
> + struct tty_ldisc *disc = NULL;
>
> + if (tty)
> + disc = tty_ldisc_ref(tty);
> + if (disc)
> + mutex_lock(&tty->atomic_write_lock);
> mutex_lock(&buf->lock);
>
> while (1) {
> @@ -501,6 +507,10 @@ static void flush_to_ldisc(struct work_s
> }
>
> mutex_unlock(&buf->lock);
> + if (disc) {
> + mutex_unlock(&tty->atomic_write_lock);
> + tty_ldisc_deref(disc);
> + }
>
> }
>
> --- a/drivers/tty/tty_port.c
> +++ b/drivers/tty/tty_port.c
> @@ -34,9 +34,7 @@ static int tty_port_default_receive_buf(
> if (!disc)
> return 0;
>
> - mutex_lock(&tty->atomic_write_lock);
> ret = tty_ldisc_receive_buf(disc, p, (char *)f, count);
> - mutex_unlock(&tty->atomic_write_lock);
>
> tty_ldisc_deref(disc);
>
>
I don't know how you did it, but this passes my testing (reproducers for
both the original issue and the lockdep splat/hang). Although given the
track record I'm not sure how much that's worth :-/


Vegard

\
 
 \ /
  Last update: 2017-06-05 00:17    [W:1.729 / U:0.088 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site