[ovs-dev] [PATCH branch-2.3 v2 4/5] ofproto: Allow in-place modifications of datapath flows.

Jarno Rajahalme jarno at ovn.org
Fri Feb 5 00:10:10 UTC 2016


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.

This patch is a backport of commit 43b2f13 to branch-2.3.
This patch also squashes in commit c56eba3b7ab0 ("ofproto-dpif-upcall: Don't
delete modified ukeys.").

Signed-off-by: Ethan J. Jackson <ethan at nicira.com>
Signed-off-by: Jarno Rajahalme <jarno at ovn.org>
---
 ofproto/ofproto-dpif-upcall.c | 219 ++++++++++++++++++++++++++++--------------
 tests/ofproto-dpif.at         |  40 ++++++++
 2 files changed, 189 insertions(+), 70 deletions(-)

diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
index f8f19c8..0e72a3a 100644
--- a/ofproto/ofproto-dpif-upcall.c
+++ b/ofproto/ofproto-dpif-upcall.c
@@ -1,4 +1,4 @@
-/* Copyright (c) 2009, 2010, 2011, 2012, 2013, 2014 Nicira, Inc.
+/* Copyright (c) 2009, 2010, 2011, 2012, 2013, 2014, 2016 Nicira, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -139,6 +139,12 @@ enum upcall_type {
     IPFIX_UPCALL                /* Per-bridge sampling. */
 };
 
