Re: [bisected] Re: tty lockdep trace

[Vegard Nossum]

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@xxxxxxxxxx>
> > > 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