[ovs-dev] [mirror v2 1/2] ofproto-dpif: Handle dest mirrors in compose_output_action().

Ethan Jackson ethan at nicira.com
Mon Jun 24 20:31:36 UTC 2013


Before this patch, the mirroring code would retroactively insert
actions for destination mirrors after actions were translated.
This relied on converting datapath output actions into ofports
which doesn't work for tunnels and patch ports.  This patch
refactors the code to handle destination mirrors at output.

Signed-off-by: Ethan Jackson <ethan at nicira.com>
---
 ofproto/ofproto-dpif-xlate.c |   51 +++++++++++++-------------------
 tests/ofproto-dpif.at        |   66 ++++++++++++++++++++++++++----------------
 2 files changed, 61 insertions(+), 56 deletions(-)

diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index a52a8cf..6512866 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -58,6 +58,7 @@ struct xlate_ctx {
 
     /* Flow at the last commit. */
     struct flow base_flow;
+    struct flow orig_flow; /* Junk unless 'hit_resubmit_limit' or mirroring. */
 
     /* Tunnel IP destination address as received.  This is stored separately
      * as the base_flow.tunnel is cleared on init to reflect the datapath
@@ -173,22 +174,22 @@ lookup_input_bundle(const struct ofproto_dpif *ofproto, ofp_port_t in_port,
 }
 
 static void
-add_mirror_actions(struct xlate_ctx *ctx, const struct flow *orig_flow)
+add_mirror_actions(struct xlate_ctx *ctx, mirror_mask_t mirrors)
 {
     struct ofproto_dpif *ofproto = ctx->ofproto;
-    mirror_mask_t mirrors;
     struct ofbundle *in_bundle;
+    struct flow xin_flow;
     uint16_t vlan;
     uint16_t vid;
-    const struct nlattr *a;
-    size_t left;
 
-    in_bundle = lookup_input_bundle(ctx->ofproto, orig_flow->in_port.ofp_port,
+    in_bundle = lookup_input_bundle(ctx->ofproto,
+                                    ctx->orig_flow.in_port.ofp_port,
                                     ctx->xin->packet != NULL, NULL);
     if (!in_bundle) {
         return;
     }
-    mirrors = in_bundle->src_mirrors;
+    mirrors |= in_bundle->src_mirrors;
+    mirrors &= ~ctx->xout->mirrors;
 
     /* Drop frames on bundles reserved for mirroring. */
     if (in_bundle->mirror_out) {
@@ -202,35 +203,18 @@ add_mirror_actions(struct xlate_ctx *ctx, const struct flow *orig_flow)
     }
 
     /* Check VLAN. */
-    vid = vlan_tci_to_vid(orig_flow->vlan_tci);
+    vid = vlan_tci_to_vid(ctx->orig_flow.vlan_tci);
     if (!input_vid_is_valid(vid, in_bundle, ctx->xin->packet != NULL)) {
         return;
     }
     vlan = input_vid_to_vlan(in_bundle, vid);
 
-    /* Look at the output ports to check for destination selections. */
-
-    NL_ATTR_FOR_EACH (a, left, ctx->xout->odp_actions.data,
-                      ctx->xout->odp_actions.size) {
-        enum ovs_action_attr type = nl_attr_type(a);
-        struct ofport_dpif *ofport;
-
-        if (type != OVS_ACTION_ATTR_OUTPUT) {
-            continue;
-        }
-
-        ofport = get_odp_port(ofproto, nl_attr_get_odp_port(a));
-        if (ofport && ofport->bundle) {
-            mirrors |= ofport->bundle->dst_mirrors;
-        }
-    }
-
     if (!mirrors) {
         return;
     }
 
-    /* Restore the original packet before adding the mirror actions. */
-    ctx->xin->flow = *orig_flow;
+    xin_flow = ctx->xin->flow;
+    ctx->xin->flow = ctx->orig_flow;
 
     while (mirrors) {
         struct ofmirror *m;
@@ -251,7 +235,7 @@ add_mirror_actions(struct xlate_ctx *ctx, const struct flow *orig_flow)
         if (m->out) {
             output_normal(ctx, m->out, vlan);
         } else if (vlan != m->out_vlan
-                   && !eth_addr_is_reserved(orig_flow->dl_dst)) {
+                   && !eth_addr_is_reserved(ctx->orig_flow.dl_dst)) {
             struct ofbundle *bundle;
 
             HMAP_FOR_EACH (bundle, hmap_node, &ofproto->bundles) {
@@ -262,6 +246,7 @@ add_mirror_actions(struct xlate_ctx *ctx, const struct flow *orig_flow)
             }
         }
     }
+    ctx->xin->flow = xin_flow;
 }
 
 /* Given 'vid', the VID obtained from the 802.1Q header that was received as
@@ -856,6 +841,10 @@ compose_output_action__(struct xlate_ctx *ctx, ofp_port_t ofp_port,
         return;
     }
 
+    if (ctx->ofproto->has_mirrors && ofport->bundle) {
+        add_mirror_actions(ctx, ofport->bundle->dst_mirrors);
+    }
+
     if (ofport->peer) {
         struct ofport_dpif *peer = ofport->peer;
         struct flow old_flow = ctx->xin->flow;
@@ -1864,7 +1853,6 @@ xlate_actions(struct xlate_in *xin, struct xlate_out *xout)
     enum slow_path_reason special;
     const struct ofpact *ofpacts;
     struct ofport_dpif *in_port;
-    struct flow orig_flow;
     struct xlate_ctx ctx;
     size_t ofpacts_len;
 
@@ -1946,7 +1934,7 @@ xlate_actions(struct xlate_in *xin, struct xlate_out *xout)
     if (ctx.ofproto->has_mirrors || hit_resubmit_limit) {
         /* Do this conditionally because the copy is expensive enough that it
          * shows up in profiles. */
