[ovs-dev] [PATCH 2/3] ofproto-dpif-xlate: Don't consider mirrors used when excluded by VLAN.

Jarno Rajahalme jarno at ovn.org
Sat Feb 6 00:41:29 UTC 2016


> 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?

> 
> Reported-by: Huanle Han <hanxueluo at gmail.com>
> Reported-at: 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>
> ---
> ofproto/ofproto-dpif-xlate.c | 13 +++++++++----
> tests/ofproto-dpif.at        | 26 ++++++++++++++++++++++++++
> 2 files changed, 35 insertions(+), 4 deletions(-)
> 
> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index 1edc1b0..8a43078 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -1621,9 +1621,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;
> -

So this is too early, as the VLAN based rejection happens below. Besides, when adding the ‘dup_mirrors’ bits below, any non-rejected mirrors will be added as well?

>     if (ctx->xin->resubmit_stats) {
>         mirror_update_stats(xbridge->mbridge, mirrors,
>                             ctx->xin->resubmit_stats->n_packets,
> @@ -1656,8 +1653,12 @@ mirror_packet(struct xlate_ctx *ctx, struct xbundle *xbundle,
>             continue;
>         }
> 
> -        mirrors &= ~dup_mirrors;

And this is removed just because this is effectively done below together with any recursively used mirrors?

> +        /* Record the mirror and its duplicates 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 +1676,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 b/tests/ofproto-dpif.at
> index a372d36..5fdf5e6 100644
> --- a/tests/ofproto-dpif.at
> +++ b/tests/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
> 
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev




More information about the dev mailing list