Thanks, applied.
On Tue, Aug 27, 2013 at 05:33:59PM -0700, Ethan Jackson wrote:
> 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