-        orig_flow = *flow;
+        ctx.orig_flow = *flow;
     }
 
     if (flow->nw_frag & FLOW_NW_FRAG_ANY) {
@@ -2009,7 +1997,8 @@ xlate_actions(struct xlate_in *xin, struct xlate_out *xout)
             } else if (!VLOG_DROP_ERR(&trace_rl)) {
                 struct ds ds = DS_EMPTY_INITIALIZER;
 
-                ofproto_trace(ctx.ofproto, &orig_flow, ctx.xin->packet, &ds);
+                ofproto_trace(ctx.ofproto, &ctx.orig_flow, ctx.xin->packet,
+                              &ds);
                 VLOG_ERR("Trace triggered by excessive resubmit "
                          "recursion:\n%s", ds_cstr(&ds));
                 ds_destroy(&ds);
@@ -2024,7 +2013,7 @@ xlate_actions(struct xlate_in *xin, struct xlate_out *xout)
             compose_output_action(&ctx, OFPP_LOCAL);
         }
         if (ctx.ofproto->has_mirrors) {
-            add_mirror_actions(&ctx, &orig_flow);
+            add_mirror_actions(&ctx, 0);
         }
         fix_sflow_action(&ctx);
     }
diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
index 426095b..5d62bcc 100644
--- a/tests/ofproto-dpif.at
+++ b/tests/ofproto-dpif.at
@@ -858,15 +858,19 @@ AT_CHECK([ovs-ofctl add-flows br0 flows.txt])
 
 flow="in_port(1),eth(src=50:54:00:00:00:05,dst=50:54:00:00:00:07),eth_type(0x0800),ipv4(src=192.168.0.1,dst=192.168.0.2,proto=1,tos=0,ttl=128,frag=no),icmp(type=8,code=0)"
 AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "$flow"], [0], [stdout])
