[ovs-dev] [PATCH] ofproto-dpif-upcall: Fix ovs-vswitchd crash.

Alex Wang alexw at nicira.com
Tue Apr 22 01:13:54 UTC 2014


On current master, caller of udpif_set_threads() can pass 0 value
on n_handlers and n_revalidators to delete all handler and revalidator
threads.

After commit 9a159f748866 (ofproto-dpif-upcall: Remove the dispatcher
thread.), udpif_set_threads() also calls the dpif_handlers_set() with
the 0 value 'n_handlers'.  Since dpif level always assume the 'n_handlers'
be non-zero, this causes warnings and even crash of ovs-vswitchd.

This commit fixes the above issue by defining separate functions for
starting and stopping handler and revalidator threads.  So udpif_set_threads()
will never be called with 0 value arguments.

Reported-by: Andy Zhou <azhou at nicira.com>
Signed-off-by: Alex Wang <alexw at nicira.com>
Co-authored-by: Ethan Jackson <ethan at nicira.com>
---
 ofproto/ofproto-dpif-upcall.c |   76 +++++++++++++++++++++++++++--------------
 1 file changed, 51 insertions(+), 25 deletions(-)

diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
index 4ee5bf5..4d87b88 100644
--- a/ofproto/ofproto-dpif-upcall.c
+++ b/ofproto/ofproto-dpif-upcall.c
@@ -223,6 +223,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 +281,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,18 +291,11 @@ 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)) {
@@ -347,6 +342,7 @@ udpif_set_threads(struct udpif *udpif, size_t n_handlers,
         for (i = 0; i < udpif->n_handlers; i++) {
             free(udpif->handlers[i].name);
         }
+
         latch_poll(&udpif->exit_latch);
 
         free(udpif->revalidators);
@@ -357,15 +353,14 @@ 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). */
+/* 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->handlers && n_handlers) {
         size_t i;
 
@@ -397,7 +392,31 @@ 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(n_handlers && n_revalidators);
+
+    ovsrcu_quiesce_start();
+    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;
+    }
 
+    udpif_start_threads(udpif, n_handlers, n_revalidators);
     ovsrcu_quiesce_end();
 }
 
@@ -413,8 +432,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 +488,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. */
-- 
1.7.9.5




More information about the dev mailing list