[ovs-dev] [PATCH 22/26] ofproto-dpif-xlate: Rewrite mirroring to better fit flow translation.

Ben Pfaff blp at nicira.com
Thu Jul 30 06:42:42 UTC 2015


Until now, mirroring has been implemented by accumulating, across the whole
translation process, a set of mirrors that should receive a mirrored
packet.  After translation was complete, mirroring restored the original
version of the packet and sent that version to the mirrors.

That implementation was ugly for multiple reasons.  First, it means that
we have to keep a copy of the original packet (or its headers, actually),
which is expensive.  Second, it doesn't really make sense to mirror a
version of a packet that is different from the one originally output.
Third, it interacted with recirculation; mirroring needed to happen only
after recirculation was complete, but this was never properly implemented,
so that (I think) mirroring never happened for packets that were
recirculated.

This commit changes how mirroring works.  Now, a packet is mirrored at the
point in translation when it becomes eligible for it: for mirrors based on
ingress port, this is at ingress; for mirrors based on egress port, this
is at egress.  (Duplicates are dropped.)  Mirroring happens on the version
of the packet as it exists when it becomes eligible.  Finally, since
mirroring happens immediately, it interacts better with recirculation
(it still isn't perfect, since duplicate mirroring will occur if a packet
is eligible for mirroring both before and after recirculation; this is
not difficult to fix and an upcoming commit later in this series will do so).

Finally, this commit removes more code from xlate_actions() than it adds,
which in my opinion makes it easier to understand.

Signed-off-by: Ben Pfaff <blp at nicira.com>
---
 ofproto/ofproto-dpif-xlate.c | 113 +++++++++++++++++++------------------------
 tests/ofproto-dpif.at        |  12 ++---
 vswitchd/vswitch.xml         |  26 ++++++++++
 3 files changed, 82 insertions(+), 69 deletions(-)

diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index 8c8da9a..cbcd2a3 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -1530,56 +1530,55 @@ lookup_input_bundle(const struct xbridge *xbridge, ofp_port_t in_port,
 }
 
 static void
