[ovs-dev] [PATCH] netdev-dpdk: add dpdk vhost-user ports

[Pravin Shelar]

On Tue, May 5, 2015 at 8:58 AM, Ciara Loftus <ciara.loftus at intel.com> wrote:
> This patch adds support for a new port type to the userspace
> datapath called dpdkvhostuser. It adds to the existing
> infrastructure of vhost-cuse, however disables vhost-cuse ports
> as the default port type, in favour of vhost-user ports.
> vhost-cuse 'dpdkvhost' ports are still available and can be
> enabled using a configure flag, steps for which are available
> in INSTALL.DPDK.md.
>
> A new dpdkvhostuser port will create a unix domain socket which
> when provided to QEMU is used to facilitate communication between
> the virtio-net device on the VM and the OVS port on the host.
>
> Signed-off-by: Ciara Loftus <ciara.loftus at intel.com>

Thanks for the patch I have couple of comments.

....

> diff --git a/configure.ac b/configure.ac
> index 068674e..3f635b4 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -165,6 +165,7 @@ AC_ARG_VAR(KARCH, [Kernel Architecture String])
>  AC_SUBST(KARCH)
>  OVS_CHECK_LINUX
>  OVS_CHECK_DPDK
> +OVS_CHECK_VHOST_CUSE
>  OVS_CHECK_PRAGMA_MESSAGE
>  AC_SUBST([OVS_CFLAGS])
>  AC_SUBST([OVS_LDFLAGS])

I think you can parse build/.config file in configure script and check
what type of vhost is configured for the DPDK build. We already do
this for kernel configuration. You can have look at macro
OVS_CHECK_LINUX_COMPAT in acinclude.m4. This way we can avoid new OVS
configure option.

> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
> index 5af15d4..54ead15 100644
> --- a/lib/netdev-dpdk.c
> +++ b/lib/netdev-dpdk.c
> @@ -28,6 +28,7 @@
>  #include <unistd.h>
>  #include <stdio.h>
>
> +#include "dirs.h"
>  #include "dp-packet.h"
>  #include "dpif-netdev.h"
>  #include "list.h"
> @@ -101,8 +102,18 @@ BUILD_ASSERT_DECL((MAX_NB_MBUF / ROUND_DOWN_POW2(MAX_NB_MBUF/MIN_NB_MBUF))
>
>  #define MAX_PKT_BURST 32           /* Max burst size for RX/TX */
>
> -/* Character device cuse_dev_name. */
> -char *cuse_dev_name = NULL;
> +/* For vhost-user, the path where sockets will be created.
> + * For vhost-cuse, the name of the character device. */
> +char *vhost_dev_or_sock = NULL;
> +
> +#ifdef VHOST_CUSE
> +char vhost_flag[] = "--cuse_dev_name";
> +char vhost_flag_default_val[] = "vhost-net";
> +#else
> +#define VHOST_USER
> +char vhost_flag[] = "--vhost_sock_dir";
> +char vhost_flag_default_val[PATH_MAX]; /* Initialized at runtime via ovs_rundir */
> +#endif

You can get rid of all the #ifdef by refactoring existing vhost code
and defining separate netdev_class for vost_user and vhost_cuse.