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

Ethan Jackson ethan at nicira.com
Wed Aug 5 23:07:26 UTC 2015


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 | 118 +++++++++++++++++++++++++++++++-----------
 1 file changed, 87 insertions(+), 31 deletions(-)

diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
index 59010c2..7abc97d 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,
+ * meaning the ukey should be deleted.  UKEY_KEEP, meaning the ukey is fine as
+ * is.  Or UKEY_MODIFY, meaning the ukey's actions should be changed but is
+ * otherwise fine.  If UKEY_MODIFY, is returned, callers should change the
+ * ukey's 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 datpaath 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,32 @@ revalidate(struct revalidator *revalidator)
         ovsrcu_quiesce();
     }
     dpif_flow_dump_thread_destroy(dump_thread);
+    ofpbuf_uninit(&odp_actions);
 }
 
 static bool
 handle_missed_revalidation(struct udpif *udpif, uint64_t reval_seq,
                            struct udpif_key *ukey)
 {
+    uint64_t odp_actions_stub[1024 / 8];
+    struct ofpbuf odp_actions = OFPBUF_STUB_INITIALIZER(odp_actions_stub);
+
     struct dpif_flow_stats stats;
-    bool keep;
+    enum reval_result result;
 
     COVERAGE_INC(revalidate_missed_dp_flow);
 
     memset(&stats, 0, sizeof stats);
     ovs_mutex_lock(&ukey->mutex);
-    keep = revalidate_ukey(udpif, ukey, &stats, reval_seq);
+    result = revalidate_ukey(udpif, ukey, &stats, &odp_actions, reval_seq);
     ovs_mutex_unlock(&ukey->mutex);
 
-    return keep;
+    ofpbuf_uninit(&odp_actions);
+
+    /* XXX: We somewhat conversvatively don't bother with in place
+     * modifications in this case.  Probably could be made to work, but this is
+     * simpler, and the edge case is rare. */
+    return result == UKEY_KEEP;
 }
 
 static void
-- 
2.1.0




More information about the dev mailing list