[PATCH] serial inter-character timeout usage with async io

[PATCH] serial inter-character timeout usage with async io

[Harvey Chapman]

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;

Re: [PATCH] serial inter-character timeout usage with async io

[Alan Cox]

> 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.

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.

Re: [PATCH] serial inter-character timeout usage with async io

[Harvey Chapman]

Alan Cox wrote:
> What are you actually trying to achieve ?

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.

> 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.

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

Re: [PATCH] serial inter-character timeout usage with async io

[Harvey Chapman]

Harvey Chapman wrote:
> 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

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.

Re: [PATCH] serial inter-character timeout usage with async io

[Alan Cox]

> 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.

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.

> 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

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