openssh unix dev - Jan 2002 - [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

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 | http://www.pobox.com/~jmknoble/

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 sshd
default 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  path
i don't like this.

i'd rather add a per-channel option to
the protocol for requesting nodelay.

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.

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 http://www.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.

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?

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

> 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.
> 
http://www.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