[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