> On Feb 5, 2016, at 7:27 PM, Ben Pfaff <b...@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 <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? > > 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 <ja...@ovn.org> > --8<--------------------------cut here-------------------------->8-- > > From: Ben Pfaff <b...@ovn.org <mailto:b...@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 <hanxue...@gmail.com <mailto:hanxue...@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 <b...@ovn.org <mailto:b...@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,@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
