Acked-by: Ethan Jackson <et...@nicira.com>
Thanks looks good.
On Tue, Aug 27, 2013 at 5:16 PM, Ben Pfaff <b...@nicira.com> wrote:
> 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 <shendri...@vmware.com>
> Signed-off-by: Ben Pfaff <b...@nicira.com>
> ---
> lib/netdev-dummy.c | 4 ++-
> lib/netdev-linux.c | 79 +++++++++++++++++++++++++++++++++------------
> lib/netdev-provider.h | 49 +++++++++++++++++++---------
> lib/netdev-vport.c | 4 ++-
> lib/netdev.c | 86
> ++++++++++++++++++++++++++++++++++++++-----------
> lib/netdev.h | 41 ++++++++++++++++++++---
> vswitchd/bridge.c | 62 +++++++++++++++--------------------
> 7 files changed, 226 insertions(+), 99 deletions(-)
>
> diff --git a/lib/netdev-dummy.c b/lib/netdev-dummy.c
> index ac26564..16a6b0b 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 80fa39b..26fb32d 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;
> }
> @@ -2139,6 +2139,41 @@ netdev_linux_dump_queues(const struct netdev *netdev_,
> }
>
> 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 70a5188..9ab58fb 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 0f5dd7a..af50597 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 bf942a0..5ed6062 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 287f6cc..bafa50e 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 a11e646..f993ba1 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)
> {
> @@ -3620,17 +3613,6 @@ queue_ids_include(const struct ovsdb_datum *queues,
> int64_t target)
> }
>
> 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)
> {
> struct ofpbuf queues_buf;
> @@ -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;
> --
> 1.7.10.4
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
