Messages in this thread | | | Subject | Re: [bisected] Re: tty lockdep trace | From | Vegard Nossum <> | Date | Sun, 4 Jun 2017 12:00:17 +0200 |
| |
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
| |