revalidator: Prevent handling the same flow twice.
[sliver-openvswitch.git] / ofproto / ofproto-dpif-upcall.c
index 4ee5bf5..906bf17 100644 (file)
@@ -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
@@ -223,6 +226,9 @@ static size_t read_upcalls(struct handler *,
                            struct hmap *);
 static void handle_upcalls(struct handler *, struct hmap *, struct upcall *,
                            size_t n_upcalls);
+static void udpif_stop_threads(struct udpif *);
+static void udpif_start_threads(struct udpif *, size_t n_handlers,
+                                size_t n_revalidators);
 static void *udpif_flow_dumper(void *);
 static void *udpif_upcall_handler(void *);
 static void *udpif_revalidator(void *);
@@ -278,8 +284,7 @@ udpif_create(struct dpif_backer *backer, struct dpif *dpif)
 void
 udpif_destroy(struct udpif *udpif)
 {
-    udpif_set_threads(udpif, 0, 0);
-    udpif_flush(udpif);
+    udpif_stop_threads(udpif);
 
     list_remove(&udpif->list_node);
     latch_destroy(&udpif->exit_latch);
@@ -289,21 +294,12 @@ udpif_destroy(struct udpif *udpif)
     free(udpif);
 }
 
-/* Tells 'udpif' how many threads it should use to handle upcalls.  Disables
- * all threads if 'n_handlers' and 'n_revalidators' is zero.  'udpif''s
- * datapath handle must have packet reception enabled before starting threads.
- */
-void
-udpif_set_threads(struct udpif *udpif, size_t n_handlers,
-                  size_t n_revalidators)
+/* Stops the handler and revalidator threads, must be enclosed in
+ * ovsrcu quiescent state unless when destroying udpif. */
+static void
+udpif_stop_threads(struct udpif *udpif)
 {
-    int error;
-
-    ovsrcu_quiesce_start();
-    /* Stop the old threads (if any). */
-    if (udpif->handlers &&
-        (udpif->n_handlers != n_handlers
-         || udpif->n_revalidators != n_revalidators)) {
+    if (udpif && (udpif->n_handlers != 0 || udpif->n_revalidators != 0)) {
         size_t i;
 
         latch_set(&udpif->exit_latch);
@@ -357,16 +353,15 @@ udpif_set_threads(struct udpif *udpif, size_t n_handlers,
         udpif->handlers = NULL;
         udpif->n_handlers = 0;
     }
+}
 
-    error = dpif_handlers_set(udpif->dpif, n_handlers);
-    if (error) {
-        VLOG_ERR("failed to configure handlers in dpif %s: %s",
-                 dpif_name(udpif->dpif), ovs_strerror(error));
-        return;
-    }
-
-    /* Start new threads (if necessary). */
-    if (!udpif->handlers && n_handlers) {
+/* Starts the handler and revalidator threads, must be enclosed in
+ * ovsrcu quiescent state. */
+static void
+udpif_start_threads(struct udpif *udpif, size_t n_handlers,
+                    size_t n_revalidators)
+{
+    if (udpif && (!udpif->handlers && !udpif->revalidators)) {
         size_t i;
 
         udpif->n_handlers = n_handlers;
@@ -397,7 +392,37 @@ udpif_set_threads(struct udpif *udpif, size_t n_handlers,
         }
         xpthread_create(&udpif->flow_dumper, NULL, udpif_flow_dumper, udpif);
     }
+}
+
+/* Tells 'udpif' how many threads it should use to handle upcalls.
+ * 'n_handlers' and 'n_revalidators' can never be zero.  'udpif''s
+ * datapath handle must have packet reception enabled before starting
+ * threads. */
+void
+udpif_set_threads(struct udpif *udpif, size_t n_handlers,
+                  size_t n_revalidators)
+{
+    int error;
+
+    ovs_assert(udpif);
+    ovs_assert(n_handlers && n_revalidators);
+
+    ovsrcu_quiesce_start();
+    if (udpif->n_handlers != n_handlers
+        || udpif->n_revalidators != n_revalidators) {
+        udpif_stop_threads(udpif);
+    }
 
+    error = dpif_handlers_set(udpif->dpif, n_handlers);
+    if (error) {
+        VLOG_ERR("failed to configure handlers in dpif %s: %s",
+                 dpif_name(udpif->dpif), ovs_strerror(error));
+        return;
+    }
+
+    if (!udpif->handlers && !udpif->revalidators) {
+        udpif_start_threads(udpif, n_handlers, n_revalidators);
+    }
     ovsrcu_quiesce_end();
 }
 
@@ -413,8 +438,11 @@ udpif_synchronize(struct udpif *udpif)
      * its main loop once. */
     size_t n_handlers = udpif->n_handlers;
     size_t n_revalidators = udpif->n_revalidators;
-    udpif_set_threads(udpif, 0, 0);
-    udpif_set_threads(udpif, n_handlers, n_revalidators);
+
+    ovsrcu_quiesce_start();
+    udpif_stop_threads(udpif);
+    udpif_start_threads(udpif, n_handlers, n_revalidators);
+    ovsrcu_quiesce_end();
 }
 
 /* Notifies 'udpif' that something changed which may render previous
@@ -466,9 +494,13 @@ udpif_flush(struct udpif *udpif)
     n_handlers = udpif->n_handlers;
     n_revalidators = udpif->n_revalidators;
 
-    udpif_set_threads(udpif, 0, 0);
+    ovsrcu_quiesce_start();
+
+    udpif_stop_threads(udpif);
     dpif_flow_flush(udpif->dpif);
-    udpif_set_threads(udpif, n_handlers, n_revalidators);
+    udpif_start_threads(udpif, n_handlers, n_revalidators);
+
+    ovsrcu_quiesce_end();
 }
 
 /* Removes all flows from all datapaths. */
@@ -1191,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;
@@ -1500,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_INC(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;
         }
@@ -1515,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);
@@ -1543,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++];