tty->low_latency + irq context

tty->low_latency + irq context

[Jiri Slaby]

Hi!

 *      tty_flip_buffer_push    -       terminal
 *      @tty: tty to push
 *
 *      Queue a push of the terminal flip buffers to the line discipline. This
 *      function must not be called from IRQ context if tty->low_latency is set.

But some drivers (mxser, nozomi, hvsi...) sets low_latency to 1 in _open and
calls tty_flip_buffer_push in isr or in functions, which are called from isr.
Is the comment correct or the drivers?

Moreover, hvsi says:
tty->low_latency = 1; /* avoid throttle/tty_flip_buffer_push race */

thanks,
-- 
http://www.fi.muni.cz/~xslaby/            Jiri Slaby
faculty of informatics, masaryk university, brno, cz
e-mail: jirislaby gmail com, gpg pubkey fingerprint:
B674 9967 0407 CE62 ACC8  22A0 32CC 55C3 39D4 7A7E

Re: tty->low_latency + irq context

[Hollis Blanchard]

On Tue, 2006-12-26 at 01:08 +0059, Jiri Slaby wrote:
> Hi!
> 
>  *      tty_flip_buffer_push    -       terminal
>  *      @tty: tty to push
>  *
>  *      Queue a push of the terminal flip buffers to the line discipline. This
>  *      function must not be called from IRQ context if tty->low_latency is set.
> 
> But some drivers (mxser, nozomi, hvsi...) sets low_latency to 1 in _open and
> calls tty_flip_buffer_push in isr or in functions, which are called from isr.
> Is the comment correct or the drivers?

The comment would be true if tty_flip_buffer_push() attempted to block
with tty->low_latency set, but it doesn't AFAICS. One possibility for
deadlock is if the tty->buf.lock spinlock is taken on behalf of a user
process...

> Moreover, hvsi says:
> tty->low_latency = 1; /* avoid throttle/tty_flip_buffer_push race */

That was a long time ago, but the race is something like this:
      * data is received, enough to completely fill the tty buffer
      * tty_flip_buffer_push() schedules flush_to_ldisc()
      * before flush_to_ldisc() runs, more data is received
      * flush_to_ldisc() truncates the incoming data (look for
        tty->receive_room)

I don't see how this is supposed to work in general. I suppose most
PC-standard char drivers are not capable of overflowing a tty buffer
before the host can empty it. I wasn't comfortable with hoping for that
condition in my driver.

Setting "low_latency" ensures that throttle will be called immediately
if the tty buffer is filled, avoiding the race.

-- 
Hollis Blanchard
IBM Linux Technology Center

Re: tty->low_latency + irq context

[Paul Fulghum]

On Tue, 2007-01-02 at 11:17 -0600, Hollis Blanchard wrote:
> On Tue, 2006-12-26 at 01:08 +0059, Jiri Slaby wrote:
> >  *      Queue a push of the terminal flip buffers to the line discipline. This
> >  *      function must not be called from IRQ context if tty->low_latency is set.
> > 
> > But some drivers (mxser, nozomi, hvsi...) sets low_latency to 1 in _open and
> > calls tty_flip_buffer_push in isr or in functions, which are called from isr.
> > Is the comment correct or the drivers?
> 
> The comment would be true if tty_flip_buffer_push() attempted to block
> with tty->low_latency set, but it doesn't AFAICS. One possibility for
> deadlock is if the tty->buf.lock spinlock is taken on behalf of a user
> process...

There is no deadlock on tty->buf.lock,
which is always acquired with spin_lock_irqsave()
and is only used by the tty buffering code.

The only deadlock I know of with the current tty buffering code
is calling tty_flip_buffer_push() with low_latency
set and from the ISR of a driver that has a problem
with the line discipline calling back into the driver.

The standard serial core has (or at least had the last time
I looked) this problem with the N_TTY ldisc:

driver gets internal spinlock in ISR
driver calls tty_flip_buffer_push with low_latency = 1
flush_to_ldisc() immediately passes data to line discipline
line discipline calls back into driver
driver tries again to get internal spinlock

With low_latency == 1, flush_to_ldisc() is deferred
until the ISR is complete and the internal spinlock is released.

I forget the exact driver callback that caused this.

-- 
Paul Fulghum
Microgate Systems, Ltd

Re: tty->low_latency + irq context

[Paul Fulghum]

Paul Fulghum wrote:
> With low_latency == 1, flush_to_ldisc() is deferred
> until the ISR is complete and the internal spinlock is released.

Oops, I meant low_latency == 0 of course.

-- 
Paul Fulghum
Microgate Systems, Ltd.

Re: tty->low_latency + irq context

[Alan]

> with tty->low_latency set, but it doesn't AFAICS. One possibility for
> deadlock is if the tty->buf.lock spinlock is taken on behalf of a user
> process...

The case to watch out for is

	flip_buffer_push -> ldisc -> driver write of echo/^S/^Q

if you call flip_buffer_push while holding your own lock you may get in
a mess on the echo path.
 
>       * data is received, enough to completely fill the tty buffer
>       * tty_flip_buffer_push() schedules flush_to_ldisc()
>       * before flush_to_ldisc() runs, more data is received
>       * flush_to_ldisc() truncates the incoming data (look for
>         tty->receive_room)
> 
> I don't see how this is supposed to work in general.

For non fake tty hardware at real speeds it wasn't a problem under about
1Mbit. Current tty layer code just uses memory buffering based on kmalloc
and has a 64K limit instead. Works better SMP, scales better and we no
longer need to do stunts like the flip buffers to scrape 56Kbit on a
386SX16

Alan

Re: tty->low_latency + irq context

[Hollis Blanchard]

On Tue, 2007-01-02 at 18:38 +0000, Alan wrote:
> > with tty->low_latency set, but it doesn't AFAICS. One possibility
> for
> > deadlock is if the tty->buf.lock spinlock is taken on behalf of a
> user
> > process...
> 
> The case to watch out for is
> 
>         flip_buffer_push -> ldisc -> driver write of echo/^S/^Q
> 
> if you call flip_buffer_push while holding your own lock you may get
> in a mess on the echo path. 

Agreed. However, that's not what the comment says:

 *      tty_flip_buffer_push    -       terminal
 *      @tty: tty to push
 *
 *      Queue a push of the terminal flip buffers to the line discipline. This
 *      function must not be called from IRQ context if tty->low_latency is set.

-- 
Hollis Blanchard
IBM Linux Technology Center