> On Feb 5, 2016, at 3:30 PM, Ben Pfaff <b...@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 <hanxue...@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 <b...@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,@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@openvswitch.org > http://openvswitch.org/mailman/listinfo/dev _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
