----- Original Message ----- > From: "Aaron Conole" <acon...@redhat.com> > To: "Bala Sankaran" <bsank...@redhat.com> > Cc: "Tiago Lam" <tiago....@intel.com>, d...@openvswitch.org > Sent: Wednesday, 29 August, 2018 5:14:41 PM > Subject: Re: [ovs-dev] [PATCH v3 6/6] system-dpdk: Connect network namespaces > via dpdkvhostuser ports > > Bala Sankaran <bsank...@redhat.com> writes: > > > ----- Original Message ----- > >> From: "Tiago Lam" <tiago....@intel.com> > >> To: "Bala Sankaran" <bsank...@redhat.com>, d...@openvswitch.org > >> Sent: Wednesday, 29 August, 2018 1:36:13 PM > >> Subject: Re: [ovs-dev] [PATCH v3 6/6] system-dpdk: Connect network > >> namespaces via dpdkvhostuser ports > >> > >> Hi Bala, > >> > >> Thanks to both you and Aaron for working on this. Seems to be a great > >> addition. > >> > >> As a general comment I agree with Ian that running everything on v17.11 > >> would be preferable, as this would enable us to run this test on any > >> given system, and not only when v18.11 is installed. But after reading > >> through your thread on the DPDK users list on the 2MB hugepages > >> limitations around virtio_user, it seems this will have to be a > >> dependency until OvS-DPDK moves to v18.11. > > Hello Tiago, > > > > I agree, I did not happen to notice a workaround for this. > > > >> > >> On 28/08/2018 18:47, Bala Sankaran wrote: > >> > This adds a new test to the 'check-dpdk' subsystem that will exercise > >> > allocations, PMDs, and the vhost-user code path. > >> > > >> > Signed-off-by: Bala Sankaran <bsank...@redhat.com> > >> > Co-authored-by: Aaron Conole <acon...@redhat.com> > >> > Signed-off-by: Aaron Conole <acon...@redhat.com> > >> > --- > >> > tests/system-dpdk.at | 77 ++++++++++++++++++++++++++++++++++++++++++++ > >> > 1 file changed, 77 insertions(+) > >> > > >> > diff --git a/tests/system-dpdk.at b/tests/system-dpdk.at > >> > index 58dc8aaae..914a1b644 100644 > >> > --- a/tests/system-dpdk.at > >> > +++ b/tests/system-dpdk.at > >> > @@ -1,3 +1,6 @@ > >> > +m4_define([CONFIGURE_VETH_OFFLOADS], > >> > + [AT_CHECK([ethtool -K $1 tx off], [0], [ignore], [ignore])]) > >> > + > >> > AT_BANNER([OVS-DPDK unit tests]) > >> > > >> > dnl > >> > > >> > -------------------------------------------------------------------------- > >> > @@ -74,3 +77,77 @@ OVS_VSWITCHD_STOP(["\@does not exist. The Open > >> > vSwitch > >> > kernel module is probably > >> > \@EAL: No free hugepages reported in hugepages-1048576kB@d"]) > >> > AT_CLEANUP > >> > dnl > >> > > >> > -------------------------------------------------------------------------- > >> > + > >> > + > >> > + > >> > +dnl > >> > -------------------------------------------------------------------------- > >> > +dnl Ping vhost-user-client port > >> > +AT_SETUP([OVS-DPDK datapath - ping vhost-user-client ports]) > > @bala: > > This is wrong here. These are vhost-user ports, not vhost-user-client ports.
Sorry about that, I will make the correction. > > >> > >> Any reason why you're using vhost-user instead of vhost-user-client? If > >> we change it to "type=dpdkvhostuserclient" in the vhu0 interface added > >> to OvS and append ",server=1" to the net_virtio_user --vdev in the > >> testpmd arguments, doesn't it just work the same? > > > > I believe I encountered an error while running the tests with a vhost-user > > client ports. That's when I switched over to vhost-user instead. I do not > > remember the error at this moment, but now that you mentioned it, I am > > thinking > > of adding another unit test that use vhost-user-client port which would > > give > > me the error and then skip to vhost-user instead. > > I also don't remember which error we hit and whether it had anything to > do with the type of port. Maybe it makes sense to have both. That way > we cover both. And if we ever completely remove the server mode ports, > we can just drop the test as well (I like that it helps us also catch > the cleanup error in that case). > > >> > >> > +AT_KEYWORDS([dpdk]) > >> > +OVS_DPDK_PRE_CHECK() > >> > +OVS_DPDK_START() > >> > + > >> > +dnl Add userspace bridge and attach it to OVS > >> > +AT_CHECK([ovs-vsctl add-br br10 -- set bridge br10 > >> > datapath_type=netdev]) > >> > +AT_CHECK([ovs-vsctl add-port br10 vhu0 -- set Interface vhu0 \ > >> > + type=dpdkvhostuser], [], > >> > + [stdout], [stderr]) > >> > +AT_CHECK([ovs-vsctl show], [], [stdout]) > >> > + > >> > +dnl Parse log file > >> > +AT_CHECK([grep "VHOST_CONFIG: vhost-user server: socket created" \ > >> > + ovs-vswitchd.log], [], [stdout]) > >> > +AT_CHECK([grep "Socket $OVS_RUNDIR/vhu0 created for vhost-user port > >> > vhu0" > >> > \ > >> > + ovs-vswitchd.log], [], [stdout]) > >> > +AT_CHECK([grep "VHOST_CONFIG: bind to $OVS_RUNDIR/vhu0" > >> > ovs-vswitchd.log], > >> > [], > >> > + [stdout]) > >> > + > >> > +dnl Set up namespaces > >> > +ADD_NAMESPACES(ns1, ns2) > >> > + > >> > +dnl execute testpmd in background > >> > +on_exit "pkill -f -x -9 'tail -f /dev/null'" > >> > +tail -f /dev/null | testpmd --socket-mem=512 \ > >> > + --vdev="net_virtio_user,path=$OVS_RUNDIR/vhu0" \ > >> > + --vdev="net_tap0,iface=tap0" --file-prefix page0 \ > >> > + --single-file-segments -- -a >$OVS_RUNDIR/testpmd-vhu0.log > >> > 2>&1 > >> > & > >> > + > >> > >> I've seen your reply to Ian's comment on the $PATH environment variable > >> on v2; That could be enough if there wasn't a requirement for DPDK > >> v18.11 for the `testpmd` bin. Since there is a separate environment > >> variable will be needed to guarantee we're executing the correct one > >> (and not the v17.11 that's currently linked with OvS, as an example). It > >> would also enable us to detect if the variable is set or not, and if > >> not, skip the test altogether. At the moment the test will just fail, if > >> the binary doesn't exist. > > > > I do not have complete familiarity on this note, I have copied Aaron along > > in this email. I suppose that he would be able to comment on this. > > I think it's okay to just use PATH, and rely on the path lookup order. > > After all, it's quite possible to have a different environment for > testpmd and put that in the path, and it should link to the correct > library versions. > > PATH=/path/to/dpdk-18.11:$PATH > > I'm hesitant to add a workaround to a problem that will disappear once > the next LTS comes along and gets integrated (since we'll need to then > revert that workaround, or deal with it appropriately). I guess Ian has > the final say on what he wants to accept into the tree for the dpdk > checks, but I like using the path in this case (and we were able to use > it even with this complicated environment). > > I do agree we shouldn't fail for missing testpmd - we should 'skip' > > @bala: > > Please add to this test: > > AT_SKIP_IF([! which testpmd >/dev/null 2>/dev/null]) Sure! > > >> > >> > >> > +dnl add veth device > >> > +ADD_VETH(tap1, ns2, br10, "172.31.110.12/24") > >> > >> The ADD_VETH() macro skips the test if $1 already exists. It would be > >> better to set up tap1 at the beginning, thus skipping the test if it > >> can't b set up, and this way it wouldn't be starting the `testpmd` > >> process unnecessarily. > > > > I had given it some thought too. I will test it out at my end and make the > > change if deemed necessary. > > > >> > >> > + > >> > +dnl give settling time to the testpmd processes - NOTE: this is bad > >> > form. > >> > +sleep 10 > >> > + > >> > +dnl move the tap devices to the namespaces > >> > +AT_CHECK([ps aux | grep testpmd], [], [stdout], [stderr]) > >> > +AT_CHECK([ip link show], [], [stdout], [stderr]) > >> > +AT_CHECK([ip link set tap0 netns ns1], [], [stdout], [stderr]) > >> > + > >> > +AT_CHECK([ip netns exec ns1 ip link show], [], [stdout], [stderr]) > >> > +AT_CHECK([ip netns exec ns1 ip link show | grep tap0], [], [stdout], > >> > [stderr]) > >> > +AT_CHECK([ip netns exec ns1 ip link set tap0 up], [], [stdout], > >> > [stderr]) > >> > +AT_CHECK([ip netns exec ns1 ip addr add 172.31.110.11/24 dev tap0], [], > >> > + [stdout], [stderr]) > >> > + > >> > +AT_CHECK([ip netns exec ns1 ip link show], [], [stdout], [stderr]) > >> > +AT_CHECK([ip netns exec ns2 ip link show], [], [stdout], [stderr]) > >> > +AT_CHECK([ip netns exec ns1 arping -c 4 -I tap0 172.31.110.12], [], > >> > [stdout], > >> > + [stderr]) > >> > >> Any specific requirement on arping? I for one didn't have it installed > >> on my system. > > > > Agreed, we could either replace arping with ping, or rather add > > functionality to install arping. The former would be preferred as > > the user might not want the tests to install tools unnecessarily. > > The tests shouldn't install anything. I think switching to ping is > fine, as it should be more commonly available. Sure, will change it to ping. Best, Bala. > > > I am glad you noticed our work. I appreciate your feedback and I > > look to working towards it. > > > > Thanks, > > Bala. > > > >> > >> > >> Tiago. > >> > >> > + > >> > +dnl clean up the testpmd now > >> > +pkill -f -x -9 'tail -f /dev/null' > >> > + > >> > +dnl Clean up > >> > +AT_CHECK([ovs-vsctl del-port br10 vhu0], [], [stdout], [stderr]) > >> > +OVS_VSWITCHD_STOP(["\@does not exist. The Open vSwitch kernel module is > >> > probably not loaded.@d > >> > +\@Failed to enable flow control@d > >> > +\@VHOST_CONFIG: recvmsg failed@d > >> > +\@VHOST_CONFIG: failed to connect to $OVS_RUNDIR/vhu0: No such file or > >> > directory@d > >> > +\@Global register is changed during@d > >> > +\@dpdkvhostuser ports are considered deprecated; please migrate to > >> > dpdkvhostuserclient ports.@d > >> > +\@failed to enumerate system datapaths: No such file or directory@d > >> > +\@EAL: Invalid NUMA socket, default to 0@d > >> > +\@EAL: WARNING: cpu flags constant_tsc=yes nonstop_tsc=no -> using > >> > unreliable clock cycles !@d > >> > +\@EAL: No free hugepages reported in hugepages-1048576kB@d"]) > >> > +AT_CLEANUP > >> > +dnl > >> > -------------------------------------------------------------------------- > >> > > >> > _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
