Move PMTU discovery code into the TX path.

Currently, the PMTU discovery code is run by a timeout callback,
independently of tunnel activity. This commit moves it into the TX
path, meaning that send_mtu_probe_handler() is only called if a
packet is about to be sent. Consequently, it has been renamed to
try_mtu() for consistency with try_tx(), try_udp() and try_sptps().

Running PMTU discovery code only as part of the TX path prevents
PMTU discovery from generating unreasonable amounts of traffic when
the "real" traffic is negligible. One extreme example is sending one
real packet and then going silent: in the current code this one little
packet will result in the entire PMTU discovery algorithm being run
from start to finish, resulting in absurd write traffic amplification.
With this patch, PMTU discovery stops as soon as "real" packets stop
flowing, and will be no more aggressive than the underlying traffic.

Furthermore, try_mtu() only runs if there is confirmed UDP
connectivity as per the UDP discovery mechanism. This prevents
unnecessary network chatter - previously, the PMTU discovery code
would send bursts of (potentially large) probe packets every second
even if there was nothing on the other side. With this patch, the
PMTU code only does that if something replied to the lightweight UDP
discovery pings.

These inefficiencies were made even worse when the node is not a
direct neighbour, as tinc will use PMTU discovery both on the
destination node *and* the relay. UDP discovery is more lightweight for
this purpose.

As a bonus, this code simplifies overall code somewhat - state is
easier to manage when code is run in predictable contexts as opposed
to "surprise callbacks". In addition, there is no need to call PMTU
discovery code outside of net_packet.c anymore, thereby simplifying
module boundaries.

