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

Message ID AM0PR04MB53313168AE080837CBFD8ECDB1699@AM0PR04MB5331.eurprd04.prod.outlook.com
State Superseded
Headers show
Series
  • [Openvpn-devel] reliable: retransmit if 3 follow-up ACKs are received
Related show

Commit Message

tincantech via Openvpn-devel March 18, 2021, 7:27 p.m.
Hi!

I'm currently preparing the OpenVPN-NL 2.5 release at Fox-IT. (We're a
bit behind the times...) I thought that one of our patches, by Steffan
Karger, could be useful in regular OpenVPN. He said that he hadn't
submitted it yet, and thought it would be a good idea to ask.

The patch increases the performance of the control channel when there is
a small amount of packet loss by resending packets more aggressively. In
reliable_send, packets are resent before the timeout expires if at least
3 later packets have been ACK'd.

The reasoning is that if we receive ACKs for later packets, the
connection seems to be functional and we should be able to try again.

This policy is similar to many TCP implementations.

Please let me know if you think this is useful, and if you would like me
to make any changes to the patch.

Signed-off-by: Steffan Karger <steffan.karger@fox-it.com>
---
 src/openvpn/reliable.c | 14 +++++++++++---
 src/openvpn/reliable.h |  1 +
 2 files changed, 12 insertions(+), 3 deletions(-)

Comments

Arne Schwabe March 24, 2021, 10:19 p.m. | #1
Am 18.03.21 um 20:27 schrieb Maximilian Fillinger via Openvpn-devel:

The sender is strange, this strange sender ends up in git as author.

> Hi!
> 
> I'm currently preparing the OpenVPN-NL 2.5 release at Fox-IT. (We're a
> bit behind the times...) I thought that one of our patches, by Steffan
> Karger, could be useful in regular OpenVPN. He said that he hadn't
> submitted it yet, and thought it would be a good idea to ask.

This paragraph ends up in the commit message which is probably
suboptiomal. Better use the --cover-letter option and put text like into
the 0000 mail.

> The patch increases the performance of the control channel when there is
> a small amount of packet loss by resending packets more aggressively. In
> reliable_send, packets are resent before the timeout expires if at least
> 3 later packets have been ACK'd.
> 
> The reasoning is that if we receive ACKs for later packets, the
> connection seems to be functional and we should be able to try again.
> 
> This policy is similar to many TCP implementations.
> 
> Please let me know if you think this is useful, and if you would like me
> to make any changes to the patch.

The code is fine but I would like to have a few more
comments/documentation to make easier to understand. See below:

>  #include "memdbg.h"


Comment here what this defines controls

> +#define N_ACK_RETRANSMIT 3
> +
>  /*
>   * verify that test - base < extent while allowing for base or test wraparound
>   */
> @@ -382,7 +384,10 @@ reliable_send_purge(struct reliable *rel, const struct reliable_ack *ack)
>                  }
>  #endif
>                  e->active = false;
> -                break;
> +            }
> +            else if (e->active && e->packet_id < pid)
> +            {

Add a comment like:

		   /* We have received an ACK for a packet with higher
		      PID, either the packet got lost or we received ACKS out of order */

> +                e->n_acks++;
>              }
>          }
>      }
> @@ -555,7 +560,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 +586,8 @@ 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 (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 +605,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 +693,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..bf0b561b 100644
> --- a/src/openvpn/reliable.h
> +++ b/src/openvpn/reliable.h
> @@ -72,6 +72,7 @@ struct reliable_entry
>      interval_t timeout;
>      time_t next_try;
>      packet_id_type packet_id;

I would like to see here a description what this does, like:

/* Number of ACKs received for packets with higher pid.
   Used for fast retransmit after 3 ACKS */

> +    size_t n_acks;
>      int opcode;
>      struct buffer buf;
>  };
>

Patch

diff --git a/src/openvpn/reliable.c b/src/openvpn/reliable.c
index 6c1f2da1..04d053bd 100644
--- a/src/openvpn/reliable.c
+++ b/src/openvpn/reliable.c
@@ -41,6 +41,8 @@ 
 
 #include "memdbg.h"
 
+#define N_ACK_RETRANSMIT 3
+
 /*
  * verify that test - base < extent while allowing for base or test wraparound
  */
@@ -382,7 +384,10 @@  reliable_send_purge(struct reliable *rel, const struct reliable_ack *ack)
                 }
 #endif
                 e->active = false;
-                break;
+            }
+            else if (e->active && e->packet_id < pid)
+            {
+                e->n_acks++;
             }
         }
     }
@@ -555,7 +560,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 +586,8 @@  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 (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 +605,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 +693,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..bf0b561b 100644
--- a/src/openvpn/reliable.h
+++ b/src/openvpn/reliable.h
@@ -72,6 +72,7 @@  struct reliable_entry
     interval_t timeout;
     time_t next_try;
     packet_id_type packet_id;
+    size_t n_acks;
     int opcode;
     struct buffer buf;
 };