[ovs-dev] [PATCH] revalidator: Prevent handling the same flow twice.

Joe Stringer joestringer at nicira.com
Wed Apr 23 04:50:52 UTC 2014


When the datapath flow table is modified while a flow dump operation is
in progress, it is possible for the same flow to be dumped twice. In
such cases, revalidators may perform redundant work, or attempt to
delete the same flow twice.

This was causing intermittent testsuite failures for test #670 -
"ofproto-dpif, active-backup bonding" where a flow (that had not
previously been dumped) was dumped, revalidated and deleted twice.

The logs show errors such as:
"failed to flow_get (No such file or directory) skb_priority(0),..."
"failed to flow_del (No such file or directory) skb_priority(0),..."

This patch adds a 'flow_exists' field to 'struct udpif_key' to track
whether the flow is (in progress) to be deleted. After doing a ukey
lookup, we check whether ukey->mark or ukey->flow indicates that the
flow has already been handled. If it has already been handled, we skip
handling the flow again.

We also defer ukey cleanup for flows that fail revalidation, so that the
ukey will still exist if the same flow is dumped twice. This allows the
above logic to work in this case.

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

diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
index 416cfdc..50a514f 100644
--- a/ofproto/ofproto-dpif-upcall.c
+++ b/ofproto/ofproto-dpif-upcall.c
@@ -45,6 +45,8 @@
 
 VLOG_DEFINE_THIS_MODULE(ofproto_dpif_upcall);
 
+COVERAGE_DEFINE(upcall_duplicate_flow);
+
 /* A thread that reads upcalls from dpif, forwards each upcall's packet,
  * and possibly sets up a kernel flow as a cache. */
 struct handler {
@@ -161,6 +163,7 @@ struct udpif_key {
     long long int created;         /* Estimation of creation time. */
 
     bool mark;                     /* Used by mark and sweep GC algorithm. */
+    bool flow_exists;              /* Ensures flows are only deleted once. */
 
     struct odputil_keybuf key_buf; /* Memory for 'key'. */
     struct xlate_cache *xcache;    /* Cache for xlate entries that
@@ -1220,6 +1223,7 @@ ukey_create(const struct nlattr *key, size_t key_len, long long int used)
     ukey->key_len = key_len;
 
     ukey->mark = false;
+    ukey->flow_exists = true;
     ukey->created = used ? used : time_msec();
     memset(&ukey->stats, 0, sizeof ukey->stats);
     ukey->xcache = NULL;
@@ -1529,9 +1533,20 @@ revalidate_udumps(struct revalidator *revalidator, struct list *udumps)
             used = ukey->created;
         }
 
+        if (ukey && (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. */
+            COVERAGE_DEFINE(upcall_duplicate_flow);
+            continue;
+        }
+
         if (must_del || (used && used < now - max_idle)) {
             struct dump_op *dop = &ops[n_ops++];
 
+            if (ukey) {
+                ukey->flow_exists = false;
+            }
             dump_op_init(dop, udump->key, udump->key_len, ukey, udump);
             continue;
         }
@@ -1544,8 +1559,10 @@ revalidate_udumps(struct revalidator *revalidator, struct list *udumps)
         ukey->mark = true;
 
         if (!revalidate_ukey(udpif, udump, ukey)) {
+            ukey->flow_exists = false;
             dpif_flow_del(udpif->dpif, udump->key, udump->key_len, NULL);
-            ukey_delete(revalidator, ukey);
+            /* The ukey will be cleaned up by revalidator_sweep().
+             * This helps to avoid deleting the same flow twice. */
         }
 
         list_remove(&udump->list_node);
@@ -1572,6 +1589,8 @@ revalidator_sweep__(struct revalidator *revalidator, bool purge)
     HMAP_FOR_EACH_SAFE (ukey, next, hmap_node, &revalidator->ukeys) {
         if (!purge && ukey->mark) {
             ukey->mark = false;
+        } else if (!ukey->flow_exists) {
+            ukey_delete(revalidator, ukey);
         } else {
             struct dump_op *op = &ops[n_ops++];
 
-- 
1.7.10.4




More information about the dev mailing list