lkml.org 
[lkml]   [2002]   [Jun]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
From
Subjectn_tty.c driver patch (semantic and performance correction) (all recent versions)
Date
Greetings,

Pleased find attached my first-ever contribution to kernel code... 8-) It is
both a "fix" and an "enhancement" (which I know is bad) but the same lines of
code are involved in both so what are you going to do?)

Kernel Versions: 2.2, 2.4, 2.5 (all of them since 1996 really 8-)

The n_tty line discipline module contains a "semantic error" that limits its
speed and usefullness in many uses. The attached patch directly addresses
serial performance in a completely backwards-compatable way.

In particular, the current handling of VMIN hugely limits, complicates, and/or
slows down optimal serial use. The most obvius example is that if you call
read(2) with a buffer size less than the current value of VMIN, the line
discipline will insist tha the read call wait for characters that can not be
returned to that call. The POSIX standard is silent on the subject of
whether this is right or wrong. Common sense says it is wrong.

What does it mean? If you are running a protocol with variable packet sizes
you either have to repeatedly reset VMIN or just set it to one (1) and
potentially bounce in and out of the kernel. In fact, if you look at
virtually any code in the world you will find that VMIN is almost always 1 or
0. In any cases where it has been set larger than 1 (I havn't found any) the
buffering of non-fixed-size frames would have to rely on timeouts (which
would enherantly cost time) or still end up doing in-user-space packet
recomposition.

Esentially, as it exists today, the in-kernel buffering possibilities must be
almost unilaterally ignored at the cost of extra context switches and such.

What this patch does to fix this "semantic error" is fairly simple, if VMIN is
larger than the current read buffer, the blocking code acts like VMIN were
set to the buffer size. (The actual value of VMIN is not changed, the lesser
of VMIN and current read size is always in effect.)

This fix handles the near-end small-read requirements. But VMIN is an
unsigned character so the entire standard can not ever be set to explicitly
satisfy a read of more than 255 characters. A given read *CAN* return more
than 255 characters if they have already been buffered, but all the
multiple-wakeup and user-space buffer reassembly caveats still apply.
Consider a simple protocol (Xmodem?) that sends 1024 byte packets at
modem-esque speeds. Such a protocol would tend to require more than four
reads per packet. Yech...

The "extension" is that if VMIN is set to 255 the effective VMIN for any given
read is the exact size of the read buffer. That is the minimum receive size
becomes "set but unconstrained".

The extension is "different than" setting VMIN to 0. When VMIN == 0, VTIME is
an absolute timeout for the entire read (the existing behavior) but when VMIN
!= 0 VTIME is the "intercharacter timer", e.g. a timeout event that is reset
to time=0 whenever any characters are received. At high baud rates this is
fine but at low baud you get back into context-switches and packet
reassembly. With the VMIN==255 version you get the full scaled timeout but
the reduced context switch and packet assembly behavior.

The patch is more comment than code 8-) and I wasn't sure how to split the
patch up into the two components (fix separate from extension) since they
involve the same lines of code. [e.g. the guidelines didn't say whether this
would be separated by making the "fix-and-extension" version a diff of the
"fix" version or the original file, so I opted for the less-messy
just-one-patch. 8-)]

The fix is more important than the enhancement. The total text of the fix is
the first non-comment chunk without the "(minimum == 255)" and without "the
other code chunk" entirely.

The second code chunk in the patch prevents a deadlock in the flow-control
mechanisim of the UART driver. If the read buffer size is "too big" the
internal driver buffer could fill and cause the serial flow-control logic to
trip, which would, of course, stop characters from arriving. This second
chunk sets a rational ceiling on the minimum_to_wake, which makes the
partial-copy mechanisim transfer the driver buffer contents into the read
buffer. This was previously handled by the assumption that VMIN was
(effectively) never larger than 255. Now that it can be effectively larger
this block sets the ceiling explicitly.

(Wow, that was a lot of text for this little thing... 8-)

Rob White
Newcastle, WA
rwhite@pobox.com

(Sorry, I don't speak manual page so I didn't patch it... 8-)

BEGIN PATCH:

--- drivers/char/n_tty.c.orig Tue Jan 29 18:26:54 2002
+++ drivers/char/n_tty.c Thu Jan 31 02:28:51 2002
@@ -18,11 +18,35 @@
* This file may be redistributed under the terms of the GNU General Public
* License.
*
+ * 2002/01/29 Fixed & Extended VMIN handling.
+ * Patch By: Robert White <rwhite@pobox.com>
+ * Problem: Where VTIME == 0 and VMIN > nr durring read,
+ * read would block for characters it couldn't possibly return.
+ * Reading variable sized blocks required multiple syscalls
+ * (termio set followed by read) or reliance on timeout.
+ * or multiple reads of fragments. For large reads n_tty
+ * was not capible of returning more than 255 bytes (bad
+ * for performance of any serial protocols. c.f. Z-MODEM etc)
+ * Fix: if VMIN > nr, use nr to set minimum instead. (adjust down)
+ * Extension: if VMIN == 255, always set minimum to nr (up OR down)
+ * (Fix and Extension apply to each read call, VMIN itself is not adjusted)
+ * Consider: (w/8 or 1500 chars from device to be delivered respectively)
+ * VMIN=30, VTIME=0, read(fd,buf,8);
+ * Old: read never returns, New: returns immed on 8th char
+ * VMIN=30, VTIME=200, read(fd,buf,8);
+ * Old: return in 20 seconds, New: returns immed on 8th char
+ * VMIN=255, VTIME=0, read(fd,buf,1500);
+ * Old: 5 reads of ~255 then deadlock, New: 1 read to complete
+ * VMIN=255, VTIME>0, read(fd,buf,1500);
+ * Old: 5 reads then timout for remainder, New: 1 read
+ * or more depending timeout occurences)
+ *
* Reduced memory usage for older ARM systems - Russell King
*
* 2000/01/20 Fixed SMP locking on put_tty_queue using bits of
* the patch by Andrew J. Kroll <ag784@freenet.buffalo.edu>
* who actually finally proved there really was a race.
+ *
*/

#include <linux/types.h>
@@ -974,6 +998,11 @@
if (!tty->icanon) {
time = (HZ / 10) * TIME_CHAR(tty);
minimum = MIN_CHAR(tty);
+ /* Added rwhite@pobox.com Jan 29, 2002 */
+ if ((minimum == 255) || (minimum > nr)) {
+ minimum = nr;
+ }
+ /* End Addition */
if (minimum) {
if (time)
tty->minimum_to_wake = 1;
@@ -1021,6 +1050,16 @@
if (((minimum - (b - buf)) < tty->minimum_to_wake) &&
((minimum - (b - buf)) >= 1))
tty->minimum_to_wake = (minimum - (b - buf));
+
+ /* Added rwhite@pobox.com Jan 29, 2002 */
+ // minimum and therefore minimum_to_wake could be much larger
+ // than the actual buffer here, so...
+ if (tty->minimum_to_wake >= TTY_FLIPBUF_SIZE) {
+ // Flow Control would deadlock at (N_TTY_BUF_SIZE -
TTY_THRESHOLD_THROTTLE)
+ // using TTY_FLIPBUF_SIZE-1 is safe and likely linear/streaming.
+ tty->minimum_to_wake = TTY_FLIPBUF_SIZE - 1;
+ }
+ /* End Addition */

if (!input_available_p(tty, 0)) {
if (test_bit(TTY_OTHER_CLOSED, &tty->flags)) {

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

\
 
 \ /
  Last update: 2005-03-22 13:22    [W:0.050 / U:27.032 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site