[PATCH v2] Enable TCP_NODELAY by default and deprecate --tcp-nodelay

From: Antonio Quartulli <[email protected]>

TCP_NODELAY disables Nagle's algorithm, which is
beneficial for tunnelled traffic and has long been
recommended for OpenVPN over TCP. Until now it had
to be requested explicitly via --tcp-nodelay or
"socket-flags TCP_NODELAY".

Enable it unconditionally on every TCP socket (dco-win
is skipped as it manages its own socket) and drop the
socket_set_flags()/link_socket_update_flags() plumbing
that only ever applied it; the SF_TCP_NODELAY sockflag
is now unused but kept reserved.

--tcp-nodelay and "socket-flags TCP_NODELAY" become
no-ops, kept for backwards compatibility; the
--tcp-nodelay helper, its SF_TCP_NODELAY_HELPER flag
and the client-side warning are dropped as well.

Measured over a p2p TCP tunnel: bulk throughput is
unchanged (~1.7 Gbit/s, within 0.4%), while
small-packet interactive latency improves sharply
where Nagle bites: a request/response turn that
spans several packets drops from ~82 ms to ~0.03 ms
round-trip. A turn of a single packet stays fast
with or without TCP_NODELAY, as Nagle cannot delay
a lone packet.

Change-Id: I434a5373f77b0f7570a6a2aafe0eaf970e2eabe7
Signed-off-by: Antonio Quartulli <[email protected]>
Acked-by: Arne Schwabe <[email protected]>
Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1797
---

This change was reviewed on Gerrit and approved by at least one
developer. I request to merge it to master.

Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1797
This mail reflects revision 2 of this Change.

Acked-by according to Gerrit (reflected above):
Arne Schwabe <[email protected]>

        
diff --git a/doc/man-sections/link-options.rst b/doc/man-sections/link-options.rst
index df8c917..ce7200f 100644
--- a/doc/man-sections/link-options.rst
+++ b/doc/man-sections/link-options.rst
@@ -465,24 +465,19 @@
   trying to group several smaller packets into a larger packet.  This can
   result in a considerably improvement in latency.
 
-  This option is pushable from server to client, and should be used on
-  both client and server for maximum effect.
+  Since OpenVPN 2.7 :code:`TCP_NODELAY` is enabled by default on every TCP
+  socket, so specifying it here is no longer necessary.
 
 --tcp-nodelay
-  This macro sets the :code:`TCP_NODELAY` socket flag on the server as well
-  as pushes it to connecting clients. The :code:`TCP_NODELAY` flag disables
-  the Nagle algorithm on TCP sockets causing packets to be transmitted
-  immediately with low latency, rather than waiting a short period of time
-  in order to aggregate several packets into a larger containing packet.
-  In VPN applications over TCP, :code:`TCP_NODELAY` is generally a good
-  latency optimization.
+  (DEPRECATED) The :code:`TCP_NODELAY` socket flag is now enabled by default
+  on every TCP socket, so this option no longer has any effect and is
+  ignored.
 
-  The macro expands as follows:
-  ::
-
-     if mode server:
-         socket-flags TCP_NODELAY
-         push "socket-flags TCP_NODELAY"
+  :code:`TCP_NODELAY` disables the Nagle algorithm on TCP sockets, causing
+  packets to be transmitted immediately with low latency, rather than
+  waiting a short period of time in order to aggregate several packets into
+  a larger containing packet. In VPN applications over TCP this is generally
+  a good latency optimization.
 
 --max-packet-size size
   This option will instruct OpenVPN to try to limit the maximum on-write packet
diff --git a/src/openvpn/helper.c b/src/openvpn/helper.c
index 4c540a6..be2e246 100644
--- a/src/openvpn/helper.c
+++ b/src/openvpn/helper.c
@@ -99,14 +99,6 @@
     return BSTR(&out);
 }
 
-static const char *
-print_str(const char *str, struct gc_arena *gc)
-{
-    struct buffer out = alloc_buf_gc(128, gc);
-    buf_printf(&out, "%s", str);
-    return BSTR(&out);
-}
-
 static void
 helper_add_route(const in_addr_t network, const in_addr_t netmask, struct options *o)
 {
@@ -587,32 +579,3 @@
         }
     }
 }
