Always learn something new when reviewing code ;-)
Acked-by: Jarno Rajahalme <jrajaha...@nicira.com>
> On Jul 29, 2015, at 11:42 PM, Ben Pfaff <b...@nicira.com> wrote:
>
> 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 <b...@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
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
