[ovs-dev] [PATCH v3 6/6] system-dpdk: Connect network namespaces via dpdkvhostuser ports

Wed Aug 29 21:14:41 UTC 2018

Bala Sankaran <bsankara at redhat.com> writes:

> ----- Original Message -----
>> From: "Tiago Lam" <tiago.lam at intel.com>
>> To: "Bala Sankaran" <bsankara at redhat.com>, dev at 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 <bsankara at redhat.com>
>> > Co-authored-by: Aaron Conole <aconole at redhat.com>
>> > Signed-off-by: Aaron Conole <aconole at 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 at 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.

>> 
>> 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])

>> 
>> 
>> > +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.

> 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. at d
>> > +\@Failed to enable flow control at d
>> > +\@VHOST_CONFIG: recvmsg failed at d
>> > +\@VHOST_CONFIG: failed to connect to $OVS_RUNDIR/vhu0: No such file or
>> > directory at d
>> > +\@Global register is changed during at d
>> > +\@dpdkvhostuser ports are considered deprecated;  please migrate to
>> > dpdkvhostuserclient ports. at d
>> > +\@failed to enumerate system datapaths: No such file or directory at d
>> > +\@EAL:   Invalid NUMA socket, default to 0 at d
>> > +\@EAL: WARNING: cpu flags constant_tsc=yes nonstop_tsc=no -> using
>> > unreliable clock cycles !@d
>> > +\@EAL: No free hugepages reported in hugepages-1048576kB at d"])
>> > +AT_CLEANUP
>> > +dnl
>> > --------------------------------------------------------------------------
>> > 
>> 