[ovs-dev] [PATCH] dpif: Refactor flow dumping interface to make better sense for batching.

Ben Pfaff blp at nicira.com
Fri May 16 23:28:44 UTC 2014


Commit a6ce4b9d251 (ofproto-dpif-upcall: Avoid use-after-free in
revalidate() corner case.) showed that it is somewhat tricky to correctly
use the existing dpif flow dumping interface to obtain batches of flows.
One has to be careful about calling dpif_flow_dump_next_may_destroy_keys()
before going on to the next flow.

A better interface is possible, one that is naturally oriented toward
retrieving batches when that is a useful optimization.  This commit
replaces the dpif interface by such a design, and updates both the
implementations and the callers to adopt it.

This is a fairly large change, but I think that the code in
ofproto-dpif-upcall is easier to understand after the change.

Signed-off-by: Ben Pfaff <blp at nicira.com>
---
 lib/dpif-linux.c              |  223 +++++++++++++++++++++++------------------
 lib/dpif-netdev.c             |  184 ++++++++++++++++++----------------
 lib/dpif-provider.h           |  118 +++++++++-------------
 lib/dpif.c                    |  173 ++++++++++++--------------------
 lib/dpif.h                    |   71 ++++++++++---
 ofproto/ofproto-dpif-upcall.c |  198 ++++++++++++++++++------------------
 ofproto/ofproto-dpif.c        |   43 ++++----
 utilities/ovs-dpctl.c         |   57 +++++------
 8 files changed, 521 insertions(+), 546 deletions(-)

diff --git a/lib/dpif-linux.c b/lib/dpif-linux.c
index abb4b51..63e66f3 100644
--- a/lib/dpif-linux.c
+++ b/lib/dpif-linux.c
@@ -1138,47 +1138,28 @@ dpif_linux_flow_del(struct dpif *dpif_, const struct dpif_flow_del *del)
     return error;
 }
 
