Thanks for the patch, I have a couple of comments:
DPDK 16.04 enables by default checksum offloads and TSO for vhostuser
device. While this seem to work ok, there seem to be a few problems with
this:
* OVS in userspace assumes that a packet is stored using a single mbuf
(it is not aware of the 'next' member or of the difference between
'data_len' and 'pkt_len')
* Most of the code in the userspace datapath is unaware of checksum
offloads. We can easily lose the offload info and send a packet with the
wrong checksum to netdev-linux, for example.
Fixing this requires some extra work on the OVS side, which we should do
eventually, but for the moment I think we should disable offloads with
something like this:
diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
index e6aac8f..8ecd85b 100644
--- a/lib/netdev-dpdk.c
+++ b/lib/netdev-dpdk.c
@@ -2240,6 +2240,9 @@ static int
dpdk_vhost_class_init(void)
{
rte_vhost_driver_callback_register(&virtio_net_device_ops);
+ rte_vhost_feature_disable(1ULL << VIRTIO_NET_F_HOST_TSO4
+ | 1ULL << VIRTIO_NET_F_HOST_TSO6
+ | 1ULL << VIRTIO_NET_F_CSUM);
ovs_thread_create("vhost_thread", start_vhost_loop, NULL);
return 0;
}
One more comment inline, otherwise I'm happy with the patch
2016-04-13 2:41 GMT-07:00 mweglicx <michalx.wegli...@intel.com>:
> Following changes are applied:
> - INSTALL.DPDK.md: CONFIG_RTE_BUILD_COMBINE_LIBS step has been
> removed because it is no longer present in DPDK configuration
> (combined library is created by default),
> - INSTALL.DPDK.md: VHost Cuse configuration is updated,
> - netdev-dpdk.c: Link speed definition is changed in DPDK and
> netdev_dpdk_get_features is updated accordingly,
> - .travis/linux-build.sh: DPDK version is updated and legacy
> flags have been removed in configuration.
>
> Signed-off-by: Michal Weglicki <michalx.wegli...@intel.com>
> Signed-off-by: Panu Matilainen <pmati...@redhat.com>
>
> v1->v2
> - link autonegotiation check is corrected.
> ---
> .travis/linux-build.sh | 5 +----
> INSTALL.DPDK.md | 23 +++++++++--------------
> lib/netdev-dpdk.c | 24 ++++++++++++------------
> 3 files changed, 22 insertions(+), 30 deletions(-)
>
> diff --git a/.travis/linux-build.sh b/.travis/linux-build.sh
> index ef712d0..a3c8e6e 100755
> --- a/.travis/linux-build.sh
> +++ b/.travis/linux-build.sh
> @@ -49,9 +49,6 @@ function install_dpdk()
> cd dpdk-$1
> fi
> find ./ -type f | xargs sed -i
> 's/max-inline-insns-single=100/max-inline-insns-single=400/'
> - sed -ri 's,(CONFIG_RTE_BUILD_COMBINE_LIBS=).*,\1y,'
> config/common_linuxapp
> - echo 'CONFIG_RTE_BUILD_FPIC=y' >>config/common_linuxapp
> - sed -ri '/EXECENV_CFLAGS = -pthread -fPIC/{s/$/\nelse ifeq
> ($(CONFIG_RTE_BUILD_FPIC),y)/;s/$/\nEXECENV_CFLAGS = -pthread -fPIC/}'
> mk/exec-env/linuxapp/rte.vars.mk
>
I'm really glad that we can get rid of the COMBINE_LIBS configuration
option.
I think the other two lines are still needed, because (for testing
purposes) we want to link DPDK into libopenvswitch.so.
> make config CC=gcc T=x86_64-native-linuxapp-gcc
> make CC=gcc RTE_KERNELDIR=$KERNELSRC
> echo "Installed DPDK source in $(pwd)"
> @@ -69,7 +66,7 @@ fi
>
> if [ "$DPDK" ]; then
> if [ -z "$DPDK_VER" ]; then
> - DPDK_VER="2.2.0"
> + DPDK_VER="16.04"
> fi
> install_dpdk $DPDK_VER
> if [ "$CC" = "clang" ]; then
> diff --git a/INSTALL.DPDK.md b/INSTALL.DPDK.md
> index 9ec8bf6..8c8cd4c 100644
> --- a/INSTALL.DPDK.md
> +++ b/INSTALL.DPDK.md
> @@ -16,7 +16,7 @@ OVS needs a system with 1GB hugepages support.
> Building and Installing:
> ------------------------
>
> -Required: DPDK 2.2
> +Required: DPDK 16.04
> Optional (if building with vhost-cuse): `fuse`, `fuse-devel`
> (`libfuse-dev`
> on Debian/Ubuntu)
>
> @@ -24,16 +24,11 @@ on Debian/Ubuntu)
> 1. Set `$DPDK_DIR`
>
> ```
> - export DPDK_DIR=/usr/src/dpdk-2.2
> + export DPDK_DIR=/usr/src/dpdk-16.04
> cd $DPDK_DIR
> ```
>
> - 2. Update `config/common_linuxapp` so that DPDK generate single lib
> file.
> - (modification also required for IVSHMEM build)
> -
> - `CONFIG_RTE_BUILD_COMBINE_LIBS=y`
> -
> - Then run `make install` to build and install the library.
> + 2. Then run `make install` to build and install the library.
> For default install without IVSHMEM:
>
> `make install T=x86_64-native-linuxapp-gcc DESTDIR=install`
> @@ -81,7 +76,7 @@ Using the DPDK with ovs-vswitchd:
>
> 1. Setup system boot
> Add the following options to the kernel bootline:
> -
> +
> `default_hugepagesz=1GB hugepagesz=1G hugepages=1`
>
> 2. Setup DPDK devices:
> @@ -496,7 +491,7 @@ the vswitchd.
> DPDK vhost:
> -----------
>
> -DPDK 2.2 supports two types of vhost:
> +DPDK 16.04 supports two types of vhost:
>
> 1. vhost-user
> 2. vhost-cuse
> @@ -517,7 +512,7 @@ with OVS.
> DPDK vhost-user Prerequisites:
> -------------------------
>
> -1. DPDK 2.2 with vhost support enabled as documented in the "Building and
> +1. DPDK 16.04 with vhost support enabled as documented in the "Building
> and
> Installing section"
>
> 2. QEMU version v2.1.0+
> @@ -635,10 +630,10 @@ with OVS.
> DPDK vhost-cuse Prerequisites:
> -------------------------
>
> -1. DPDK 2.2 with vhost support enabled as documented in the "Building and
> +1. DPDK 16.04 with vhost support enabled as documented in the "Building
> and
> Installing section"
> As an additional step, you must enable vhost-cuse in DPDK by setting
> the
> - following additional flag in `config/common_linuxapp`:
> + following additional flag in `config/common_base`:
>
> `CONFIG_RTE_LIBRTE_VHOST_USER=n`
>
> @@ -938,7 +933,7 @@ Restrictions:
> this with smaller page sizes.
>
> Platform and Network Interface:
> - - By default with DPDK 2.2, a maximum of 64 TX queues can be used with
> an
> + - By default with DPDK 16.04, a maximum of 64 TX queues can be used
> with an
> Intel XL710 Network Interface on a platform with more than 64 logical
> cores. If a user attempts to add an XL710 interface as a DPDK port
> type to
> a system as described above, an error will be reported that
> initialization
> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
> index e09b471..adf1d13 100644
> --- a/lib/netdev-dpdk.c
> +++ b/lib/netdev-dpdk.c
> @@ -1740,35 +1740,35 @@ netdev_dpdk_get_features(const struct netdev
> *netdev_,
> link = dev->link;
> ovs_mutex_unlock(&dev->mutex);
>
> - if (link.link_duplex == ETH_LINK_AUTONEG_DUPLEX) {
> - if (link.link_speed == ETH_LINK_SPEED_AUTONEG) {
> - *current = NETDEV_F_AUTONEG;
> - }
> - } else if (link.link_duplex == ETH_LINK_HALF_DUPLEX) {
> - if (link.link_speed == ETH_LINK_SPEED_10) {
> + if (link.link_duplex == ETH_LINK_HALF_DUPLEX) {
> + if (link.link_speed == ETH_SPEED_NUM_10M) {
> *current = NETDEV_F_10MB_HD;
> }
> - if (link.link_speed == ETH_LINK_SPEED_100) {
> + if (link.link_speed == ETH_SPEED_NUM_100M) {
> *current = NETDEV_F_100MB_HD;
> }
> - if (link.link_speed == ETH_LINK_SPEED_1000) {
> + if (link.link_speed == ETH_SPEED_NUM_1G) {
> *current = NETDEV_F_1GB_HD;
> }
> } else if (link.link_duplex == ETH_LINK_FULL_DUPLEX) {
> - if (link.link_speed == ETH_LINK_SPEED_10) {
> + if (link.link_speed == ETH_SPEED_NUM_10M) {
> *current = NETDEV_F_10MB_FD;
> }
> - if (link.link_speed == ETH_LINK_SPEED_100) {
> + if (link.link_speed == ETH_SPEED_NUM_100M) {
> *current = NETDEV_F_100MB_FD;
> }
> - if (link.link_speed == ETH_LINK_SPEED_1000) {
> + if (link.link_speed == ETH_SPEED_NUM_1G) {
> *current = NETDEV_F_1GB_FD;
> }
> - if (link.link_speed == ETH_LINK_SPEED_10000) {
> + if (link.link_speed == ETH_SPEED_NUM_10G) {
> *current = NETDEV_F_10GB_FD;
> }
> }
>
> + if (link.link_autoneg) {
> + *current |= NETDEV_F_AUTONEG;
> + }
> +
> return 0;
> }
>
> --
> 1.9.3
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
