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

[Loftus, Ciara]

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

Thanks for the feedback. Replies inline.

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

I have included something similar to this in the new version. The configure flag is gone.

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

I've created separate netdev classes for each in the new version. One #ifdef remains, for netdev_register_provider, as only one of either vhost-cuse or vhost-user ports can be used for an instance of the switch. The rest of the #ifdefs are gone, however.