netdev: Fix deadlock when netdev_dump_queues() callback calls into netdev.
authorBen Pfaff <blp@nicira.com>
Wed, 28 Aug 2013 00:15:53 +0000 (17:15 -0700)
committerBen Pfaff <blp@nicira.com>
Wed, 28 Aug 2013 04:50:40 +0000 (21:50 -0700)
We have a call chain like this:

    iface_configure_qos() calls
        netdev_dump_queues(), which calls
            netdev_linux_dump_queues(), which calls back through 'cb' to
                qos_unixctl_show_cb(), which calls
                    netdev_delete_queue(), which calls
                        netdev_linux_delete_queue().

Both netdev_dump_queues() and netdev_linux_delete_queue() take the same
mutex in the same netdev, which deadlocks.

This commit fixes the problem by getting rid of the callback.

netdev_linux_dump_queue_stats() would benefit from the same treatment but
it's less urgent because I don't see any callbacks from that function that
call back into a netdev function.

Bug #19319.
Reported-by: Scott Hendricks <shendricks@vmware.com>
Signed-off-by: Ben Pfaff <blp@nicira.com>
lib/netdev-dummy.c
lib/netdev-linux.c
lib/netdev-provider.h
lib/netdev-vport.c
lib/netdev.c
lib/netdev.h
vswitchd/bridge.c

index ac26564..16a6b0b 100644 (file)
@@ -708,7 +708,9 @@ static const struct netdev_class dummy_class = {
     NULL,                       /* set_queue */
     NULL,                       /* delete_queue */
     NULL,                       /* get_queue_stats */
-    NULL,                       /* dump_queues */
+    NULL,                       /* queue_dump_start */
+    NULL,                       /* queue_dump_next */
+    NULL,                       /* queue_dump_done */
     NULL,                       /* dump_queue_stats */
 
     NULL,                       /* get_in4 */
index 80fa39b..26fb32d 100644 (file)
@@ -2100,35 +2100,35 @@ start_queue_dump(const struct netdev *netdev, struct nl_dump *dump)
     return true;
 }
 
+struct netdev_linux_queue_state {
+    unsigned int *queues;
+    size_t cur_queue;
+    size_t n_queues;
+};
+
 static int
