upcall: Defer ukey deletion until after pushing stats.

It is possible for a datapath to dump the same flow twice, for instance
if the flow is the last in a batch of flows to be dumped, then a new
flow is inserted into the same bucket before the flow dumper fetches
another batch.

In this case, datapath flow stats may be duplicated: The revalidator
records the stats from the first flow, using the ukey to get the stats
delta. The ukey is deleted, then the revalidator reads the second
(duplicate) flow and cannot lookup the ukey for the delta. As such, it
will push the stats as-is.

This patch reduces the likelihood of such stats duplications by
deferring ukey deletion until after stats are pushed for deleted flows.

Signed-off-by: Joe Stringer <joestringer@nicira.com>
Signed-off-by: Ben Pfaff <blp@nicira.com>

diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
index fbc9ed8..7ff55a0 100644 (file)
--- a/ofproto/ofproto-dpif-upcall.c
+++ b/ofproto/ofproto-dpif-upcall.c
@@ -1375,12 +1375,14 @@ revalidate_udumps(struct revalidator *revalidator, struct list *udumps)
 {
     struct udpif *udpif = revalidator->udpif;
 
-    struct {
-        struct dpif_flow_stats ukey_stats;    /* Stats stored in the ukey. */
-        struct dpif_flow_stats stats;         /* Stats for 'op'. */
-        struct dpif_op op;                    /* Flow del operation. */
-    } ops[REVALIDATE_MAX_BATCH];
+    struct dump_op {
+        struct udpif_key *ukey;
+        struct udpif_flow_dump *udump;
+        struct dpif_flow_stats stats; /* Stats for 'op'. */
+        struct dpif_op op;            /* Flow del operation. */
+    };
 
+    struct dump_op ops[REVALIDATE_MAX_BATCH];
     struct dpif_op *opsp[REVALIDATE_MAX_BATCH];
     struct udpif_flow_dump *udump, *next_udump;
     size_t n_ops, i, n_flows;
@@ -1413,21 +1415,15 @@ revalidate_udumps(struct revalidator *revalidator, struct list *udumps)
         }
 
         if (must_del || (used && used < now - max_idle)) {
-            struct dpif_flow_stats *ukey_stats = &ops[n_ops].ukey_stats;
-            struct dpif_op *op = &ops[n_ops].op;
+            struct dump_op *dop = &ops[n_ops++];
+            struct dpif_op *op = &dop->op;
 
+            dop->ukey = ukey;
+            dop->udump = udump;
             op->type = DPIF_OP_FLOW_DEL;
             op->u.flow_del.key = udump->key;
             op->u.flow_del.key_len = udump->key_len;
             op->u.flow_del.stats = &ops[n_ops].stats;
-            n_ops++;
-
-            if (ukey) {
-                *ukey_stats = ukey->stats;
-                ukey_delete(revalidator, ukey);
-            } else {
-                memset(ukey_stats, 0, sizeof *ukey_stats);
-            }
 
             continue;
         }
@@ -1466,7 +1462,7 @@ revalidate_udumps(struct revalidator *revalidator, struct list *udumps)
     for (i = 0; i < n_ops; i++) {
         struct dpif_flow_stats push, *stats, *ukey_stats;
 
-        ukey_stats  = &ops[i].ukey_stats;
+        ukey_stats = &ops[i].ukey->stats;
         stats = ops[i].op.u.flow_del.stats;
         push.used = MAX(stats->used, ukey_stats->used);
         push.tcp_flags = stats->tcp_flags | ukey_stats->tcp_flags;
@@ -1499,6 +1495,17 @@ revalidate_udumps(struct revalidator *revalidator, struct list *udumps)
         }
     }
 
+    for (i = 0; i < n_ops; i++) {
+        struct udpif_key *ukey = ops[i].ukey;
+
+        /* Look up the ukey to prevent double-free if the datapath dumps the
+         * same flow twice. */
+        ukey = ukey_lookup(revalidator, ops[i].udump);
+        if (ukey) {
+            ukey_delete(revalidator, ukey);
+        }
+    }
+
     LIST_FOR_EACH_SAFE (udump, next_udump, list_node, udumps) {
         list_remove(&udump->list_node);
         free(udump);