+enum reval_result {
+    UKEY_KEEP,
+    UKEY_DELETE,
+    UKEY_MODIFY
+};
+
 struct upcall {
     struct flow_miss *flow_miss;    /* This upcall's flow_miss. */
 
@@ -1215,15 +1221,23 @@ 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 nlattr *mask, size_t mask_len,
                 const struct nlattr *actions, size_t actions_len,
-                const struct dpif_flow_stats *stats)
+                const struct dpif_flow_stats *stats,
+                struct ofpbuf *odp_actions)
     OVS_REQUIRES(ukey->mutex)
 {
-    uint64_t odp_actions_stub[1024 / 8];
-    struct ofpbuf odp_actions;
     struct xlate_out xout, *xoutp;
     struct netflow *netflow;
     struct ofproto_dpif *ofproto;
@@ -1231,17 +1245,18 @@ revalidate_ukey(struct udpif *udpif, struct udpif_key *ukey,
     struct flow flow, dp_mask;
     uint32_t *dp32, *xout32;
     odp_port_t odp_in_port;
+    enum reval_result result;
     struct xlate_in xin;
     long long int last_used;
     int error;
     size_t i;
-    bool may_learn, ok;
+    bool may_learn;
 
-    ofpbuf_use_stub(&odp_actions, odp_actions_stub, sizeof(odp_actions_stub));
-    ok = false;
+    result = UKEY_DELETE;
     xoutp = NULL;
     netflow = NULL;
 
+    ofpbuf_clear(odp_actions);
     last_used = ukey->stats.used;
     push.used = stats->used;
     push.tcp_flags = stats->tcp_flags;
@@ -1254,21 +1269,20 @@ revalidate_ukey(struct udpif *udpif, struct udpif_key *ukey,
 
     if (udpif->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 && !udpif->need_revalidate) {
-        ok = true;
+        result = UKEY_KEEP;
         goto exit;
     }
 
     may_learn = push.n_packets > 0;
     if (ukey->xcache && !udpif->need_revalidate) {
         xlate_push_stats(ukey->xcache, may_learn, &push);
-        ok = true;
+        result = UKEY_KEEP;
         goto exit;
     }
 
@@ -1286,7 +1300,7 @@ revalidate_ukey(struct udpif *udpif, struct udpif_key *ukey,
     }
 
     xlate_in_init(&xin, ofproto, &flow, NULL, push.tcp_flags, NULL,
-                  &odp_actions);
+                  odp_actions);
     xin.resubmit_stats = push.n_packets ? &push : NULL;
     xin.xcache = ukey->xcache;
     xin.may_learn = may_learn;
@@ -1295,18 +1309,13 @@ revalidate_ukey(struct udpif *udpif, struct udpif_key *ukey,
     xoutp = &xout;
 
     if (!udpif->need_revalidate) {
-        ok = true;
+        result = UKEY_KEEP;
         goto exit;
     }
 
     if (xout.slow) {
-        ofpbuf_clear(&odp_actions);
-        compose_slow_path(udpif, &xout, &flow, odp_in_port, &odp_actions);
-    }
-
-    if (actions_len != ofpbuf_size(&odp_actions)
-        || memcmp(ofpbuf_data(&odp_actions), actions, actions_len)) {
-        goto exit;
+        ofpbuf_clear(odp_actions);
+        compose_slow_path(udpif, &xout, &flow, odp_in_port, odp_actions);
     }
 
     if (odp_flow_key_to_mask(mask, mask_len, &dp_mask, &flow)
@@ -1326,37 +1335,67 @@ revalidate_ukey(struct udpif *udpif, struct udpif_key *ukey,
             goto exit;
         }
     }
-    ok = true;
+
+    if (actions_len != ofpbuf_size(odp_actions)
+        || memcmp(ofpbuf_data(odp_actions), actions, actions_len)) {
+        /* The datapath 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 (netflow) {
-        if (!ok) {
+        if (result == UKEY_DELETE) {
             netflow_flow_clear(netflow, &flow);
         }
         netflow_unref(netflow);
     }
     xlate_out_uninit(xoutp);
-    ofpbuf_uninit(&odp_actions);
-    return ok;
+    return result;
 }
 
 struct dump_op {
     struct udpif_key *ukey;
+    struct ofpbuf *buf;           /* Storage that is freed when op is done. */
     struct dpif_flow_stats stats; /* Stats for 'op'. */
     struct dpif_op op;            /* Flow del operation. */
 };
 
 static void
-dump_op_init(struct dump_op *op, const struct nlattr *key, size_t key_len,
-             struct udpif_key *ukey)
+delete_op_init(struct dump_op *op, const struct nlattr *key, size_t key_len,
+               struct udpif_key *ukey)
 {
     op->ukey = ukey;
+    op->buf = NULL;
     op->op.type = DPIF_OP_FLOW_DEL;
     op->op.u.flow_del.key = key;
     op->op.u.flow_del.key_len = key_len;
     op->op.u.flow_del.stats = &op->stats;
 }
 
+/* Takes ownership of 'buf'. */
+static void
+modify_op_init(struct dump_op *op, const struct nlattr *key, size_t key_len,
+               const struct nlattr *mask, size_t mask_len,
+               const struct nlattr *actions, size_t actions_len,
+               struct udpif_key *ukey, struct ofpbuf *buf)
+{
+    op->ukey = ukey;
+    op->buf = buf;
+    op->op.type = DPIF_OP_FLOW_PUT;
+    op->op.u.flow_put.flags = DPIF_FP_MODIFY;
+    op->op.u.flow_put.key = key;
+    op->op.u.flow_put.key_len = key_len;
+    op->op.u.flow_put.mask = mask;
+    op->op.u.flow_put.mask_len = mask_len;
+    op->op.u.flow_put.actions = actions;
+    op->op.u.flow_put.actions_len = actions_len;
+    op->op.u.flow_put.stats = NULL;
+}
+
 static void
 push_dump_ops__(struct udpif *udpif, struct dump_op *ops, size_t n_ops)
 {
@@ -1373,7 +1412,13 @@ push_dump_ops__(struct udpif *udpif, struct dump_op *ops, size_t n_ops)
         struct dump_op *op = &ops[i];
         struct dpif_flow_stats *push, *stats, push_buf;
 
+        /* Only deleted flows need their stats pushed. */
+        if (op->op.type != DPIF_OP_FLOW_DEL) {
+            goto next;
+        }
+
         stats = op->op.u.flow_del.stats;
+
         if (op->ukey) {
             push = &push_buf;
             ovs_mutex_lock(&op->ukey->mutex);
@@ -1398,7 +1443,7 @@ push_dump_ops__(struct udpif *udpif, struct dump_op *ops, size_t n_ops)
                 if (op->ukey->xcache) {
                     xlate_push_stats(op->ukey->xcache, may_learn, push);
                     ovs_mutex_unlock(&op->ukey->mutex);
-                    continue;
+                    goto next;
                 }
                 ovs_mutex_unlock(&op->ukey->mutex);
             }
@@ -1421,6 +1466,8 @@ push_dump_ops__(struct udpif *udpif, struct dump_op *ops, size_t n_ops)
                 }
             }
         }
+next:
+        ofpbuf_delete(op->buf);   /* Free storage. */
     }
 }
 
