[ovs-dev] [PATCH 2/5] upcall: Defer ukey deletion until after pushing stats.

Joe Stringer joestringer at nicira.com
Tue Feb 11 21:55:33 UTC 2014


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 at nicira.com>
---
 ofproto/ofproto-dpif-upcall.c |   41 ++++++++++++++++++++++++-----------------
 1 file changed, 24 insertions(+), 17 deletions(-)

diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
index e0a5aed..90329f1 100644
--- a/ofproto/ofproto-dpif-upcall.c
+++ b/ofproto/ofproto-dpif-upcall.c
@@ -1360,17 +1360,19 @@ exit:
     return ok;
 }
 
+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. */
+};
+
 static void
 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 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;
@@ -1403,21 +1405,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;
         }
@@ -1456,7 +1452,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;
@@ -1489,6 +1485,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);
-- 
1.7.9.5




More information about the dev mailing list