[ovs-dev] [PATCH RFC v2 1/1] netdev-dpdk: add dpdk vhost-user ports

[Loftus, Ciara]

> On 04/24/2015 04:01 PM, Flavio Leitner wrote:
> > On Fri, 24 Apr 2015 14:17:17 +0300
> > Panu Matilainen <pmatilai at redhat.com> wrote:
> >
> >> Hi,
> >>
> >> A few comments inline...
> >>
> >> On 04/21/2015 01:10 PM, Ciara Loftus 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. Refer
> >>> to the documentation for enabling vhost-cuse ports if desired.
> >>>
> >>> 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>
> > [...]
> >
> >>> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
> >>> index f69154b..deb8b83 100644
> >>> --- a/lib/netdev-dpdk.c
> >>> +++ b/lib/netdev-dpdk.c
> >>> @@ -101,8 +101,13 @@ 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 */
> >>>
> >>> +#ifdef VHOST_CUSE
> >>>    /* Character device cuse_dev_name. */
> >>>    char *cuse_dev_name = NULL;
> >>> +#else
> >>> +#define VHOST_USER_PORT_SOCK_PATH "/tmp/%s" /* Socket
> Location
> >>> Template */
> >>
> >> Using /tmp for these seems like asking for trouble to me, how about
> >> somewhere /var/run? At least it should be configurable via cli,
> >> similar to the cuse device name.
> >
> > Why using /tmp would be problematic?
> 
> Because it opens up a whole can of worms that is best avoided since
> there's no good reason to open it in this case, AFAICS. Predictable
> names in a world-writable directory and all, and these are not really
> temporary files in the usual sense anyway.
> 
> Current rte_vhost_driver_register() will unconditionally nuke any file
> in the path passed to it. I'm sure you can imagine some unwanted
> scenarios if 'ovs-vsctl add-port' goes around deleting files in a
> world-shared directory :)
> 
> >
> > Anyway, systemd has the RunTimeDirectory= option to make sure the
> > private directory is created (under /run) when the service is
> > started and that's the default RPM %{_rundir} where the sock.db, .pid
> > files and other OVS files are created. It seems you could get the right
> > path using ovs_rundir().
> 
> Right.
> 
> 	- Panu -
> 
> >
> > fbl
> >

Thanks for the feedback, I appreciate the input. The latest revision of the patch sets the default socket location to the return value of ovs_rundir(). The user can also specify an alternate socket location using the --vhost-sock-dir flag on the ovs-vswitchd command line. 

Thanks,
Ciara