/drivers/char/n_tty.c drops characters

/drivers/char/n_tty.c drops characters

[Denis Joseph Barrow]

Hi all,
While I may be taking this out of context but
from a git log on include/linux/tty_ldisc.c Alan Cox says. 

>  Finally many drivers had invalid and unsafe attempts to avoid buffer
>     overflows by directly invoking tty methods extracted out of the innards
>     of work queue structs. These are no longer needed and all go away. That
>     fixes various random hangs with serial ports on overflow.

Maybe my interpretation of this statement is incorrect but
is Alan implying that ttys will no longer drop charaters ? 
if this is what Alan is saying it is simply not true but it can be implemented.

If tty's use the /drivers/char/n_tty.c line discipline.
a quick look at drivers/char/tty_io.c sees that
tty_flip_buffer_push calls directly or indirectly flush_to_ldisc
which in turns copies to disc->ops->receive_buf normally n_tty_receive_buf
which normally is the ntty line discipline ( the default )

The simplest code path to follow in n_tty_receive_buf if tty->real_raw is set is
below
	if (tty->real_raw) {
		spin_lock_irqsave(&tty->read_lock, cpuflags);
		i = min(N_TTY_BUF_SIZE - tty->read_cnt,
			N_TTY_BUF_SIZE - tty->read_head);
		i = min(count, i);
		memcpy(tty->read_buf + tty->read_head, cp, i);
		tty->read_head = (tty->read_head + i) & (N_TTY_BUF_SIZE-1);
		tty->read_cnt += i;
		cp += i;
		count -= i;

		i = min(N_TTY_BUF_SIZE - tty->read_cnt,
			N_TTY_BUF_SIZE - tty->read_head);
		i = min(count, i);
		memcpy(tty->read_buf + tty->read_head, cp, i);
		tty->read_head = (tty->read_head + i) & (N_TTY_BUF_SIZE-1);
		tty->read_cnt += i;
		spin_unlock_irqrestore(&tty->read_lock, cpuflags);
	}

My understanding of this code might be imperfect but this really looks
likes this blindly copies into a ring buffer of N_TTY_BUF_SIZE &
doesn't return any indication an attempt to copy a tty buffer of larger than N_TTY_BUF_SIZE
& overflows the buffer before the task activated by the code at the bottom of n_tty_receive_buf

i.e.
if (waitqueue_active(&tty->read_wait))
			wake_up_interruptible(&tty->read_wait);
which typically wakes up the read_chan function in the same file.

The main things I see wrong with this code is performance
& dropping characters from serial devices, this which might be quite
acceptable for reading keystrokes but is not for high speed modems.

My main complaints are
1) there is no mechanism for informing the tty layer that not all the
characters are copied from the tty layer to the line discipline.

2) The use of a a ring buffer in the line discipline,
we are memcpying at least twice in the kernel before passing the
buffer back to userland.

3) There is no mechanism for tty_io.c of informing the
char driver which is feeding the tty that it can
release flow control & start feeding read buffers
to the device hardware again to restart io. 

Less important points in point 3.
In my attempts I gave up on
I wrapped disc->ops->receive_buf with my own receive_buf
callback but this would break if the line discipline changed.

I also accidently made my code recursive because I was
calling calling tty_flip_buf calling disc->ops_receive buf
again & stealing a lock again as the tty->low_latency
flag was set in the driver, so the developer should be
warned not to use this flag if we put the callback to
start flow control in flush_to_line_disc.

Maybe it might be an idea to have a user settable minimum
characters available in the tty_layer before the callback
gets done.

4) Maybe the tty_io layer should be using a ring buffer
but this is just a silly & probably plain wrong opinion of mine.

I currently don't know this code & all the line disciplines
well enough to fix this code reliably & I think for all the
line discipline be a patch of over 300 lines. I don't think
I have the street cred to get a patch like this into such a critical part
of the mainstream kernel & probably rightly so.


-- 
best regards,
D.J. Barrow

Re: /drivers/char/n_tty.c drops characters

[Denis Joseph Barrow]

Hi Alan,
Sorry for wasting your valuable time.
There are tty_throttle & tty_unthrottle functions
should if working properly do everything I want.

