[ovs-dev] question about mirror code

Ben Pfaff blp at ovn.org
Fri Feb 5 23:31:22 UTC 2016


On Tue, Jan 26, 2016 at 12:13:08AM +0800, Huanle Han wrote:
> 2016-01-25 1:00 GMT+08:00 Ben Pfaff <blp at ovn.org>:
> 
> > On Sun, Jan 24, 2016 at 11:59:57PM +0800, Huanle Han wrote:
> > > I think a more line "mirrors &= ~ctx->mirrors;" is needed at the end of
> > the
> > > "while" loop.
> > > Otherwise, duplicated mirror action may be applied in recursive
> > > mirror_packet called by output_normal.
> >
> > Did you arrive at that conclusion from testing or code inspection?  I
> > believe that dup_mirrors always includes the mirror we're handling, so
> > that "mirrors &= ~dup_mirrors;" in the while loop should include that
> > case; also note the "mirrors &= ~ctx->mirrors;" at the beginning of the
> > function.
> >
> > The test case 2 confirm this conclusion. Suppose we have 2 mirrors A and B
> (they are not duplicated). "mirror_packet" may be called recursively:
>  mirror_packet()
>     output_normal(ctx, xbundle, out_vlan)
>         ......
>             mirror_packet()
>  when it's iterating mirror A on top "mirror_packet", mirror B is applied
> in the recursive "mirror_packet". So we should reject the mirror B in top
> mirror_packet on next iteration.
>  "mirrors &= ~ctx->mirrors;" at the beginning of the function or "mirrors
> &= ~dup_mirrors;" in while loop could not include this case.
> 
> > Here are the test cases:
> > >
> > > Test case for the bug I mentioned in previous mail:
> > > 1. setup port:  vnet0 tag = 201, vnet1 tag=200
> > > 2. setup flow: add-flow in_port=vnet0,actions:vnet1
> > > 3. setup mirror( mirror1 or mirror2 ). and packet from vnet0 to vnet1
> > >    a) mirror1:  select_all=false, select_src_port=[         ],
> > > select_dst_port=[vnet1], select_vlan=[200], output=vnet2
> > >        ==> result: this mirror works.
> > >    b) mirror2:  select_all=false, select_src_port=[vnet0],
> > > select_dst_port=[vnet1], select_vlan=[200], output=vnet2
> > >        ==> result: this mirror doesn't work.
> > >
> > > Test case for the bug I mentioned in this mail:
> > > 1.  set port: vnet0, vnet1, vnet2
> > > 2.  setup 2 mirrors:
> > >     mirror1:  select_all=true,output_vlan=500
> > >     mirror2:  select_all=true,output_vlan=501
> > > 3. packet from vnet0, result: duplicated mirror actions are generated.
> >
> > Do you mean that you ran these test cases with the patch I supplied, and
> > saw this bug?
> >
> 
> Yes, I ran them with your patch.
> I think the patch have fixed the problem in test case 1.
> But It leads to a new bug(test case 2). That's why I saied "mirrors &=
> ~ctx->mirrors;" is needed.

OK, I understand now.  I sent a 3-patch series to fix it.  You are
probably most interested in patch 2:
        https://patchwork.ozlabs.org/patch/579676/



More information about the dev mailing list