-netdev_linux_dump_queues(const struct netdev *netdev_,
-                         netdev_dump_queues_cb *cb, void *aux)
+netdev_linux_queue_dump_start(const struct netdev *netdev_, void **statep)
 {
-    struct netdev_linux *netdev = netdev_linux_cast(netdev_);
+    const struct netdev_linux *netdev = netdev_linux_cast(netdev_);
     int error;
 
     ovs_mutex_lock(&netdev->mutex);
     error = tc_query_qdisc(netdev_);
     if (!error) {
         if (netdev->tc->ops->class_get) {
-            struct tc_queue *queue, *next_queue;
-            struct smap details;
-
-            smap_init(&details);
-            HMAP_FOR_EACH_SAFE (queue, next_queue, hmap_node,
-                                &netdev->tc->queues) {
-                int retval;
-
-                smap_clear(&details);
-
-                retval = netdev->tc->ops->class_get(netdev_, queue, &details);
-                if (!retval) {
-                    (*cb)(queue->queue_id, &details, aux);
-                } else {
-                    error = retval;
-                }
+            struct netdev_linux_queue_state *state;
+            struct tc_queue *queue;
+            size_t i;
+
+            *statep = state = xmalloc(sizeof *state);
+            state->n_queues = hmap_count(&netdev->tc->queues);
+            state->cur_queue = 0;
+            state->queues = xmalloc(state->n_queues * sizeof *state->queues);
+
+            i = 0;
+            HMAP_FOR_EACH (queue, hmap_node, &netdev->tc->queues) {
+                state->queues[i++] = queue->queue_id;
             }
-            smap_destroy(&details);
         } else {
             error = EOPNOTSUPP;
         }
@@ -2138,6 +2138,41 @@ netdev_linux_dump_queues(const struct netdev *netdev_,
     return error;
 }
 
+static int
+netdev_linux_queue_dump_next(const struct netdev *netdev_, void *state_,
+                             unsigned int *queue_idp, struct smap *details)
+{
+    const struct netdev_linux *netdev = netdev_linux_cast(netdev_);
+    struct netdev_linux_queue_state *state = state_;
+    int error = EOF;
+
+    ovs_mutex_lock(&netdev->mutex);
+    while (state->cur_queue < state->n_queues) {
+        unsigned int queue_id = state->queues[state->cur_queue++];
+        struct tc_queue *queue = tc_find_queue(netdev_, queue_id);
+
+        if (queue) {
+            *queue_idp = queue_id;
+            error = netdev->tc->ops->class_get(netdev_, queue, details);
+            break;
+        }
+    }
+    ovs_mutex_unlock(&netdev->mutex);
+
+    return error;
+}
+
+static int
+netdev_linux_queue_dump_done(const struct netdev *netdev OVS_UNUSED,
+                             void *state_)
+{
+    struct netdev_linux_queue_state *state = state_;
+
+    free(state->queues);
+    free(state);
+    return 0;
+}
+
 static int
 netdev_linux_dump_queue_stats(const struct netdev *netdev_,
                               netdev_dump_queue_stats_cb *cb, void *aux)
@@ -2580,7 +2615,9 @@ netdev_linux_change_seq(const struct netdev *netdev_)
     netdev_linux_set_queue,                                     \
     netdev_linux_delete_queue,                                  \
     netdev_linux_get_queue_stats,                               \
-    netdev_linux_dump_queues,                                   \
+    netdev_linux_queue_dump_start,                              \
+    netdev_linux_queue_dump_next,                               \
+    netdev_linux_queue_dump_done,                               \
     netdev_linux_dump_queue_stats,                              \
                                                                 \
     netdev_linux_get_in4,                                       \
index 70a5188..9ab58fb 100644 (file)
@@ -478,22 +478,39 @@ struct netdev_class {
     int (*get_queue_stats)(const struct netdev *netdev, unsigned int queue_id,
                            struct netdev_queue_stats *stats);
 
-    /* Iterates over all of 'netdev''s queues, calling 'cb' with the queue's
-     * ID, its configuration, and the 'aux' specified by the caller.  The order
-     * of iteration is unspecified, but (when successful) each queue is visited
-     * exactly once.
-     *
-     * 'cb' will not modify or free the 'details' argument passed in.  It may
-     * delete or modify the queue passed in as its 'queue_id' argument.  It may
-     * modify but will not delete any other queue within 'netdev'.  If 'cb'
-     * adds new queues, then ->dump_queues is allowed to visit some queues
-     * twice or not at all.
-     */
-    int (*dump_queues)(const struct netdev *netdev,
-                       void (*cb)(unsigned int queue_id,
-                                  const struct smap *details,
-                                  void *aux),
-                       void *aux);
+    /* Attempts to begin dumping the queues in 'netdev'.  On success, returns 0
+     * and initializes '*statep' with any data needed for iteration.  On
+     * failure, returns a positive errno value.
+     *
+     * May be NULL if 'netdev' does not support QoS at all. */
+    int (*queue_dump_start)(const struct netdev *netdev, void **statep);
+
+    /* Attempts to retrieve another queue from 'netdev' for 'state', which was
+     * initialized by a successful call to the 'queue_dump_start' function for
+     * 'netdev'.  On success, stores a queue ID into '*queue_id' and fills
+     * 'details' with the configuration of the queue with that ID.  Returns EOF
+     * if the last queue has been dumped, or a positive errno value on error.
+     * This function will not be called again once it returns nonzero once for
+     * a given iteration (but the 'queue_dump_done' function will be called
+     * afterward).
+     *
+     * The caller initializes and clears 'details' before calling this
+     * function.  The caller takes ownership of the string key-values pairs
+     * added to 'details'.
+     *
+     * The returned contents of 'details' should be documented as valid for the
+     * given 'type' in the "other_config" column in the "Queue" table in
+     * vswitchd/vswitch.xml (which is built as ovs-vswitchd.conf.db(8)).
+     *
+     * May be NULL if 'netdev' does not support QoS at all. */
+    int (*queue_dump_next)(const struct netdev *netdev, void *state,
+                           unsigned int *queue_id, struct smap *details);
+
+    /* Releases resources from 'netdev' for 'state', which was initialized by a
+     * successful call to the 'queue_dump_start' function for 'netdev'.
+     *
+     * May be NULL if 'netdev' does not support QoS at all. */
+    int (*queue_dump_done)(const struct netdev *netdev, void *state);
 
     /* Iterates over all of 'netdev''s queues, calling 'cb' with the queue's
      * ID, its statistics, and the 'aux' specified by the caller.  The order of
index 0f5dd7a..af50597 100644 (file)
@@ -727,7 +727,9 @@ get_stats(const struct netdev *netdev, struct netdev_stats *stats)
     NULL,                       /* set_queue */             \
     NULL,                       /* delete_queue */          \
     NULL,                       /* get_queue_stats */       \
-    NULL,                       /* dump_queues */           \
+    NULL,                       /* queue_dump_start */      \
+    NULL,                       /* queue_dump_next */       \
+    NULL,                       /* queue_dump_done */       \
     NULL,                       /* dump_queue_stats */      \
                                                             \
     NULL,                       /* get_in4 */               \
index bf942a0..5ed6062 100644 (file)
@@ -1398,30 +1398,78 @@ netdev_get_queue_stats(const struct netdev *netdev, unsigned int queue_id,
     return retval;
 }
 
-/* Iterates over all of 'netdev''s queues, calling 'cb' with the queue's ID,
- * its configuration, and the 'aux' specified by the caller.  The order of
- * iteration is unspecified, but (when successful) each queue is visited
- * exactly once.
+/* Initializes 'dump' to begin dumping the queues in a netdev.
+ *
+ * This function provides no status indication.  An error status for the entire
+ * dump operation is provided when it is completed by calling
+ * netdev_queue_dump_done().
+ */
+void
+netdev_queue_dump_start(struct netdev_queue_dump *dump,
+                        const struct netdev *netdev)
+{
+    dump->netdev = netdev_ref(netdev);
+    if (netdev->netdev_class->queue_dump_start) {
+        dump->error = netdev->netdev_class->queue_dump_start(netdev,
+                                                             &dump->state);
+    } else {
+        dump->error = EOPNOTSUPP;
+    }
+}
+
+/* Attempts to retrieve another queue from 'dump', which must have been
+ * initialized with netdev_queue_dump_start().  On success, stores a new queue
+ * ID into '*queue_id', fills 'details' with configuration details for the
+ * queue, and returns true.  On failure, returns false.
  *
- * Calling this function may be more efficient than calling netdev_get_queue()
- * for every queue.
+ * Queues are not necessarily dumped in increasing order of queue ID (or any
+ * other predictable order).
  *
- * 'cb' must not modify or free the 'details' argument passed in.  It may
- * delete or modify the queue passed in as its 'queue_id' argument.  It may
- * modify but must not delete any other queue within 'netdev'.  'cb' should not
- * add new queues because this may cause some queues to be visited twice or not
- * at all.
+ * Failure might indicate an actual error or merely that the last queue has
+ * been dumped.  An error status for the entire dump operation is provided when
+ * it is completed by calling netdev_queue_dump_done().
  *
- * Returns 0 if successful, otherwise a positive errno value.  On error, some
- * configured queues may not have been included in the iteration. */
+ * The returned contents of 'details' should be documented as valid for the
+ * given 'type' in the "other_config" column in the "Queue" table in
+ * vswitchd/vswitch.xml (which is built as ovs-vswitchd.conf.db(8)).
+ *
+ * The caller must initialize 'details' (e.g. with smap_init()) before calling
+ * this function.  This function will clear and replace its contents.  The
+ * caller must free 'details' when it is no longer needed (e.g. with
+ * smap_destroy()). */
+bool
+netdev_queue_dump_next(struct netdev_queue_dump *dump,
+                       unsigned int *queue_id, struct smap *details)
+{
+    const struct netdev *netdev = dump->netdev;
+
+    if (dump->error) {
+        return false;
+    }
+
+    dump->error = netdev->netdev_class->queue_dump_next(netdev, dump->state,
+                                                        queue_id, details);
+
+    if (dump->error) {
+        netdev->netdev_class->queue_dump_done(netdev, dump->state);
+        return false;
+    }
+    return true;
+}
+
+/* Completes queue table dump operation 'dump', which must have been
+ * initialized with netdev_queue_dump_start().  Returns 0 if the dump operation
+ * was error-free, otherwise a positive errno value describing the problem. */
 int
-netdev_dump_queues(const struct netdev *netdev,
-                   netdev_dump_queues_cb *cb, void *aux)
+netdev_queue_dump_done(struct netdev_queue_dump *dump)
 {
-    const struct netdev_class *class = netdev->netdev_class;
-    return (class->dump_queues
-            ? class->dump_queues(netdev, cb, aux)
-            : EOPNOTSUPP);
+    const struct netdev *netdev = dump->netdev;
+    if (!dump->error && netdev->netdev_class->queue_dump_done) {
+        dump->error = netdev->netdev_class->queue_dump_done(netdev,
+                                                            dump->state);
+    }
+    netdev_close(dump->netdev);
+    return dump->error == EOF ? 0 : dump->error;
 }
 
 /* Iterates over all of 'netdev''s queues, calling 'cb' with the queue's ID,
index 287f6cc..bafa50e 100644 (file)
@@ -47,7 +47,17 @@ extern "C" {
  *      These functions are conditionally thread-safe: they may be called from
  *      different threads only on different netdev_rx objects.  (The client may
  *      create multiple netdev_rx objects for a single netdev and access each
- *      of those from a different thread.) */
+ *      of those from a different thread.)
+ *
+ *    NETDEV_FOR_EACH_QUEUE
+ *    netdev_queue_dump_next()
+ *    netdev_queue_dump_done()
+ *
+ *      These functions are conditionally thread-safe: they may be called from
+ *      different threads only on different netdev_queue_dump objects.  (The
+ *      client may create multiple netdev_queue_dump objects for a single
+ *      netdev and access each of those from a different thread.)
+ */
 
 struct netdev;
 struct netdev_class;
@@ -271,10 +281,31 @@ int netdev_delete_queue(struct netdev *, unsigned int queue_id);
 int netdev_get_queue_stats(const struct netdev *, unsigned int queue_id,
                            struct netdev_queue_stats *);
 
-typedef void netdev_dump_queues_cb(unsigned int queue_id,
-                                   const struct smap *details, void *aux);
-int netdev_dump_queues(const struct netdev *,
-                       netdev_dump_queues_cb *, void *aux);
+struct netdev_queue_dump {
+    struct netdev *netdev;
+    int error;
+    void *state;
+};
+void netdev_queue_dump_start(struct netdev_queue_dump *,
+                             const struct netdev *);
+bool netdev_queue_dump_next(struct netdev_queue_dump *,
+                            unsigned int *queue_id, struct smap *details);
+int netdev_queue_dump_done(struct netdev_queue_dump *);
+
+/* Iterates through each queue in NETDEV, using DUMP as state.  Fills QUEUE_ID
+ * and DETAILS with information about queues.  The client must initialize and
+ * destroy DETAILS.
+ *
+ * Arguments all have pointer type.
+ *
+ * If you break out of the loop, then you need to free the dump structure by
+ * hand using netdev_queue_dump_done(). */
+#define NETDEV_QUEUE_FOR_EACH(QUEUE_ID, DETAILS, DUMP, NETDEV)  \
+    for (netdev_queue_dump_start(DUMP, NETDEV);                 \
+         (netdev_queue_dump_next(DUMP, QUEUE_ID, DETAILS)       \
+          ? true                                                \
+          : (netdev_queue_dump_done(DUMP), false));             \
+        )
 
 typedef void netdev_dump_queue_stats_cb(unsigned int queue_id,
                                         struct netdev_queue_stats *,
index a11e646..f993ba1 100644 (file)
@@ -2557,13 +2557,11 @@ struct qos_unixctl_show_cbdata {
 };
 
 static void
-qos_unixctl_show_cb(unsigned int queue_id,
-                    const struct smap *details,
-                    void *aux)
+qos_unixctl_show_queue(unsigned int queue_id,
+                       const struct smap *details,
+                       struct iface *iface,
+                       struct ds *ds)
 {
-    struct qos_unixctl_show_cbdata *data = aux;
-    struct ds *ds = data->ds;
-    struct iface *iface = data->iface;
     struct netdev_queue_stats stats;
     struct smap_node *node;
     int error;
@@ -2607,8 +2605,6 @@ qos_unixctl_show(struct unixctl_conn *conn, int argc OVS_UNUSED,
     struct iface *iface;
     const char *type;
     struct smap_node *node;
-    struct qos_unixctl_show_cbdata data;
-    int error;
 
     iface = iface_find(argv[1]);
     if (!iface) {
@@ -2619,20 +2615,22 @@ qos_unixctl_show(struct unixctl_conn *conn, int argc OVS_UNUSED,
     netdev_get_qos(iface->netdev, &type, &smap);
 
     if (*type != '\0') {
+        struct netdev_queue_dump dump;
+        struct smap details;
+        unsigned int queue_id;
+
         ds_put_format(&ds, "QoS: %s %s\n", iface->name, type);
 
         SMAP_FOR_EACH (node, &smap) {
             ds_put_format(&ds, "%s: %s\n", node->key, node->value);
         }
 
-        data.ds = &ds;
-        data.iface = iface;
-        error = netdev_dump_queues(iface->netdev, qos_unixctl_show_cb, &data);
-
-        if (error) {
-            ds_put_format(&ds, "failed to dump queues: %s",
-                          ovs_strerror(error));
+        smap_init(&details);
+        NETDEV_QUEUE_FOR_EACH (&queue_id, &details, &dump, iface->netdev) {
+            qos_unixctl_show_queue(queue_id, &details, iface, &ds);
         }
+        smap_destroy(&details);
+
         unixctl_command_reply(conn, ds_cstr(&ds));
     } else {
         ds_put_format(&ds, "QoS not configured on %s\n", iface->name);
@@ -3605,11 +3603,6 @@ iface_clear_db_record(const struct ovsrec_interface *if_cfg)
     }
 }
 
-struct iface_delete_queues_cbdata {
-    struct netdev *netdev;
-    const struct ovsdb_datum *queues;
-};
-
 static bool
 queue_ids_include(const struct ovsdb_datum *queues, int64_t target)
 {
@@ -3619,17 +3612,6 @@ queue_ids_include(const struct ovsdb_datum *queues, int64_t target)
     return ovsdb_datum_find_key(queues, &atom, OVSDB_TYPE_INTEGER) != UINT_MAX;
 }
 
-static void
-iface_delete_queues(unsigned int queue_id,
-                    const struct smap *details OVS_UNUSED, void *cbdata_)
-{
-    struct iface_delete_queues_cbdata *cbdata = cbdata_;
-
-    if (!queue_ids_include(cbdata->queues, queue_id)) {
-        netdev_delete_queue(cbdata->netdev, queue_id);
-    }
-}
-
 static void
 iface_configure_qos(struct iface *iface, const struct ovsrec_qos *qos)
 {
@@ -3640,7 +3622,10 @@ iface_configure_qos(struct iface *iface, const struct ovsrec_qos *qos)
     if (!qos || qos->type[0] == '\0' || qos->n_queues < 1) {
         netdev_set_qos(iface->netdev, NULL, NULL);
     } else {
-        struct iface_delete_queues_cbdata cbdata;
+        const struct ovsdb_datum *queues;
+        struct netdev_queue_dump dump;
+        unsigned int queue_id;
+        struct smap details;
         bool queue_zero;
         size_t i;
 
@@ -3648,10 +3633,15 @@ iface_configure_qos(struct iface *iface, const struct ovsrec_qos *qos)
         netdev_set_qos(iface->netdev, qos->type, &qos->other_config);
 
         /* Deconfigure queues that were deleted. */
-        cbdata.netdev = iface->netdev;
-        cbdata.queues = ovsrec_qos_get_queues(qos, OVSDB_TYPE_INTEGER,
-                                              OVSDB_TYPE_UUID);
-        netdev_dump_queues(iface->netdev, iface_delete_queues, &cbdata);
+        queues = ovsrec_qos_get_queues(qos, OVSDB_TYPE_INTEGER,
+                                       OVSDB_TYPE_UUID);
+        smap_init(&details);
+        NETDEV_QUEUE_FOR_EACH (&queue_id, &details, &dump, iface->netdev) {
+            if (!queue_ids_include(queues, queue_id)) {
+                netdev_delete_queue(iface->netdev, queue_id);
+            }
+        }
+        smap_destroy(&details);
 
         /* Configure queues for 'iface'. */
         queue_zero = false;