Hi Joseph,
On 04/15/2015 01:39 PM, Joseph Salisbury wrote:
> From: Peter Hurley <pe...@hurleysoftware.com>
>
> BugLink: http://bugs.launchpad.net/bugs/1381005
>
> In canon mode, the read buffer head will advance over the buffer tail
> if the input > 4095 bytes without receiving a line termination char.
>
> Discard additional input until a line termination is received.
> Before evaluating for overflow, the 'room' value is normalized for
> I_PARMRK and 1 byte is reserved for line termination (even in !icanon
> mode, in case the mode is switched). The following table shows the
> transform:
>
> actual buffer | 'room' value before overflow calc
> space avail | !I_PARMRK | I_PARMRK
> --------------------------------------------------
> 0 | -1 | -1
> 1 | 0 | 0
> 2 | 1 | 0
> 3 | 2 | 0
> 4+ | 3 | 1
>
> When !icanon or when icanon and the read buffer contains newlines,
> normalized 'room' values of -1 and 0 are clamped to 0, and
> 'overflow' is 0, so read_head is not adjusted and the input i/o loop
> exits (setting no_room if called from flush_to_ldisc()). No input
> is discarded since the reader does have input available to read
> which ensures forward progress.
>
> When icanon and the read buffer does not contain newlines and the
> normalized 'room' value is 0, then overflow and room are reset to 1,
> so that the i/o loop will process the next input char normally
> (except for parity errors which are ignored). Thus, erasures, signalling
> chars, 7-bit mode, etc. will continue to be handled properly.
>
> If the input char processed was not a line termination char, then
> the canon_head index will not have advanced, so the normalized 'room'
> value will now be -1 and 'overflow' will be set, which indicates the
> read_head can safely be reset, effectively erasing the last char
> processed.
>
> If the input char processed was a line termination, then the
> canon_head index will have advanced, so 'overflow' is cleared to 0,
> the read_head is not reset, and 'room' is cleared to 0, which exits
> the i/o loop (because the reader now have input available to read
> which ensures forward progress).
>
> Note that it is possible for a line termination to be received, and
> for the reader to copy the line to the user buffer before the
> input i/o loop is ready to process the next input char. This is
> why the i/o loop recomputes the room/overflow state with every
> input char while handling overflow.
>
> Finally, if the input data was processed without receiving
> a line termination (so that overflow is still set), the pty
> driver must receive a write wakeup. A pty writer may be waiting
> to write more data in n_tty_write() but without unthrottling
> here that wakeup will not arrive, and forward progress will halt.
> (Normally, the pty writer is woken when the reader reads data out
> of the buffer and more space become available).
Thanks for doing this!
(I can now cross off the 1st item on my TODO list)
comments below.
> Signed-off-by: Peter Hurley <pe...@hurleysoftware.com>
> Signed-off-by: Greg Kroah-Hartman <gre...@linuxfoundation.org>
> (backported from commit fb5ef9e7da39968fec6d6f37f20a23d23740c75e)
Please note this is essentially also a backport of commit
06c49f9fa31f ("n_tty: Fix PARMRK over-throttling") as well, since
it incorporates the results.
> Signed-off-by: Joseph Salisbury <joseph.salisb...@canonical.com>
> ---
> drivers/tty/n_tty.c | 108
> +++++++++++++++++++++++++++++++++++++++-------------
> 1 file changed, 81 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
> index 4ddfa60..f190136 100644
> --- a/drivers/tty/n_tty.c
> +++ b/drivers/tty/n_tty.c
> @@ -247,8 +247,8 @@ static void n_tty_write_wakeup(struct tty_struct *tty)
>
> static void n_tty_check_throttle(struct tty_struct *tty)
> {
> - if (tty->driver->type == TTY_DRIVER_TYPE_PTY)
> - return;
Ok.
> + struct n_tty_data *ldata = tty->disc_data;
> +
Drop this. Nothing in n_tty_check_throttle() uses 'ldata' as a result
of this commit.
> /*
> * Check the remaining room for the input canonicalization
> * mode. We don't want to throttle the driver if we're in
> @@ -1512,23 +1512,6 @@ n_tty_receive_char_lnext(struct tty_struct *tty,
> unsigned char c, char flag)
> n_tty_receive_char_flagged(tty, c, flag);
> }
>
> -/**
> - * n_tty_receive_buf - data receive
> - * @tty: terminal device
> - * @cp: buffer
> - * @fp: flag buffer
> - * @count: characters
> - *
> - * Called by the terminal driver when a block of characters has
> - * been received. This function must be called from soft contexts
> - * not from interrupt context. The driver is responsible for making
> - * calls one at a time and in order (or using flush_to_ldisc)
> - *
> - * n_tty_receive_buf()/producer path:
> - * claims non-exclusive termios_rwsem
> - * publishes read_head and canon_head
> - */
> -
> static void
> n_tty_receive_buf_real_raw(struct tty_struct *tty, const unsigned char *cp,
> char *fp, int count)
> @@ -1684,24 +1667,85 @@ static void __receive_buf(struct tty_struct *tty,
> const unsigned char *cp,
> }
> }
>
> +/**
> + * n_tty_receive_buf_common - process input
> + * @tty: device to receive input
> + * @cp: input chars
> + * @fp: flags for each char (if NULL, all chars are TTY_NORMAL)
> + * @count: number of input chars in @cp
> + *
> + * Called by the terminal driver when a block of characters has
> + * been received. This function must be called from soft contexts
> + * not from interrupt context. The driver is responsible for making
> + * calls one at a time and in order (or using flush_to_ldisc)
> + *
> + * Returns the # of input chars from @cp which were processed.
> + *
> + * In canonical mode, the maximum line length is 4096 chars (including
> + * the line termination char); lines longer than 4096 chars are
> + * truncated. After 4095 chars, input data is still processed but
> + * not stored. Overflow processing ensures the tty can always
> + * receive more input until at least one line can be read.
> + *
> + * In non-canonical mode, the read buffer will only accept 4095 chars;
> + * this provides the necessary space for a newline char if the input
> + * mode is switched to canonical.
> + *
> + * Note it is possible for the read buffer to _contain_ 4096 chars
> + * in non-canonical mode: the read buffer could already contain the
> + * maximum canon line of 4096 chars when the mode is switched to
> + * non-canonical.
> + *
> + * n_tty_receive_buf()/producer path:
> + * claims non-exclusive termios_rwsem
> + * publishes commit_head or canon_head
> + */
> static int
> n_tty_receive_buf_common(struct tty_struct *tty, const unsigned char *cp,
> char *fp, int count, int flow)
> {
> struct n_tty_data *ldata = tty->disc_data;
> - int room, n, rcvd = 0;
> + int room, n, rcvd = 0, overflow;
>
> down_read(&tty->termios_rwsem);
>
> while (1) {
> - room = receive_room(tty);
> + /*
> + * When PARMRK is set, each input char may take up to 3 chars
> + * in the read buf; reduce the buffer space avail by 3x
> + *
> + * If we are doing input canonicalization, and there are no
> + * pending newlines, let characters through without limit, so
> + * that erase characters will be handled. Other excess
> + * characters will be beeped.
> + *
> + * paired with store in *_copy_from_read_buf() -- guarantees
> + * the consumer has loaded the data in read_buf up to the new
> + * read_tail (so this producer will not overwrite unread data)
> + */
> + size_t tail = smp_load_acquire(&ldata->read_tail);
smp_load_acquire() is part of another fix not associated with this problem.
This line should simply be
size_t tail = ldata->read_tail;
Then this fix can be applied across 3.12~3.19, inclusive.
Thanks again.
Regards,
Peter Hurley
> +
> + room = N_TTY_BUF_SIZE - (ldata->read_head - tail);
> + if (I_PARMRK(tty))
> + room = (room + 2) / 3;
> + room--;
> + if (room <= 0) {
> + overflow = ldata->icanon && ldata->canon_head == tail;
> + if (overflow && room < 0)
> + ldata->read_head--;
> + room = overflow;
> + ldata->no_room = flow && !room;
> + } else
> + overflow = 0;
> +
> n = min(count, room);
> - if (!n) {
> - if (flow && !room)
> - ldata->no_room = 1;
> + if (!n)
> break;
> - }
> - __receive_buf(tty, cp, fp, n);
> +
> + /* ignore parity errors if handling overflow */
> + if (!overflow || !fp || *fp != TTY_PARITY)
> + __receive_buf(tty, cp, fp, n);
> +
> cp += n;
> if (fp)
> fp += n;
> @@ -1710,7 +1754,17 @@ n_tty_receive_buf_common(struct tty_struct *tty, const
> unsigned char *cp,
> }
>
> tty->receive_room = room;
> - n_tty_check_throttle(tty);
> +
> + /* Unthrottle if handling overflow on pty */
> + if (tty->driver->type == TTY_DRIVER_TYPE_PTY) {
> + if (overflow) {
> + tty_set_flow_change(tty, TTY_UNTHROTTLE_SAFE);
> + tty_unthrottle_safe(tty);
> + __tty_set_flow_change(tty, 0);
> + }
> + } else
> + n_tty_check_throttle(tty);
> +
> up_read(&tty->termios_rwsem);
>
> return rcvd;
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
