From: Ben Pfaff Date: Wed, 28 Aug 2013 00:15:53 +0000 (-0700) Subject: netdev: Fix deadlock when netdev_dump_queues() callback calls into netdev. X-Git-Tag: sliver-openvswitch-2.0.90-1~18^2~6 X-Git-Url: http://git.onelab.eu/?a=commitdiff_plain;h=89454bf477d1dc95357792677ccbd4d483ab42d8;p=sliver-openvswitch.git netdev: Fix deadlock when netdev_dump_queues() callback calls into netdev. 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 Signed-off-by: Ben Pfaff --- diff --git a/lib/netdev-dummy.c b/lib/netdev-dummy.c index ac2656490..16a6b0b5a 100644 --- a/lib/netdev-dummy.c +++ b/lib/netdev-dummy.c @@ -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 */ diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c index 80fa39b47..26fb32d82 100644 --- a/lib/netdev-linux.c +++ b/lib/netdev-linux.c @@ -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, \ diff --git a/lib/netdev-provider.h b/lib/netdev-provider.h index 70a5188ed..9ab58fb22 100644 --- a/lib/netdev-provider.h +++ b/lib/netdev-provider.h @@ -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 diff --git a/lib/netdev-vport.c b/lib/netdev-vport.c index 0f5dd7ab3..af5059739 100644 --- a/lib/netdev-vport.c +++ b/lib/netdev-vport.c @@ -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 */ \ diff --git a/lib/netdev.c b/lib/netdev.c index bf942a0f4..5ed60624c 100644 --- a/lib/netdev.c +++ b/lib/netdev.c @@ -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, diff --git a/lib/netdev.h b/lib/netdev.h index 287f6cc55..bafa50eba 100644 --- a/lib/netdev.h +++ b/lib/netdev.h @@ -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 *, diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c index a11e64696..f993ba126 100644 --- a/vswitchd/bridge.c +++ b/vswitchd/bridge.c @@ -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;