[ovs-dev] [PATCH] dpif-netdev: Implement batched flow dumping.

Ryan Wilson wryan at nicira.com
Wed Jun 18 21:50:35 UTC 2014


Previously, flows were retrieved one by one when dumping flows for
datapaths of type 'netdev'. This increased contention for the dump's
mutex, negatively affecting revalidator performance.

This patch retrieves batches of flows when dumping flows for datapaths
of type 'netdev'.

Signed-off-by: Ryan Wilson <wryan at nicira.com>
---
 lib/dpif-linux.c              |    7 ++--
 lib/dpif-netdev.c             |   87 +++++++++++++++++++++++------------------
 lib/dpif-provider.h           |    9 +++--
 lib/dpif.c                    |   10 ++---
 lib/dpif.h                    |    4 +-
 ofproto/ofproto-dpif-upcall.c |    5 ++-
 ofproto/ofproto-dpif.c        |    4 +-
 utilities/ovs-dpctl.c         |    4 +-
 8 files changed, 72 insertions(+), 58 deletions(-)

diff --git a/lib/dpif-linux.c b/lib/dpif-linux.c
index afe9340..a46e2db 100644
--- a/lib/dpif-linux.c
+++ b/lib/dpif-linux.c
@@ -1203,13 +1203,13 @@ dpif_linux_flow_dump_thread_cast(struct dpif_flow_dump_thread *thread)
 }
 
 static struct dpif_flow_dump_thread *
-dpif_linux_flow_dump_thread_create(struct dpif_flow_dump *dump_)
+dpif_linux_flow_dump_thread_create(struct dpif_flow_dump *dump_, int max_flows)
 {
     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);
+    dpif_flow_dump_thread_init(&thread->up, &dump->up, max_flows);
     thread->dump = dump;
     ofpbuf_init(&thread->nl_flows, NL_DUMP_BUFSIZE);
     thread->nl_actions = NULL;
@@ -1243,12 +1243,13 @@ dpif_linux_flow_to_dpif_flow(struct dpif_flow *dpif_flow,
 
 static int
 dpif_linux_flow_dump_next(struct dpif_flow_dump_thread *thread_,
-                          struct dpif_flow *flows, int max_flows)
+                          struct dpif_flow *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 max_flows = thread_->max_flows;
     int n_flows;
 
     ofpbuf_delete(thread->nl_actions);
diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index 6c281fe..36c442d 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -1423,8 +1423,8 @@ dpif_netdev_flow_dump_destroy(struct dpif_flow_dump *dump_)
 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;
+    struct odputil_keybuf *keybuf;
+    struct odputil_keybuf *maskbuf;
 };
 
 static struct dpif_netdev_flow_dump_thread *
@@ -1434,14 +1434,16 @@ dpif_netdev_flow_dump_thread_cast(struct dpif_flow_dump_thread *thread)
 }
 
 static struct dpif_flow_dump_thread *
-dpif_netdev_flow_dump_thread_create(struct dpif_flow_dump *dump_)
+dpif_netdev_flow_dump_thread_create(struct dpif_flow_dump *dump_, int max_flows)
 {
     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);
+    dpif_flow_dump_thread_init(&thread->up, &dump->up, max_flows);
     thread->dump = dump;
+    thread->keybuf = xmalloc(max_flows * sizeof *thread->keybuf);
+    thread->maskbuf = xmalloc(max_flows * sizeof *thread->maskbuf);
     return &thread->up;
 }
 
