[ovs-dev] [branch-2.3] revalidator: Eliminate duplicate flow handling.

Joe Stringer joestringer at nicira.com
Wed May 28 01:19:01 UTC 2014


A series of bugs have been identified recently that are caused by a
combination of the awkward flow dump API, possibility of duplicate flows
in a flow dump, and premature optimisation of the revalidator logic.
This patch attempts to simplify the revalidator logic by combining
multiple critical sections into one, which should make the state more
consistent.

The new flow of logic is:
+ Lookup the ukey.
+ If the ukey doesn't exist, create it.
+ Insert the ukey into the udpif. If we can't insert it, skip this flow.
+ Lock the ukey. If we can't lock it, skip it.
+ Determine if the ukey was already handled. If it has, skip it.
+ Revalidate.
+ Update ukey's fields (mark, flow_exists).
+ Unlock the ukey.

Previously, we would attempt process a flow without creating a ukey if
it hadn't been dumped before and it was due to be deleted. This patch
changes this to always create a ukey, allowing the ukey's
mutex to be used as the basis for preventing a flow from being handled
twice. This is expected to cause some minor performance regression for
cases like TCP_CRR, in favour of correctness and readability.

Bug #1252997.

Signed-off-by: Joe Stringer <joestringer at nicira.com>
---
 ofproto/ofproto-dpif-upcall.c |   64 ++++++++++++++++++-----------------------
 1 file changed, 28 insertions(+), 36 deletions(-)

diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
index 64444d9..906ccb4 100644
--- a/ofproto/ofproto-dpif-upcall.c
+++ b/ofproto/ofproto-dpif-upcall.c
@@ -1205,6 +1205,7 @@ revalidate_ukey(struct udpif *udpif, struct udpif_key *ukey,
                 const struct nlattr *mask, size_t mask_len,
                 const struct nlattr *actions, size_t actions_len,
                 const struct dpif_flow_stats *stats)
+    OVS_REQUIRES(ukey->mutex)
 {
     uint64_t slow_path_buf[128 / 8];
     struct xlate_out xout, *xoutp;
@@ -1225,7 +1226,6 @@ revalidate_ukey(struct udpif *udpif, struct udpif_key *ukey,
     xoutp = NULL;
     netflow = NULL;
 
-    ovs_mutex_lock(&ukey->mutex);
     last_used = ukey->stats.used;
     push.used = stats->used;
     push.tcp_flags = stats->tcp_flags;
@@ -1236,11 +1236,6 @@ revalidate_ukey(struct udpif *udpif, struct udpif_key *ukey,
         ? stats->n_bytes - ukey->stats.n_bytes
         : 0;
 
-    if (!ukey->flow_exists) {
-        /* Don't bother revalidating if the flow was already deleted. */
-        goto exit;
-    }
-
     if (udpif->need_revalidate && last_used
         && !should_revalidate(push.n_packets, last_used)) {
         ok = false;
@@ -1320,7 +1315,6 @@ revalidate_ukey(struct udpif *udpif, struct udpif_key *ukey,
     ok = true;
 
 exit:
-    ovs_mutex_unlock(&ukey->mutex);
     if (netflow) {
         if (!ok) {
             netflow_expire(netflow, &flow);
@@ -1460,25 +1454,38 @@ revalidate(struct revalidator *revalidator)
         ukey = ukey_lookup(udpif, key, key_len, hash);
 
         used = stats->used;
-        if (ukey) {
-            ovs_mutex_lock(&ukey->mutex);
-
-            if (ukey->mark || !ukey->flow_exists) {
-                /* The flow has already been dumped. This can occasionally
-                 * occur if the datapath is changed in the middle of a flow
-                 * dump. Rather than perform the same work twice, skip the
-                 * flow this time. */
-                ovs_mutex_unlock(&ukey->mutex);
+        if (!ukey) {
+            ukey = ukey_create(key, key_len, used);
+            if (!udpif_insert_ukey(udpif, ukey, hash)) {
+                /* The same ukey has already been created. This means that
+                 * another revalidator is processing this flow
+                 * concurrently, so don't bother processing it. */
                 COVERAGE_INC(upcall_duplicate_flow);
+                ukey_delete(NULL, ukey);
                 goto next;
             }
+        }
 
-            if (!used) {
-                used = ukey->created;
-            }
+        if (ovs_mutex_trylock(&ukey->mutex)) {
+            /* The flow has been dumped, and is being handled by another
+             * revalidator concurrently. This can occasionally occur if the
+             * datapath is changed in the middle of a flow dump. Rather than
+             * perform the same work twice, skip the flow this time. */
+            COVERAGE_INC(upcall_duplicate_flow);
+            goto next;
+        }
+
+        if (ukey->mark || !ukey->flow_exists) {
+            /* The flow has already been dumped and handled by another
+             * revalidator during this flow dump operation. Skip it. */
+            COVERAGE_INC(upcall_duplicate_flow);
             ovs_mutex_unlock(&ukey->mutex);
+            goto next;
         }
 
+        if (!used) {
+            used = ukey->created;
+        }
         n_flows = udpif_get_n_flows(udpif);
         max_idle = ofproto_max_idle;
         if (n_flows > flow_limit) {
@@ -1488,30 +1495,15 @@ revalidate(struct revalidator *revalidator)
         if ((used && used < now - max_idle) || n_flows > flow_limit * 2) {
             mark = false;
         } else {
-            if (!ukey) {
-                ukey = ukey_create(key, key_len, used);
-                if (!udpif_insert_ukey(udpif, ukey, hash)) {
-                    /* The same ukey has already been created. This means that
-                     * another revalidator is processing this flow
-                     * concurrently, so don't bother processing it. */
-                    ukey_delete(NULL, ukey);
-                    goto next;
-                }
-            }
-
             mark = revalidate_ukey(udpif, ukey, mask, mask_len, actions,
                                    actions_len, stats);
         }
-
-        if (ukey) {
-            ovs_mutex_lock(&ukey->mutex);
-            ukey->mark = ukey->flow_exists = mark;
-            ovs_mutex_unlock(&ukey->mutex);
-        }
+        ukey->mark = ukey->flow_exists = mark;
 
         if (!mark) {
             dump_op_init(&ops[n_ops++], key, key_len, ukey);
         }
+        ovs_mutex_unlock(&ukey->mutex);
 
     next:
         may_destroy = dpif_flow_dump_next_may_destroy_keys(&udpif->dump,
-- 
1.7.10.4




More information about the dev mailing list