Thanks for testing! I applied this to master, branch-2.6, and branch-2.5, adding Sugesh as reporter.
On Fri, Sep 16, 2016 at 08:34:55AM +0000, Zoltán Balogh wrote: > > Hi Ben, > > I tested your patch on latest master commit. > $ git rev-parse HEAD > 258b27d35a8aad8231f8c5308b0d5232dc966915 > > Mirroring works fine in my setup. The patch fixed the reported mirroring > issue. > Could you please add Sugesh Chandran as reporter to the commit log too? > We were working together when the bug was detected. > > Reported-by: Sugesh Chandran <sugesh.chand...@intel.com> > > Best regards, > Zoltán > > > -----Original Message----- > From: Ben Pfaff [mailto:b...@ovn.org] > Sent: Thursday, September 15, 2016 8:44 PM > To: Zoltán Balogh <zoltan.bal...@ericsson.com> > Cc: discuss@openvswitch.org > Subject: Re: [ovs-discuss] mirror ports on multiple bridges - egress problem > > On Thu, Sep 15, 2016 at 11:05:58AM -0700, Ben Pfaff wrote: > > On Thu, Sep 15, 2016 at 07:52:03AM +0000, Zoltán Balogh wrote: > > > It seems that for each datapath flow rule, there can be only one mirror > > > port. > > > I presume the chosen port could depend on the processing order of output > > > ports > > > when the datapath flow is constructed. > > > Is this a planned limitation or a bug? > > > > It should be possible for a packet to be mirrored multiple times, > > whether on ingress or egress, so this sounds like a bug. Let me see if > > I can figure anything out. > > > > From your output it looks like you're working from recent master. Is > > that correct? > > Would you please test this proposed fix? I have not tested it myself, > except to see that it compiles. > > --8<--------------------------cut here-------------------------->8-- > > From: Ben Pfaff <b...@ovn.org> > Date: Thu, 15 Sep 2016 11:43:46 -0700 > Subject: [PATCH] ofproto-dpif-xlate: Fix treatment of mirrors across patch > ports. > MIME-Version: 1.0 > Content-Type: text/plain; charset=UTF-8 > Content-Transfer-Encoding: 8bit > > When the bridges on both sides of a patch port included mirrors, the > translation code incorrectly conflated them instead of treating them as > independent. > > Reported-by: Zoltán Balogh <zoltan.bal...@ericsson.com> > Reported-at: > http://openvswitch.org/pipermail/discuss/2016-September/022689.html > Signed-off-by: Ben Pfaff <b...@ovn.org> > --- > ofproto/ofproto-dpif-xlate.c | 28 +++++++++++++++++++++++++--- > 1 file changed, 25 insertions(+), 3 deletions(-) > > diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c > index 6854da3..358edd6 100644 > --- a/ofproto/ofproto-dpif-xlate.c > +++ b/ofproto/ofproto-dpif-xlate.c > @@ -2894,7 +2894,6 @@ compose_output_action__(struct xlate_ctx *ctx, > ofp_port_t ofp_port, > > ofpbuf_use_stub(&ctx->stack, new_stack, sizeof new_stack); > ofpbuf_use_stub(&ctx->action_set, actset_stub, sizeof actset_stub); > - ctx->xbridge = peer->xbridge; > flow->in_port.ofp_port = peer->ofp_port; > flow->metadata = htonll(0); > memset(&flow->tunnel, 0, sizeof flow->tunnel); > @@ -2903,6 +2902,26 @@ compose_output_action__(struct xlate_ctx *ctx, > ofp_port_t ofp_port, > ctx->conntracked = false; > clear_conntrack(flow); > > + /* When the patch port points to a different bridge, then the mirrors > + * for that bridge clearly apply independently to the packet, so we > + * reset the mirror bitmap to zero and then restore it after the > packet > + * returns. > + * > + * When the patch port points to the same bridge, this is more of a > + * design decision: can mirrors be re-applied to the packet after it > + * re-enters the bridge, or should we treat that as doubly mirroring > a > + * single packet? The former may be cleaner, since it respects the > + * model in which a patch port is like a physical cable plugged from > + * one switch port to another, but the latter may be less surprising > to > + * users. We take the latter choice, for now at least. (To use the > + * former choice, hard-code 'independent_mirrors' to "true".) */ > + mirror_mask_t old_mirrors = ctx->mirrors; > + bool independent_mirrors = peer->xbridge != ctx->xbridge; > + if (independent_mirrors) { > + ctx->mirrors = 0; > + } > + ctx->xbridge = peer->xbridge; > + > /* The bridge is now known so obtain its table version. */ > ctx->xin->tables_version > = ofproto_dpif_get_tables_version(ctx->xbridge->ofproto); > @@ -2921,10 +2940,10 @@ compose_output_action__(struct xlate_ctx *ctx, > ofp_port_t ofp_port, > * the learning action look at the packet, then drop it. */ > struct flow old_base_flow = ctx->base_flow; > size_t old_size = ctx->odp_actions->size; > - mirror_mask_t old_mirrors = ctx->mirrors; > + mirror_mask_t old_mirrors2 = ctx->mirrors; > > xlate_table_action(ctx, flow->in_port.ofp_port, 0, true, > true); > - ctx->mirrors = old_mirrors; > + ctx->mirrors = old_mirrors2; > ctx->base_flow = old_base_flow; > ctx->odp_actions->size = old_size; > > @@ -2933,6 +2952,9 @@ compose_output_action__(struct xlate_ctx *ctx, > ofp_port_t ofp_port, > } > } > > + if (independent_mirrors) { > + ctx->mirrors = old_mirrors; > + } > ctx->xin->flow = old_flow; > ctx->xbridge = xport->xbridge; > ofpbuf_uninit(&ctx->action_set); > -- > 2.1.3 > _______________________________________________ discuss mailing list discuss@openvswitch.org http://openvswitch.org/mailman/listinfo/discuss
