[Openvpn-devel] Speedup TCP remote hosts connections

Message ID 20200927233224.2859-1-themiron@yandex-team.ru
State Superseded
Headers show
Series [Openvpn-devel] Speedup TCP remote hosts connections | expand

Commit Message

Vladislav Grishenko Sept. 27, 2020, 1:32 p.m. UTC
For non-blocking TCP/Unix connection, OpenVPN checks was it established in
loop and if not - sleeps or handles management for next one second. Since
the first check is made right after the connection attempt, it will likely
be always unsuccessful, resulting in redundant wait for one or more seconds:

    00:00:00.667607 fcntl(5, F_SETFL, O_RDONLY|O_NONBLOCK) = 0
    00:00:00.667713 connect(5, {...}, 16) = -1 EINPROGRESS (Operation now in progress)
    00:00:00.667832 poll([{fd=5, events=POLLOUT}], 1, 0) = 0 (Timeout)
    00:00:00.667954 nanosleep({tv_sec=1, tv_nsec=0}, 0x7fff52450270) = 0
    00:00:01.668608 poll([{fd=5, events=POLLOUT}], 1, 0) = 1 ([{fd=5, revents=POLLOUT}])

With this patch openvpn_connect() will wait for connection establishment for
the first second (if possible) and will return success immediately: this
speedups connection and provides real connection time that can be used for
detection of the fastest remote server in subsequent patches:

    00:00:00.019133 fcntl(5, F_SETFL, O_RDONLY|O_NONBLOCK) = 0
    00:00:00.019265 connect(5, {...}, 16) = -1 EINPROGRESS (Operation now in progress)
    00:00:00.019388 poll([{fd=5, events=POLLOUT}], 1, 1000) = 1 ([{fd=5, revents=POLLOUT}])

If connection still can't established - this should be treated as either too
slow/far or non-responding server, so imprecise connection checks every next
one second in loop will be performed as usual.

v2: cosmetics, decrease connection_timeout to avoid wait more than it in total

Signed-off-by: Vladislav Grishenko <themiron@yandex-team.ru>
---
 src/openvpn/socket.c | 22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

Comments

Arne Schwabe Oct. 1, 2020, 12:47 a.m. UTC | #1
Am 28.09.20 um 01:32 schrieb Vladislav Grishenko:
> For non-blocking TCP/Unix connection, OpenVPN checks was it established in
> loop and if not - sleeps or handles management for next one second. Since
> the first check is made right after the connection attempt, it will likely
> be always unsuccessful, resulting in redundant wait for one or more seconds:

Okay, I looked at the code and you are right. That code is a mess. It
basically calls select/pool with a timeout of 0, then actively does
select/pool in management for 1s and does this in a loop. That this
totally wrong indeed.

I think we should be more radical here. Always do a 1s poll/select
timeout and do a 0s management sleep/poll. That will also make the
code a bit simpler.

Arne
Vladislav Grishenko Oct. 1, 2020, 12:47 p.m. UTC | #2
Hi Arne,

> Okay, I looked at the code and you are right. That code is a mess. It basically
> calls select/pool with a timeout of 0, then actively does select/pool in
> management for 1s and does this in a loop. That this totally wrong indeed.
> 
> I think we should be more radical here. Always do a 1s poll/select timeout and
> do a 0s management sleep/poll. That will also make the code a bit simpler.

Othe thing, management_sleep(0) results in infinite wait, so I;ve changed api in subsequent patch.
Now, management_sleep() can handle only non-negative value and managemet_event_loop_n_seconds() can take negative values to have infinite wait, if necessary.
Since there were no negative or zero parameter for management_sleep() users, no side effect behavior change is expected.
Seems, a bit simpler can't be achieved :)

--
Best Regards, Vladislav Grishenko

Patch

diff --git a/src/openvpn/socket.c b/src/openvpn/socket.c
index 76bdbfc5..049216ff 100644
--- a/src/openvpn/socket.c
+++ b/src/openvpn/socket.c
@@ -1464,13 +1464,27 @@  openvpn_connect(socket_descriptor_t sd,
 #endif
         )
     {
+        /* Non-blocking connection is in the progress and can be likely
+         * established within the first second with no need for imprecise
+         * sleeping in loop.
+         */
+        bool connect_wait = (connect_timeout > 0);
         while (true)
         {
 #if POLL
+            int timeout = 0;
             struct pollfd fds[1];
+
             fds[0].fd = sd;
             fds[0].events = POLLOUT;
-            status = poll(fds, 1, 0);
+            if (connect_wait)
+            {
+                connect_wait = false;
+                connect_timeout--;
+                timeout = 1000;
+            }
+
+            status = poll(fds, 1, timeout);
 #else
             fd_set writes;
             struct timeval tv;
@@ -1479,6 +1493,12 @@  openvpn_connect(socket_descriptor_t sd,
             openvpn_fd_set(sd, &writes);
             tv.tv_sec = 0;
             tv.tv_usec = 0;
+            if (connect_wait)
+            {
+                connect_wait = false;
+                connect_timeout--;
+                tv.tv_sec = 1;
+            }
 
             status = select(sd + 1, NULL, &writes, NULL, &tv);
 #endif