[PATCH] select fix

[PATCH] select fix

[Andries.Brouwer]

Recently people complained on lk about a problem: one sees

select(2, NULL, [1], NULL, NULL)        = 1 (out [1])
write(1, "hi\n", 3)                     = -1 EAGAIN (Resource temporarily unavailable)

for a stopped tty opened with O_NONBLOCK. This violates POSIX,
and the 100% CPU use in a select loop does not look pretty either.
The below fixes this.

Andries

diff -u --recursive --new-file -X /linux/dontdiff a/drivers/char/n_tty.c b/drivers/char/n_tty.c
--- a/drivers/char/n_tty.c	Thu Jul  3 09:26:43 2003
+++ b/drivers/char/n_tty.c	Tue Jul 29 16:53:10 2003
@@ -1231,7 +1231,8 @@
 }
 
 /* Called without the kernel lock held - fine */
-static unsigned int normal_poll(struct tty_struct * tty, struct file * file, poll_table *wait)
+static unsigned int
+normal_poll(struct tty_struct *tty, struct file *file, poll_table *wait)
 {
 	unsigned int mask = 0;
 
@@ -1251,27 +1252,27 @@
 		else
 			tty->minimum_to_wake = 1;
 	}
-	if (tty->driver->chars_in_buffer(tty) < WAKEUP_CHARS)
+	if (!tty->stopped && tty->driver->chars_in_buffer(tty) < WAKEUP_CHARS)
 		mask |= POLLOUT | POLLWRNORM;
 	return mask;
 }
 
 struct tty_ldisc tty_ldisc_N_TTY = {
-	TTY_LDISC_MAGIC,	/* magic */
-	"n_tty",		/* name */
-	0,			/* num */
-	0,			/* flags */
-	n_tty_open,		/* open */
-	n_tty_close,		/* close */
-	n_tty_flush_buffer,	/* flush_buffer */
-	n_tty_chars_in_buffer,	/* chars_in_buffer */
-	read_chan,		/* read */
-	write_chan,		/* write */
-	n_tty_ioctl,		/* ioctl */
-	n_tty_set_termios,	/* set_termios */
-	normal_poll,		/* poll */
-	n_tty_receive_buf,	/* receive_buf */
-	n_tty_receive_room,	/* receive_room */
-	n_tty_write_wakeup	/* write_wakeup */
+	.magic =		TTY_LDISC_MAGIC,
+	.name =			"n_tty",
+	.num =			0,
+	.flags =		0,
+	.open =			n_tty_open,
+	.close =		n_tty_close,
+	.flush_buffer =		n_tty_flush_buffer,
+	.chars_in_buffer =	n_tty_chars_in_buffer,
+	.read =			read_chan,
+	.write =		write_chan,
+	.ioctl =		n_tty_ioctl,
+	.set_termios =		n_tty_set_termios,
+	.poll =			normal_poll,
+	.receive_buf =		n_tty_receive_buf,
+	.receive_room =		n_tty_receive_room,
+	.write_wakeup =		n_tty_write_wakeup
 };
 

Re: [PATCH] select fix

[Andrew Morton]

Andries.Brouwer@cwi.nl wrote:
>
> Recently people complained on lk about a problem: one sees
> 
> select(2, NULL, [1], NULL, NULL)        = 1 (out [1])
> write(1, "hi\n", 3)                     = -1 EAGAIN (Resource temporarily unavailable)
> 
> for a stopped tty opened with O_NONBLOCK. This violates POSIX,
> and the 100% CPU use in a select loop does not look pretty either.
> The below fixes this.
> ...
>
> -	if (tty->driver->chars_in_buffer(tty) < WAKEUP_CHARS)
> +	if (!tty->stopped && tty->driver->chars_in_buffer(tty) < WAKEUP_CHARS)
>  		mask |= POLLOUT | POLLWRNORM;

Manfred sent a patch through esterday which addresses it this way:

-	if (tty->driver->chars_in_buffer(tty) < WAKEUP_CHARS)
+	if (tty->driver->chars_in_buffer(tty) < WAKEUP_CHARS &&
+			tty->driver->write_room(tty) > 0)

Any preferences?

Re: [PATCH] select fix

[Valdis.Kletnieks]