diff --git a/src/graph.c b/src/graph.c
index c0a5d0a60575c7fe52a6707fef939fce28dd3114..c95ab91e629d668a275e35862cc028272a7b6419 100644 (file)
--- a/src/graph.c
+++ b/src/graph.c
@@ -242,7 +242,6 @@ static void check_reachability(void) {
                        n->mtuprobes = 0;
 
                        timeout_del(&n->udp_ping_timeout);
-                       timeout_del(&n->mtutimeout);
 
                        char *name;
                        char *address;

diff --git a/src/net.h b/src/net.h
index 35e8799774cbd0b68f02fa12327971f60a0bf5e7..49a3e173d6da6028092075996d319775eccfe0f0 100644 (file)
--- a/src/net.h
+++ b/src/net.h
@@ -205,7 +205,6 @@ extern void terminate_connection(struct connection_t *, bool);
 extern bool node_read_ecdsa_public_key(struct node_t *);
 extern bool read_ecdsa_public_key(struct connection_t *);
 extern bool read_rsa_public_key(struct connection_t *);
-extern void send_mtu_probe(struct node_t *);
 extern void handle_device_data(void *, int);
 extern void handle_meta_connection_data(struct connection_t *);
 extern void regenerate_key(void);

diff --git a/src/net_packet.c b/src/net_packet.c
index f717dd780872b7040f4975fe1761863021a076d1..90fda375b3686ea3e440783a2c80434f327e5362 100644 (file)
--- a/src/net_packet.c
+++ b/src/net_packet.c
@@ -78,12 +78,17 @@ static void send_udp_probe_packet(node_t *n, int len) {
        send_udppacket(n, &packet);
 }
 
-static void send_mtu_probe_handler(void *data) {
-       node_t *n = data;
+// This function tries to determines the MTU of a node.
+// By calling this function repeatedly, n->minmtu will be progressively increased, and at some point, n->mtu will be fixed to n->minmtu.
+// If the MTU is already fixed, this function checks if it can be increased.
+static void try_mtu(node_t *n) {
+       if(!(n->options & OPTION_PMTU_DISCOVERY))
+               return;
 
-       if(!n->status.reachable || !n->status.validkey) {
-               logger(DEBUG_TRAFFIC, LOG_INFO, "Trying to send MTU probe to unreachable or rekeying node %s (%s)", n->name, n->hostname);
+       if(udp_discovery && !n->status.udp_confirmed) {
                n->mtuprobes = 0;
+               n->minmtu = 0;
+               n->maxmtu = MTU;
                return;
        }
 
@@ -91,6 +96,18 @@ static void send_mtu_probe_handler(void *data) {
           mtuprobes ==    30: fix MTU, and go to 31
           mtuprobes ==    31: send one >maxmtu probe every pingtimeout */
 
+       struct timeval now;
+       gettimeofday(&now, NULL);
+       struct timeval elapsed;
+       timersub(&now, &n->probe_sent_time, &elapsed);
+       if(n->mtuprobes < 31) {
+               if(n->mtuprobes != 0 && elapsed.tv_sec < 1)
+                       return;
+       } else {
+               if(elapsed.tv_sec < pingtimeout)
+                       return;
+       }
+
        if(n->mtuprobes == 30 || (n->mtuprobes < 30 && n->minmtu >= n->maxmtu)) {
                if(n->minmtu > n->maxmtu)
                        n->minmtu = n->maxmtu;
@@ -107,7 +124,6 @@ static void send_mtu_probe_handler(void *data) {
                   to detect PMTU increases. */
                if(n->maxmtu + 8 < MTU)
                        send_udp_probe_packet(n, n->maxmtu + 8);
-               timeout = pingtimeout;
        } else {
                /* Probes are sent in batches of three, with random sizes between the
                   lower and upper boundaries for the MTU thus far discovered. */
@@ -118,12 +134,12 @@ static void send_mtu_probe_handler(void *data) {
 
                        send_udp_probe_packet(n, MAX(len, 64));
                }
-               timeout = 1;
                n->mtuprobes++;
        }
 
        n->probe_counter = 0;
-       gettimeofday(&n->probe_time, NULL);
+       n->probe_sent_time = now;
+       n->probe_time = now;
 
        /* Calculate the packet loss of incoming traffic by comparing the rate of
           packets received to the rate with which the sequence number has increased.
@@ -137,13 +153,6 @@ static void send_mtu_probe_handler(void *data) {
 
        n->prev_received_seqno = n->received_seqno;
        n->prev_received = n->received;
-
-       timeout_set(&n->mtutimeout, &(struct timeval){timeout, rand() % 100000});
-}
-
-void send_mtu_probe(node_t *n) {
-       timeout_add(&n->mtutimeout, send_mtu_probe_handler, n, &(struct timeval){1, 0});
-       send_mtu_probe_handler(n);
 }
 
 static void udp_probe_timeout_handler(void *data) {
@@ -961,8 +970,10 @@ static void try_tx(node_t *n) {
        if(!n->status.sptps && !via->status.validkey && via->last_req_key + 10 <= now.tv_sec) {
                send_req_key(via);
                via->last_req_key = now.tv_sec;
-       } else if(via == n || !n->status.sptps || (via->options >> 24) >= 4)
+       } else if(via == n || !n->status.sptps || (via->options >> 24) >= 4) {
                try_udp(via);
+               try_mtu(via);
+       }
 
        /* If we don't know how to reach "via" yet, then try to reach it through a relay. */
        if(n->status.sptps && !via->status.udp_confirmed && via->nexthop != via && (via->nexthop->options >> 24) >= 4)

diff --git a/src/node.c b/src/node.c
index 0da7e605a9cc246874bbfedcd9ae07f16c169d53..6623f46711cb0cbcdec279811f76c67687922f60 100644 (file)
--- a/src/node.c
+++ b/src/node.c
@@ -93,7 +93,6 @@ void free_node(node_t *n) {
        sptps_stop(&n->sptps);
 
        timeout_del(&n->udp_ping_timeout);
-       timeout_del(&n->mtutimeout);
 
        if(n->hostname)
                free(n->hostname);

diff --git a/src/node.h b/src/node.h
index 1863db94f81b5c268cb39099ea1cea997d05d998..08e6017671fb24817f8cc186dd3c844e51303da4 100644 (file)
--- a/src/node.h
+++ b/src/node.h
@@ -94,7 +94,7 @@ typedef struct node_t {
        length_t minmtu;                        /* Probed minimum MTU */
        length_t maxmtu;                        /* Probed maximum MTU */
        int mtuprobes;                          /* Number of probes */
-       timeout_t mtutimeout;                   /* Probe event */
+       struct timeval probe_sent_time;         /* Time the last probe was sent */
        struct timeval probe_time;              /* Time the last probe was sent or received */
        int probe_counter;                      /* Number of probes received since last burst was sent */
        float rtt;                              /* Last measured round trip time */

diff --git a/src/protocol_key.c b/src/protocol_key.c
index cfa2f53ffe0ff36b759009f6779b68ecb65d66eb..aaf0f3309261db1dafe40659165758157042b1c6 100644 (file)
--- a/src/protocol_key.c
+++ b/src/protocol_key.c
@@ -404,11 +404,6 @@ bool ans_key_h(connection_t *c, const char *request) {
                                sockaddr_t sa = str2sockaddr(address, port);
                                update_node_udp(from, &sa);
                        }
-
-                       /* Don't send probes if we can't send UDP packets directly to that node.
-                          TODO: the indirect (via) condition can change at any time as edges are added and removed, so this should probably be moved to graph.c. */
-                       if((from->via == myself || from->via == from) && from->options & OPTION_PMTU_DISCOVERY && !(from->options & OPTION_TCPONLY))
-                               send_mtu_probe(from);
                }
 
                return true;
@@ -468,9 +463,6 @@ bool ans_key_h(connection_t *c, const char *request) {
                update_node_udp(from, &sa);
        }
 
-       if(from->options & OPTION_PMTU_DISCOVERY && !(from->options & OPTION_TCPONLY))
-               send_mtu_probe(from);
-
        return true;
 #endif
 }