Tobias Ringstrom
2002-Jan-26 19:47 UTC
[PATCH] Added NoDelay config option and nodelay subsystem option
Hello again! Since there was some resistance against adding TCP_NODELAY uncontionally, I've made another patch. The new patch contains the following: * Added a NoDelay yes/no (default no) config option to ssh and sshd * Added -oNoDelay=yes to the ssh command line for sftp. * Changed the sshd subsystem config option syntax from Subsystem name path to Subsystem name options path where options is similar to the option field in fstab. The only options right now are "defaults" and "nodelay". I also added a backwards compatible hack: If the parser finds a slash (/) as the first character of the options field, it emits a warning and assumes defaults. * Changed the sshd_config template to use nodelay by default for the sftp subsystem. I have not updated the documentation yet, but I can do that if you find the patch acceptable. So, what do you all think? The patch is for openssh-3.0.2p1. /Tobias -------------- next part -------------- diff -ru openssh-3.0.2p1.orig/readconf.c openssh-3.0.2p1.cfgnodelay/readconf.c --- openssh-3.0.2p1.orig/readconf.c Wed Oct 3 19:39:39 2001 +++ openssh-3.0.2p1.cfgnodelay/readconf.c Sat Jan 26 19:42:34 2002 @@ -115,7 +115,8 @@ oKbdInteractiveAuthentication, oKbdInteractiveDevices, oHostKeyAlias, oDynamicForward, oPreferredAuthentications, oHostbasedAuthentication, oHostKeyAlgorithms, oBindAddress, oSmartcardDevice, - oClearAllForwardings, oNoHostAuthenticationForLocalhost + oClearAllForwardings, oNoHostAuthenticationForLocalhost, + oNoDelay } OpCodes; /* Textual representations of the tokens. */ @@ -187,6 +188,7 @@ { "smartcarddevice", oSmartcardDevice }, { "clearallforwardings", oClearAllForwardings }, { "nohostauthenticationforlocalhost", oNoHostAuthenticationForLocalhost }, + { "nodelay", oNoDelay }, { NULL, 0 } }; @@ -678,6 +680,10 @@ *intptr = value; break; + case oNoDelay: + intptr = &options->nodelay; + goto parse_flag; + default: fatal("process_config_line: Unimplemented opcode %d", opcode); } @@ -799,6 +805,7 @@ options->bind_address = NULL; options->smartcard_device = NULL; options->no_host_authentication_for_localhost = - 1; + options->nodelay = -1; } /* @@ -919,6 +926,8 @@ clear_forwardings(options); if (options->no_host_authentication_for_localhost == - 1) options->no_host_authentication_for_localhost = 0; + if (options->nodelay == -1) + options->nodelay = 0; /* options->proxy_command should not be set by default */ /* options->user will be set in the main program if appropriate */ /* options->hostname will be set in the main program if appropriate */ diff -ru openssh-3.0.2p1.orig/readconf.h openssh-3.0.2p1.cfgnodelay/readconf.h --- openssh-3.0.2p1.orig/readconf.h Wed Oct 3 19:39:39 2001 +++ openssh-3.0.2p1.cfgnodelay/readconf.h Sat Jan 26 17:34:31 2002 @@ -102,6 +102,7 @@ Forward remote_forwards[SSH_MAX_FORWARDS_PER_DIRECTION]; int clear_forwardings; int no_host_authentication_for_localhost; + int nodelay; } Options; diff -ru openssh-3.0.2p1.orig/servconf.c openssh-3.0.2p1.cfgnodelay/servconf.c --- openssh-3.0.2p1.orig/servconf.c Tue Nov 13 14:03:15 2001 +++ openssh-3.0.2p1.cfgnodelay/servconf.c Sat Jan 26 20:09:29 2002 @@ -109,6 +109,7 @@ options->client_alive_count_max = -1; options->authorized_keys_file = NULL; options->authorized_keys_file2 = NULL; + options->nodelay = -1; } void @@ -229,6 +230,8 @@ } if (options->authorized_keys_file == NULL) options->authorized_keys_file = _PATH_SSH_USER_PERMITTED_KEYS; + if (options->nodelay == -1) + options->nodelay = 0; } /* Keyword tokens. */ @@ -261,6 +264,7 @@ sBanner, sReverseMappingCheck, sHostbasedAuthentication, sHostbasedUsesNameFromPacketOnly, sClientAliveInterval, sClientAliveCountMax, sAuthorizedKeysFile, sAuthorizedKeysFile2, + sNoDelay, sDeprecated } ServerOpCodes; @@ -334,6 +338,7 @@ { "clientalivecountmax", sClientAliveCountMax }, { "authorizedkeysfile", sAuthorizedKeysFile }, { "authorizedkeysfile2", sAuthorizedKeysFile2 }, + { "nodelay", sNoDelay }, { NULL, 0 } }; @@ -801,15 +806,30 @@ fatal("%s line %d: Missing subsystem name.", filename, linenum); for (i = 0; i < options->num_subsystems; i++) - if(strcmp(arg, options->subsystem_name[i]) == 0) + if(strcmp(arg, options->subsystem[i].name) == 0) fatal("%s line %d: Subsystem '%s' already defined.", filename, linenum, arg); - options->subsystem_name[options->num_subsystems] = xstrdup(arg); + options->subsystem[options->num_subsystems].name = xstrdup(arg); arg = strdelim(&cp); if (!arg || *arg == '\0') - fatal("%s line %d: Missing subsystem command.", + fatal("%s line %d: Missing subsystem options and command.", filename, linenum); - options->subsystem_command[options->num_subsystems] = xstrdup(arg); + if (arg[0] == '/') { + log("%s line %d: Subsystem option starts with '/'. " + "Assuming old syntax.", filename, linenum); + } else { + if (strcmp(arg, "nodelay") == 0) + options->subsystem[options->num_subsystems].nodelay = 1; + else if (strcmp(arg, "defaults") != 0) { + fatal("%s line %d: Unknown subsystem option.", + filename, linenum); + } + arg = strdelim(&cp); + if (!arg || *arg == '\0') + fatal("%s line %d: Missing subsystem command.", + filename, linenum); + } + options->subsystem[options->num_subsystems].command = xstrdup(arg); options->num_subsystems++; break; @@ -859,6 +879,10 @@ intptr = &options->client_alive_count_max; goto parse_int; + case sNoDelay: + intptr = &options->nodelay; + goto parse_flag; + case sDeprecated: log("%s line %d: Deprecated option %s", filename, linenum, arg); diff -ru openssh-3.0.2p1.orig/servconf.h openssh-3.0.2p1.cfgnodelay/servconf.h --- openssh-3.0.2p1.orig/servconf.h Wed Sep 12 18:40:06 2001 +++ openssh-3.0.2p1.cfgnodelay/servconf.h Sat Jan 26 17:47:25 2002 @@ -34,6 +34,12 @@ typedef struct { + char *name; + int nodelay; + char *command; +} Subsystem; + +typedef struct { u_int num_ports; u_int ports_from_cmdline; u_short ports[MAX_PORTS]; /* Port number to listen on. */ @@ -108,8 +114,7 @@ char *deny_groups[MAX_DENY_GROUPS]; u_int num_subsystems; - char *subsystem_name[MAX_SUBSYSTEMS]; - char *subsystem_command[MAX_SUBSYSTEMS]; + Subsystem subsystem[MAX_SUBSYSTEMS]; int max_startups_begin; int max_startups_rate; @@ -129,6 +134,7 @@ char *authorized_keys_file; /* File containing public keys */ char *authorized_keys_file2; int pam_authentication_via_kbd_int; + int nodelay; } ServerOptions; diff -ru openssh-3.0.2p1.orig/session.c openssh-3.0.2p1.cfgnodelay/session.c --- openssh-3.0.2p1.orig/session.c Sun Dec 2 00:37:08 2001 +++ openssh-3.0.2p1.cfgnodelay/session.c Sat Jan 26 19:33:57 2002 @@ -1695,13 +1695,19 @@ log("subsystem request for %s", subsys); for (i = 0; i < options.num_subsystems; i++) { - if (strcmp(subsys, options.subsystem_name[i]) == 0) { - cmd = options.subsystem_command[i]; + if (strcmp(subsys, options.subsystem[i].name) == 0) { + cmd = options.subsystem[i].command; if (stat(cmd, &st) < 0) { error("subsystem: cannot stat %s: %s", cmd, strerror(errno)); break; } + if (options.subsystem[i].nodelay) { + int sock = packet_get_connection_out(), on = 1; + if (setsockopt(sock, IPPROTO_TCP, TCP_NODELAY, + (void *)&on, sizeof(on)) < 0) + error("setsockopt TCP_NODELAY: %.100s", strerror(errno)); + } debug("subsystem: exec() %s", cmd); s->is_subsystem = 1; do_exec(s, cmd); diff -ru openssh-3.0.2p1.orig/sftp.c openssh-3.0.2p1.cfgnodelay/sftp.c --- openssh-3.0.2p1.orig/sftp.c Thu Sep 20 02:57:56 2001 +++ openssh-3.0.2p1.cfgnodelay/sftp.c Sat Jan 26 19:47:36 2002 @@ -119,6 +119,7 @@ addargs(&args, "-oForwardX11 no"); addargs(&args, "-oForwardAgent no"); addargs(&args, "-oClearAllForwardings yes"); + addargs(&args, "-oNoDelay yes"); ll = SYSLOG_LEVEL_INFO; infile = stdin; /* Read from STDIN unless changed by -b */ diff -ru openssh-3.0.2p1.orig/ssh_config openssh-3.0.2p1.cfgnodelay/ssh_config --- openssh-3.0.2p1.orig/ssh_config Wed Apr 4 03:58:48 2001 +++ openssh-3.0.2p1.cfgnodelay/ssh_config Sat Jan 26 19:47:14 2002 @@ -33,3 +33,4 @@ # Protocol 2,1 # Cipher blowfish # EscapeChar ~ +# NoDelay yes diff -ru openssh-3.0.2p1.orig/sshconnect.c openssh-3.0.2p1.cfgnodelay/sshconnect.c --- openssh-3.0.2p1.orig/sshconnect.c Wed Oct 10 07:07:45 2001 +++ openssh-3.0.2p1.cfgnodelay/sshconnect.c Sat Jan 26 17:38:50 2002 @@ -377,6 +377,12 @@ sizeof(on)) < 0) error("setsockopt SO_KEEPALIVE: %.100s", strerror(errno)); + /* Set nodelay if requested. */ + if (options.nodelay && + setsockopt(sock, IPPROTO_TCP, TCP_NODELAY, (void *)&on, + sizeof(on)) < 0) + error("setsockopt TCP_NODELAY: %.100s", strerror(errno)); + /* Set the connection. */ packet_set_connection(sock, sock); diff -ru openssh-3.0.2p1.orig/sshd.c openssh-3.0.2p1.cfgnodelay/sshd.c --- openssh-3.0.2p1.orig/sshd.c Mon Nov 12 01:07:12 2001 +++ openssh-3.0.2p1.cfgnodelay/sshd.c Sat Jan 26 17:38:36 2002 @@ -1125,6 +1125,12 @@ sizeof(on)) < 0) error("setsockopt SO_KEEPALIVE: %.100s", strerror(errno)); + /* Set nodelay if requested. */ + if (options.nodelay && + setsockopt(sock_in, IPPROTO_TCP, TCP_NODELAY, (void *)&on, + sizeof(on)) < 0) + error("setsockopt TCP_NODELAY: %.100s", strerror(errno)); + /* * Register our connection. This turns encryption off because we do * not have a key. diff -ru openssh-3.0.2p1.orig/sshd_config openssh-3.0.2p1.cfgnodelay/sshd_config --- openssh-3.0.2p1.orig/sshd_config Fri Sep 21 01:15:44 2001 +++ openssh-3.0.2p1.cfgnodelay/sshd_config Sat Jan 26 19:46:49 2002 @@ -9,6 +9,7 @@ #Protocol 2,1 #ListenAddress 0.0.0.0 #ListenAddress :: +#NoDelay yes # HostKey for protocol version 1 HostKey /etc/ssh_host_key @@ -77,4 +78,4 @@ #Banner /etc/issue.net #ReverseMappingCheck yes -Subsystem sftp /usr/libexec/sftp-server +Subsystem sftp nodelay /usr/libexec/sftp-server
Jim Knoble
2002-Jan-26 23:09 UTC
[PATCH] Added NoDelay config option and nodelay subsystem option
Circa 2002-Jan-26 20:47:45 +0100 dixit Tobias Ringstrom: : * Changed the sshd subsystem config option syntax from : Subsystem name path : to : Subsystem name options path : where options is similar to the option field in fstab. The only options : right now are "defaults" and "nodelay". I also added a backwards : compatible hack: If the parser finds a slash (/) as the first character : of the options field, it emits a warning and assumes defaults. Why not: Subsystem name path [options] ? That way you don't need the cruft to detect forward-slashes, and you can omit the warnings as well, since the options are ... well, optional. -- jim knoble | jmknoble at pobox.com | pobox.com/~jmknoble
Markus Friedl
2002-Jan-27 14:25 UTC
[PATCH] Added NoDelay config option and nodelay subsystem option
On Sat, Jan 26, 2002 at 08:47:45PM +0100, Tobias Ringstrom wrote:> Hello again! > > Since there was some resistance against adding TCP_NODELAY uncontionally, > I've made another patch. The new patch contains the following: > > * Added a NoDelay yes/no (default no) config option to ssh and sshddefault yes :)> * Added -oNoDelay=yes to the ssh command line for sftp. > > * Changed the sshd subsystem config option syntax from > Subsystem name path > to > Subsystem name options pathi don't like this. i'd rather add a per-channel option to the protocol for requesting nodelay.
Kevin Steves
2002-Jan-27 23:47 UTC
[PATCH] Added NoDelay config option and nodelay subsystem option
On Sat, 26 Jan 2002, Tobias Ringstrom wrote: :So, what do you all think? The patch is for openssh-3.0.2p1. what is the overall methodology you have for openssh and nagle? i'd rather not have a tunable for every possible TCP endpoint and usage variation. my somewhat high-level thoughts are currently: non-ssh connection endpoints (those not going over the SSH transport) should not use nagle: connect_to(), channel_post_port_lister() for the ssh transport connection, we use nagle by default only if it is a session channel with a pty and shell or exec channel other channel types would cause connection to be nodelay on both sides (as determined and set by ssh and sshd) nodelay yes/no would be available to change default for pty,shell,exec only behavior.
Nicolas Williams
2002-Jan-28 16:36 UTC
[PATCH] Added NoDelay config option and nodelay subsystem option
On Sun, Jan 27, 2002 at 03:47:03PM -0800, Kevin Steves wrote:> On Sat, 26 Jan 2002, Tobias Ringstrom wrote: > for the ssh transport connection, we use nagle by default only if it is a > session channel with a pty and shell or exec channel > > other channel types would cause connection to be nodelay on both sides (as > determined and set by ssh and sshd)Forwarded SSH agents should not require TCP_NODELAY. Forwarded TCP ports in general, and forwarded X11 displays specifically, should. Turning off Nagle when a non-agent forwarded port is opened would be a simple patch.> nodelay yes/no would be available to change default for pty,shell,exec > only behavior.Yeah. Nico -- -DISCLAIMER: an automatically appended disclaimer may follow. By posting- -to a public e-mail mailing list I hereby grant permission to distribute- -and copy this message.- Visit our website at ubswarburg.com This message contains confidential information and is intended only for the individual named. If you are not the named addressee you should not disseminate, distribute or copy this e-mail. Please notify the sender immediately by e-mail if you have received this e-mail by mistake and delete this e-mail from your system. E-mail transmission cannot be guaranteed to be secure or error-free as information could be intercepted, corrupted, lost, destroyed, arrive late or incomplete, or contain viruses. The sender therefore does not accept liability for any errors or omissions in the contents of this message which arise as a result of e-mail transmission. If verification is required please request a hard-copy version. This message is provided for informational purposes and should not be construed as a solicitation or offer to buy or sell any securities or related financial instruments.
Kevin Steves
2002-Jan-29 20:02 UTC
[PATCH] Added NoDelay config option and nodelay subsystem option
On Tue, 29 Jan 2002, Rick Jones wrote: :> Yes. But the problem probably is that the old Nagle algorithm and TCP :> congestion control in general probably needs revision to really work :> in newer networks. : :Hmm, an interesting comment to leave dangling out there, but probably :best for tcp-impl :) well, most issues seem to stem from poor intereaction with fixed-timer delayed ACKs. are any stack doing something more intelligent?
Rick Jones
2002-Jan-29 20:20 UTC
[PATCH] Added NoDelay config option and nodelay subsystem option
Kevin Steves wrote:> :Hmm, an interesting comment to leave dangling out there, but probably > :best for tcp-impl :) > > well, most issues seem to stem from poor intereaction with fixed-timer > delayed ACKs. are any stack doing something more intelligent?Only if they can be clairvoyant. One might initially say that any sub-MSS send should get an immediate ACK, but then you start sending all these otherwise bogus ACKs for otherwise correctly behaving apps - for example, the request/response of a single-byte (sub-mss) netperf TCP_RR test (think really fast typist :) would go from two TCP segments per transaction to four. That would basically double the CPU time spent in the networking stack. The stack would need to know whether or not the recieving app was going to send data the other way any time soon and whether or not the other side had more data to send. The best one can do from what I can think of as I type is to use a shorter standalone ACK interval and work to make sure that apps which interact poorly with Nagle are either writen to behave properly or if they are already behaving properly to (as much as I hate to say) disable Nagle. rick jones -- Wisdom Teeth are impacted, people are affected by the effects of events. these opinions are mine, all mine; HP might not want them anyway... :) feel free to post, OR email to raj in cup.hp.com but NOT BOTH...
mouring
2002-Feb-01 15:15 UTC
[PATCH] Added NoDelay config option and nodelay subsystem option
> On Fri, 1 Feb 2002, Markus Friedl wrote: > > > On Fri, Feb 01, 2002 at 11:38:02AM +0100, Tobias Ringstrom wrote: > > > IMHO, you > > > would also benefit from a development model with a development and a > > > stable branch. > > > > I don't see why? > > > > The OpenSSH-current from the CVS is the development branch. > > I don't see how this helps for testing your patches. I don't > > think candidate patches should go into the development > > before everyone tests the patches. Even patches for > > the development branch need testing before the get into > > the development branch. > > Testing yes, but how much? I think mailing list members are much more > likely to test release candidates (or even snapshots) than to patch and > test individual patches. >openssh.com/portable.html has snapshots every night. Yet no large mass of people are testing it.> Basic testing by the developer and a quick review should be enough to > apply the patch, IMHO. If further user based testing reveals problems, > they can be fixed, or the patch reverted. > > > However, we're going to have a time based release schedule, > > OpenBSD benefits from this, and OpenSSH will probaly, too. > > Just add release candidates some time before the release, and I'm sure it > will work out nicely. >There is a at least 3 calls I try to make before we release portable tree asking for people to do compile tests of the current snapshots.. Some days I hear feedback. Some days I don't. Without flying out to every person's house on this list and beeting them with a blunt object until they finally download and test there is not much we can do. (Personal Note: Feel safe.. I'm not a person to do such things. =) I always try to get a Linux (Mandrake is this flavor of the month), OpenStep 4.2, and Solaris 2.5.1 and 7 compiling in before we make a release. I know a few others in the portable group try to test on all SCOs before the relase, but there are too many possible combinations to test and I know for a fact I'm not in the position to test everything from krb4 to s/key at my house (Even if I've compiled s/key in the past to help debug why it was broken). Doing OpenSSH-3.1.0-Pre2, pre3, pre4-mouring-fixing-bug release will not help us. Hell it barely works in the Linux community!!! =) On a side note: ANY PATCHES THAT ARE NOT IN BUGZILLA WILL/MAY BE LOST. PLEASE ENSURE THEY GET INTO THE TRACKING SYSTEM. - Ben