On Wed, Apr 30, 2014 at 02:22:24AM -0700, Andy Zhou wrote: > On Wed, Apr 30, 2014 at 1:35 AM, Simon Horman <ho...@verge.net.au> wrote: > > On Wed, Apr 30, 2014 at 01:10:02AM -0700, Andy Zhou wrote: > >> Simon, > >> > >> Thanks for explaining. It is now clear what the test is about. > >> > >> I spent some time today to study this. The root cause seems to be in > >> how packet out are > >> implemented, not with recirculation or bond. A reasonable solution > >> may be treating patch ports as > >> LOCAL ports for packet out. I have prototyped this solution and passed > >> the test. Any thoughts? > > > > Will this also handle other ports that are not present in the datapath > > for packet_out? In particular the CONTROLLER port? > > This is a good idea, The patch I sent out will only handle patch ports. Let me > send a v2 to address CONTROLLER as well.
Thanks, I'll see if I can test it using recirculation for MPLS. > > > > >> I will clean it up and send it out soon. > > > > Thanks. > > > >> > >> Andy > >> > >> On Tue, Apr 29, 2014 at 1:55 AM, Simon Horman <ho...@verge.net.au> wrote: > >> > On Tue, Apr 29, 2014 at 12:40:18AM -0700, Andy Zhou wrote: > >> >> If p7 and p37 are now connected together as patch ports, would any > >> >> traffic injected from p7 bypass (in the user space) the bond > >> >> interfaces all-together? > >> > > >> > I believe what happens is that the normal action forwards > >> > the packet to all ports that are connected to br0 to other than p7. > >> > That is the packet is forwarded to bond0. > >> > > >> > Without this patch, the packet is then executed in the datapath > >> > and an upcall is made as a result of a recirculation action. > >> > In handle_upcalls() the call to xlate_receive() fails because > >> > the in_port does not exist in the datapath. > >> > > >> > > >> > To be honest I'm not particularly concerned if patch-ports work in > >> > conjunction with bonding or recirculation in general. And indeed they > >> > don't > >> > even with this series applied. The purpose of this patch was to > >> > illustrate > >> > the problem that without other patches in this series recirculation > >> > doesn't > >> > work in cases where the in_port doesn't exist in the datapath. And in the > >> > case of bonding the only example I could come up with involved a > >> > patch-port. > >> > > >> > In the case of recirculation for MPLS, which is also added by this > >> > series, > >> > a more realistic example can be constructed using packet_out with > >> > CONTROLLER as the in_port. This case I am inclined to care about. > >> > Particularly as it is used extensively by Ryu's test-suite which can be > >> > run > >> > using Open vSwitch's make check-ryu target. > >> > > >> > I did not use CONTROLLER as the in_port for the bonding test as for > >> > reasons > >> > I don't fully understand packets with the CONTROLLER as the in_port do > >> > not > >> > seem to be able to be handled by a normal action. And it it my > >> > understanding that a normal action is needed to forward packets to a > >> > bond. > >> > > >> >> > >> >> On Sun, Apr 27, 2014 at 6:24 PM, Simon Horman <ho...@verge.net.au> > >> >> wrote: > >> >> > On Fri, Apr 25, 2014 at 12:46:16AM -0700, Andy Zhou wrote: > >> >> >> Simon, I thought this test exposed a bug that is fixed with this > >> >> >> patch series. However, when I apply this patch alone against current > >> >> >> master, it passed fine. (I tried many times). So I must > >> >> >> misunderstand the intention of the test, or why current master failed > >> >> >> to address > >> >> >> packet out. Would you please shed some light on this? > >> >> > > >> >> > Hi Andy, > >> >> > > >> >> > thanks for pointing that out. My intention was, as you suggest, > >> >> > to provide a test that fails without previous patches in the series. > >> >> > Unfortunately I managed to create a test that passes regardless > >> >> > of the presence of other patches in the series :( > >> >> > > >> >> > Could you please take a look at the updated test below? > >> >> > It should fail without earlier patches in the series. > >> >> > > >> >> > From: Simon Horman <ho...@verge.net.au> > >> >> > > >> >> > [PATCH] ofproto-dpif: Bonding with in_port that isn't present in the > >> >> > datapath > >> >> > > >> >> > This tests exercises execution of actions in ovs-vswitchd > >> >> > in the case where a packet is processed due to a packet out message > >> >> > with an in_port that doesn't exist in the datapath and translation > >> >> > results in recirc actions due to bonding. > >> >> > > >> >> > Signed-off-by: Simon Horman <ho...@verge.net.au> > >> >> > > >> >> > --- > >> >> > v2 > >> >> > * First Post > >> >> > > >> >> > v2.1 > >> >> > * Use a patch-port instead of NONE as the in_port for packet_out. > >> >> > This has the intended effect of causing a packet with an in_port > >> >> > that does not exist in the datapath to be output to a bonding > >> >> > interface. > >> >> > --- > >> >> > tests/ofproto-dpif.at | 90 > >> >> > +++++++++++++++++++++++++++++++++++++++++++++++++++ > >> >> > 1 file changed, 90 insertions(+) > >> >> > > >> >> > diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at > >> >> > index 89c8ad7..2113464 100644 > >> >> > --- a/tests/ofproto-dpif.at > >> >> > +++ b/tests/ofproto-dpif.at > >> >> > @@ -197,6 +197,96 @@ AT_CHECK([test `grep in_port.6 br1_flows.txt |wc > >> >> > -l` -gt 7]) > >> >> > OVS_VSWITCHD_STOP() > >> >> > AT_CLEANUP > >> >> > > >> >> > +AT_SETUP([ofproto-dpif, balance-tcp bonding, packet-out]) > >> >> > +# Create br0 with interfaces bond0(p1, p2, p3), bond1(p4,p5,p6) and > >> >> > p7, and > >> >> > +# br1 with interfaces bond10(p11, p12, p13) and > >> >> > bond11(p14,p15,p16), and > >> >> > +# br2 with interfaces bond20(p21, p22, p23) and > >> >> > bond21(p24,p25,p26) > >> >> > +# br3 with interface p37 > >> >> > +# bond0 <-> bond10 (unix socket backed interfaces) > >> >> > +# bond1 <-> bond20 (unix socket backed interfaces) > >> >> > +# p7 <-> p37 (patch interface) > >> >> > +# Send some traffic, make sure the traffic are spread based on L4 > >> >> > headers. > >> >> > +OVS_VSWITCHD_START( > >> >> > + [add-bond br0 bond0 p1 p2 p3 bond_mode=balance-tcp lacp=active \ > >> >> > + other-config:lacp-time=fast > >> >> > other-config:bond-rebalance-interval=0 --\ > >> >> > + set interface p1 type=dummy > >> >> > options:pstream=punix:$OVS_RUNDIR/p1.sock ofport_request=1 -- \ > >> >> > + set interface p2 type=dummy > >> >> > options:pstream=punix:$OVS_RUNDIR/p2.sock ofport_request=2 -- \ > >> >> > + set interface p3 type=dummy > >> >> > options:pstream=punix:$OVS_RUNDIR/p3.sock ofport_request=3 -- \ > >> >> > + add-bond br0 bond1 p4 p5 p6 bond_mode=balance-tcp lacp=active \ > >> >> > + other-config:lacp-time=fast > >> >> > other-config:bond-rebalance-interval=0 --\ > >> >> > + set interface p4 type=dummy > >> >> > options:pstream=punix:$OVS_RUNDIR/p4.sock ofport_request=4 -- \ > >> >> > + set interface p5 type=dummy > >> >> > options:pstream=punix:$OVS_RUNDIR/p5.sock ofport_request=5 -- \ > >> >> > + set interface p6 type=dummy > >> >> > options:pstream=punix:$OVS_RUNDIR/p6.sock ofport_request=6 -- \ > >> >> > + add-port br0 p7 -- \ > >> >> > + set interface p7 type=patch options:peer=p37 ofport_request=7 -- \ > >> >> > + add-br br1 -- \ > >> >> > + set bridge br1 other-config:hwaddr=aa:66:aa:66:00:00 -- \ > >> >> > + set bridge br1 datapath-type=dummy other-config:datapath-id=1234 > >> >> > fail-mode=secure -- \ > >> >> > + add-bond br1 bond10 p11 p12 p13 bond_mode=balance-tcp lacp=active > >> >> > \ > >> >> > + other-config:lacp-time=fast > >> >> > other-config:bond-rebalance-interval=0 --\ > >> >> > + set interface p11 type=dummy > >> >> > options:stream=unix:$OVS_RUNDIR/p1.sock ofport_request=1 -- \ > >> >> > + set interface p12 type=dummy > >> >> > options:stream=unix:$OVS_RUNDIR/p2.sock ofport_request=3 -- \ > >> >> > + set interface p13 type=dummy > >> >> > options:stream=unix:$OVS_RUNDIR/p3.sock ofport_request=2 -- \ > >> >> > + add-bond br1 bond11 p14 p15 p16 bond_mode=balance-tcp lacp=active > >> >> > \ > >> >> > + other-config:lacp-time=fast > >> >> > other-config:bond-rebalance-interval=0 --\ > >> >> > + set interface p14 type=dummy > >> >> > options:stream=unix:$OVS_RUNDIR/p7.sock ofport_request=4 -- \ > >> >> > + set interface p15 type=dummy > >> >> > options:stream=unix:$OVS_RUNDIR/p8.sock ofport_request=5 -- \ > >> >> > + set interface p16 type=dummy > >> >> > options:stream=unix:$OVS_RUNDIR/p9.sock ofport_request=6 -- \ > >> >> > + add-br br2 -- \ > >> >> > + set bridge br2 other-config:hwaddr=aa:66:aa:66:00:02 -- \ > >> >> > + set bridge br2 datapath-type=dummy other-config:datapath-id=1235 > >> >> > fail-mode=secure -- \ > >> >> > + add-bond br2 bond20 p21 p22 p23 bond_mode=balance-tcp lacp=active > >> >> > \ > >> >> > + other-config:lacp-time=fast > >> >> > other-config:bond-rebalance-interval=0 --\ > >> >> > + set interface p21 type=dummy > >> >> > options:stream=unix:$OVS_RUNDIR/p4.sock ofport_request=1 -- \ > >> >> > + set interface p22 type=dummy > >> >> > options:stream=unix:$OVS_RUNDIR/p5.sock ofport_request=2 -- \ > >> >> > + set interface p23 type=dummy > >> >> > options:stream=unix:$OVS_RUNDIR/p6.sock ofport_request=3 -- \ > >> >> > + add-br br3 -- \ > >> >> > + set bridge br3 other-config:hwaddr=aa:66:aa:66:00:03 -- \ > >> >> > + set bridge br3 datapath-type=dummy other-config:datapath-id=1236 > >> >> > fail-mode=secure -- \ > >> >> > + add-port br3 p37 -- \ > >> >> > + set interface p37 type=patch options:peer=p7 ofport_request=37 -- > >> >> > ]) > >> >> > +AT_CHECK([ovs-appctl netdev-dummy/set-admin-state up], 0, [OK > >> >> > +]) > >> >> > +AT_CHECK([ovs-ofctl add-flow br1 action=normal]) > >> >> > +AT_CHECK([ovs-ofctl add-flow br2 action=normal]) > >> >> > +AT_CHECK([ovs-appctl upcall/disable-megaflows], [0], [megaflows > >> >> > disabled > >> >> > +], []) > >> >> > +sleep 1; > >> >> > +ovs-appctl time/stop > >> >> > +ovs-appctl time/warp 100 > >> >> > +ovs-appctl lacp/show > lacp.txt > >> >> > +ovs-appctl bond/show > bond.txt > >> >> > + > >> >> > +ovs-appctl vlog/set dpif,file,dbg > >> >> > +#ovs-appctl vlog/set ofproto,file,dbg > >> >> > + > >> >> > +dnl The input is a TCP/IP frame which tcpdump -vve shows as: > >> >> > +dnl 60:66:66:66:00:01 > 50:54:00:00:00:07, ethertype IPv4 (0x0800), > >> >> > length 58: (tos 0x0, ttl 255, id 0, offset 0, flags [none], proto TCP > >> >> > (6), length 44) > >> >> > +dnl 192.168.0.1.80 > 192.168.0.2.$port: Flags [none], cksum > >> >> > 0x7744 (correct), seq 42:46, win 10000, length 4 > >> >> > +( > >> >> > +for i in `seq 0 255` ; > >> >> > + do > >> >> > + port=$(printf "%02x" $i) > >> >> > + ovs-ofctl -O OpenFlow13 packet-out br0 7 normal "50 54 00 00 00 > >> >> > 07 60 66 66 66 00 01 08 00 45 00 00 2c 00 00 00 00 ff 06 3a 78 c0 a8 > >> >> > 00 01 c0 a8 00 02 00 50 00 $port 00 00 00 2a 00 00 00 2a 50 00 27 10 > >> >> > 77 44 00 00 48 4f 47 45" > >> >> > +done > >> >> > +) > >> >> > + > >> >> > +ovs-appctl time/warp 100 > >> >> > +ovs-appctl time/warp 100 > >> >> > +ovs-appctl time/warp 100 > >> >> > +AT_CHECK([ovs-appctl dpif/dump-flows br1 |grep tcp > br1_flows.txt]) > >> >> > +AT_CHECK([ovs-appctl dpif/dump-flows br2 |grep tcp > br2_flows.txt]) > >> >> > +# Make sure there is resonable distribution to all three ports. > >> >> > +# We don't want to make this check precise, in case hash function > >> >> > changes. > >> >> > +AT_CHECK([test `grep in_port.11 br1_flows.txt |wc -l` -gt 7]) > >> >> > +AT_CHECK([test `grep in_port.12 br1_flows.txt |wc -l` -gt 7]) > >> >> > +AT_CHECK([test `grep in_port.13 br1_flows.txt |wc -l` -gt 7]) > >> >> > +AT_CHECK([test `grep in_port.21 br2_flows.txt |wc -l` -gt 7]) > >> >> > +AT_CHECK([test `grep in_port.22 br2_flows.txt |wc -l` -gt 7]) > >> >> > +AT_CHECK([test `grep in_port.23 br2_flows.txt |wc -l` -gt 7]) > >> >> > +OVS_VSWITCHD_STOP() > >> >> > +AT_CLEANUP > >> >> > + > >> >> > AT_SETUP([ofproto-dpif - resubmit]) > >> >> > OVS_VSWITCHD_START > >> >> > ADD_OF_PORTS([br0], [1], [10], [11], [12], [13], [14], [15], > >> >> > -- > >> >> > 1.8.5.2 > >> >> > > >> >> > >> > _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
