[ovs-dev] [PATCH v8 4/4] system-dpdk: add negotiation check for userspace-tso

Ilya Maximets i.maximets at ovn.org
Wed Oct 21 14:51:35 UTC 2020


On 7/9/20 6:48 PM, Gowrishankar Muthukrishnan wrote:
> This patch adds minimal check for userspace-tso in system-dpdk
> tests, starting with verification on virtio negotiation.
> 
> Signed-off-by: Gowrishankar Muthukrishnan <gmuthukr at redhat.com>
> ---
> v8:
>  - split patch into logical units of changes (as this series)
> 
> ---
>  tests/system-dpdk.at | 280 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 280 insertions(+)
> 
> diff --git a/tests/system-dpdk.at b/tests/system-dpdk.at
> index 623bd34..ef03fb2 100644
> --- a/tests/system-dpdk.at
> +++ b/tests/system-dpdk.at
> @@ -245,3 +245,283 @@ 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 validate negotiation when both ovs and testpmd have tso on

This is not a useful comment.  It just repeats the test name.

> +AT_SETUP([OVS-DPDK - validate negotiation when both ovs and testpmd have tso on])
> +AT_KEYWORDS([dpdk])

This set of keywords is strange.  I'm already running 'make check-dpdk'.
'dpdk' keyword doesn't make much sense here.  Should it be 'tso' instead?

> +OVS_DPDK_PRE_CHECK()
> +AT_SKIP_IF([! which testpmd >/dev/null 2>/dev/null])
> +OVS_DB_START()
> +AT_CHECK([ovs-vsctl --no-wait set Open_vSwitch . other_config:userspace-tso-enable=true])
> +OVS_DPDK_START()
> +AT_CHECK([grep -c 'Userspace TCP Segmentation Offloading support enabled' \
> +          ovs-vswitchd.log],[ignore],[dnl
> +1
> +])

This looks racy.  I think, it's better to use OVS_WAIT_UNTIL here.
Also, it's better to just check that grep succeded instead of checking
the value of counter.

> +dnl Find number of sockets
> +SET_NUMA_NODE([512])

This should not be needed.

> +
> +dnl Add userspace bridge and attach it to OVS
> +AT_CHECK([ovs-vsctl add-br br10 -- set bridge br10 datapath_type=netdev])
> +
> +dnl Add vhostuser port (client mode)
> +AT_CHECK([ovs-vsctl add-port br10 dpdkvhostuserclient0 -- set Interface \
> +          dpdkvhostuserclient0 \
> +          type=dpdkvhostuserclient \
> +          options:vhost-server-path=$OVS_RUNDIR/dpdkvhostclient0], [],
> +         [stdout], [stderr])
> +AT_CHECK([ovs-vsctl show], [], [stdout])
> +
> +dnl Execute testpmd in background
> +on_exit "pkill -f -x -9 'tail -f /dev/null'"
> +AT_CHECK([echo "show device info all" > CMDFILE])
> +AT_CHECK([echo "stop" >> CMDFILE])
> +AT_CHECK([echo "port stop 0" >> CMDFILE])
> +AT_CHECK([echo "tso set 1500 0" >> CMDFILE])
> +AT_CHECK([echo "csum set tcp hw 0" >> CMDFILE])
> +AT_CHECK([echo "port start 0" >> CMDFILE])
> +AT_CHECK([echo "start" >> CMDFILE])
> +AT_CHECK([echo "show port 0 tx_offload capabilities" >> CMDFILE])
> +AT_CHECK([echo "show port 0 tx_offload configuration" >> CMDFILE])

This should be AT_DATA.

> +tail -f /dev/null | testpmd --socket-mem="$(cat NUMA_NODE)" --no-pci\

-socket-mem should not be needed.


And, basically, all the same comments for all the tests below.
Actually, all these tests are mostly copies of each other, it should be
better to have a single function called 4 times with different arguments.

Best regards, Ilya Maximets.


More information about the dev mailing list