[ovs-dev] [PATCH 3/3] mirror: do not mirror packet on recirculation

Ben Pfaff blp at ovn.org
Thu Feb 1 20:47:07 UTC 2018


Thanks a lot for the extra details.

I'm having some trouble getting the setup just right so that I can
reproduce your results.  So far, I've managed to set it up, but I don't
yet see the duplication.  Can you help me by providing a full set of
commands?

On Tue, Jan 30, 2018 at 11:12:34PM +0800, Huanle Han wrote:
> Thanks for your review.
> 
> Here is a test case:
> 1. add a trunk, balance-tcp bond to a bridge
> 2. add a access port tag=xxx to same bridge
> 3. add a mirror, which mirrors all ports in vlan=xxx to another out port
> 4. send packet from access port to bond (simply use arp). as a result,
> *mirror send 2 duplicated packets to outport *
> 
> Commands for example:
> br=br1
> src_port=vnet1
> mirror_port=vnet12
> 
> ovs-vsctl --if-exists del-port $mirror_port -- add-port $br $mirror_port
> ovs-vsctl --if-exists del-port $src_port -- add-port $br $src_port tag=199
> 
> ovs-vsctl -- set Bridge $br mirrors=@m --  \
>     --id=@p0 get Port $mirror_port -- \
>     --id=@m create Mirror select_all=true name=mm select_vlan=199
> output_port=@p0
> 
> Result of datapath flows: 11,which is the mirror outport, is outputed twice.
> recirc_id(0x2),dp_hash(0xb8/0xff),in_port(10),eth_type(0x8100),vlan(vid=199),encap(eth_type(0x0806),
> packets:9, bytes:414, used:0.605s, actions:9,11
> recirc_id(0),in_port(10),eth(src=fa:da:41:1d:30:0e,dst=ff:ff:ff:ff:ff:ff),eth_type(0x0806),arp(sip=7.7.7.18,tip=7.7.7.192,op=1/0xff),
> packets:9, bytes:378, used:0.604s,
> actions:push_vlan(vid=199,pcp=0),11,pop_vlan,7,push_vlan(vid=199,pcp=0),12,hash(hash_l4(0),recirc(0x2)
> 
> About patch:
> I didn't notice 'frozen_state' thing util you pointed it out. And
> after review the code, I think ovs doesn't save the mirror information
> in frozen_state for bond recirc. My old patch is naive. I would
> appreciate it if you would fix it.
> 
> 
> On Thu, Jan 25, 2018 at 3:30 AM, Ben Pfaff <blp at ovn.org> wrote:
> > On Wed, Jan 24, 2018 at 09:41:12AM -0800, Ben Pfaff wrote:
> >> From: Huanle Han <hanxueluo at gmail.com>
> >>
> >> Signed-off-by: Huanle Han <hanxueluo at gmail.com>
> >> ---
> >>  ofproto/ofproto-dpif-xlate.c | 4 ++++
> >>  1 file changed, 4 insertions(+)
> >>
> >> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> >> index 9d6ca94afc82..23938c8c8cf3 100644
> >> --- a/ofproto/ofproto-dpif-xlate.c
> >> +++ b/ofproto/ofproto-dpif-xlate.c
> >> @@ -1931,6 +1931,10 @@ mirror_packet(struct xlate_ctx *ctx, struct xbundle *xbundle,
> >>          return;
> >>      }
> >>
> >> +    if (ctx->xin->flow.recirc_id != 0) {
> >> +        return;
> >> +    }
> >> +
> >
> > Can you help me understand what cases this addresses?  The frozen_state
> > that comes along with a recirculation should keep track of what mirrors
> > have already been output, which should prevent duplicate mirroring on
> > recirculation.  If it doesn't work in every case, then probably we
> > should address that instead of just disabling mirroring on
> > recirculation.
> >
> > Thanks,
> >
> > Ben.


More information about the dev mailing list