From 1f8675481e8cc976931bd0a85851d780f7cf2a33 Mon Sep 17 00:00:00 2001 From: Alex Wang Date: Mon, 21 Apr 2014 17:31:11 -0700 Subject: [PATCH] ofproto-dpif-upcall: Fix ovs-vswitchd crash. 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 Signed-off-by: Alex Wang Co-authored-by: Ethan Jackson Acked-by: Ethan Jackson --- ofproto/ofproto-dpif-upcall.c | 75 +++++++++++++++++++++++------------ 1 file changed, 50 insertions(+), 25 deletions(-) diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c index 4ee5bf59c..8e43e8489 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)) { @@ -357,15 +352,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 +391,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 +431,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 +487,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. */ -- 2.43.0