@@ -1451,29 +1453,35 @@ 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->keybuf);
+    free(thread->maskbuf);
     free(thread);
 }
 
 /* XXX the caller must use 'actions' without quiescing */
 static int
 dpif_netdev_flow_dump_next(struct dpif_flow_dump_thread *thread_,
-                           struct dpif_flow *f, int max_flows OVS_UNUSED)
+                           struct dpif_flow *f)
 {
     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;
+    int max_flows = thread_->max_flows;
+    int n_flows = 0;
 
     ovs_mutex_lock(&dump->mutex);
-    error = dump->status;
-    if (!error) {
+
+    while (n_flows < max_flows && !dump->status) {
+        struct dp_netdev_flow *netdev_flow;
+        struct flow_wildcards wc;
+        struct dp_netdev_actions *dp_actions;
         struct hmap_node *node;
+        struct ofpbuf buf;
+        struct dpif_flow *flow = &f[n_flows];
+        struct odputil_keybuf *keybuf = &thread->keybuf[n_flows];
+        struct odputil_keybuf *maskbuf = &thread->maskbuf[n_flows];
 
         fat_rwlock_rdlock(&dp->cls.rwlock);
         node = hmap_at_position(&dp->flow_table, &dump->bucket, &dump->offset);
@@ -1482,40 +1490,41 @@ dpif_netdev_flow_dump_next(struct dpif_flow_dump_thread *thread_,
         }
         fat_rwlock_unlock(&dp->cls.rwlock);
         if (!node) {
-            dump->status = error = EOF;
+            dump->status = EOF;
+            break;
         }
-    }
-    ovs_mutex_unlock(&dump->mutex);
-    if (error) {
-        return 0;
-    }
 
-    minimask_expand(&netdev_flow->cr.match.mask, &wc);
+        minimask_expand(&netdev_flow->cr.match.mask, &wc);
 
-    /* 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, true);
-    f->key = ofpbuf_data(&buf);
-    f->key_len = ofpbuf_size(&buf);
+        /* Key. */
+        ofpbuf_use_stack(&buf, keybuf, sizeof *keybuf);
+        odp_flow_key_from_flow(&buf, &netdev_flow->flow, &wc.masks,
+                               netdev_flow->flow.in_port.odp_port, true);
+        flow->key = ofpbuf_data(&buf);
+        flow->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, true);
-    f->mask = ofpbuf_data(&buf);
-    f->mask_len = ofpbuf_size(&buf);
+        /* Mask. */
+        ofpbuf_use_stack(&buf, maskbuf, sizeof *maskbuf);
+        odp_flow_key_from_mask(&buf, &wc.masks, &netdev_flow->flow,
+                               odp_to_u32(wc.masks.in_port.odp_port),
+                               SIZE_MAX, true);
+        flow->mask = ofpbuf_data(&buf);
+        flow->mask_len = ofpbuf_size(&buf);
 
-    /* Actions. */
-    dp_actions = dp_netdev_flow_get_actions(netdev_flow);
-    f->actions = dp_actions->actions;
-    f->actions_len = dp_actions->size;
+        /* Actions. */
+        dp_actions = dp_netdev_flow_get_actions(netdev_flow);
+        flow->actions = dp_actions->actions;
+        flow->actions_len = dp_actions->size;
 
-    /* Stats. */
-    get_dpif_flow_stats(netdev_flow, &f->stats);
+        /* Stats. */
+        get_dpif_flow_stats(netdev_flow, &flow->stats);
+
+        n_flows++;
+    }
+
+    ovs_mutex_unlock(&dump->mutex);
 
-    return 1;
+    return n_flows;
 }
 
 static int
diff --git a/lib/dpif-provider.h b/lib/dpif-provider.h
index b762ac0..cc954c9 100644
--- a/lib/dpif-provider.h
+++ b/lib/dpif-provider.h
@@ -63,13 +63,16 @@ dpif_flow_dump_init(struct dpif_flow_dump *dump, const struct dpif *dpif)
 
 struct dpif_flow_dump_thread {
     struct dpif *dpif;
+    int max_flows;
 };
 
 static inline void
 dpif_flow_dump_thread_init(struct dpif_flow_dump_thread *thread,
-                           struct dpif_flow_dump *dump)
+                           struct dpif_flow_dump *dump,
+                           int max_flows)
 {
     thread->dpif = dump->dpif;
+    thread->max_flows = max_flows;
 }
 
 /* Datapath interface class structure, to be defined by each implementation of
@@ -312,11 +315,11 @@ struct dpif_class {
     int (*flow_dump_destroy)(struct dpif_flow_dump *dump);
 
     struct dpif_flow_dump_thread *(*flow_dump_thread_create)(
-        struct dpif_flow_dump *dump);
+        struct dpif_flow_dump *dump, int max_flows);
     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);
+                          struct dpif_flow *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 cace47b..3d3cbba 100644
--- a/lib/dpif.c
+++ b/lib/dpif.c
@@ -1000,9 +1000,10 @@ dpif_flow_dump_destroy(struct dpif_flow_dump *dump)
 
 /* 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)
+dpif_flow_dump_thread_create(struct dpif_flow_dump *dump, int max_flows)
 {
-    return dump->dpif->dpif_class->flow_dump_thread_create(dump);
+    ovs_assert(max_flows > 0);
+    return dump->dpif->dpif_class->flow_dump_thread_create(dump, max_flows);
 }
 
 /* Releases 'thread'. */
