On 04/15/2015 02:25 PM, Peter Hurley wrote: > 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; >> Thanks for the feedback, Peter. I'll put together a V2 of this with all your suggestions and resend.
Thanks, Joe -- 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/
