[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <20180711072709.GA663@jagdpanzerIV>
Date: Wed, 11 Jul 2018 16:27:09 +0900
From: Sergey Senozhatsky <sergey.senozhatsky.work@...il.com>
To: Petr Mladek <pmladek@...e.com>,
Alan Cox <gnomes@...rguk.ukuu.org.uk>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Jiri Slaby <jslaby@...e.com>
Cc: Sergey Senozhatsky <sergey.senozhatsky@...il.com>,
Tejun Heo <tj@...nel.org>,
Sergey Senozhatsky <sergey.senozhatsky.work@...il.com>,
Tetsuo Handa <penguin-kernel@...ove.sakura.ne.jp>,
Steven Rostedt <rostedt@...dmis.org>,
Peter Zijlstra <peterz@...radead.org>,
Linus Torvalds <torvalds@...ux-foundation.org>,
Andrew Morton <akpm@...ux-foundation.org>,
Dmitry Vyukov <dvyukov@...gle.com>,
linux-kernel@...r.kernel.org
Subject: Re: printk() from NMI backtrace can delay a lot
Cc-ng Alan, Greg, Jiri
A lockdep report: https://lore.kernel.org/lkml/20180703043021.GA547@jagdpanzerIV/T/#u
On (07/10/18 13:50), Petr Mladek wrote:
> >
> > Another option *possibly* could be...
> >
> > ... maybe we can brake another lock dependency. I don't quite understand,
> > and surely I'm missing something here, why serial driver call
> > tty_flip_buffer_push() under uart_port->lock. E.g.
> >
> > serial_driver_handle_irq()
> > {
> > spin_lock(uart_port->lock);
> >
> > .. TX() / RX()
> >
> > tty_flip_buffer_push(uart_port->tty_port);
> > spin_unlock(uart_port->lock);
> > }
> >
> > it might be the case that we can do
> >
> > serial_driver_handle_irq()
> > {
> > spin_loc(uart_port->lock);
> >
> > .. TX / RX
> >
> > spin_unlock(uart_port->lock);
> >
> > tty_flip_buffer_push(uart_port->tty_port);
>
> Hmm, this looks racy. For example, I see the following in
> serial_lpc32xx_interrupt():
>
> tty_insert_flip_char(tport, 0, TTY_OVERRUN);
> tty_schedule_flip(tport);
>
> where tty_insert_flip_char() manipulates flag/char/used:
>
> *flag_buf_ptr(tb, tb->used) = flag;
> *char_buf_ptr(tb, tb->used++) = ch;
>
> and tty_schedule_flip() copies "used" -> "commit":
>
> smp_store_release(&buf->tail->commit, buf->tail->used);
> queue_work(system_unbound_wq, &buf->work);
I'm lacking some ["a lot of", actually] knowledge here.
Alan, Jiri could you help us?
Correct me if I'm wrong. I thought that flip buffers are used to "cache"
received chars/commands from the device before device "sends" (flushes)
them to ldisc. So chars are added to flip buffers by the device itself - RX
function, which is most commonly called from the device's IRQ handler.
That's why we see things like
foo_irq_handler()
{
... spin_lock(uart_port->lock);
foo_TX_chars();
tty_flip_buffer_push(); // tty_schedule_flip()
...
spin_unlock(uart_port->lock);
}
or
foo_irq_handler()
{
... spin_lock(uart_port->lock);
foo_TX_chars()
{
...
tty_insert_flip_char();
tty_schedule_flip();
}
...
spin_unlock(uart_port->lock);
}
So it seems that flip buffers are for RX routines. Is this right?
Thus, if foo_irq_handler()->tty_flip_buffer_push() raced with something, then
it must have been another IRQ that appended data to the same uart_port flip
buffer. Which, probably, should not happen. There should be no other race
conditions. Correct?
So I'm still wondering if we can safely change this
foo_irq_handler()
{
... spin_lock(uart_port->lock);
foo_TX_chars();
tty_flip_buffer_push(); // tty_schedule_flip()
...
spin_unlock(uart_port->lock);
}
to this
foo_irq_handler()
{
... spin_lock(uart_port->lock);
foo_TX_chars();
...
spin_unlock(uart_port->lock);
tty_flip_buffer_push(); // tty_schedule_flip()
}
Alan, Jiri, can we do this?
> So far, the best (and realistic?) idea seems to be switching to
> printk_deferred() context when port->lock is taken. It would
> be a well defined pattern that people might get used to.
Hmm. Not sure, maybe I'm missing something. In this particular case we
don't call printk() under port->lock, so it doesn't matter if we are in
"normal" printk mode or in some "safe" printk mode. What we have is:
UART port->lock --> WQ pool->lock
Which is OK, and port->lock is sort of "innocent".
It's the stuff that we do under WQ pool->lock that hurts (deadlock).
WQ pool->lock -> printk -> UART port->lock
If we want printk_deferred() / printk_safe() to help us here, then
we need to switch to printk_deferred() / printk_safe() every time
we take WQ pool->lock. Which is, basically, what I have already
suggested.
But I'd rather try to move tty_flip_buffer_push() out of uart_port->lock
scope [if possible], so we would break the
uart_port->lock -> WQ pool->lock
dependency.
-ss
Powered by blists - more mailing lists