-
-/*
- *
- * HELPER DIRECTIVE:
- *
- * tcp-nodelay
- *
- * EXPANDS TO:
- *
- * if mode server:
- *   socket-flags TCP_NODELAY
- *   push "socket-flags TCP_NODELAY"
- */
-void
-helper_tcp_nodelay(struct options *o)
-{
-    if (o->server_flags & SF_TCP_NODELAY_HELPER)
-    {
-        if (o->mode == MODE_SERVER)
-        {
-            o->sockflags |= SF_TCP_NODELAY;
-            push_option(o, print_str("socket-flags TCP_NODELAY", &o->gc), M_USAGE);
-        }
-        else
-        {
-            o->sockflags |= SF_TCP_NODELAY;
-        }
-    }
-}
diff --git a/src/openvpn/helper.h b/src/openvpn/helper.h
index d29257c..d4e7015 100644
--- a/src/openvpn/helper.h
+++ b/src/openvpn/helper.h
@@ -35,6 +35,4 @@
 
 void helper_client_server(struct options *o);
 
-void helper_tcp_nodelay(struct options *o);
-
 #endif
diff --git a/src/openvpn/init.c b/src/openvpn/init.c
index caaa769..914d191 100644
--- a/src/openvpn/init.c
+++ b/src/openvpn/init.c
@@ -2650,15 +2650,6 @@
         }
     }
 
-    if (found & OPT_P_SOCKFLAGS)
-    {
-        msg(D_PUSH, "OPTIONS IMPORT: --socket-flags option modified");
-        for (int i = 0; i < c->c1.link_sockets_num; i++)
-        {
-            link_socket_update_flags(c->c2.link_sockets[i], c->options.sockflags);
-        }
-    }
-
     if (found & OPT_P_PERSIST)
     {
         msg(D_PUSH, "OPTIONS IMPORT: --persist options modified");
diff --git a/src/openvpn/options.c b/src/openvpn/options.c
index 87218d4..f3af379 100644
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -486,8 +486,7 @@
     "                  virtual address table to v.\n"
     "--bcast-buffers n : Allocate n broadcast buffers.\n"
     "--tcp-queue-limit n : Maximum number of queued TCP output packets.\n"
-    "--tcp-nodelay   : Macro that sets TCP_NODELAY socket flag on the server\n"
-    "                  as well as pushes it to connecting clients.\n"
+    "--tcp-nodelay   : (DEPRECATED) TCP_NODELAY is now enabled by default.\n"
     "--learn-address cmd : Run command cmd to validate client virtual addresses.\n"
     "--connect-freq n s : Allow a maximum of n new connections per s seconds.\n"
     "--connect-freq-initial n s : Allow a maximum of n replies for initial connections attempts per s seconds.\n"
@@ -2644,12 +2643,6 @@
                       "verify-client-cert");
         MUST_BE_FALSE(options->ssl_flags & SSLF_USERNAME_AS_COMMON_NAME, "username-as-common-name");
         MUST_BE_FALSE(options->ssl_flags & SSLF_AUTH_USER_PASS_OPTIONAL, "auth-user-pass-optional");
-        if (options->server_flags & SF_TCP_NODELAY_HELPER)
-        {
-            msg(M_WARN, "WARNING: setting tcp-nodelay on the client side will not "
-                        "affect the server. To have TCP_NODELAY in both direction use "
-                        "tcp-nodelay in the server configuration instead.");
-        }
         MUST_BE_UNDEF(auth_user_pass_verify_script, "auth-user-pass-verify");
         MUST_BE_UNDEF(auth_token_generate, "auth-gen-token");
 #if PORT_SHARE
@@ -3769,7 +3762,6 @@
     /* must be called after helpers that might set --mode */
     helper_setdefault_topology(o);
     helper_keepalive(o);
