[ovs-dev] [PATCH v2 2/2] ofproto: Allow in-place modifications of datapath flows.

Ethan J. Jackson ethan at nicira.com
Tue Aug 11 01:46:05 UTC 2015


From: Ethan Jackson <ethan at nicira.com>

There are certain use cases (such as bond rebalancing) where a
datapath flow's actions may change, while it's wildcard pattern
remains the same.  Before this patch, revalidators would note the
change, delete the flow, and wait for the handlers to install an
updated version.  This is inefficient, as many packets could get
punted to userspace before the new flow is finally installed.

To improve the situation, this patch implements in place modification
of datapath flows.  If the revalidators detect the only change to a
given ukey is its actions, instead of deleting it, it does a put with
the MODIFY flag set.

Signed-off-by: Ethan Jackson <ethan at nicira.com>
---
 ofproto/ofproto-dpif-upcall.c | 169 +++++++++++++++++++++++++++---------------
 tests/ofproto-dpif.at         |  40 ++++++++++
 2 files changed, 150 insertions(+), 59 deletions(-)

diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
index fddb535..8ef61bc 100644
--- a/ofproto/ofproto-dpif-upcall.c
+++ b/ofproto/ofproto-dpif-upcall.c
@@ -150,6 +150,12 @@ enum upcall_type {
     IPFIX_UPCALL                /* Per-bridge sampling. */
 };
 