-add_mirror_actions(struct xlate_ctx *ctx, const struct flow *orig_flow)
+mirror_packet(struct xlate_ctx *ctx, struct xbundle *xbundle,
+              mirror_mask_t mirrors)
 {
-    const struct xbridge *xbridge = ctx->xbridge;
-    mirror_mask_t mirrors;
-    struct xbundle *in_xbundle;
-    uint16_t vlan;
-    uint16_t vid;
-
-    mirrors = ctx->mirrors;
-    ctx->mirrors = 0;
-
-    in_xbundle = lookup_input_bundle(xbridge, orig_flow->in_port.ofp_port,
-                                     ctx->xin->packet != NULL, NULL);
-    if (!in_xbundle) {
+    bool warn = ctx->xin->packet != NULL;
+    uint16_t vid = vlan_tci_to_vid(ctx->xin->flow.vlan_tci);
+    if (!input_vid_is_valid(vid, xbundle, warn)) {
         return;
     }
-    mirrors |= xbundle_mirror_src(xbridge, in_xbundle);
+    uint16_t vlan = input_vid_to_vlan(xbundle, vid);
 
-    /* Check VLAN. */
-    vid = vlan_tci_to_vid(orig_flow->vlan_tci);
-    if (!input_vid_is_valid(vid, in_xbundle, ctx->xin->packet != NULL)) {
-        return;
-    }
-    vlan = input_vid_to_vlan(in_xbundle, vid);
+    const struct xbridge *xbridge = ctx->xbridge;
 
+    /* Don't mirror to destinations that we've already mirrored to. */
+    mirrors &= ~ctx->mirrors;
     if (!mirrors) {
         return;
     }
 
-    /* Restore the original packet before adding the mirror actions. */
-    ctx->xin->flow = *orig_flow;
+    /* Record these mirrors so that we don't mirror to them again. */
+    ctx->mirrors |= mirrors;
+
+    if (ctx->xin->resubmit_stats) {
+        mirror_update_stats(xbridge->mbridge, mirrors,
+                            ctx->xin->resubmit_stats->n_packets,
+                            ctx->xin->resubmit_stats->n_bytes);
+    }
+    if (ctx->xin->xcache) {
+        struct xc_entry *entry;
+
+        entry = xlate_cache_add_entry(ctx->xin->xcache, XC_MIRROR);
+        entry->u.mirror.mbridge = mbridge_ref(xbridge->mbridge);
+        entry->u.mirror.mirrors = mirrors;
+    }
 
     while (mirrors) {
+        const unsigned long *vlans;
         mirror_mask_t dup_mirrors;
         struct ofbundle *out;
-        const unsigned long *vlans;
-        bool vlan_mirrored;
-        bool has_mirror;
         int out_vlan;
 
-        has_mirror = mirror_get(xbridge->mbridge, raw_ctz(mirrors),
-                                &vlans, &dup_mirrors, &out, &out_vlan);
+        bool has_mirror = mirror_get(xbridge->mbridge, raw_ctz(mirrors),
+                                     &vlans, &dup_mirrors, &out, &out_vlan);
         ovs_assert(has_mirror);
 
         if (vlans) {
             ctx->wc->masks.vlan_tci |= htons(VLAN_CFI | VLAN_VID_MASK);
         }
-        vlan_mirrored = !vlans || bitmap_is_set(vlans, vlan);
 
-        if (!vlan_mirrored) {
+        if (vlans && !bitmap_is_set(vlans, vlan)) {
             mirrors = zero_rightmost_1bit(mirrors);
             continue;
         }
@@ -1593,7 +1592,7 @@ add_mirror_actions(struct xlate_ctx *ctx, const struct flow *orig_flow)
                 output_normal(ctx, out_xbundle, vlan);
             }
         } else if (vlan != out_vlan
-                   && !eth_addr_is_reserved(orig_flow->dl_dst)) {
+                   && !eth_addr_is_reserved(ctx->xin->flow.dl_dst)) {
             struct xbundle *xbundle;
 
             LIST_FOR_EACH (xbundle, list_node, &xbridge->xbundles) {
@@ -1606,6 +1605,20 @@ add_mirror_actions(struct xlate_ctx *ctx, const struct flow *orig_flow)
     }
 }
 
+static void
+mirror_ingress_packet(struct xlate_ctx *ctx)
+{
+    if (mbridge_has_mirrors(ctx->xbridge->mbridge)) {
+        bool warn = ctx->xin->packet != NULL;
+        struct xbundle *xbundle = lookup_input_bundle(
+            ctx->xbridge, ctx->xin->flow.in_port.ofp_port, warn, NULL);
+        if (xbundle) {
+            mirror_packet(ctx, xbundle,
+                          xbundle_mirror_src(ctx->xbridge, xbundle));
+        }
+    }
+}
+
 /* Given 'vid', the VID obtained from the 802.1Q header that was received as
  * part of a packet (specify 0 if there was no 802.1Q header), and 'in_xbundle',
  * the bundle on which the packet was received, returns the VLAN to which the
@@ -2812,11 +2825,6 @@ compose_output_action__(struct xlate_ctx *ctx, ofp_port_t ofp_port,
         }
     }
 
-    if (mbridge_has_mirrors(ctx->xbridge->mbridge) && xport->xbundle) {
-        ctx->mirrors |= xbundle_mirror_dst(xport->xbundle->xbridge,
-                                           xport->xbundle);
-    }
-
     if (xport->peer) {
         const struct xport *peer = xport->peer;
         struct flow old_flow = ctx->xin->flow;
@@ -3032,6 +3040,12 @@ compose_output_action__(struct xlate_ctx *ctx, ofp_port_t ofp_port,
         ctx->nf_output_iface = ofp_port;
     }
 
+    if (mbridge_has_mirrors(ctx->xbridge->mbridge) && xport->xbundle) {
+        mirror_packet(ctx, xport->xbundle,
+                      xbundle_mirror_dst(xport->xbundle->xbridge,
+                                         xport->xbundle));
+    }
+
  out:
     /* Restore flow */
     flow->vlan_tci = flow_vlan_tci;
@@ -4878,13 +4892,6 @@ xlate_actions(struct xlate_in *xin, struct xlate_out *xout)
     }
     xout->fail_open = ctx.rule && rule_dpif_is_fail_open(ctx.rule);
 
-    struct flow orig_flow;
-    if (mbridge_has_mirrors(xbridge->mbridge)) {
-        /* Do this conditionally because the copy is expensive enough that it
-         * shows up in profiles. */
-        orig_flow = *flow;
-    }
-
     /* Get the proximate input port of the packet.  (If xin->recirc,
      * flow->in_port is the ultimate input port of the packet.) */
     struct xport *in_port = get_ofp_port(xbridge,
@@ -4947,6 +4954,7 @@ xlate_actions(struct xlate_in *xin, struct xlate_out *xout)
                 OVS_NOT_REACHED();
             }
 
+            mirror_ingress_packet(&ctx);
             do_xlate_actions(ofpacts, ofpacts_len, &ctx);
 
             /* We've let OFPP_NORMAL and the learning action look at the
@@ -4986,11 +4994,6 @@ xlate_actions(struct xlate_in *xin, struct xlate_out *xout)
         if (user_cookie_offset) {
             fix_sflow_action(&ctx, user_cookie_offset);
         }
-        /* Only mirror fully processed packets. */
-        if (!exit_recirculates(&ctx)
-            && mbridge_has_mirrors(xbridge->mbridge)) {
-            add_mirror_actions(&ctx, &orig_flow);
-        }
     }
 
     if (nl_attr_oversized(ctx.odp_actions->size)) {
@@ -5005,22 +5008,6 @@ xlate_actions(struct xlate_in *xin, struct xlate_out *xout)
         ctx.xout->slow |= SLOW_ACTION;
     }
 