@@ -1031,13 +1032,12 @@ dpif_flow_dump_thread_destroy(struct dpif_flow_dump_thread *thread)
  * dpif_flow_dump_next() for 'thread'. */
 int
 dpif_flow_dump_next(struct dpif_flow_dump_thread *thread,
-                    struct dpif_flow *flows, int max_flows)
+                    struct dpif_flow *flows)
 {
     struct dpif *dpif = thread->dpif;
     int n;
 
-    ovs_assert(max_flows > 0);
-    n = dpif->dpif_class->flow_dump_next(thread, flows, max_flows);
+    n = dpif->dpif_class->flow_dump_next(thread, flows);
     if (n > 0) {
         struct dpif_flow *f;
 
diff --git a/lib/dpif.h b/lib/dpif.h
index f080cde..4a11d2e 100644
--- a/lib/dpif.h
+++ b/lib/dpif.h
@@ -558,7 +558,7 @@ 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 *);
+    struct dpif_flow_dump *, int max_flows);
 void dpif_flow_dump_thread_destroy(struct dpif_flow_dump_thread *);
 
 /* A datapath flow as dumped by dpif_flow_dump_next(). */
@@ -572,7 +572,7 @@ struct dpif_flow {
     struct dpif_flow_stats stats; /* Flow statistics. */
 };
 int dpif_flow_dump_next(struct dpif_flow_dump_thread *,
-                        struct dpif_flow *flows, int max_flows);
+                        struct dpif_flow *flows);
 
 /* Operation batching interface.
  *
diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
index b38f226..a25f6b9 100644
--- a/ofproto/ofproto-dpif-upcall.c
+++ b/ofproto/ofproto-dpif-upcall.c
@@ -1332,7 +1332,8 @@ revalidate(struct revalidator *revalidator)
 
     dump_seq = seq_read(udpif->dump_seq);
     atomic_read(&udpif->flow_limit, &flow_limit);
-    dump_thread = dpif_flow_dump_thread_create(udpif->dump);
+    dump_thread = dpif_flow_dump_thread_create(udpif->dump,
+                                               REVALIDATE_MAX_BATCH);
     for (;;) {
         struct dump_op ops[REVALIDATE_MAX_BATCH];
         int n_ops = 0;
@@ -1346,7 +1347,7 @@ revalidate(struct revalidator *revalidator)
         size_t n_dp_flows;
         bool kill_them_all;
 
-        n_dumped = dpif_flow_dump_next(dump_thread, flows, ARRAY_SIZE(flows));
+        n_dumped = dpif_flow_dump_next(dump_thread, flows);
         if (!n_dumped) {
             break;
         }
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 9e4a455..e3f8945 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -4486,8 +4486,8 @@ ofproto_unixctl_dpif_dump_flows(struct unixctl_conn *conn,
 
     ds_init(&ds);
     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)) {
+    flow_dump_thread = dpif_flow_dump_thread_create(flow_dump, 1);
+    while (dpif_flow_dump_next(flow_dump_thread, &f)) {
         if (!ofproto_dpif_contains_flow(ofproto, f.key, f.key_len)) {
             continue;
         }
diff --git a/utilities/ovs-dpctl.c b/utilities/ovs-dpctl.c
index 62fc1dd..c842782 100644
--- a/utilities/ovs-dpctl.c
+++ b/utilities/ovs-dpctl.c
@@ -792,8 +792,8 @@ dpctl_dump_flows(int argc, char *argv[])
 
     ds_init(&ds);
     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)) {
+    flow_dump_thread = dpif_flow_dump_thread_create(flow_dump, 1);
+    while (dpif_flow_dump_next(flow_dump_thread, &f)) {
         if (filter) {
             struct flow flow;
             struct flow_wildcards wc;
-- 
1.7.9.5




More information about the dev mailing list