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
