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
};
[email protected] 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?
Andrew Morton wrote:
>[email protected] 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
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....
[email protected] 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
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
From: Manfred Spraul <[email protected]>
[email protected] 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