Acked-by: Ethan Jackson <et...@nicira.com>
On Mon, Apr 21, 2014 at 6:13 PM, Alex Wang <al...@nicira.com> wrote:
> 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 <az...@nicira.com>
> Signed-off-by: Alex Wang <al...@nicira.com>
> Co-authored-by: Ethan Jackson <et...@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
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