-AT_CHECK_UNQUOTED([tail -1 stdout], [0],
-  [Datapath actions: 2,3
-])
+actual=`tail -1 stdout | sed 's/Datapath actions: //'`
+expected="2,3"
+AT_CHECK([ovs-dpctl normalize-actions "$flow" "$expected"], [0], [stdout])
+mv stdout expout
+AT_CHECK([ovs-dpctl normalize-actions "$flow" "$actual"], [0], [expout])
 
 flow="in_port(2),eth(src=50:54:00:00:00:05,dst=50:54:00:00:00:07),eth_type(0x0800),ipv4(src=192.168.0.1,dst=192.168.0.2,proto=1,tos=0,ttl=128,frag=no),icmp(type=8,code=0)"
 AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "$flow"], [0], [stdout])
-AT_CHECK_UNQUOTED([tail -1 stdout], [0],
-  [Datapath actions: 1,3
-])
+actual=`tail -1 stdout | sed 's/Datapath actions: //'`
+expected="1,3"
+AT_CHECK([ovs-dpctl normalize-actions "$flow" "$expected"], [0], [stdout])
+mv stdout expout
+AT_CHECK([ovs-dpctl normalize-actions "$flow" "$actual"], [0], [expout])
 
 OVS_VSWITCHD_STOP
 AT_CLEANUP
@@ -888,15 +892,19 @@ AT_CHECK([ovs-ofctl add-flows br0 flows.txt])
 
 flow="in_port(1),eth(src=50:54:00:00:00:05,dst=50:54:00:00:00:07),eth_type(0x0800),ipv4(src=192.168.0.1,dst=192.168.0.2,proto=1,tos=0,ttl=128,frag=no),icmp(type=8,code=0)"
 AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "$flow"], [0], [stdout])
-AT_CHECK_UNQUOTED([tail -1 stdout], [0],
-  [Datapath actions: 2,3
-])
+actual=`tail -1 stdout | sed 's/Datapath actions: //'`
+expected="2,3"
+AT_CHECK([ovs-dpctl normalize-actions "$flow" "$expected"], [0], [stdout])
+mv stdout expout
+AT_CHECK([ovs-dpctl normalize-actions "$flow" "$actual"], [0], [expout])
 
 flow="in_port(2),eth(src=50:54:00:00:00:05,dst=50:54:00:00:00:07),eth_type(0x0800),ipv4(src=192.168.0.1,dst=192.168.0.2,proto=1,tos=0,ttl=128,frag=no),icmp(type=8,code=0)"
 AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "$flow"], [0], [stdout])
-AT_CHECK_UNQUOTED([tail -1 stdout], [0],
-  [Datapath actions: 1
-])
+actual=`tail -1 stdout | sed 's/Datapath actions: //'`
+expected="1"
+AT_CHECK([ovs-dpctl normalize-actions "$flow" "$expected"], [0], [stdout])
+mv stdout expout
+AT_CHECK([ovs-dpctl normalize-actions "$flow" "$actual"], [0], [expout])
 OVS_VSWITCHD_STOP
 AT_CLEANUP
 
@@ -914,7 +922,7 @@ AT_CHECK([ovs-ofctl add-flow br0 action=output:1])
 flow="icmp,dl_src=50:54:00:00:00:05,dl_dst=50:54:00:00:00:07,nw_src=192.168.0.1,nw_dst=192.168.0.2,nw_ttl=128,icmp_type=8,icmp_code=0"
 AT_CHECK([ovs-appctl ofproto/trace br0 "$flow"], [0], [stdout])
 AT_CHECK_UNQUOTED([tail -1 stdout], [0],
-  [Datapath actions: 1,2
+  [Datapath actions: 2,1
 ])
 
 OVS_VSWITCHD_STOP
@@ -937,9 +945,11 @@ AT_CHECK([ovs-ofctl add-flows br0 flows.txt])
 
 flow="in_port(1),eth(src=50:54:00:00:00:05,dst=50:54:00:00:00:07),eth_type(0x0800),ipv4(src=192.168.0.1,dst=192.168.0.2,proto=1,tos=0,ttl=128,frag=no),icmp(type=8,code=0)"
 AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "$flow"], [0], [stdout])
