[ovs-dev] [PATCH 1/2] revalidator: Eliminate duplicate flow handling.

[Joe Stringer]

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 improves code correctness and readability.

Bug #1252997.

Signed-off-by: Joe Stringer <joestringer at nicira.com>
---
The original version of this patch has been merged to branch-2.3, and
noted a potential performance drop. After testing this patch against
master in netperf TCP_CRR, there was no measurable difference so I have
dropped that comment from the commit message.
---
 ofproto/ofproto-dpif-upcall.c |   74 ++++++++++++++++++-----------------------
 1 file changed, 33 insertions(+), 41 deletions(-)

diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
index db0f17e..1c82b6b 100644
--- a/ofproto/ofproto-dpif-upcall.c
+++ b/ofproto/ofproto-dpif-upcall.c
@@ -1097,6 +1097,7 @@ should_revalidate(uint64_t packets, long long int used)
 static bool
 revalidate_ukey(struct udpif *udpif, struct udpif_key *ukey,
                 const struct dpif_flow *f)
+    OVS_REQUIRES(ukey->mutex)
 {
     uint64_t slow_path_buf[128 / 8];
     struct xlate_out xout, *xoutp;
@@ -1117,7 +1118,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 = f->stats.used;
     push.tcp_flags = f->stats.tcp_flags;
@@ -1128,11 +1128,6 @@ revalidate_ukey(struct udpif *udpif, struct udpif_key *ukey,
                     ? f->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;
@@ -1212,7 +1207,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);
@@ -1371,54 +1365,52 @@ revalidate(struct revalidator *revalidator)
             uint32_t hash = hash_bytes(f->key, f->key_len, udpif->secret);
             struct udpif_key *ukey = ukey_lookup(udpif, f->key, f->key_len,
                                                  hash);
-            bool mark;
-
-            if (ukey) {
-                bool already_dumped;
-
-                ovs_mutex_lock(&ukey->mutex);
-                already_dumped = ukey->mark || !ukey->flow_exists;
-                if (!used) {
-                    used = ukey->created;
-                }
-                ovs_mutex_unlock(&ukey->mutex);
-
-                if (already_dumped) {
-                    /* 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. */
+            bool already_dumped, mark;
+
+            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);
                     continue;
                 }
             }
 
+            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);
+                continue;
+            }
+
+            already_dumped = ukey->mark || !ukey->flow_exists;
+            if (already_dumped) {
+                /* 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);
+                continue;
+            }
+
+            if (!used) {
+                used = ukey->created;
+            }
             if (kill_them_all || (used && used < now - max_idle)) {
                 mark = false;
             } else {
-                if (!ukey) {
-                    ukey = ukey_create(f->key, f->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);
-                        continue;
-                    }
-                }
-
                 mark = revalidate_ukey(udpif, ukey, f);
             }
-
-            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++], f->key, f->key_len, ukey);
             }
+            ovs_mutex_unlock(&ukey->mutex);
         }
 
         if (n_ops) {
-- 
1.7.10.4