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

Ethan Jackson ethan at nicira.com
Tue Apr 22 01:19:20 UTC 2014


Acked-by: Ethan Jackson <ethan at nicira.com>


On Mon, Apr 21, 2014 at 6:13 PM, Alex Wang <alexw at 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 <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
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev



More information about the dev mailing list