Alan Cox wrote:
>> Maybe my interpretation of this statement is incorrect but
>> is Alan implying that ttys will no longer drop charaters ? 
>> if this is what Alan is saying it is simply not true but it can be implemented.
> 
> The current queue code will not drop characters queued to the line
> discipline unless we are in extreme circumstances [64K queued on this
> tty, out of memory]
> 
>> My understanding of this code might be imperfect but this really looks
>> likes this blindly copies into a ring buffer of N_TTY_BUF_SIZE &
>> doesn't return any indication an attempt to copy a tty buffer of larger than N_TTY_BUF_SIZE
>> & overflows the buffer before the task activated by the code at the bottom of n_tty_receive_buf
> 
> There is no check needed. See flush_to_ldisc
> 
>             if (count > tty->receive_room)
>                     count = tty->receive_room;
> 
> So if we are dropping characters on this path it means
> tty->receive_room is either getting set too large or the n_tty driver is
> shrinking it by more than the bytes it gets a some point.
> 
> Most of the network drivers actually just set it to "send everything" as
> they want overruns on the serial side to drop bytes to get proper TCP
> queueing but n_tty does try to manage the value.
> 
>> My main complaints are
>> 1) there is no mechanism for informing the tty layer that not all the
>> characters are copied from the tty layer to the line discipline.
> 
> The tty layer doesn't need to know. What will the tty driver do if it
> finds this - queue bytes ? We already queue bytes in the core code now
> for up to 64K. Flow control ? - it is late for flow control at that point,
> and the core code already provides flow control callbacks.
> 
>> 2) The use of a a ring buffer in the line discipline,
>> we are memcpying at least twice in the kernel before passing the
>> buffer back to userland.
> 
> For n_tty this really does not worry me. n_tty is not a performance
> critical path for any use I know of. For the ppp layer we do a copy from
> the tty buffers to the socket buffers and we have to do that to unpack
> the async padding. That one is the one that bothers me more. I'm willing
> to be enlightened however.
> 
> We do try to keep all the transfers cache-hot. In theory you could tweak
> the ldisc code so that the tty_buffer object ownership is transferred to
> the ldisc which then frees the buffer back when it is done. That may have
> possibilities.
> 
>> 3) There is no mechanism for tty_io.c of informing the
>> char driver which is feeding the tty that it can
>> release flow control & start feeding read buffers
>> to the device hardware again to restart io. 
> 
> driver->ops->unthrottle()
> 
> I am not 100% certain we call throttle early enough and unthrottle
> at times appropriate for devices with large internal buffers like some of
> the USB hardware but those are points that can be adjusted.


-- 
best regards,
D.J. Barrow

Re: /drivers/char/n_tty.c drops characters

[Alan Cox]

> Maybe my interpretation of this statement is incorrect but
> is Alan implying that ttys will no longer drop charaters ? 
> if this is what Alan is saying it is simply not true but it can be implemented.

The current queue code will not drop characters queued to the line
discipline unless we are in extreme circumstances [64K queued on this
tty, out of memory]

> My understanding of this code might be imperfect but this really looks
> likes this blindly copies into a ring buffer of N_TTY_BUF_SIZE &
> doesn't return any indication an attempt to copy a tty buffer of larger than N_TTY_BUF_SIZE
> & overflows the buffer before the task activated by the code at the bottom of n_tty_receive_buf

There is no check needed. See flush_to_ldisc

            if (count > tty->receive_room)
                    count = tty->receive_room;

So if we are dropping characters on this path it means
tty->receive_room is either getting set too large or the n_tty driver is
shrinking it by more than the bytes it gets a some point.

Most of the network drivers actually just set it to "send everything" as
they want overruns on the serial side to drop bytes to get proper TCP
queueing but n_tty does try to manage the value.

> My main complaints are
> 1) there is no mechanism for informing the tty layer that not all the
> characters are copied from the tty layer to the line discipline.

The tty layer doesn't need to know. What will the tty driver do if it
finds this - queue bytes ? We already queue bytes in the core code now
for up to 64K. Flow control ? - it is late for flow control at that point,
and the core code already provides flow control callbacks.

> 2) The use of a a ring buffer in the line discipline,
> we are memcpying at least twice in the kernel before passing the
> buffer back to userland.

For n_tty this really does not worry me. n_tty is not a performance
critical path for any use I know of. For the ppp layer we do a copy from
the tty buffers to the socket buffers and we have to do that to unpack
the async padding. That one is the one that bothers me more. I'm willing
to be enlightened however.

We do try to keep all the transfers cache-hot. In theory you could tweak
the ldisc code so that the tty_buffer object ownership is transferred to
the ldisc which then frees the buffer back when it is done. That may have
possibilities.

> 3) There is no mechanism for tty_io.c of informing the
> char driver which is feeding the tty that it can
> release flow control & start feeding read buffers
> to the device hardware again to restart io. 

driver->ops->unthrottle()

I am not 100% certain we call throttle early enough and unthrottle
at times appropriate for devices with large internal buffers like some of
the USB hardware but those are points that can be adjusted.