-    helper_tcp_nodelay(o);
 
     if (o->mode == MODE_SERVER)
     {
@@ -6535,11 +6527,9 @@
         VERIFY_PERMISSION(OPT_P_SOCKFLAGS);
         for (j = 1; j < MAX_PARMS && p[j]; ++j)
         {
-            if (streq(p[j], "TCP_NODELAY"))
-            {
-                options->sockflags |= SF_TCP_NODELAY;
-            }
-            else
+            /* TCP_NODELAY is enabled by default; the flag is still accepted
+             * for backwards compatibility but no longer has any effect */
+            if (!streq(p[j], "TCP_NODELAY"))
             {
                 msg(msglevel, "unknown socket flag: %s", p[j]);
             }
@@ -7715,7 +7705,7 @@
     else if (streq(p[0], "tcp-nodelay") && !p[1])
     {
         VERIFY_PERMISSION(OPT_P_GENERAL);
-        options->server_flags |= SF_TCP_NODELAY_HELPER;
+        msg(M_WARN, "DEPRECATED OPTION: --tcp-nodelay is ignored, it is now enabled by default.");
     }
     else if (streq(p[0], "stale-routes-check") && p[1] && !p[3])
     {
diff --git a/src/openvpn/options.h b/src/openvpn/options.h
index a111cf8..7aa7928 100644
--- a/src/openvpn/options.h
+++ b/src/openvpn/options.h
@@ -470,8 +470,7 @@
     unsigned int server_netbits_ipv6;    /* IPv6 */
 
 #define SF_NOPOOL                (1 << 0)
-#define SF_TCP_NODELAY_HELPER    (1 << 1)
-#define SF_NO_PUSH_ROUTE_GATEWAY (1 << 2)
+#define SF_NO_PUSH_ROUTE_GATEWAY (1 << 1)
     unsigned int server_flags;
 
     bool server_bridge_proxy_dhcp;
diff --git a/src/openvpn/socket.c b/src/openvpn/socket.c
index df2cc9e..b73a5df 100644
--- a/src/openvpn/socket.c
+++ b/src/openvpn/socket.c
@@ -516,34 +516,6 @@
 #endif
 }
 
-static bool
-socket_set_flags(socket_descriptor_t sd, unsigned int sockflags)
-{
-    /* SF_TCP_NODELAY doesn't make sense for dco-win */
-    if ((sockflags & SF_TCP_NODELAY) && (!(sockflags & SF_DCO_WIN)))
-    {
-        return socket_set_tcp_nodelay(sd, 1);
-    }
-    else
-    {
-        return true;
-    }
-}
-
-bool
-link_socket_update_flags(struct link_socket *sock, unsigned int sockflags)
-{
-    if (sock && socket_defined(sock->sd))
-    {
-        sock->sockflags |= sockflags;
-        return socket_set_flags(sock->sd, sock->sockflags);
-    }
-    else
-    {
-        return false;
-    }
-}
-
 void
 link_socket_update_buffer_sizes(struct link_socket *sock, int rcvbuf, int sndbuf)
 {
@@ -1485,8 +1457,12 @@
 static void
 phase2_set_socket_flags(struct link_socket *sock)
 {
-    /* set misc socket parameters */
-    socket_set_flags(sock->sd, sock->sockflags);
+    /* TCP_NODELAY is enabled by default on every TCP socket; dco-win is
+     * skipped as it manages its own socket */
+    if (proto_is_tcp(sock->info.proto) && !(sock->sockflags & SF_DCO_WIN))
+    {
+        socket_set_tcp_nodelay(sock->sd, 1);
+    }
 
     /* set socket to non-blocking mode */
     set_nonblock(sock->sd);
diff --git a/src/openvpn/socket.h b/src/openvpn/socket.h
index cd4e8ed..5883592 100644
--- a/src/openvpn/socket.h
+++ b/src/openvpn/socket.h
@@ -207,7 +207,7 @@
     int mtu; /* OS discovered MTU, or 0 if unknown */
 
 #define SF_USE_IP_PKTINFO    (1 << 0)
-#define SF_TCP_NODELAY       (1 << 1)
+#define SF_TCP_NODELAY       (1 << 1) /* unused: flag always enabled */
 #define SF_PORT_SHARE        (1 << 2)
 #define SF_HOST_RANDOMIZE    (1 << 3)
 #define SF_GETADDRINFO_DGRAM (1 << 4)
@@ -390,8 +390,6 @@
 
 void setenv_trusted(struct env_set *es, const struct link_socket_info *info);
 
-bool link_socket_update_flags(struct link_socket *sock, unsigned int sockflags);
-
 void link_socket_update_buffer_sizes(struct link_socket *sock, int rcvbuf, int sndbuf);
 
 /*