+enum reval_result {
+    UKEY_KEEP,
+    UKEY_DELETE,
+    UKEY_MODIFY
+};
+
 struct upcall {
     struct ofproto_dpif *ofproto;  /* Parent ofproto. */
     const struct recirc_id_node *recirc; /* Recirculation context. */
@@ -1663,33 +1669,41 @@ should_revalidate(const struct udpif *udpif, uint64_t packets,
     return false;
 }
 
-static bool
+/* Verifies that the datapath actions of 'ukey' are still correct, and pushes
+ * 'stats' for it.
+ *
+ * Returns a recommended action for 'ukey', options include:
+ *      UKEY_DELETE The ukey should be deleted.
+ *      UKEY_KEEP   The ukey is fine as is.
+ *      UKEY_MODIFY The ukey's actions should be changed but is otherwise
+ *                  fine.  Callers should change the actions to those found
+ *                  in the caller supplied 'odp_actions' buffer. */
+static enum reval_result
 revalidate_ukey(struct udpif *udpif, struct udpif_key *ukey,
-                const struct dpif_flow_stats *stats, uint64_t reval_seq)
+                const struct dpif_flow_stats *stats,
+                struct ofpbuf *odp_actions, uint64_t reval_seq)
     OVS_REQUIRES(ukey->mutex)
 {
-    uint64_t odp_actions_stub[1024 / 8];
-    struct ofpbuf odp_actions = OFPBUF_STUB_INITIALIZER(odp_actions_stub);
-
     struct xlate_out xout, *xoutp;
     struct netflow *netflow;
     struct ofproto_dpif *ofproto;
     struct dpif_flow_stats push;
     struct flow flow, dp_mask;
     struct flow_wildcards wc;
+    enum reval_result result;
     uint64_t *dp64, *xout64;
     ofp_port_t ofp_in_port;
     struct xlate_in xin;
     long long int last_used;
     int error;
     size_t i;
-    bool ok;
     bool need_revalidate;
 
-    ok = false;
+    result = UKEY_DELETE;
     xoutp = NULL;
     netflow = NULL;
 
+    ofpbuf_clear(odp_actions);
     need_revalidate = (ukey->reval_seq != reval_seq);
     last_used = ukey->stats.used;
     push.used = stats->used;
@@ -1703,20 +1717,19 @@ revalidate_ukey(struct udpif *udpif, struct udpif_key *ukey,
 
     if (need_revalidate && last_used
         && !should_revalidate(udpif, push.n_packets, last_used)) {
-        ok = false;
         goto exit;
     }
 
     /* We will push the stats, so update the ukey stats cache. */
     ukey->stats = *stats;
     if (!push.n_packets && !need_revalidate) {
-        ok = true;
+        result = UKEY_KEEP;
         goto exit;
     }
 
     if (ukey->xcache && !need_revalidate) {
         xlate_push_stats(ukey->xcache, &push);
-        ok = true;
+        result = UKEY_KEEP;
         goto exit;
     }
 
@@ -1739,7 +1752,7 @@ revalidate_ukey(struct udpif *udpif, struct udpif_key *ukey,
     }
 
     xlate_in_init(&xin, ofproto, &flow, ofp_in_port, NULL, push.tcp_flags,
-                  NULL, need_revalidate ? &wc : NULL, &odp_actions);
+                  NULL, need_revalidate ? &wc : NULL, odp_actions);
     if (push.n_packets) {
         xin.resubmit_stats = &push;
         xin.may_learn = true;
@@ -1749,18 +1762,14 @@ revalidate_ukey(struct udpif *udpif, struct udpif_key *ukey,
     xoutp = &xout;
 
     if (!need_revalidate) {
-        ok = true;
+        result = UKEY_KEEP;
         goto exit;
     }
 
     if (xout.slow) {
-        ofpbuf_clear(&odp_actions);
+        ofpbuf_clear(odp_actions);
         compose_slow_path(udpif, &xout, &flow, flow.in_port.odp_port,
-                          &odp_actions);
-    }
-
-    if (!ofpbuf_equal(&odp_actions, ukey->actions)) {
-        goto exit;
+                          odp_actions);
     }
 
     if (odp_flow_key_to_mask(ukey->mask, ukey->mask_len, ukey->key,
@@ -1781,18 +1790,24 @@ revalidate_ukey(struct udpif *udpif, struct udpif_key *ukey,
         }
     }
 
-    ok = true;
+    if (!ofpbuf_equal(odp_actions, ukey->actions)) {
+        /* The datpath mask was OK, but the actions seem to have changed.
+         * Let's modify it in place. */
+        result = UKEY_MODIFY;
+        goto exit;
+    }
+
+    result = UKEY_KEEP;
 
 exit:
-    if (ok) {
+    if (result != UKEY_DELETE) {
         ukey->reval_seq = reval_seq;
     }
-    if (netflow && !ok) {
+    if (netflow && result != UKEY_DELETE) {
         netflow_flow_clear(netflow, &flow);
     }
     xlate_out_uninit(xoutp);
-    ofpbuf_uninit(&odp_actions);
-    return ok;
+    return result;
 }
 
 static void
@@ -1823,6 +1838,23 @@ delete_op_init(struct udpif *udpif, struct ukey_op *op, struct udpif_key *ukey)
 }
 
 static void
+modify_op_init(struct ukey_op *op, struct udpif_key *ukey)
+{
+    op->ukey = ukey;
+    op->dop.type = DPIF_OP_FLOW_PUT;
+    op->dop.u.flow_put.flags = DPIF_FP_MODIFY;
+    op->dop.u.flow_put.key = ukey->key;
+    op->dop.u.flow_put.key_len = ukey->key_len;
+    op->dop.u.flow_put.mask = ukey->mask;
+    op->dop.u.flow_put.mask_len = ukey->mask_len;
+    op->dop.u.flow_put.ufid = &ukey->ufid;
+    op->dop.u.flow_put.pmd_id = ukey->pmd_id;
+    op->dop.u.flow_put.stats = NULL;
+    op->dop.u.flow_put.actions = ukey->actions->data;
+    op->dop.u.flow_put.actions_len = ukey->actions->size;
+}
+
+static void
 push_ukey_ops__(struct udpif *udpif, struct ukey_op *ops, size_t n_ops)
 {
     struct dpif_op *opsp[REVALIDATE_MAX_BATCH];
@@ -1841,6 +1873,12 @@ push_ukey_ops__(struct udpif *udpif, struct ukey_op *ops, size_t n_ops)
         stats = op->dop.u.flow_del.stats;
         push = &push_buf;
 
+        if (op->dop.type == DPIF_OP_FLOW_PUT) {
+            /* This was an in place modification.  Don't bother pushing stats
+             * for now. */
+            continue;
+        }
+
         if (op->ukey) {
             ovs_mutex_lock(&op->ukey->mutex);
             push->used = MAX(stats->used, op->ukey->stats.used);
@@ -1926,6 +1964,9 @@ log_unexpected_flow(const struct dpif_flow *flow, int error)
 static void
 revalidate(struct revalidator *revalidator)
 {
+    uint64_t odp_actions_stub[1024 / 8];
+    struct ofpbuf odp_actions = OFPBUF_STUB_INITIALIZER(odp_actions_stub);
+
     struct udpif *udpif = revalidator->udpif;
     struct dpif_flow_dump_thread *dump_thread;
     uint64_t dump_seq, reval_seq;
@@ -1973,8 +2014,9 @@ revalidate(struct revalidator *revalidator)
 
         for (f = flows; f < &flows[n_dumped]; f++) {
             long long int used = f->stats.used;
+            enum reval_result result;
             struct udpif_key *ukey;
-            bool already_dumped, keep;
+            bool already_dumped;
             int error;
 
             if (ukey_acquire(udpif, f, &ukey, &error)) {
@@ -2008,15 +2050,20 @@ revalidate(struct revalidator *revalidator)
                 used = ukey->created;
             }
             if (kill_them_all || (used && used < now - max_idle)) {
-                keep = false;
+                result = UKEY_DELETE;
             } else {
-                keep = revalidate_ukey(udpif, ukey, &f->stats, reval_seq);
+                result = revalidate_ukey(udpif, ukey, &f->stats, &odp_actions,
+                                         reval_seq);
             }
             ukey->dump_seq = dump_seq;
-            ukey->flow_exists = keep;
+            ukey->flow_exists = result != UKEY_DELETE;
 
-            if (!keep) {
+            if (result == UKEY_DELETE) {
                 delete_op_init(udpif, &ops[n_ops++], ukey);
+            } else if (result == UKEY_MODIFY) {
+                ofpbuf_delete(ukey->actions);
+                ukey->actions = ofpbuf_clone(&odp_actions);
+                modify_op_init(&ops[n_ops++], ukey);
             }
             ovs_mutex_unlock(&ukey->mutex);
         }
@@ -2027,23 +2074,7 @@ revalidate(struct revalidator *revalidator)
         ovsrcu_quiesce();
     }
     dpif_flow_dump_thread_destroy(dump_thread);
-}
-
-static bool
-handle_missed_revalidation(struct udpif *udpif, uint64_t reval_seq,
-                           struct udpif_key *ukey)
-{
-    struct dpif_flow_stats stats;
-    bool keep;
-
-    COVERAGE_INC(revalidate_missed_dp_flow);
-
-    memset(&stats, 0, sizeof stats);
-    ovs_mutex_lock(&ukey->mutex);
-    keep = revalidate_ukey(udpif, ukey, &stats, reval_seq);
-    ovs_mutex_unlock(&ukey->mutex);
-
-    return keep;
+    ofpbuf_uninit(&odp_actions);
 }
 
 static void
@@ -2060,28 +2091,45 @@ revalidator_sweep__(struct revalidator *revalidator, bool purge)
     ovs_assert(slice < udpif->n_revalidators);
 
     for (int i = slice; i < N_UMAPS; i += udpif->n_revalidators) {
+        uint64_t odp_actions_stub[1024 / 8];
+        struct ofpbuf odp_actions = OFPBUF_STUB_INITIALIZER(odp_actions_stub);
+
         struct ukey_op ops[REVALIDATE_MAX_BATCH];
         struct udpif_key *ukey;
         struct umap *umap = &udpif->ukeys[i];
         size_t n_ops = 0;
 
         CMAP_FOR_EACH(ukey, cmap_node, &umap->cmap) {
-            bool flow_exists, seq_mismatch;
+            enum reval_result result;
 
             /* Handler threads could be holding a ukey lock while it installs a
              * new flow, so don't hang around waiting for access to it. */
             if (ovs_mutex_trylock(&ukey->mutex)) {
                 continue;
             }
-            flow_exists = ukey->flow_exists;
-            seq_mismatch = (ukey->dump_seq != dump_seq
-                            && ukey->reval_seq != reval_seq);
-
-            if (flow_exists
-                && (purge
-                    || (seq_mismatch
-                        && !handle_missed_revalidation(udpif, reval_seq,
-                                                       ukey)))) {
+
+            if (!ukey->flow_exists) {
+                ovs_mutex_lock(&umap->mutex);
+                ukey_delete(umap, ukey);
+                ovs_mutex_unlock(&umap->mutex);
+                ovs_mutex_unlock(&ukey->mutex);
+                continue;
+            }
+
+            if (purge) {
+                result = UKEY_DELETE;
+            } else if (ukey->dump_seq == dump_seq
+                     || ukey->reval_seq == reval_seq) {
+                result = UKEY_KEEP;
+            } else {
+                struct dpif_flow_stats stats;
+                COVERAGE_INC(revalidate_missed_dp_flow);
+                memset(&stats, 0, sizeof stats);
+                result = revalidate_ukey(udpif, ukey, &stats, &odp_actions,
+                                         reval_seq);
+            }
+
+            if (result == UKEY_DELETE) {
                 struct ukey_op *op = &ops[n_ops++];
 
                 delete_op_init(udpif, op, ukey);
@@ -2089,17 +2137,20 @@ revalidator_sweep__(struct revalidator *revalidator, bool purge)
                     push_ukey_ops(udpif, umap, ops, n_ops);
                     n_ops = 0;
                 }
-            } else if (!flow_exists) {
-                ovs_mutex_lock(&umap->mutex);
-                ukey_delete(umap, ukey);
-                ovs_mutex_unlock(&umap->mutex);
+            } else if (result == UKEY_MODIFY) {
+                ofpbuf_delete(ukey->actions);
+                ukey->actions = ofpbuf_clone(&odp_actions);
+                modify_op_init(&ops[n_ops++], ukey);
             }
+
             ovs_mutex_unlock(&ukey->mutex);
         }
 
         if (n_ops) {
             push_ukey_ops(udpif, umap, ops, n_ops);
         }
+
+        ofpbuf_uninit(&odp_actions);
         ovsrcu_quiesce();
     }
 }
diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
index 58c426b..121f84d 100644
--- a/tests/ofproto-dpif.at
+++ b/tests/ofproto-dpif.at
@@ -6985,3 +6985,43 @@ recirc_id=0,ip,in_port=1,dl_vlan=10,nw_frag=no, actions: <del>
 ])
 OVS_VSWITCHD_STOP
 AT_CLEANUP
+
+# Tests in place modification of installed datapath flows.
+AT_SETUP([ofproto-dpif - in place modification])
+OVS_VSWITCHD_START(
+  [add-port br0 p1 -- set Interface p1 type=dummy ofport_request=1])
+AT_CHECK([ovs-appctl vlog/set dpif:dbg dpif_netdev:dbg])
+
+AT_CHECK([ovs-ofctl del-flows br0])
+AT_CHECK([ovs-ofctl add-flow br0 in_port=1,actions=mod_vlan_vid:3,output:local])
+
+ovs-appctl vlog/set PATTERN:ANY:'%c|%p|%m'
+
+ovs-appctl time/stop
+
+for i in 1 2 3; do
+    ovs-appctl netdev-dummy/receive p1 'in_port(1),eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),eth_type(0x1234)'
+done
+
+AT_CHECK([ovs-appctl dpif/dump-flows br0 | STRIP_UFID | STRIP_USED | sort], [0], [dnl
+recirc_id(0),in_port(1),eth_type(0x1234), packets:2, bytes:120, used:0.0s, actions:push_vlan(vid=3,pcp=0),100
+])
+
+AT_CHECK([ovs-ofctl add-flow br0 priority=60000,in_port=1,actions=mod_vlan_vid:4,output:local])
+
+ovs-appctl time/warp 500
+ovs-appctl time/warp 500
+
+for i in 1 2 3; do
+    ovs-appctl netdev-dummy/receive p1 'in_port(1),eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),eth_type(0x1234)'
+done
+
+AT_CHECK([ovs-appctl dpif/dump-flows br0 | STRIP_UFID | STRIP_USED | sort], [0], [dnl
+recirc_id(0),in_port(1),eth_type(0x1234), packets:5, bytes:300, used:0.0s, actions:push_vlan(vid=4,pcp=0),100
+])
+
+AT_CHECK([cat ovs-vswitchd.log | grep 'modify' | STRIP_UFID ], [0], [dnl
+dpif|DBG|dummy at ovs-dummy: put[[modify]] skb_priority(0/0),skb_mark(0/0),recirc_id(0),dp_hash(0/0),in_port(1),eth(src=50:54:00:00:00:09/00:00:00:00:00:00,dst=50:54:00:00:00:0a/00:00:00:00:00:00),eth_type(0x1234), actions:push_vlan(vid=4,pcp=0),100
+])
+OVS_VSWITCHD_STOP
+AT_CLEANUP
-- 
2.1.0




More information about the dev mailing list