serial inter-character timeout usage with async io

I'm looking for comments, thanks.

This patch adds a timeout mode to n_tty that will only send SIGIO signals (for input) when the UART's IIR indicates that a character timeout has occurred. This reduces overhead by reducing the number of SIGIO signals sent and consequently the unnecessary wake-ups of a sleeping process waiting for serial input asynchronously.

two boolean values "timeout_enabled" and "timeout" are added to tty_struct which enable/disable the mode and record whether an inter-character timeout occurred during the last interrupt, respectively.

A new ioctl, TIOCSTIMEOUT, is added to set/clear "timeout_enabled." This seemed to be the cleanest way to do that (that I saw, at least).

In n_tty_receive_buf(), some additional tests were added before the kill_async() call to accommodate the old behavior and the newer timeout behavior as well as adding a check to make sure the read buffer doesn't overflow without a timeout received.

This should be 100% backwards compatible since the timeout mode is disabled by default.

Currently, I only have this code enabled for i386 and arm, and only for the 8250 and pxa serial drivers. If no one thinks this is a terrible idea, I'll modify all of the other ioctl.h files and resubmit the patch.

Thanks,

Harvey

diff -urp -x '*.orig' -x '*.rej' -x '*~' linux-2.6.22.1.orig/drivers/char/n_tty.c linux-2.6.22.1/drivers/char/n_tty.c
--- linux-2.6.22.1.orig/drivers/char/n_tty.c 2007-07-10
14:56:30.000000000 -0400
+++ linux-2.6.22.1/drivers/char/n_tty.c 2007-08-07 18:23:44.000000000 -0400
@@ -957,7 +957,17 @@ static void n_tty_receive_buf(struct tty

n_tty_set_room(tty);

- if (!tty->icanon && (tty->read_cnt >= tty->minimum_to_wake)) {
+ /*
+ * Make sure we're not in canonical mode. If not in timeout mode,
+ * use vmin setting as usual. If in timeout mode, only send if the
+ * last interrupt was a timeout. As a last resort, check to make
+ * sure the read buffer doesn't get too close to full. The factor of
+ * 4 used is an arbitrary number.
+ */
+ if (!tty->icanon &&
+ ((!tty->timeout_enabled && (tty->read_cnt >= tty->minimum_to_wake)) ||
+ tty->timeout ||
+ tty->receive_room < (4 * TTY_THRESHOLD_THROTTLE))) {
kill_fasync(&tty->fasync, SIGIO, POLL_IN); if (waitqueue_active(&tty->read_wait))
wake_up_interruptible(&tty->read_wait);
diff -urp -x '*.orig' -x '*.rej' -x '*~' linux-2.6.22.1.orig/drivers/char/tty_ioctl.c linux-2.6.22.1/drivers/char/tty_ioctl.c
--- linux-2.6.22.1.orig/drivers/char/tty_ioctl.c 2007-07-10
14:56:30.000000000 -0400
+++ linux-2.6.22.1/drivers/char/tty_ioctl.c 2007-08-07
15:11:23.000000000 -0400
@@ -852,6 +852,15 @@ int n_tty_ioctl(struct tty_struct * tty,
(arg ? CLOCAL : 0));
mutex_unlock(&tty->termios_mutex); return 0;
+
+ case TIOCSTIMEOUT:
+ if (get_user(arg, (unsigned int __user *) arg))
+ return -EFAULT;
+ tty->timeout_enabled = (arg ? 1 : 0);
+ tty->minimum_to_wake = 0;
+ MIN_CHAR(tty) = 0;
+ return 0;
+
default:
return -ENOIOCTLCMD;
}
diff -urp -x '*.orig' -x '*.rej' -x '*~' linux-2.6.22.1.orig/drivers/serial/8250.c linux-2.6.22.1/drivers/serial/8250.c
--- linux-2.6.22.1.orig/drivers/serial/8250.c 2007-07-10
14:56:30.000000000 -0400
+++ linux-2.6.22.1/drivers/serial/8250.c 2007-08-07 17:29:24.000000000 -0400
@@ -1466,6 +1466,12 @@ static irqreturn_t serial8250_interrupt(

iir = serial_in(up, UART_IIR); if (!(iir & UART_IIR_NO_INT)) {
+ /* Flag an inter-character timeout */
+ if (up->port.info->tty->timeout_enabled &&
+ (iir & UART_IIR_TOD) == UART_IIR_TOD)
+ up->port.info->tty->timeout = 1;
+ else
+ up->port.info->tty->timeout = 0;
serial8250_handle_port(up);

handled = 1;
diff -urp -x '*.orig' -x '*.rej' -x '*~' linux-2.6.22.1.orig/drivers/serial/pxa.c linux-2.6.22.1/drivers/serial/pxa.c
--- linux-2.6.22.1.orig/drivers/serial/pxa.c 2007-07-10
14:56:30.000000000 -0400
+++ linux-2.6.22.1/drivers/serial/pxa.c 2007-08-07 17:29:59.000000000 -0400
@@ -238,6 +238,12 @@ static inline irqreturn_t serial_pxa_irq
iir = serial_in(up, UART_IIR);
if (iir & UART_IIR_NO_INT)
return IRQ_NONE;
+ /* Flag an inter-character timeout */
+ if (up->port.info->tty->timeout_enabled &&
+ (iir & UART_IIR_TOD) == UART_IIR_TOD)
+ up->port.info->tty->timeout = 1;
+ else
+ up->port.info->tty->timeout = 0;
lsr = serial_in(up, UART_LSR);
if (lsr & UART_LSR_DR)
receive_chars(up, &lsr);
diff -urp -x '*.orig' -x '*.rej' -x '*~' linux-2.6.22.1.orig/include/asm-arm/ioctls.h linux-2.6.22.1/include/asm-arm/ioctls.h
--- linux-2.6.22.1.orig/include/asm-arm/ioctls.h 2007-07-10
14:56:30.000000000 -0400
+++ linux-2.6.22.1/include/asm-arm/ioctls.h 2007-08-07
15:11:23.000000000 -0400
@@ -69,6 +69,7 @@
#define TIOCMIWAIT 0x545C /* wait for a change on serial input line(s) */
#define TIOCGICOUNT 0x545D /* read serial port inline interrupt counts */
#define FIOQSIZE 0x545E
+#define TIOCSTIMEOUT 0x545F

/* Used for packet mode */
#define TIOCPKT_DATA 0
diff -urp -x '*.orig' -x '*.rej' -x '*~'
linux-2.6.22.1.orig/include/asm-i386/ioctls.h
linux-2.6.22.1/include/asm-i386/ioctls.h
--- linux-2.6.22.1.orig/include/asm-i386/ioctls.h 2007-07-10
14:56:30.000000000 -0400
+++ linux-2.6.22.1/include/asm-i386/ioctls.h 2007-08-07
18:33:49.000000000 -0400
@@ -72,6 +72,7 @@

 #define TIOCGHAYESESP   0x545E  /* Get Hayes ESP configuration */
 #define TIOCSHAYESESP   0x545F  /* Set Hayes ESP configuration */

#define FIOQSIZE 0x5460
+#define TIOCSTIMEOUT 0x5461

/* Used for packet mode */
#define TIOCPKT_DATA 0
diff -urp -x '*.orig' -x '*.rej' -x '*~' linux-2.6.22.1.orig/include/linux/tty.h linux-2.6.22.1/include/linux/tty.h
--- linux-2.6.22.1.orig/include/linux/tty.h 2007-07-10
14:56:30.000000000 -0400
+++ linux-2.6.22.1/include/linux/tty.h 2007-08-07 15:11:23.000000000 -0400
@@ -227,6 +227,7 @@ struct tty_struct {
unsigned int column;
unsigned char lnext:1, erasing:1, raw:1, real_raw:1, icanon:1; unsigned char closing:1;
+ unsigned char timeout:1, timeout_enabled:1;
unsigned short minimum_to_wake;
unsigned long overrun_time;
int num_overrun;

What are you actually trying to achieve ?

Firstly we don't want stuff with deep internal knowledge of devices as you'll have to modify and test something like 30 device drivers and it will rapidly be unmaintainable.

I'm trying to reduce the load on embedded systems running Linux and using serial ports.

I'm trying to alleviate the following typical software scenario:

A Linux program is connected via serial port to some other serial device using asynchronous I/O and the other serial device sends a 150 byte sequence to the Linux program. The Linux program will receive 18 intermediate SIGIOs (one for each 8-byte threshold interrupt generated by the UART) followed by 1 final inter-character timeout (when the UART FIFO contains the last 6 bytes). The program upon each SIGIO would read() 8 bytes and do some parsing of the data buffer and realize that the whole sequence had not yet been received. Repeat 17 more times in this example.

This doesn't change the Linux program code at all. It reduces the load.

For reference:

IIR : Interrupt identification register (RO)

Bit 3 Bit 2 Bit 1
0 0 0 Modem status change MSR read
0 0 1 Transmitter holding register empty IIR read or THR write
0 1 0 Received data available RBR read
0 1 1 Line status change LSR read
1 1 0 Character timeout (16550) RBR read

I agree. However, the inter-character timeout is present (to the best of my knowledge and google's) in all serial UARTs from the 16550A onward (starting around 1994 and Windows 3.1). Before that, the IIR timeout bit(3) was reserved and read 0. In addition, the two drivers modified, 8250.c and pxa.c are already using the inter-character timeout, they just don't identify it specifically. In the ISRs, they simply check to see if any of bits 1, 2, or 3 are set. In the example above, on the last interrupt, the timeout bit(3) and the receive bit(2) would be set causing a call to serial8250_handle_port(), but the ISR doesn't (currently) care that it was the timeout that generated the last interrupt.

Some thoughts after writing that:
1. The ioctl should not allow the timeout mode to be set unless the UART is a 16550A or newer.
2. The ioctl should have a way of determining if the driver supports the timeout mode in its ISR. If it doesn't the user will sit and wait until 3,584 (4,096-512) bytes have been received. Definitely not the desired effect, although that would only happen if the user enabled the timeout mode on a non-supporting driver. This could be implemented with one additional bit field (Note: I've been trying to minimize the code impact for this timeout functionality in the hopes of making the changes more palatable to maintainers).

I hope that has cleared up my motivation, and I'm going to make the changes noted above to the patch.

Thanks,

Harvey

Further reading (if you're interested):
http://www.lammertbies.nl/comm/info/serial-uart.html#IIR
http://en.wikipedia.org/wiki/16550_UART
http://www.national.com/ds/PC/PC16550D.pdf
http://en.wikibooks.org/wiki/Serial_Programming:8250_UART_Programming

Well, not entirely true, it actually checks bit 0 which indicates that there was an interrupt, period. Bits 1, 2, and 3 indicate why the interrupt occurred, but 8250.c and pxa.c ignore those bits.

That seems a particularly odd way to implement block I/O. Just use a thread for this and the VMIN/VTIME functionality in the n_tty layer - that is why it is there.

Almost all our drivers are not 16550A. A large part of the serial world is now USB and current PCs (especially laptops) often don't include any 16550A devices at all.

Your changes seem to be - hardware specific, not supportable by many devices, and designed to improve a particularly crazy way of handling the I/O in the first place. That doesn't make a good candidate for the upstream kernel.

Alan