Fixes TCP drops due to ARP timeouts
authorBarret Rhoden <brho@cs.berkeley.edu>
Wed, 6 Aug 2014 02:00:32 +0000 (19:00 -0700)
committerBarret Rhoden <brho@cs.berkeley.edu>
Wed, 6 Aug 2014 02:12:11 +0000 (19:12 -0700)
The root of the problem is that arpents were using u32s to store the
msec since boot.  Machines that run for over 50 days will wrap around.

Since NOW was a u64, the arpent ctimes were getting set to weird
numbers, and ultimately the arpents all appeared to be stale.

Stale arps get their blocklists pruned to one block (which is a
questionable bit of code).  These blocks were TCP packets.

So if a machine was running for more than 50 days, and if multiple TCP
packets were sent to the same arpent quickly, one of them could be
dropped, which throws off the entire TCP stream.

c89 happens to be a machine with a TSC that doesn't reset when
soft-rebooting (via our reboot()), so it acted like a machine that had
been up for about 3 months.

Arguably, we could have NOW (and the others, like seconds() and
milliseconds()) be cast to a u32, so that at least both NOW and ctime
are the same width, but that seems really shoddy.

kern/include/ip.h
kern/src/net/arp.c
kern/src/net/ethermedium.c

index 8631251..523feb9 100644 (file)
@@ -495,11 +495,11 @@ struct arpent {
        struct arpent *hash;
        struct block *hold;
        struct block *last;
-       unsigned int ctime;                     /* time entry was created or refreshed */
-       unsigned int utime;                     /* time entry was last used */
+       uint64_t ctime;                 /* time entry was created or refreshed */
+       uint64_t utime;                 /* time entry was last used */
        uint8_t state;
        struct arpent *nextrxt;         /* re-transmit chain */
-       unsigned int rtime;                     /* time for next retransmission */
+       uint64_t rtime;                 /* time for next retransmission */
        uint8_t rxtsrem;
        struct Ipifc *ifc;
        uint8_t ifcid;                          /* must match ifc->id */
index 00e07b0..769cc24 100644 (file)
@@ -127,7 +127,7 @@ static struct arpent *newarp6(struct arp *arp, uint8_t * ip, struct Ipifc *ifc,
 
        memmove(a->ip, ip, sizeof(a->ip));
        a->utime = NOW;
-       a->ctime = 0;
+       a->ctime = a->utime;
        a->type = m;
 
        a->rtime = NOW + ReTransTimer;
@@ -295,6 +295,10 @@ struct block *arpresolve(struct arp *arp, struct arpent *a, struct medium *type,
        a->utime = NOW;
        bp = a->hold;
        a->hold = NULL;
+       /* brho: it looks like we return the entire hold list, though it might be
+        * purged by now via some other crazy arp list management.  our callers
+        * can't handle the arp's b->list stuff. */
+       assert(!bp->list);
        qunlock(&arp->qlock);
 
        return bp;
index 5a47bae..b2a11c2 100644 (file)
@@ -338,10 +338,15 @@ etherbwrite(struct Ipifc *ifc, struct block *bp, int version, uint8_t * ip)
        uint8_t mac[6];
        Etherrock *er = ifc->arg;
 
-       /* get mac address of destination */
+       /* get mac address of destination.
+        *
+        * Locking is tricky here.  If we get arpent 'a' back, the f->arp is
+        * qlocked.  if multicastarp returns bp, then it unlocked it for us.  if
+        * not, sendarp or resolveaddr6 unlocked it for us.  yikes. */
        a = arpget(er->f->arp, bp, version, ifc, ip, mac);
        if (a) {
-               /* check for broadcast or multicast */
+               /* check for broadcast or multicast.  if it is either, this sorts that
+                * out and returns the bp for the first packet on the arp's hold list.*/
                bp = multicastarp(er->f, a, ifc->m, mac);
                if (bp == NULL) {
                        switch (version) {
@@ -511,7 +516,8 @@ static void etherremmulti(struct Ipifc *ifc, uint8_t * a, uint8_t * unused)
 /*
  *  send an ethernet arp
  *  (only v4, v6 uses the neighbor discovery, rfc1970)
- */
+ *
+ * May drop packets on stale arps. */
 static void sendarp(struct Ipifc *ifc, struct arpent *a)
 {
        int n;
@@ -525,7 +531,8 @@ static void sendarp(struct Ipifc *ifc, struct arpent *a)
                return;
        }
 
-       /* remove all but the last message */
+       /* remove all but the last message.  brho: this might be unnecessary.  we'll
+        * eventually send them.  but they should be quite stale at this point. */
        while ((bp = a->hold) != NULL) {
                if (bp == a->last)
                        break;