-    /* Update mirror stats only for packets really received by the bridge. */
-    if (!xin->recirc && mbridge_has_mirrors(xbridge->mbridge)) {
-        if (ctx.xin->resubmit_stats) {
-            mirror_update_stats(xbridge->mbridge, ctx.mirrors,
-                                ctx.xin->resubmit_stats->n_packets,
-                                ctx.xin->resubmit_stats->n_bytes);
-        }
-        if (ctx.xin->xcache) {
-            struct xc_entry *entry;
-
-            entry = xlate_cache_add_entry(ctx.xin->xcache, XC_MIRROR);
-            entry->u.mirror.mbridge = mbridge_ref(xbridge->mbridge);
-            entry->u.mirror.mirrors = ctx.mirrors;
-        }
-    }
-
     /* Do netflow only for packets really received by the bridge and not sent
      * to the controller.  We consider packets sent to the controller to be
      * part of the control plane rather than the data plane. */
diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
index 2656ca7..9bfb794 100644
--- a/tests/ofproto-dpif.at
+++ b/tests/ofproto-dpif.at
@@ -3954,13 +3954,13 @@ 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
+  [Datapath actions: 3,2
 ])
 
 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
+  [Datapath actions: 3,1
 ])
 
 OVS_VSWITCHD_STOP
@@ -3984,7 +3984,7 @@ 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
+  [Datapath actions: 3,2
 ])
 
 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)"
@@ -4074,7 +4074,7 @@ 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
+  [Datapath actions: 3,2
 ])
 
 OVS_VSWITCHD_STOP
@@ -4098,13 +4098,13 @@ 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
+  [Datapath actions: 3,push_vlan(vid=17,pcp=0),2
 ])
 
 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
+  [Datapath actions: 3,1
 ])
 
 OVS_VSWITCHD_STOP
diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml
index 483a9de..17035bb 100644
--- a/vswitchd/vswitch.xml
+++ b/vswitchd/vswitch.xml
@@ -3424,6 +3424,32 @@
     traffic may also be referred to as SPAN or RSPAN, depending on how
     the mirrored traffic is sent.</p>
 
+    <p>
+      When a packet enters an Open vSwitch bridge, it becomes eligible for
+      mirroring based on its ingress port and VLAN.  As the packet travels
+      through the flow tables, each time it is output to a port, it becomes
+      eligible for mirroring based on the egress port and VLAN.  In Open
+      vSwitch 2.5 and later, mirroring occurs just after a packet first becomes
+      eligible, using the packet as it exists at that point; in Open vSwitch
+      2.4 and earlier, mirroring occurs only after a packet has traversed all
+      the flow tables, using the original packet as it entered the bridge.
+      This makes a difference only when the flow table modifies the packet: in
+      Open vSwitch 2.4, the modifications are never visible to mirrors, whereas
+      in Open vSwitch 2.5 and later modifications made before the first output
+      that makes it eligible for mirroring to a particular destination are
+      visible.
+    </p>
+
+    <p>
+      A packet that enters an Open vSwitch bridge is mirrored to a particular
+      destination only once, even if it is eligible for multiple reasons.  For
+      example, a packet would be mirrored to a particular <ref
+      column="output_port"/> only once, even if it is selected for mirroring to
+      that port by <ref column="select_dst_port"/> and <ref
+      column="select_src_port"/> in the same or different <ref table="Mirror"/>
+      records.
+    </p>
+
     <column name="name">
       Arbitrary identifier for the <ref table="Mirror"/>.
     </column>
-- 
2.1.3




More information about the dev mailing list