-AT_CHECK_UNQUOTED([tail -1 stdout], [0],
-  [Datapath actions: 2,3
-])
+actual=`tail -1 stdout | sed 's/Datapath actions: //'`
+expected="2,3"
+AT_CHECK([ovs-dpctl normalize-actions "$flow" "$expected"], [0], [stdout])
+mv stdout expout
+AT_CHECK([ovs-dpctl normalize-actions "$flow" "$actual"], [0], [expout])
 
 flow="in_port(2),eth(src=50:54:00:00:00:05,dst=50:54:00:00:00:07),eth_type(0x0800),ipv4(src=192.168.0.1,dst=192.168.0.2,proto=1,tos=0,ttl=128,frag=no),icmp(type=8,code=0)"
 AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "$flow"], [0], [stdout])
@@ -978,9 +988,11 @@ AT_CHECK_UNQUOTED([tail -1 stdout], [0],
 
 flow="in_port(1),eth(src=50:54:00:00:00:05,dst=50:54:00:00:00:07),eth_type(0x8100),vlan(vid=11,pcp=0),encap(eth_type(0x0800),ipv4(src=192.168.0.1,dst=192.168.0.2,proto=1,tos=0,ttl=128,frag=no),icmp(type=8,code=0))"
 AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "$flow"], [0], [stdout])
-AT_CHECK_UNQUOTED([tail -1 stdout], [0],
-  [Datapath actions: 2,3
-])
+actual=`tail -1 stdout | sed 's/Datapath actions: //'`
+expected="2,3"
+AT_CHECK([ovs-dpctl normalize-actions "$flow" "$expected"], [0], [stdout])
+mv stdout expout
+AT_CHECK([ovs-dpctl normalize-actions "$flow" "$actual"], [0], [expout])
 
 OVS_VSWITCHD_STOP
 AT_CLEANUP
@@ -1002,15 +1014,19 @@ AT_CHECK([ovs-ofctl add-flows br0 flows.txt])
 
 flow="in_port(1),eth(src=50:54:00:00:00:05,dst=50:54:00:00:00:07),eth_type(0x0800),ipv4(src=192.168.0.1,dst=192.168.0.2,proto=1,tos=0,ttl=128,frag=no),icmp(type=8,code=0)"
 AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "$flow"], [0], [stdout])
-AT_CHECK_UNQUOTED([tail -1 stdout], [0],
-  [Datapath actions: push_vlan(vid=17,pcp=0),2,pop_vlan,3
-])
+actual=`tail -1 stdout | sed 's/Datapath actions: //'`
+expected="push_vlan(vid=17,pcp=0),2,pop_vlan,3"
+AT_CHECK([ovs-dpctl normalize-actions "$flow" "$expected"], [0], [stdout])
+mv stdout expout
+AT_CHECK([ovs-dpctl normalize-actions "$flow" "$actual"], [0], [expout])
 
 flow="in_port(2),eth(src=50:54:00:00:00:05,dst=50:54:00:00:00:07),eth_type(0x0800),ipv4(src=192.168.0.1,dst=192.168.0.2,proto=1,tos=0,ttl=128,frag=no),icmp(type=8,code=0)"
 AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "$flow"], [0], [stdout])
-AT_CHECK_UNQUOTED([tail -1 stdout], [0],
-  [Datapath actions: 1,3
-])
+actual=`tail -1 stdout | sed 's/Datapath actions: //'`
+expected="1,3"
+AT_CHECK([ovs-dpctl normalize-actions "$flow" "$expected"], [0], [stdout])
+mv stdout expout
+AT_CHECK([ovs-dpctl normalize-actions "$flow" "$actual"], [0], [expout])
 
 OVS_VSWITCHD_STOP
 AT_CLEANUP
-- 
1.7.9.5




More information about the dev mailing list