[ovs-dev] [PATCH] ofproto-dpif-upcall: Fix ovs-vswitchd crash.
Alex Wang
alexw at nicira.com
Tue Apr 22 02:37:03 UTC 2014
Sorry, seemed my patch caused more issues,
There is a typo in the udpif_stop_threads() function,
There are also unit test failures,
I'm addressing it,
On Mon, Apr 21, 2014 at 6:19 PM, Ethan Jackson <ethan at nicira.com> wrote:
> 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openvswitch.org/pipermail/ovs-dev/attachments/20140421/35e7794d/attachment-0005.html>
More information about the dev
mailing list