[ovs-dev] [PATCH 2/3] ofproto-dpif-xlate: Don't consider mirrors used when excluded by VLAN.
Jarno Rajahalme
jarno at ovn.org
Mon Feb 8 19:14:23 UTC 2016
> On Feb 5, 2016, at 7:27 PM, Ben Pfaff <blp at ovn.org> wrote:
>
> On Fri, Feb 05, 2016 at 04:41:29PM -0800, Jarno Rajahalme wrote:
>>
>>> On Feb 5, 2016, at 3:30 PM, Ben Pfaff <blp at ovn.org> wrote:
>>>
>>> Mirroring is supposed to happen at most once for any destination on a given
>>> packet, so the implementation keeps track of which mirrors have already
>>> been used. However, until this commit it did that incorrectly: it
>>> considered a mirror "used" even if it had been rejected on the basis of
>>> VLAN. This commit fixes the problem.
>>
>> So even if a mirror has been rejected on the basis of a VLAN, it
>> should still be considered for output (later)? Can you describe a
>> scenario where this makes a difference? E.g., is there a case where a
>> packet is not sent when it should have been sent, or did we get
>> duplicate mirroring due to this, as tested against by the new test
>> case?
>
> I think the best way to answer this is to rewrite the commit message.
> Here's a new version. It also adds more comments to the code.
>
Ben,
Thanks for taking the time to explain this in detail. I guess we must have the documentary piece of the commit message in a document somewhere, but I had missed that so was not aware of it in this level of detail. It makes all sense now,
Acked-by: Jarno Rajahalme <jarno at ovn.org>
> --8<--------------------------cut here-------------------------->8--
>
> From: Ben Pfaff <blp at ovn.org <mailto:blp at ovn.org>>
> Date: Fri, 5 Feb 2016 19:16:01 -0800
> Subject: [PATCH] ofproto-dpif-xlate: Don't consider mirrors used when excluded
> by VLAN.
>
> Mirrors can be configured to select packets for mirroring on the basis
> of multiple criteria: input ports, output ports, and VLANs. A packet P
> is to be mirrored if there exists a mirror M such that either:
>
> - P ingresses on an input port selected by M, or
>
> - P egresses on an output port selected by M
>
> AND P is in a VLAN selected by M.
>
> In addition, every mirror has a destination, which can be an output port
> or an output VLAN. Either way, if a packet is mirrored to a particular
> destination, it is done only once, even if different mirrors both select
> a packet and have the same destination.
>
> Since commit efbc3b7c4006c (ofproto-dpif-xlate: Rewrite mirroring to better
> fit flow translation.), these requirements have been implemented
> incorrectly: if a packet satisfies one of the bulleted requirements
> above for mirror M1, but not the VLAN selection requirement for M1,
> then it was not sent to M's destination, but it was still considered
> as having been sent to M1's destination for the purpose of avoid output
> duplication. Thus, if P satisfied *all* of the requirements for a
> second mirror M2, if M1 and M2 had the same destination, the packet was
> still not mirrored. This commit fixes that problem.
>
> (The issue only occurred if M1 happened to have a smaller index than
> M2 in OVS's internal data structures. That's just a matter of luck.)
>
> Reported-by: Huanle Han <hanxueluo at gmail.com <mailto:hanxueluo at gmail.com>>
> Reported-at: http://openvswitch.org/pipermail/dev/2016-January/064531.html <http://openvswitch.org/pipermail/dev/2016-January/064531.html>
> Fixes: 7efbc3b7c4006c (ofproto-dpif-xlate: Rewrite mirroring to better fit flow translation.)
> Signed-off-by: Ben Pfaff <blp at ovn.org <mailto:blp at ovn.org>>
> ---
> ofproto/ofproto-dpif-xlate.c | 25 ++++++++++++++++++++-----
> tests/ofproto-dpif.at <http://ofproto-dpif.at/> | 26 ++++++++++++++++++++++++++
> 2 files changed, 46 insertions(+), 5 deletions(-)
>
> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index a6ea067..7138c6c 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -1602,10 +1602,15 @@ lookup_input_bundle(const struct xbridge *xbridge, ofp_port_t in_port,
> return NULL;
> }
>
> +/* Mirrors the packet represented by 'ctx' to appropriate mirror destinations,
> + * given the packet is ingressing or egressing on 'xbundle', which has ingress
> + * or egress (as appropriate) mirrors 'mirrors'. */
> static void
> mirror_packet(struct xlate_ctx *ctx, struct xbundle *xbundle,
> mirror_mask_t mirrors)
> {
> + /* Figure out what VLAN the packet is in (because mirrors can select
> + * packets on basis of VLAN). */
> 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)) {
> @@ -1621,9 +1626,6 @@ mirror_packet(struct xlate_ctx *ctx, struct xbundle *xbundle,
> return;
> }
>
> - /* 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,
> @@ -1637,27 +1639,36 @@ mirror_packet(struct xlate_ctx *ctx, struct xbundle *xbundle,
> entry->u.mirror.mirrors = mirrors;
> }
>
> + /* 'mirrors' is a bit-mask of candidates for mirroring. Iterate as long as
> + * some candidates remain. */
> while (mirrors) {
> const unsigned long *vlans;
> mirror_mask_t dup_mirrors;
> struct ofbundle *out;
> int out_vlan;
>
> + /* Get the details of the mirror represented by the rightmost 1-bit. */
> bool has_mirror = mirror_get(xbridge->mbridge, raw_ctz(mirrors),
> &vlans, &dup_mirrors, &out, &out_vlan);
> ovs_assert(has_mirror);
>
> + /* If this mirror selects on the basis of VLAN, and it does not select
> + * 'vlan', then discard this mirror and go on to the next one. */
> if (vlans) {
> ctx->wc->masks.vlan_tci |= htons(VLAN_CFI | VLAN_VID_MASK);
> }
> -
> if (vlans && !bitmap_is_set(vlans, vlan)) {
> mirrors = zero_rightmost_1bit(mirrors);
> continue;
> }
>
> - mirrors &= ~dup_mirrors;
> + /* Record the mirror, and the mirrors that output to the same
> + * destination, so that we don't mirror to them again. This must be
> + * done now to ensure that output_normal(), below, doesn't recursively
> + * output to the same mirrors. */
> ctx->mirrors |= dup_mirrors;
> +
> + /* Send the packet to the mirror. */
> if (out) {
> struct xlate_cfg *xcfg = ovsrcu_get(struct xlate_cfg *, &xcfgp);
> struct xbundle *out_xbundle = xbundle_lookup(xcfg, out);
> @@ -1675,6 +1686,10 @@ mirror_packet(struct xlate_ctx *ctx, struct xbundle *xbundle,
> }
> }
> }
> +
> + /* output_normal() could have recursively output (to different
> + * mirrors), so make sure that we don't send duplicates. */
> + mirrors &= ~ctx->mirrors;
> }
> }
>
> diff --git a/tests/ofproto-dpif.at <http://ofproto-dpif.at/> b/tests/ofproto-dpif.at <http://ofproto-dpif.at/>
> index a372d36..5fdf5e6 100644
> --- a/tests/ofproto-dpif.at <http://ofproto-dpif.at/>
> +++ b/tests/ofproto-dpif.at <http://ofproto-dpif.at/>
> @@ -4148,6 +4148,32 @@ AT_CHECK([ovs-dpctl normalize-actions "$flow" "$actual"], [0], [expout])
> OVS_VSWITCHD_STOP
> AT_CLEANUP
>
> +# This verifies that we don't get duplicate mirroring when mirror_packet()
> +# might be invoked recursively, as a check against regression.
> +AT_SETUP([ofproto-dpif - multiple VLAN output mirrors])
> +OVS_VSWITCHD_START
> +add_of_ports br0 1 2 3
> +ovs-vsctl \
> + -- set Bridge br0 fail-mode=standalone mirrors=@m1, at m2 \
> + -- --id=@m1 create Mirror name=m1 select_all=true output_vlan=500 \
> + -- --id=@m2 create Mirror name=m2 select_all=true output_vlan=501 \
> + -- set Port br0 tag=0 \
> + -- set Port p1 tag=0 \
> + -- set Port p2 tag=500 \
> + -- set Port p3 tag=501
> +
> +flow='in_port=1'
> +AT_CHECK([ovs-appctl ofproto/trace br0 "$flow"], [0], [stdout])
> +AT_CHECK([tail -1 stdout | sed 's/Datapath actions: //
> +s/,/\
> +/g' | sort], [0], [100
> +2
> +3
> +])
> +
> +OVS_VSWITCHD_STOP
> +AT_CLEANUP
> +
> # This test verifies that mirror state is preserved across recirculation.
> #
> # Otherwise, post-recirculation the ingress and the output to port 4
> --
> 2.1.3
More information about the dev
mailing list