[-- Attachment #1: Type: text/plain, Size: 694 bytes --]

On Tue, 29 Jul 2003 10:36:30 PDT, Andrew Morton said:

> > -	if (tty->driver->chars_in_buffer(tty) < WAKEUP_CHARS)
> > +	if (!tty->stopped && tty->driver->chars_in_buffer(tty) < WAKEUP_CHARS)
> >  		mask |= POLLOUT | POLLWRNORM;
> 
> Manfred sent a patch through esterday which addresses it this way:
> 
> -	if (tty->driver->chars_in_buffer(tty) < WAKEUP_CHARS)
> +	if (tty->driver->chars_in_buffer(tty) < WAKEUP_CHARS &&
> +			tty->driver->write_room(tty) > 0)
> 
> Any preferences?

Would including all 3 conditions make sense?  Not sure if it should be A&B&C, or
A&(B|C) though, but it certainly smells like the write_room() and tty->stopped
checks are covering 2 different corner cases....

[-- Attachment #2: Type: application/pgp-signature, Size: 226 bytes --]

Re: [PATCH] select fix

[Manfred Spraul]

Andrew Morton wrote:

>Andries.Brouwer@cwi.nl wrote:
>  
>
>>-	if (tty->driver->chars_in_buffer(tty) < WAKEUP_CHARS)
>>+	if (!tty->stopped && tty->driver->chars_in_buffer(tty) < WAKEUP_CHARS)
>> 		mask |= POLLOUT | POLLWRNORM;
>>    
>>
>
>Manfred sent a patch through esterday which addresses it this way:
>
>-	if (tty->driver->chars_in_buffer(tty) < WAKEUP_CHARS)
>+	if (tty->driver->chars_in_buffer(tty) < WAKEUP_CHARS &&
>+			tty->driver->write_room(tty) > 0)
>
>Any preferences?
>  
>
Who should implement tty->stopped? AFAICS tty->stopped is implemented in 
the drivers right now, and my patch would leave that unchanged.

--
    Manfred

Re: [PATCH] select fix

[Manfred Spraul]

Valdis.Kletnieks@vt.edu wrote:

>On Tue, 29 Jul 2003 10:36:30 PDT, Andrew Morton said:
>
>  
>
>>>-	if (tty->driver->chars_in_buffer(tty) < WAKEUP_CHARS)
>>>+	if (!tty->stopped && tty->driver->chars_in_buffer(tty) < WAKEUP_CHARS)
>>> 		mask |= POLLOUT | POLLWRNORM;
>>>      
>>>
>>Manfred sent a patch through esterday which addresses it this way:
>>
>>-	if (tty->driver->chars_in_buffer(tty) < WAKEUP_CHARS)
>>+	if (tty->driver->chars_in_buffer(tty) < WAKEUP_CHARS &&
>>+			tty->driver->write_room(tty) > 0)
>>
>>Any preferences?
>>    
>>
>
>Would including all 3 conditions make sense?  Not sure if it should be A&B&C, or
>A&(B|C) though, but it certainly smells like the write_room() and tty->stopped
>checks are covering 2 different corner cases....
>  
>
No. select() and write() must agree when -EAGAIN happens.
write() will fail if write_room() returns 0. Additionally, we want to 
delay wakeups a bit, to reduce context switches.
The problem is that the console driver implements stopping by returning 
0 from ->write_room() - therefore "less than WAKEUP_CHARS in buffer" is 
not equivalent to "write will not return -EAGAIN", and thus user space 
loops. My patch fixes that by checking ->write_room() in normal_poll.

Perhaps the Right Thing (tm) is
 >    if (tty->driver->write_room(tty) > WAKEUP_CHARS)
 >        mask |= POLLOUT | POLLWRNORM;

but I simply to not understand the tty layer at all, thus I proposed the 
minimal patch.

--
    Manfred

Re: [PATCH] select fix

[Andries.Brouwer]

	From: Manfred Spraul <manfred@colorfullife.com>

	Valdis.Kletnieks@vt.edu wrote:

	> Would including all 3 conditions make sense?

I hesitated for a moment. These conditions are not entirely
equivalent and neither implies the other. But Manfreds
version is better. Many drivers have a write_room() that
only counts characters and the corresponding write() will fill
the buffer and only check tty->stopped before actually transmitting.
So write_room() is a better predictor.

Andries

Re: [PATCH] select fix

[Andries.Brouwer]

current:

        if (tty->driver->chars_in_buffer(tty) < WAKEUP_CHARS)
                mask |= POLLOUT | POLLWRNORM;

Andries:

>> if (!tty->stopped &&

Manfred:

>> if (.. && tty->driver->write_room(tty) > 0)

Andrew:

> Any preferences?

I prefer Manfred's version.


Andries