-struct dpif_linux_flow_state {
-    struct dpif_linux_flow flow;
-    struct dpif_flow_stats stats;
-    struct ofpbuf buffer;         /* Always used to store flows. */
-    struct ofpbuf *tmp;           /* Used if kernel does not supply actions. */
-};
-
-struct dpif_linux_flow_iter {
-    struct nl_dump dump;
+struct dpif_linux_flow_dump {
+    struct dpif_flow_dump up;
+    struct nl_dump nl_dump;
     atomic_int status;
 };
 
-static void
-dpif_linux_flow_dump_state_init(void **statep)
-{
-    struct dpif_linux_flow_state *state;
-
-    *statep = state = xmalloc(sizeof *state);
-    ofpbuf_init(&state->buffer, NL_DUMP_BUFSIZE);
-    state->tmp = NULL;
-}
-
-static void
-dpif_linux_flow_dump_state_uninit(void *state_)
+static struct dpif_linux_flow_dump *
+dpif_linux_flow_dump_cast(struct dpif_flow_dump *dump)
 {
-    struct dpif_linux_flow_state *state = state_;
-
-    ofpbuf_uninit(&state->buffer);
-    ofpbuf_delete(state->tmp);
-    free(state);
+    return CONTAINER_OF(dump, struct dpif_linux_flow_dump, up);
 }
 
-static int
-dpif_linux_flow_dump_start(const struct dpif *dpif_, void **iterp)
+static struct dpif_flow_dump *
+dpif_linux_flow_dump_create(const struct dpif *dpif_)
 {
     const struct dpif_linux *dpif = dpif_linux_cast(dpif_);
-    struct dpif_linux_flow_iter *iter;
+    struct dpif_linux_flow_dump *dump;
     struct dpif_linux_flow request;
     struct ofpbuf *buf;
 
-    *iterp = iter = xmalloc(sizeof *iter);
+    dump = xmalloc(sizeof *dump);
+    dpif_flow_dump_init(&dump->up, dpif_);
 
     dpif_linux_flow_init(&request);
     request.cmd = OVS_FLOW_CMD_GET;
@@ -1186,90 +1167,137 @@ dpif_linux_flow_dump_start(const struct dpif *dpif_, void **iterp)
 
     buf = ofpbuf_new(1024);
     dpif_linux_flow_to_ofpbuf(&request, buf);
-    nl_dump_start(&iter->dump, NETLINK_GENERIC, buf);
+    nl_dump_start(&dump->nl_dump, NETLINK_GENERIC, buf);
     ofpbuf_delete(buf);
-    atomic_init(&iter->status, 0);
+    atomic_init(&dump->status, 0);
 
-    return 0;
+    return &dump->up;
 }
 
 static int
-dpif_linux_flow_dump_next(const struct dpif *dpif_, void *iter_, void *state_,
-                          const struct nlattr **key, size_t *key_len,
-                          const struct nlattr **mask, size_t *mask_len,
-                          const struct nlattr **actions, size_t *actions_len,
-                          const struct dpif_flow_stats **stats)
+dpif_linux_flow_dump_destroy(struct dpif_flow_dump *dump_)
 {
-    const struct dpif_linux *dpif = dpif_linux_cast(dpif_);
-    struct dpif_linux_flow_iter *iter = iter_;
-    struct dpif_linux_flow_state *state = state_;
-    struct ofpbuf buf;
-    int error;
+    struct dpif_linux_flow_dump *dump = dpif_linux_flow_dump_cast(dump_);
+    unsigned int nl_status = nl_dump_done(&dump->nl_dump);
+    int dump_status;
+
+    atomic_read(&dump->status, &dump_status);
+    free(dump);
+    return dump_status ? dump_status : nl_status;
+}
+
+struct dpif_linux_flow_dump_thread {
+    struct dpif_flow_dump_thread up;
+    struct dpif_linux_flow_dump *dump;
+    struct dpif_linux_flow flow;
+    struct dpif_flow_stats stats;
+    struct ofpbuf nl_flows;     /* Always used to store flows. */
+    struct ofpbuf *nl_actions;  /* Used if kernel does not supply actions. */
+};
+
+static struct dpif_linux_flow_dump_thread *
+dpif_linux_flow_dump_thread_cast(struct dpif_flow_dump_thread *thread)
+{
+    return CONTAINER_OF(thread, struct dpif_linux_flow_dump_thread, up);
+}
+
+static struct dpif_flow_dump_thread *
+dpif_linux_flow_dump_thread_create(struct dpif_flow_dump *dump_)
+{
+    struct dpif_linux_flow_dump *dump = dpif_linux_flow_dump_cast(dump_);
+    struct dpif_linux_flow_dump_thread *thread;
+
+    thread = xmalloc(sizeof *thread);
+    dpif_flow_dump_thread_init(&thread->up, &dump->up);
+    thread->dump = dump;
+    ofpbuf_init(&thread->nl_flows, NL_DUMP_BUFSIZE);
+    thread->nl_actions = NULL;
+
+    return &thread->up;
+}
 
-    do {
-        ofpbuf_delete(state->tmp);
-        state->tmp = NULL;
+static void
+dpif_linux_flow_dump_thread_destroy(struct dpif_flow_dump_thread *thread_)
+{
+    struct dpif_linux_flow_dump_thread *thread
+        = dpif_linux_flow_dump_thread_cast(thread_);
+
+    ofpbuf_uninit(&thread->nl_flows);
+    ofpbuf_delete(thread->nl_actions);
+    free(thread);
+}
+
+static void
+dpif_linux_flow_to_dpif_flow(struct dpif_flow *dpif_flow,
+                             struct dpif_linux_flow *linux_flow)
+{
+    dpif_flow->key = linux_flow->key;
+    dpif_flow->key_len = linux_flow->key_len;
+    dpif_flow->mask = linux_flow->mask;
+    dpif_flow->mask_len = linux_flow->mask_len;
+    dpif_flow->actions = linux_flow->actions;
+    dpif_flow->actions_len = linux_flow->actions_len;
+    dpif_linux_flow_get_stats(linux_flow, &dpif_flow->stats);
+}
+
+static int
+dpif_linux_flow_dump_next(struct dpif_flow_dump_thread *thread_,
+                          struct dpif_flow *flows, int max_flows)
+{
+    struct dpif_linux_flow_dump_thread *thread
+        = dpif_linux_flow_dump_thread_cast(thread_);
+    struct dpif_linux_flow_dump *dump = thread->dump;
+    struct dpif_linux *dpif = dpif_linux_cast(thread->up.dpif);
+    int n_flows;
+
+    ofpbuf_delete(thread->nl_actions);
+    thread->nl_actions = NULL;
+
+    n_flows = 0;
+    while (!n_flows
+           || (n_flows < max_flows && ofpbuf_size(&thread->nl_flows))) {
+        struct dpif_linux_flow linux_flow;
+        struct ofpbuf nl_flow;
+        int error;
 
-        if (!nl_dump_next(&iter->dump, &buf, &state->buffer)) {
-            return EOF;
+        /* Try to grab another flow. */
+        if (!nl_dump_next(&dump->nl_dump, &nl_flow, &thread->nl_flows)) {
+            break;
         }
 
-        error = dpif_linux_flow_from_ofpbuf(&state->flow, &buf);
+        /* Convert the flow to our output format. */
+        error = dpif_linux_flow_from_ofpbuf(&linux_flow, &nl_flow);
         if (error) {
-            atomic_store(&iter->status, error);
-            return error;
+            atomic_store(&dump->status, error);
+            break;
         }
 
-        if (actions && !state->flow.actions) {
-            error = dpif_linux_flow_get__(dpif, state->flow.key,
-                                          state->flow.key_len,
-                                          &state->flow, &state->tmp);
+        if (linux_flow.actions) {
+            /* Common case: the flow includes actions. */
+            dpif_linux_flow_to_dpif_flow(&flows[n_flows++], &linux_flow);
+        } else {
+            /* Rare case: the flow does not include actions.  Retrieve this
+             * individual flow again to get the actions. */
+            error = dpif_linux_flow_get__(dpif, linux_flow.key,
+                                          linux_flow.key_len, &linux_flow,
+                                          &thread->nl_actions);
             if (error == ENOENT) {
                 VLOG_DBG("dumped flow disappeared on get");
+                continue;
             } else if (error) {
                 VLOG_WARN("error fetching dumped flow: %s",
                           ovs_strerror(error));
+                atomic_store(&dump->status, error);
+                break;
             }
-        }
-    } while (error);
 
-    if (actions) {
-        *actions = state->flow.actions;
-        *actions_len = state->flow.actions_len;
-    }
-    if (key) {
-        *key = state->flow.key;
-        *key_len = state->flow.key_len;
-    }
-    if (mask) {
-        *mask = state->flow.mask;
-        *mask_len = state->flow.mask ? state->flow.mask_len : 0;
-    }
-    if (stats) {
-        dpif_linux_flow_get_stats(&state->flow, &state->stats);
-        *stats = &state->stats;
+            /* Save this flow.  Then exit, because we only have one buffer to
+             * handle this case. */
+            dpif_linux_flow_to_dpif_flow(&flows[n_flows++], &linux_flow);
+            break;
+        }
     }
-    return error;
-}
-
-static bool
-dpif_linux_flow_dump_next_may_destroy_keys(void *state_)
-{
-    struct dpif_linux_flow_state *state = state_;
-
-    return ofpbuf_size(&state->buffer) ? false : true;
-}
-
-static int
-dpif_linux_flow_dump_done(const struct dpif *dpif OVS_UNUSED, void *iter_)
-{
-    struct dpif_linux_flow_iter *iter = iter_;
-    int dump_status;
-    unsigned int nl_status = nl_dump_done(&iter->dump);
-
-    atomic_read(&iter->status, &dump_status);
-    free(iter);
-    return dump_status ? dump_status : nl_status;
+    return n_flows;
 }
 
 static void
@@ -1878,12 +1906,11 @@ const struct dpif_class dpif_linux_class = {
     dpif_linux_flow_put,
     dpif_linux_flow_del,
     dpif_linux_flow_flush,
-    dpif_linux_flow_dump_state_init,
-    dpif_linux_flow_dump_start,
+    dpif_linux_flow_dump_create,
+    dpif_linux_flow_dump_destroy,
+    dpif_linux_flow_dump_thread_create,
+    dpif_linux_flow_dump_thread_destroy,
     dpif_linux_flow_dump_next,
-    dpif_linux_flow_dump_next_may_destroy_keys,
-    dpif_linux_flow_dump_done,
-    dpif_linux_flow_dump_state_uninit,
     dpif_linux_execute,
     dpif_linux_operate,
     dpif_linux_recv_set,
diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index a255a96..3922e5e 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -1371,134 +1371,141 @@ dpif_netdev_flow_del(struct dpif *dpif, const struct dpif_flow_del *del)
     return error;
 }
 
-struct dp_netdev_flow_state {
-    struct odputil_keybuf keybuf;
-    struct odputil_keybuf maskbuf;
-    struct dpif_flow_stats stats;
-};
-
-struct dp_netdev_flow_iter {
+struct dpif_netdev_flow_dump {
+    struct dpif_flow_dump up;
     uint32_t bucket;
     uint32_t offset;
     int status;
     struct ovs_mutex mutex;
 };
 
-static void
-dpif_netdev_flow_dump_state_init(void **statep)
+static struct dpif_netdev_flow_dump *
+dpif_netdev_flow_dump_cast(struct dpif_flow_dump *dump)
 {
-    struct dp_netdev_flow_state *state;
-
-    *statep = state = xmalloc(sizeof *state);
+    return CONTAINER_OF(dump, struct dpif_netdev_flow_dump, up);
 }
 
-static void
-dpif_netdev_flow_dump_state_uninit(void *state_)
+static struct dpif_flow_dump *
+dpif_netdev_flow_dump_create(const struct dpif *dpif_)
 {
-    struct dp_netdev_flow_state *state = state_;
+    struct dpif_netdev_flow_dump *dump;
 
-    free(state);
+    dump = xmalloc(sizeof *dump);
+    dpif_flow_dump_init(&dump->up, dpif_);
+    dump->bucket = 0;
+    dump->offset = 0;
+    dump->status = 0;
+    ovs_mutex_init(&dump->mutex);
+
+    return &dump->up;
 }
 
 static int
-dpif_netdev_flow_dump_start(const struct dpif *dpif OVS_UNUSED, void **iterp)
+dpif_netdev_flow_dump_destroy(struct dpif_flow_dump *dump_)
 {
-    struct dp_netdev_flow_iter *iter;
+    struct dpif_netdev_flow_dump *dump = dpif_netdev_flow_dump_cast(dump_);
 
-    *iterp = iter = xmalloc(sizeof *iter);
-    iter->bucket = 0;
-    iter->offset = 0;
-    iter->status = 0;
-    ovs_mutex_init(&iter->mutex);
+    ovs_mutex_destroy(&dump->mutex);
+    free(dump);
     return 0;
 }
 
+struct dpif_netdev_flow_dump_thread {
+    struct dpif_flow_dump_thread up;
+    struct dpif_netdev_flow_dump *dump;
+    struct odputil_keybuf keybuf;
+    struct odputil_keybuf maskbuf;
+};
+
+static struct dpif_netdev_flow_dump_thread *
+dpif_netdev_flow_dump_thread_cast(struct dpif_flow_dump_thread *thread)
+{
+    return CONTAINER_OF(thread, struct dpif_netdev_flow_dump_thread, up);
+}
+
+static struct dpif_flow_dump_thread *
+dpif_netdev_flow_dump_thread_create(struct dpif_flow_dump *dump_)
+{
+    struct dpif_netdev_flow_dump *dump = dpif_netdev_flow_dump_cast(dump_);
+    struct dpif_netdev_flow_dump_thread *thread;
+
+    thread = xmalloc(sizeof *thread);
+    dpif_flow_dump_thread_init(&thread->up, &dump->up);
+    thread->dump = dump;
+    return &thread->up;
+}
+
+static void
+dpif_netdev_flow_dump_thread_destroy(struct dpif_flow_dump_thread *thread_)
+{
+    struct dpif_netdev_flow_dump_thread *thread
+        = dpif_netdev_flow_dump_thread_cast(thread_);
+
+    free(thread);
+}
+
 /* XXX the caller must use 'actions' without quiescing */
 static int
-dpif_netdev_flow_dump_next(const struct dpif *dpif, void *iter_, void *state_,
-                           const struct nlattr **key, size_t *key_len,
-                           const struct nlattr **mask, size_t *mask_len,
-                           const struct nlattr **actions, size_t *actions_len,
-                           const struct dpif_flow_stats **stats)
-{
-    struct dp_netdev_flow_iter *iter = iter_;
-    struct dp_netdev_flow_state *state = state_;
-    struct dp_netdev *dp = get_dp_netdev(dpif);
+dpif_netdev_flow_dump_next(struct dpif_flow_dump_thread *thread_,
+                           struct dpif_flow *f, int max_flows OVS_UNUSED)
+{
+    struct dpif_netdev_flow_dump_thread *thread
+        = dpif_netdev_flow_dump_thread_cast(thread_);
+    struct dpif_netdev_flow_dump *dump = thread->dump;
+    struct dpif_netdev *dpif = dpif_netdev_cast(thread->up.dpif);
+    struct dp_netdev *dp = get_dp_netdev(&dpif->dpif);
     struct dp_netdev_flow *netdev_flow;
     struct flow_wildcards wc;
+    struct dp_netdev_actions *dp_actions;
+    struct ofpbuf buf;
     int error;
 
-    ovs_mutex_lock(&iter->mutex);
-    error = iter->status;
+    ovs_mutex_lock(&dump->mutex);
+    error = dump->status;
     if (!error) {
         struct hmap_node *node;
 
         fat_rwlock_rdlock(&dp->cls.rwlock);
-        node = hmap_at_position(&dp->flow_table, &iter->bucket, &iter->offset);
+        node = hmap_at_position(&dp->flow_table, &dump->bucket, &dump->offset);
         if (node) {
             netdev_flow = CONTAINER_OF(node, struct dp_netdev_flow, node);
         }
         fat_rwlock_unlock(&dp->cls.rwlock);
         if (!node) {
-            iter->status = error = EOF;
+            dump->status = error = EOF;
         }
     }
-    ovs_mutex_unlock(&iter->mutex);
+    ovs_mutex_unlock(&dump->mutex);
     if (error) {
-        return error;
+        return 0;
     }
 
     minimask_expand(&netdev_flow->cr.match.mask, &wc);
 
-    if (key) {
-        struct ofpbuf buf;
-
-        ofpbuf_use_stack(&buf, &state->keybuf, sizeof state->keybuf);
-        odp_flow_key_from_flow(&buf, &netdev_flow->flow, &wc.masks,
-                               netdev_flow->flow.in_port.odp_port);
-
-        *key = ofpbuf_data(&buf);
-        *key_len = ofpbuf_size(&buf);
-    }
-
-    if (key && mask) {
-        struct ofpbuf buf;
-
-        ofpbuf_use_stack(&buf, &state->maskbuf, sizeof state->maskbuf);
-        odp_flow_key_from_mask(&buf, &wc.masks, &netdev_flow->flow,
-                               odp_to_u32(wc.masks.in_port.odp_port),
-                               SIZE_MAX);
-
-        *mask = ofpbuf_data(&buf);
-        *mask_len = ofpbuf_size(&buf);
-    }
-
-    if (actions || stats) {
-        if (actions) {
-            struct dp_netdev_actions *dp_actions =
-                dp_netdev_flow_get_actions(netdev_flow);
+    /* Key. */
+    ofpbuf_use_stack(&buf, &thread->keybuf, sizeof thread->keybuf);
+    odp_flow_key_from_flow(&buf, &netdev_flow->flow, &wc.masks,
+                           netdev_flow->flow.in_port.odp_port);
+    f->key = ofpbuf_data(&buf);
+    f->key_len = ofpbuf_size(&buf);
+
+    /* Mask. */
+    ofpbuf_use_stack(&buf, &thread->maskbuf, sizeof thread->maskbuf);
+    odp_flow_key_from_mask(&buf, &wc.masks, &netdev_flow->flow,
+                           odp_to_u32(wc.masks.in_port.odp_port),
+                           SIZE_MAX);
+    f->mask = ofpbuf_data(&buf);
+    f->mask_len = ofpbuf_size(&buf);
 
-            *actions = dp_actions->actions;
-            *actions_len = dp_actions->size;
-        }
-
-        if (stats) {
-            get_dpif_flow_stats(netdev_flow, &state->stats);
-            *stats = &state->stats;
-        }
-    }
-
-    return 0;
-}
+    /* Actions. */
+    dp_actions = dp_netdev_flow_get_actions(netdev_flow);
+    f->actions = dp_actions->actions;
+    f->actions_len = dp_actions->size;
 
-static int
-dpif_netdev_flow_dump_done(const struct dpif *dpif OVS_UNUSED, void *iter_)
-{
-    struct dp_netdev_flow_iter *iter = iter_;
+    /* Stats. */
+    get_dpif_flow_stats(netdev_flow, &f->stats);
 
-    ovs_mutex_destroy(&iter->mutex);
-    free(iter);
-    return 0;
+    return 1;
 }
 
 static int
@@ -2219,12 +2226,11 @@ const struct dpif_class dpif_netdev_class = {
     dpif_netdev_flow_put,
     dpif_netdev_flow_del,
     dpif_netdev_flow_flush,
-    dpif_netdev_flow_dump_state_init,
-    dpif_netdev_flow_dump_start,
+    dpif_netdev_flow_dump_create,
+    dpif_netdev_flow_dump_destroy,
+    dpif_netdev_flow_dump_thread_create,
+    dpif_netdev_flow_dump_thread_destroy,
     dpif_netdev_flow_dump_next,
-    NULL,
-    dpif_netdev_flow_dump_done,
-    dpif_netdev_flow_dump_state_uninit,
     dpif_netdev_execute,
     NULL,                       /* operate */
     dpif_netdev_recv_set,
diff --git a/lib/dpif-provider.h b/lib/dpif-provider.h
index 615f2c6..d0b2eaf 100644
--- a/lib/dpif-provider.h
+++ b/lib/dpif-provider.h
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2009, 2010, 2011, 2012, 2013 Nicira, Inc.
+ * Copyright (c) 2009, 2010, 2011, 2012, 2013, 2014 Nicira, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -51,6 +51,27 @@ static inline void dpif_assert_class(const struct dpif *dpif,
     ovs_assert(dpif->dpif_class == dpif_class);
 }
 
+struct dpif_flow_dump {
+    struct dpif *dpif;
+};
+
+static inline void
+dpif_flow_dump_init(struct dpif_flow_dump *dump, const struct dpif *dpif)
+{
+    dump->dpif = CONST_CAST(struct dpif *, dpif);
+}
+
+struct dpif_flow_dump_thread {
+    struct dpif *dpif;
+};
+
+static inline void
+dpif_flow_dump_thread_init(struct dpif_flow_dump_thread *thread,
+                           struct dpif_flow_dump *dump)
+{
+    thread->dpif = dump->dpif;
+}
+
 /* Datapath interface class structure, to be defined by each implementation of
  * a datapath interface.
  *
@@ -272,78 +293,29 @@ struct dpif_class {
      * packets. */
     int (*flow_flush)(struct dpif *dpif);
 
-    /* Allocates thread-local state for use with the function 'flow_dump_next'.
-     * On return, initializes '*statep' with any private data needed for
-     * iteration. */
-    void (*flow_dump_state_init)(void **statep);
-
-    /* Attempts to begin dumping the flows in a dpif.  On success, returns 0
-     * and initializes '*iterp' with any shared data needed for iteration.
-     * On failure, returns a positive errno value. */
-    int (*flow_dump_start)(const struct dpif *dpif, void **iterp);
-
-    /* Attempts to retrieve another flow from 'dpif' for 'iter', using
-     * 'state' for storage. 'iter' must have been initialized by a successful
-     * call to the 'flow_dump_start' function for 'dpif'. 'state' must have
-     * been initialised with a call to the 'flow_dump_state_init' function for
-     * 'dpif.
-     *
-     * On success, updates the output parameters as described below and returns
-     * 0. Returns EOF if the end of the flow table has been reached, or a
-     * positive errno value on error. Multiple threads may use the same 'dpif'
-     * and 'iter' with this function, but all other parameters must be
-     * different for each thread. If this function returns non-zero,
-     * subsequent calls with the same arguments will also return non-zero.
-     *
-     * On success:
-     *
-     *     - If 'key' and 'key_len' are nonnull, then '*key' and '*key_len'
-     *       must be set to Netlink attributes with types OVS_KEY_ATTR_*
-     *       representing the dumped flow's key.
-     *
-     *     - If 'mask' and 'mask_len' are nonnull then '*mask' and '*mask_len'
-     *       must be set to Netlink attributes with types of OVS_KEY_ATTR_*
-     *       representing the dumped flow's mask.
-     *
-     *     - If 'actions' and 'actions_len' are nonnull then they should be set
-     *       to Netlink attributes with types OVS_ACTION_ATTR_* representing
-     *       the dumped flow's actions.
-     *
-     *     - If 'stats' is nonnull then it should be set to the dumped flow's
-     *       statistics.
-     *
-     * All of the returned data is owned by 'dpif', not by the caller, and the
-     * caller must not modify or free it.  'dpif' must guarantee that it
-     * remains accessible and unchanging until at least the next call to
-     * 'flow_dump_next' or 'flow_dump_done' for 'iter' and 'state'. */
-    int (*flow_dump_next)(const struct dpif *dpif, void *iter, void *state,
-                          const struct nlattr **key, size_t *key_len,
-                          const struct nlattr **mask, size_t *mask_len,
-                          const struct nlattr **actions, size_t *actions_len,
-                          const struct dpif_flow_stats **stats);
-
-    /* Determines whether the next call to 'flow_dump_next' with 'state' will
-     * modify or free the keys that it previously returned. 'state' must have
-     * been initialized by a call to 'flow_dump_state_init' for 'dpif'.
-     *
-     * 'dpif' guarantees that data returned by flow_dump_next() will remain
-     * accessible and unchanging until the next call. This function provides a
-     * way for callers to determine whether that guarantee extends beyond the
-     * next call.
-     *
-     * Returns true if the next call to flow_dump_next() is expected to be
-     * destructive to previously returned keys for 'state', false otherwise. */
-    bool (*flow_dump_next_may_destroy_keys)(void *state);
-
-    /* Releases resources from 'dpif' for 'iter', which was initialized by a
-     * successful call to the 'flow_dump_start' function for 'dpif'. Callers
-     * must ensure that this function is called once within a given iteration,
-     * as the final flow dump operation. */
-    int (*flow_dump_done)(const struct dpif *dpif, void *iter);
-
-    /* Releases 'state' which was initialized by a call to the
-     * 'flow_dump_state_init' function for this 'dpif'. */
-    void (*flow_dump_state_uninit)(void *statep);
+    /* Flow dumping interface.
+     *
+     * This is the back-end for the flow dumping interface described in
+     * dpif.h.  Please read the comments there first, because this code
+     * closely follows it.
+     *
+     * 'flow_dump_create' and 'flow_dump_thread_create' must always return an
+     * initialized and usable data structure and defer error return until
+     * flow_dump_destroy().  This hasn't been a problem for the dpifs that
+     * exist so far.
+     *
+     * 'flow_dump_create' and 'flow_dump_thread_create' must initialize the
+     * structures that they return with dpif_flow_dump_init() and
+     * dpif_flow_dump_thread_init(), respectively. */
+    struct dpif_flow_dump *(*flow_dump_create)(const struct dpif *dpif);
+    int (*flow_dump_destroy)(struct dpif_flow_dump *dump);
+
+    struct dpif_flow_dump_thread *(*flow_dump_thread_create)(
+        struct dpif_flow_dump *dump);
+    void (*flow_dump_thread_destroy)(struct dpif_flow_dump_thread *thread);
+
+    int (*flow_dump_next)(struct dpif_flow_dump_thread *thread,
+                          struct dpif_flow *flows, int max_flows);
 
     /* Performs the 'execute->actions_len' bytes of actions in
      * 'execute->actions' on the Ethernet frame in 'execute->packet'
diff --git a/lib/dpif.c b/lib/dpif.c
index 41b8eb7..84dba28 100644
--- a/lib/dpif.c
+++ b/lib/dpif.c
@@ -970,133 +970,84 @@ dpif_flow_del(struct dpif *dpif,
     return dpif_flow_del__(dpif, &del);
 }
 
-/* Allocates thread-local state for use with the 'flow_dump_next' function for
- * 'dpif'. On return, initializes '*statep' with any private data needed for
- * iteration. */
-void
-dpif_flow_dump_state_init(const struct dpif *dpif, void **statep)
+/* Creates and returns a new 'struct dpif_flow_dump' for iterating through the
+ * flows in 'dpif'.
+ *
+ * This function always successfully returns a dpif_flow_dump.  Error
+ * reporting is deferred to dpif_flow_dump_destroy(). */
+struct dpif_flow_dump *
+dpif_flow_dump_create(const struct dpif *dpif)
 {
-    dpif->dpif_class->flow_dump_state_init(statep);
+    return dpif->dpif_class->flow_dump_create(dpif);
 }
 
-/* Releases 'state' which was initialized by a call to the
- * 'flow_dump_state_init' function for 'dpif'. */
-void
-dpif_flow_dump_state_uninit(const struct dpif *dpif, void *state)
+/* Destroys 'dump', which must have been created with dpif_flow_dump_create().
+ * All dpif_flow_dump_thread structures previously created for 'dump' must
+ * previously have been destroyed.
+ *
+ * Returns 0 if the dump operation was error-free, otherwise a positive errno
+ * value describing the problem. */
+int
+dpif_flow_dump_destroy(struct dpif_flow_dump *dump)
 {
-    dpif->dpif_class->flow_dump_state_uninit(state);
+    const struct dpif *dpif = dump->dpif;
+    int error = dpif->dpif_class->flow_dump_destroy(dump);
+    log_operation(dpif, "flow_dump_destroy", error);
+    return error == EOF ? 0 : error;
 }
 
-/* Initializes 'dump' to begin dumping the flows in a dpif. On sucess,
- * initializes 'dump' with any data needed for iteration and returns 0.
- * Otherwise, returns a positive errno value describing the problem. */
-int
-dpif_flow_dump_start(struct dpif_flow_dump *dump, const struct dpif *dpif)
+/* Returns new thread-local state for use with dpif_flow_dump_next(). */
+struct dpif_flow_dump_thread *
+dpif_flow_dump_thread_create(struct dpif_flow_dump *dump)
 {
-    int error;
-    dump->dpif = dpif;
-    error = dpif->dpif_class->flow_dump_start(dpif, &dump->iter);
-    log_operation(dpif, "flow_dump_start", error);
-    return error;
+    return dump->dpif->dpif_class->flow_dump_thread_create(dump);
 }
 
-/* Attempts to retrieve another flow from 'dump', using 'state' for
- * thread-local storage. 'dump' must have been initialized with a successful
- * call to dpif_flow_dump_start(), and 'state' must have been initialized with
- * dpif_flow_state_init().
- *
- * On success, updates the output parameters as described below and returns
- * true. Otherwise, returns false. Failure might indicate an actual error or
- * merely the end of the flow table. An error status for the entire dump
- * operation is provided when it is completed by calling dpif_flow_dump_done().
- * Multiple threads may use the same 'dump' with this function, but all other
- * parameters must not be shared.
- *
- * On success, if 'key' and 'key_len' are nonnull then '*key' and '*key_len'
- * will be set to Netlink attributes with types OVS_KEY_ATTR_* representing the
- * dumped flow's key.  If 'actions' and 'actions_len' are nonnull then they are
- * set to Netlink attributes with types OVS_ACTION_ATTR_* representing the
- * dumped flow's actions.  If 'stats' is nonnull then it will be set to the
- * dumped flow's statistics.
- *
- * All of the returned data is owned by 'dpif', not by the caller, and the
- * caller must not modify or free it.  'dpif' guarantees that it remains
- * accessible and unchanging until at least the next call to 'flow_dump_next'
- * or 'flow_dump_done' for 'dump' and 'state'. */
-bool
-dpif_flow_dump_next(struct dpif_flow_dump *dump, void *state,
-                    const struct nlattr **key, size_t *key_len,
-                    const struct nlattr **mask, size_t *mask_len,
-                    const struct nlattr **actions, size_t *actions_len,
-                    const struct dpif_flow_stats **stats)
+/* Releases 'thread'. */
+void
+dpif_flow_dump_thread_destroy(struct dpif_flow_dump_thread *thread)
 {
-    const struct dpif *dpif = dump->dpif;
-    int error;
-
-    error = dpif->dpif_class->flow_dump_next(dpif, dump->iter, state,
-                                             key, key_len, mask, mask_len,
-                                             actions, actions_len, stats);
-    if (error) {
-        if (key) {
-            *key = NULL;
-            *key_len = 0;
-        }
-        if (mask) {
-            *mask = NULL;
-            *mask_len = 0;
-        }
-        if (actions) {
-            *actions = NULL;
-            *actions_len = 0;
-        }
-        if (stats) {
-            *stats = NULL;
-        }
-    }
-    if (error == EOF) {
-        VLOG_DBG_RL(&dpmsg_rl, "%s: dumped all flows", dpif_name(dpif));
-    } else if (should_log_flow_message(error)) {
-        log_flow_message(dpif, error, "flow_dump",
-                         key ? *key : NULL, key ? *key_len : 0,
-                         mask ? *mask : NULL, mask ? *mask_len : 0,
-                         stats ? *stats : NULL, actions ? *actions : NULL,
-                         actions ? *actions_len : 0);
-    }
-    return !error;
+    thread->dpif->dpif_class->flow_dump_thread_destroy(thread);
 }
 
-/* Determines whether the next call to 'dpif_flow_dump_next' for 'dump' and
- * 'state' will modify or free the keys that it previously returned. 'state'
- * must have been initialized by a call to 'dpif_flow_dump_state_init' for
- * 'dump'.
+/* Attempts to retrieve up to 'max_flows' more flows from 'thread'.  Returns 0
+ * if and only if no flows remained to be retrieved, otherwise a positive
+ * number reflecting the number of elements in 'flows[]' that were updated.
+ * The number of flows returned might be less than 'max_flows' because
+ * fewer than 'max_flows' remained, because this particular datapath does not
+ * benefit from batching, or because an error occurred partway through
+ * retrieval.  Thus, the caller should continue calling until a 0 return value,
+ * even if intermediate return values are less than 'max_flows'.
  *
- * 'dpif' guarantees that data returned by flow_dump_next() will remain
- * accessible and unchanging until the next call. This function provides a way
- * for callers to determine whether that guarantee extends beyond the next
- * call.
+ * No error status is immediately provided.  An error status for the entire
+ * dump operation is provided when it is completed by calling
+ * dpif_flow_dump_destroy().
  *
- * Returns true if the next call to flow_dump_next() is expected to be
- * destructive to previously returned keys for 'state', false otherwise. */
-bool
-dpif_flow_dump_next_may_destroy_keys(struct dpif_flow_dump *dump, void *state)
-{
-    const struct dpif *dpif = dump->dpif;
-    return (dpif->dpif_class->flow_dump_next_may_destroy_keys
-            ? dpif->dpif_class->flow_dump_next_may_destroy_keys(state)
-            : true);
-}
-
-/* Completes flow table dump operation 'dump', which must have been initialized
- * with a successful call to dpif_flow_dump_start().  Returns 0 if the dump
- * operation was error-free, otherwise a positive errno value describing the
- * problem. */
+ * All of the data stored into 'flows' is owned by the datapath, not by the
+ * caller, and the caller must not modify or free it.  The datapath guarantees
+ * that it remains accessible and unchanged until at least the next call to
+ * dpif_flow_dump_next() for 'thread'. */
 int
-dpif_flow_dump_done(struct dpif_flow_dump *dump)
+dpif_flow_dump_next(struct dpif_flow_dump_thread *thread,
+                    struct dpif_flow *flows, int max_flows)
 {
-    const struct dpif *dpif = dump->dpif;
-    int error = dpif->dpif_class->flow_dump_done(dpif, dump->iter);
-    log_operation(dpif, "flow_dump_done", error);
-    return error == EOF ? 0 : error;
+    struct dpif *dpif = thread->dpif;
+    int n;
+
+    ovs_assert(max_flows > 0);
+    n = dpif->dpif_class->flow_dump_next(thread, flows, max_flows);
+    if (n > 0) {
+        struct dpif_flow *f;
+
+        for (f = flows; f < &flows[n] && should_log_flow_message(0); f++) {
+            log_flow_message(dpif, 0, "flow_dump",
+                             f->key, f->key_len, f->mask, f->mask_len,
+                             &f->stats, f->actions, f->actions_len);
+        }
+    } else {
+        VLOG_DBG_RL(&dpmsg_rl, "%s: dumped all flows", dpif_name(dpif));
+    }
+    return n;
 }
 
 struct dpif_execute_helper_aux {
diff --git a/lib/dpif.h b/lib/dpif.h
index e7aca8e..af322f6 100644
--- a/lib/dpif.h
+++ b/lib/dpif.h
@@ -525,22 +525,63 @@ int dpif_flow_del(struct dpif *,
 int dpif_flow_get(const struct dpif *,
                   const struct nlattr *key, size_t key_len,
                   struct ofpbuf **actionsp, struct dpif_flow_stats *);
-
-struct dpif_flow_dump {
-    const struct dpif *dpif;
-    void *iter;
+
+/* Flow dumping interface
+ * ======================
+ *
+ * This interface allows iteration through all of the flows currently installed
+ * in a datapath.  It is somewhat complicated by two requirements:
+ *
+ *    - Efficient support for dumping flows in parallel from multiple threads.
+ *
+ *    - Allow callers to avoid making unnecessary copies of data returned by
+ *      the interface across several flows in cases where the dpif
+ *      implementation has to maintain a copy of that information anyhow.
+ *      (That is, allow the client visibility into any underlying batching as
+ *      part of its own batching.)
+ *
+ *
+ * Usage
+ * -----
+ *
+ * 1. Call dpif_flow_dump_create().
+ * 2. In each thread that participates in the dump (which may be just a single
+ *    thread if parallelism isn't important):
+ *        (a) Call dpif_flow_dump_thread_create().
+ *        (b) Call dpif_flow_dump_next() repeatedly until it returns 0.
+ *        (c) Call dpif_flow_dump_thread_destroy().
+ * 3. Call dpif_flow_dump_destroy().
+ *
+ *
+ * Notes
+ * -----
+ *
+ * All error reporting is deferred to the call to dpif_flow_dump_destroy().
+ *
+ * Previous versions of this interface allowed to caller to say whether it
+ * actually needed actions or statistics, to allow for optimizations in cases
+ * where they were not needed.  This version always provided actions and
+ * statistics because every existing caller requests them.
+ */
+struct dpif_flow_dump *dpif_flow_dump_create(const struct dpif *);
+int dpif_flow_dump_destroy(struct dpif_flow_dump *);
+
+struct dpif_flow_dump_thread *dpif_flow_dump_thread_create(
+    struct dpif_flow_dump *);
+void dpif_flow_dump_thread_destroy(struct dpif_flow_dump_thread *);
+
+/* A datapath flow as dumped by dpif_flow_dump_next(). */
+struct dpif_flow {
+    const struct nlattr *key;     /* Flow key, as OVS_KEY_ATTR_* attrs. */
+    size_t key_len;               /* 'key' length in bytes. */
+    const struct nlattr *mask;    /* Flow mask, as OVS_KEY_ATTR_* attrs. */
+    size_t mask_len;              /* 'mask' length in bytes. */
+    const struct nlattr *actions; /* Actions, as OVS_ACTION_ATTR_ */
+    size_t actions_len;           /* 'actions' length in bytes. */
+    struct dpif_flow_stats stats; /* Flow statistics. */
 };
-void dpif_flow_dump_state_init(const struct dpif *, void **statep);
-int dpif_flow_dump_start(struct dpif_flow_dump *, const struct dpif *);
-bool dpif_flow_dump_next(struct dpif_flow_dump *, void *state,
-                         const struct nlattr **key, size_t *key_len,
-                         const struct nlattr **mask, size_t *mask_len,
-                         const struct nlattr **actions, size_t *actions_len,
-                         const struct dpif_flow_stats **);
-bool dpif_flow_dump_next_may_destroy_keys(struct dpif_flow_dump *dump,
-                                          void *state);
-int dpif_flow_dump_done(struct dpif_flow_dump *);
-void dpif_flow_dump_state_uninit(const struct dpif *, void *state);
+int dpif_flow_dump_next(struct dpif_flow_dump_thread *,
+                        struct dpif_flow *flows, int max_flows);
 
 /* Operation batching interface.
  *
diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
index 193f4cd..fab299e 100644
--- a/ofproto/ofproto-dpif-upcall.c
+++ b/ofproto/ofproto-dpif-upcall.c
@@ -102,7 +102,7 @@ struct udpif {
     bool need_revalidate;              /* As indicated by 'reval_seq'. */
     bool reval_exit;                   /* Set by leader on 'exit_latch. */
     pthread_barrier_t reval_barrier;   /* Barrier used by revalidators. */
-    struct dpif_flow_dump dump;        /* DPIF flow dump state. */
+    struct dpif_flow_dump *dump;       /* DPIF flow dump state. */
     long long int dump_duration;       /* Duration of the last flow dump. */
     struct seq *dump_seq;              /* Increments each dump iteration. */
 
@@ -580,7 +580,7 @@ udpif_revalidator(void *arg)
 
             start_time = time_msec();
             if (!udpif->reval_exit) {
-                dpif_flow_dump_start(&udpif->dump, udpif->dpif);
+                udpif->dump = dpif_flow_dump_create(udpif->dpif);
             }
         }
 
@@ -601,7 +601,7 @@ udpif_revalidator(void *arg)
         if (leader) {
             long long int duration;
 
-            dpif_flow_dump_done(&udpif->dump);
+            dpif_flow_dump_destroy(udpif->dump);
             seq_change(udpif->dump_seq);
 
             duration = MAX(time_msec() - start_time, 1);
@@ -1202,9 +1202,7 @@ should_revalidate(uint64_t packets, long long int used)
 
 static bool
 revalidate_ukey(struct udpif *udpif, struct udpif_key *ukey,
-                const struct nlattr *mask, size_t mask_len,
-                const struct nlattr *actions, size_t actions_len,
-                const struct dpif_flow_stats *stats)
+                const struct dpif_flow *f)
 {
     uint64_t slow_path_buf[128 / 8];
     struct xlate_out xout, *xoutp;
@@ -1227,14 +1225,14 @@ revalidate_ukey(struct udpif *udpif, struct udpif_key *ukey,
 
     ovs_mutex_lock(&ukey->mutex);
     last_used = ukey->stats.used;
-    push.used = stats->used;
-    push.tcp_flags = stats->tcp_flags;
-    push.n_packets = stats->n_packets > ukey->stats.n_packets
-        ? stats->n_packets - ukey->stats.n_packets
-        : 0;
-    push.n_bytes = stats->n_bytes > ukey->stats.n_bytes
-        ? stats->n_bytes - ukey->stats.n_bytes
-        : 0;
+    push.used = f->stats.used;
+    push.tcp_flags = f->stats.tcp_flags;
+    push.n_packets = (f->stats.n_packets > ukey->stats.n_packets
+                      ? f->stats.n_packets - ukey->stats.n_packets
+                      : 0);
+    push.n_bytes = (f->stats.n_bytes > ukey->stats.n_bytes
+                    ? f->stats.n_bytes - ukey->stats.n_bytes
+                    : 0);
 
     if (!ukey->flow_exists) {
         /* Don't bother revalidating if the flow was already deleted. */
@@ -1248,7 +1246,7 @@ revalidate_ukey(struct udpif *udpif, struct udpif_key *ukey,
     }
 
     /* We will push the stats, so update the ukey stats cache. */
-    ukey->stats = *stats;
+    ukey->stats = f->stats;
     if (!push.n_packets && !udpif->need_revalidate) {
         ok = true;
         goto exit;
@@ -1295,12 +1293,12 @@ revalidate_ukey(struct udpif *udpif, struct udpif_key *ukey,
         compose_slow_path(udpif, &xout, &flow, odp_in_port, &xout_actions);
     }
 
-    if (actions_len != ofpbuf_size(&xout_actions)
-        || memcmp(ofpbuf_data(&xout_actions), actions, actions_len)) {
+    if (f->actions_len != ofpbuf_size(&xout_actions)
+        || memcmp(ofpbuf_data(&xout_actions), f->actions, f->actions_len)) {
         goto exit;
     }
 
-    if (odp_flow_key_to_mask(mask, mask_len, &dp_mask, &flow)
+    if (odp_flow_key_to_mask(f->mask, f->mask_len, &dp_mask, &flow)
         == ODP_FIT_ERROR) {
         goto exit;
     }
@@ -1433,109 +1431,105 @@ static void
 revalidate(struct revalidator *revalidator)
 {
     struct udpif *udpif = revalidator->udpif;
-
-    struct dump_op ops[REVALIDATE_MAX_BATCH];
-    const struct nlattr *key, *mask, *actions;
-    size_t key_len, mask_len, actions_len;
-    const struct dpif_flow_stats *stats;
-    long long int now;
+    struct dpif_flow_dump_thread *dump_thread;
     unsigned int flow_limit;
-    size_t n_ops;
-    void *state;
 
-    n_ops = 0;
-    now = time_msec();
     atomic_read(&udpif->flow_limit, &flow_limit);
+    dump_thread = dpif_flow_dump_thread_create(udpif->dump);
+    for (;;) {
+        struct dump_op ops[REVALIDATE_MAX_BATCH];
+        int n_ops = 0;
 
-    dpif_flow_dump_state_init(udpif->dpif, &state);
-    while (dpif_flow_dump_next(&udpif->dump, state, &key, &key_len, &mask,
-                               &mask_len, &actions, &actions_len, &stats)) {
-        struct udpif_key *ukey;
-        bool mark, may_destroy;
-        long long int used, max_idle;
-        uint32_t hash;
-        size_t n_flows;
-
-        hash = hash_bytes(key, key_len, udpif->secret);
-        ukey = ukey_lookup(udpif, key, key_len, hash);
-
-        used = stats->used;
-        if (!used && ukey) {
-            ovs_mutex_lock(&ukey->mutex);
-
-            if (ukey->mark || !ukey->flow_exists) {
-                /* The flow has already been dumped. This can occasionally
-                 * occur if the datapath is changed in the middle of a flow
-                 * dump. Rather than perform the same work twice, skip the
-                 * flow this time. */
-                ovs_mutex_unlock(&ukey->mutex);
-                COVERAGE_INC(upcall_duplicate_flow);
-                goto next;
-            }
+        struct dpif_flow flows[REVALIDATE_MAX_BATCH];
+        const struct dpif_flow *f;
+        int n_dumped;
 
-            used = ukey->created;
-            ovs_mutex_unlock(&ukey->mutex);
-        }
+        long long int max_idle;
+        long long int now;
+        size_t n_dp_flows;
+        bool kill_them_all;
 
-        n_flows = udpif_get_n_flows(udpif);
-        max_idle = ofproto_max_idle;
-        if (n_flows > flow_limit) {
-            max_idle = 100;
+        n_dumped = dpif_flow_dump_next(dump_thread, flows, ARRAY_SIZE(flows));
+        if (!n_dumped) {
+            break;
         }
 
-        if ((used && used < now - max_idle) || n_flows > flow_limit * 2) {
-            mark = false;
-        } else {
-            if (!ukey) {
-                ukey = ukey_create(key, key_len, used);
-                if (!udpif_insert_ukey(udpif, ukey, hash)) {
-                    /* The same ukey has already been created. This means that
-                     * another revalidator is processing this flow
-                     * concurrently, so don't bother processing it. */
-                    ukey_delete(NULL, ukey);
-                    goto next;
+        now = time_msec();
+
+        /* In normal operation we want to keep flows around until they have
+         * been idle for 'ofproto_max_idle' milliseconds.  However:
+         *
+         *     - If the number of datapath flows climbs above 'flow_limit',
+         *       drop that down to 100 ms to try to bring the flows down to
+         *       the limit.
+         *
+         *     - If the number of datapath flows climbs above twice
+         *       'flow_limit', delete all the datapath flows as an emergency
+         *       measure.  (We reassess this condition for the next batch of
+         *       datapath flows, so we will recover before all the flows are
+         *       gone.) */
+        n_dp_flows = udpif_get_n_flows(udpif);
+        kill_them_all = n_dp_flows > flow_limit * 2;
+        max_idle = n_dp_flows > flow_limit ? 100 : ofproto_max_idle;
+
+        for (f = flows; f < &flows[n_dumped]; f++) {
+            long long int used = f->stats.used;
+            uint32_t hash = hash_bytes(f->key, f->key_len, udpif->secret);
+            struct udpif_key *ukey = ukey_lookup(udpif, f->key, f->key_len,
+                                                 hash);
+            bool mark;
+
+            if (!used && ukey) {
+                bool already_dumped;
+
+                ovs_mutex_lock(&ukey->mutex);
+                already_dumped = ukey->mark || !ukey->flow_exists;
+                used = ukey->created;
+                ovs_mutex_unlock(&ukey->mutex);
+
+                if (already_dumped) {
+                    /* The flow has already been dumped. This can occasionally
+                     * occur if the datapath is changed in the middle of a flow
+                     * dump. Rather than perform the same work twice, skip the
+                     * flow this time. */
+                    COVERAGE_INC(upcall_duplicate_flow);
+                    continue;
                 }
             }
 
-            mark = revalidate_ukey(udpif, ukey, mask, mask_len, actions,
-                                   actions_len, stats);
-        }
-
-        if (ukey) {
-            ovs_mutex_lock(&ukey->mutex);
-            ukey->mark = ukey->flow_exists = mark;
-            ovs_mutex_unlock(&ukey->mutex);
-        }
+            if (kill_them_all || (used && used < now - max_idle)) {
+                mark = false;
+            } else {
+                if (!ukey) {
+                    ukey = ukey_create(f->key, f->key_len, used);
+                    if (!udpif_insert_ukey(udpif, ukey, hash)) {
+                        /* The same ukey has already been created. This means
+                         * that another revalidator is processing this flow
+                         * concurrently, so don't bother processing it. */
+                        ukey_delete(NULL, ukey);
+                        continue;
+                    }
+                }
 
-        if (!mark) {
-            dump_op_init(&ops[n_ops++], key, key_len, ukey);
-        }
+                mark = revalidate_ukey(udpif, ukey, f);
+            }
 
-    next:
-        may_destroy = dpif_flow_dump_next_may_destroy_keys(&udpif->dump,
-                                                           state);
+            if (ukey) {
+                ovs_mutex_lock(&ukey->mutex);
+                ukey->mark = ukey->flow_exists = mark;
+                ovs_mutex_unlock(&ukey->mutex);
+            }
 
-        /* Only update 'now' immediately before 'buffer' will be updated.
-         * This gives us the current time relative to the time the datapath
-         * will write into 'stats'. */
-        if (may_destroy) {
-            now = time_msec();
+            if (!mark) {
+                dump_op_init(&ops[n_ops++], f->key, f->key_len, ukey);
+            }
         }
 
-        /* Only do a dpif_operate when we've hit our maximum batch, or when our
-         * memory is about to be clobbered by the next call to
-         * dpif_flow_dump_next(). */
-        if (n_ops == REVALIDATE_MAX_BATCH || (n_ops && may_destroy)) {
+        if (n_ops) {
             push_dump_ops__(udpif, ops, n_ops);
-            n_ops = 0;
         }
     }
-
-    if (n_ops) {
-        push_dump_ops__(udpif, ops, n_ops);
-    }
-
-    dpif_flow_dump_state_uninit(udpif->dpif, state);
+    dpif_flow_dump_thread_destroy(dump_thread);
 }
 
 static void
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index fdb4cd6..123acfe 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -4416,21 +4416,18 @@ ofproto_unixctl_dpif_dump_flows(struct unixctl_conn *conn,
                                 int argc OVS_UNUSED, const char *argv[],
                                 void *aux OVS_UNUSED)
 {
-    struct ds ds = DS_EMPTY_INITIALIZER;
-    const struct dpif_flow_stats *stats;
     const struct ofproto_dpif *ofproto;
-    struct dpif_flow_dump flow_dump;
-    const struct nlattr *actions;
-    const struct nlattr *mask;
-    const struct nlattr *key;
-    size_t actions_len;
-    size_t mask_len;
-    size_t key_len;
+
+    struct ds ds = DS_EMPTY_INITIALIZER;
     bool verbosity = false;
+
     struct dpif_port dpif_port;
     struct dpif_port_dump port_dump;
     struct hmap portno_names;
-    void *state = NULL;
+
+    struct dpif_flow_dump *flow_dump;
+    struct dpif_flow_dump_thread *flow_dump_thread;
+    struct dpif_flow f;
     int error;
 
     ofproto = ofproto_dpif_lookup(argv[argc - 1]);
@@ -4449,30 +4446,24 @@ ofproto_unixctl_dpif_dump_flows(struct unixctl_conn *conn,
     }
 
     ds_init(&ds);
-    error = dpif_flow_dump_start(&flow_dump, ofproto->backer->dpif);
-    if (error) {
-        goto exit;
-    }
-    dpif_flow_dump_state_init(ofproto->backer->dpif, &state);
-    while (dpif_flow_dump_next(&flow_dump, state, &key, &key_len,
-                               &mask, &mask_len, &actions, &actions_len,
-                               &stats)) {
-        if (!ofproto_dpif_contains_flow(ofproto, key, key_len)) {
+    flow_dump = dpif_flow_dump_create(ofproto->backer->dpif);
+    flow_dump_thread = dpif_flow_dump_thread_create(flow_dump);
+    while (dpif_flow_dump_next(flow_dump_thread, &f, 1)) {
+        if (!ofproto_dpif_contains_flow(ofproto, f.key, f.key_len)) {
             continue;
         }
 
-        odp_flow_format(key, key_len, mask, mask_len, &portno_names, &ds,
-                        verbosity);
+        odp_flow_format(f.key, f.key_len, f.mask, f.mask_len,
+                        &portno_names, &ds, verbosity);
         ds_put_cstr(&ds, ", ");
-        dpif_flow_stats_format(stats, &ds);
+        dpif_flow_stats_format(&f.stats, &ds);
         ds_put_cstr(&ds, ", actions:");
-        format_odp_actions(&ds, actions, actions_len);
+        format_odp_actions(&ds, f.actions, f.actions_len);
         ds_put_char(&ds, '\n');
     }
-    dpif_flow_dump_state_uninit(ofproto->backer->dpif, state);
-    error = dpif_flow_dump_done(&flow_dump);
+    dpif_flow_dump_thread_destroy(flow_dump_thread);
+    error = dpif_flow_dump_destroy(flow_dump);
 
-exit:
     if (error) {
         ds_clear(&ds);
         ds_put_format(&ds, "dpif/dump_flows failed: %s", ovs_strerror(errno));
diff --git a/utilities/ovs-dpctl.c b/utilities/ovs-dpctl.c
index ccc55b5..8b3d96a 100644
--- a/utilities/ovs-dpctl.c
+++ b/utilities/ovs-dpctl.c
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2008, 2009, 2010, 2011, 2012, 2013 Nicira, Inc.
+ * Copyright (c) 2008, 2009, 2010, 2011, 2012, 2013, 2014 Nicira, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -747,24 +747,23 @@ dpctl_dump_dps(int argc OVS_UNUSED, char *argv[] OVS_UNUSED)
 static void
 dpctl_dump_flows(int argc, char *argv[])
 {
-    const struct dpif_flow_stats *stats;
-    const struct nlattr *actions;
-    struct dpif_flow_dump flow_dump;
-    const struct nlattr *key;
-    const struct nlattr *mask;
-    struct dpif_port dpif_port;
-    struct dpif_port_dump port_dump;
-    struct hmap portno_names;
-    struct simap names_portno;
-    size_t actions_len;
     struct dpif *dpif;
-    size_t key_len;
-    size_t mask_len;
     struct ds ds;
-    char *name, *filter = NULL;
+    char *name;
+
+    char *filter = NULL;
     struct flow flow_filter;
     struct flow_wildcards wc_filter;
-    void *state = NULL;
+
+    struct dpif_port_dump port_dump;
+    struct dpif_port dpif_port;
+    struct hmap portno_names;
+    struct simap names_portno;
+
+    struct dpif_flow_dump_thread *flow_dump_thread;
+    struct dpif_flow_dump *flow_dump;
+    struct dpif_flow f;
+
     int error;
 
     if (argc > 1 && !strncmp(argv[argc - 1], "filter=", 7)) {
@@ -792,22 +791,17 @@ dpctl_dump_flows(int argc, char *argv[])
     }
 
     ds_init(&ds);
-    error = dpif_flow_dump_start(&flow_dump, dpif);
-    if (error) {
-        goto exit;
-    }
-    dpif_flow_dump_state_init(dpif, &state);
-    while (dpif_flow_dump_next(&flow_dump, state, &key, &key_len,
-                               &mask, &mask_len, &actions, &actions_len,
-                               &stats)) {
+    flow_dump = dpif_flow_dump_create(dpif);
+    flow_dump_thread = dpif_flow_dump_thread_create(flow_dump);
+    while (dpif_flow_dump_next(flow_dump_thread, &f, 1)) {
         if (filter) {
             struct flow flow;
             struct flow_wildcards wc;
             struct match match, match_filter;
             struct minimatch minimatch;
 
-            odp_flow_key_to_flow(key, key_len, &flow);
-            odp_flow_key_to_mask(mask, mask_len, &wc.masks, &flow);
+            odp_flow_key_to_flow(f.key, f.key_len, &flow);
+            odp_flow_key_to_mask(f.mask, f.mask_len, &wc.masks, &flow);
             match_init(&match, &flow, &wc);
 
             match_init(&match_filter, &flow_filter, &wc);
@@ -821,19 +815,18 @@ dpctl_dump_flows(int argc, char *argv[])
             minimatch_destroy(&minimatch);
         }
         ds_clear(&ds);
-        odp_flow_format(key, key_len, mask, mask_len, &portno_names, &ds,
-                        verbosity);
+        odp_flow_format(f.key, f.key_len, f.mask, f.mask_len,
+                        &portno_names, &ds, verbosity);
         ds_put_cstr(&ds, ", ");
 
-        dpif_flow_stats_format(stats, &ds);
+        dpif_flow_stats_format(&f.stats, &ds);
         ds_put_cstr(&ds, ", actions:");
-        format_odp_actions(&ds, actions, actions_len);
+        format_odp_actions(&ds, f.actions, f.actions_len);
         printf("%s\n", ds_cstr(&ds));
     }
-    dpif_flow_dump_state_uninit(dpif, state);
-    error = dpif_flow_dump_done(&flow_dump);
+    dpif_flow_dump_thread_destroy(flow_dump_thread);
+    error = dpif_flow_dump_destroy(flow_dump);
 
-exit:
     if (error) {
         ovs_fatal(error, "Failed to dump flows from datapath");
     }
-- 
1.7.10.4




More information about the dev mailing list