[Openvpn-devel,1/1] reliable: retransmit if 3 follow-up ACKs are received

Message ID E1lRfW3-0001sy-Te@sfs-ml-4.v29.lw.sourceforge.com
State Accepted
Headers show
Series reliable: retransmit if 3 follow-up ACKs are received | expand

Commit Message

Maximilian Fillinger March 31, 2021, 7:03 a.m. UTC
From: Steffan Karger <steffan.karger@fox-it.com>

To improve the control channel performance under packet loss conditions,
add a more aggressive retransmit policy similar to what many TCP
implementations do: retransmit a packet if the ACK timeout expires (like
we already do), *or* if three ACKs for follow-up packets are received.

The rationale behind this is that if follow-up packets *are* received, the
connection is apparently functional and we should be able to retransmit
immediately. This significantly improves performance for connections with
low (up to a few percent) packet loss.
---
 src/openvpn/reliable.c | 20 +++++++++++++++++---
 src/openvpn/reliable.h |  7 +++++++
 2 files changed, 24 insertions(+), 3 deletions(-)

Comments

Arne Schwabe March 31, 2021, 11:22 p.m. UTC | #1
Am 31.03.21 um 20:03 schrieb Max Fillinger:
> From: Steffan Karger <steffan.karger@fox-it.com>
> 
> To improve the control channel performance under packet loss conditions,
> add a more aggressive retransmit policy similar to what many TCP
> implementations do: retransmit a packet if the ACK timeout expires (like
> we already do), *or* if three ACKs for follow-up packets are received.
> 
> The rationale behind this is that if follow-up packets *are* received, the
> connection is apparently functional and we should be able to retransmit
> immediately. This significantly improves performance for connections with
> low (up to a few percent) packet loss.

This is a reasonable approach and thanks for adding the comments.

Acked-By: Arne Schwabe <arne@rfc2549.org>

Patch

diff --git a/src/openvpn/reliable.c b/src/openvpn/reliable.c
index 6c1f2da1..15b90fbe 100644
--- a/src/openvpn/reliable.c
+++ b/src/openvpn/reliable.c
@@ -382,7 +382,14 @@  reliable_send_purge(struct reliable *rel, const struct reliable_ack *ack)
                 }
 #endif
                 e->active = false;
-                break;
+            }
+            else if (e->active && e->packet_id < pid)
+            {
+                /* We have received an ACK for a packet with a higher PID. Either
+                 * we have received ACKs out of or order or the packet has been
+                 * lost. We count the number of ACKs to determine if we should
+                 * resend it early. */
+                e->n_acks++;
             }
         }
     }
@@ -555,7 +562,7 @@  reliable_can_send(const struct reliable *rel)
         if (e->active)
         {
             ++n_active;
-            if (now >= e->next_try)
+            if (now >= e->next_try || e->n_acks >= N_ACK_RETRANSMIT)
             {
                 ++n_current;
             }
@@ -581,7 +588,12 @@  reliable_send(struct reliable *rel, int *opcode)
     for (i = 0; i < rel->size; ++i)
     {
         struct reliable_entry *e = &rel->array[i];
-        if (e->active && local_now >= e->next_try)
+
+        /* If N_ACK_RETRANSMIT later packets have received ACKs, we assume
+         * that the packet was lost and resend it even if the timeout has
+         * not expired yet. */
+        if (e->active
+            && (e->n_acks >= N_ACK_RETRANSMIT || local_now >= e->next_try))
         {
             if (!best || reliable_pid_min(e->packet_id, best->packet_id))
             {
@@ -599,6 +611,7 @@  reliable_send(struct reliable *rel, int *opcode)
         /* constant timeout, no backoff */
         best->next_try = local_now + best->timeout;
 #endif
+        best->n_acks = 0;
         *opcode = best->opcode;
         dmsg(D_REL_DEBUG, "ACK reliable_send ID " packet_id_format " (size=%d to=%d)",
              (packet_id_print_type)best->packet_id, best->buf.len,
@@ -686,6 +699,7 @@  reliable_mark_active_incoming(struct reliable *rel, struct buffer *buf,
             e->opcode = opcode;
             e->next_try = 0;
             e->timeout = 0;
+            e->n_acks = 0;
             dmsg(D_REL_DEBUG, "ACK mark active incoming ID " packet_id_format, (packet_id_print_type)e->packet_id);
             return;
         }
diff --git a/src/openvpn/reliable.h b/src/openvpn/reliable.h
index a84d4290..97e6dce7 100644
--- a/src/openvpn/reliable.h
+++ b/src/openvpn/reliable.h
@@ -52,6 +52,10 @@ 
                                  *   the reliability layer for one VPN
                                  *   tunnel in one direction can store. */
 
+#define N_ACK_RETRANSMIT 3      /**< We retry sending a packet early if
+                                 *   this many later packets have been
+                                 *   ACKed. */
+
 /**
  * The acknowledgment structure in which packet IDs are stored for later
  * acknowledgment.
@@ -72,6 +76,9 @@  struct reliable_entry
     interval_t timeout;
     time_t next_try;
     packet_id_type packet_id;
+    size_t n_acks;  /* Number of acks received for packets with higher PID.
+                     * Used for fast retransmission when there were at least
+                     * N_ACK_RETRANSMIT. */
     int opcode;
     struct buffer buf;
 };