@@ -1432,13 +1479,18 @@ push_dump_ops(struct revalidator *revalidator,
 
     push_dump_ops__(revalidator->udpif, ops, n_ops);
     for (i = 0; i < n_ops; i++) {
-        ukey_delete(revalidator, ops[i].ukey);
+        if (ops[i].op.type == DPIF_OP_FLOW_DEL) {
+            ukey_delete(revalidator, ops[i].ukey);
+        }
     }
 }
 
 static void
 revalidate(struct revalidator *revalidator)
 {
+    uint64_t odp_actions_stub[1024 / 8];
+    struct ofpbuf odp_actions;
+
     struct udpif *udpif = revalidator->udpif;
 
     struct dump_op ops[REVALIDATE_MAX_BATCH];
@@ -1454,14 +1506,17 @@ revalidate(struct revalidator *revalidator)
     now = time_msec();
     atomic_read(&udpif->flow_limit, &flow_limit);
 
+    ofpbuf_use_stub(&odp_actions, odp_actions_stub, sizeof odp_actions_stub);
+
     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;
+        bool may_destroy;
         long long int used, max_idle;
         uint32_t hash;
         size_t n_flows;
+        enum reval_result result;
 
         hash = hash_bytes(key, key_len, udpif->secret);
         ukey = ukey_lookup(udpif, key, key_len, hash);
@@ -1506,15 +1561,21 @@ revalidate(struct revalidator *revalidator)
         }
 
         if ((used && used < now - max_idle) || n_flows > flow_limit * 2) {
-            mark = false;
+            result = UKEY_DELETE;
         } else {
-            mark = revalidate_ukey(udpif, ukey, mask, mask_len, actions,
-                                   actions_len, stats);
+            result = revalidate_ukey(udpif, ukey, mask, mask_len, actions,
+                                     actions_len, stats, &odp_actions);
         }
-        ukey->mark = ukey->flow_exists = mark;
+        ukey->mark = ukey->flow_exists = result != UKEY_DELETE;
+
+        if (result == UKEY_DELETE) {
+            delete_op_init(&ops[n_ops++], key, key_len, ukey);
+        } else if (result == UKEY_MODIFY) {
+            struct ofpbuf *actions_copy = ofpbuf_clone(&odp_actions);
 
-        if (!mark) {
-            dump_op_init(&ops[n_ops++], key, key_len, ukey);
+            modify_op_init(&ops[n_ops++], key, key_len, mask, mask_len,
+                           ofpbuf_data(actions_copy),
+                           ofpbuf_size(actions_copy), ukey, actions_copy);
         }
         ovs_mutex_unlock(&ukey->mutex);
 
@@ -1543,62 +1604,79 @@ revalidate(struct revalidator *revalidator)
     }
 
     dpif_flow_dump_state_uninit(udpif->dpif, state);
-}
-
-/* Called with exclusive access to 'revalidator' and 'ukey'. */
-static bool
-handle_missed_revalidation(struct revalidator *revalidator,
-                           struct udpif_key *ukey)
-    OVS_NO_THREAD_SAFETY_ANALYSIS
-{
-    struct udpif *udpif = revalidator->udpif;
-    struct nlattr *mask, *actions;
-    size_t mask_len, actions_len;
-    struct dpif_flow_stats stats;
-    struct ofpbuf *buf;
-    bool keep = false;
-
-    COVERAGE_INC(revalidate_missed_dp_flow);
-
-    if (!dpif_flow_get(udpif->dpif, ukey->key, ukey->key_len, &buf,
-                       &mask, &mask_len, &actions, &actions_len, &stats)) {
-        keep = revalidate_ukey(udpif, ukey, mask, mask_len, actions,
-                               actions_len, &stats);
-        ofpbuf_delete(buf);
-    }
-
-    return keep;
+    ofpbuf_uninit(&odp_actions);
 }
 
 static void
 revalidator_sweep__(struct revalidator *revalidator, bool purge)
     OVS_NO_THREAD_SAFETY_ANALYSIS
 {
+    uint64_t odp_actions_stub[1024 / 8];
+    struct ofpbuf odp_actions;
     struct dump_op ops[REVALIDATE_MAX_BATCH];
     struct udpif_key *ukey, *next;
     size_t n_ops;
 
     n_ops = 0;
 
+    ofpbuf_use_stub(&odp_actions, odp_actions_stub, sizeof odp_actions_stub);
+
     /* During garbage collection, this revalidator completely owns its ukeys
      * map, and therefore doesn't need to do any locking. */
     HMAP_FOR_EACH_SAFE (ukey, next, hmap_node, revalidator->ukeys) {
         if (ukey->flow_exists) {
             bool missed_flow = !ukey->mark;
+            enum reval_result result;
+            struct nlattr *mask;
+            size_t mask_len;
+            struct ofpbuf *buf = NULL;
 
             ukey->mark = false;
-            if (purge
-                || (missed_flow
-                    && revalidator->udpif->need_revalidate
-                    && !handle_missed_revalidation(revalidator, ukey))) {
-                struct dump_op *op = &ops[n_ops++];
-
-                dump_op_init(op, ukey->key, ukey->key_len, ukey);
-                if (n_ops == REVALIDATE_MAX_BATCH) {
-                    push_dump_ops(revalidator, ops, n_ops);
-                    n_ops = 0;
+            result = purge ? UKEY_DELETE : UKEY_KEEP;
+
+            if (missed_flow && revalidator->udpif->need_revalidate) {
+                struct udpif *udpif = revalidator->udpif;
+                struct nlattr *actions;
+                size_t actions_len;
+                struct dpif_flow_stats stats;
+
+                COVERAGE_INC(revalidate_missed_dp_flow);
+
+                if (dpif_flow_get(udpif->dpif, ukey->key, ukey->key_len, &buf,
+                                  &mask, &mask_len, &actions, &actions_len,
+                                  &stats)) {
+                    result = UKEY_DELETE;
+                } else {
+                    result = revalidate_ukey(udpif, ukey, mask, mask_len,
+                                             actions, actions_len, &stats,
+                                             &odp_actions);
                 }
             }
+
+            if (result == UKEY_DELETE) {
+                delete_op_init(&ops[n_ops++], ukey->key, ukey->key_len, ukey);
+            } else if (result == UKEY_MODIFY) {
+                /* Append 'actions' to 'buf', properly adjusting 'mask' in
+                 * case this expands 'buf'. */
+                struct nlattr *actions;
+
+                buf->frame = mask;
+                actions = ofpbuf_put(buf, ofpbuf_data(&odp_actions),
+                                     ofpbuf_size(&odp_actions));
+                mask = buf->frame;
+
+                /* Takes ownership of 'buf'. */
+                modify_op_init(&ops[n_ops++], ukey->key, ukey->key_len, mask,
+                               mask_len, actions, ofpbuf_size(&odp_actions),
+                               ukey, buf);
+                buf = NULL;
+            }
+            ofpbuf_delete(buf);
+
+            if (n_ops == REVALIDATE_MAX_BATCH) {
+                push_dump_ops(revalidator, ops, n_ops);
+                n_ops = 0;
+            }
         } else {
             ukey_delete(revalidator, ukey);
         }
@@ -1607,6 +1685,7 @@ revalidator_sweep__(struct revalidator *revalidator, bool purge)
     if (n_ops) {
         push_dump_ops(revalidator, ops, n_ops);
     }
+    ofpbuf_uninit(&odp_actions);
 }
 
 static void
diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
index 9351ea8..dd76ddb 100644
--- a/tests/ofproto-dpif.at
+++ b/tests/ofproto-dpif.at
@@ -4799,3 +4799,43 @@ skb_priority(0),skb_mark(0/0),in_port(1),eth(src=50:54:00:00:00:09/00:00:00:00:0
 ])
 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_USED | sort], [0], [dnl
+skb_priority(0),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_USED | sort], [0], [dnl
+skb_priority(0),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'], [0], [dnl
+dpif|DBG|dummy at ovs-dummy: put[[modify]] skb_priority(0),skb_mark(0/0),recirc_id(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.4




More information about the dev mailing list