[ovs-dev] [PATCH v5 1/1] netdev-dpdk: add dpdk vhost ports

Tahhan, Maryam maryam.tahhan at intel.com
Thu Nov 13 06:14:25 UTC 2014


Hi Pravin, 
Thanks for the detailed review, comments inline.
One other issue is a dependency on Socket 0 for vhost. This was found by J.B. Broccard <j.b.broccard at oracle.com> so this needs to be addressed also.

Thanks
Maryam
> -----Original Message-----
> From: Pravin Shelar [mailto:pshelar at nicira.com]
> Sent: Tuesday, October 21, 2014 12:00 AM
> To: Tahhan, Maryam
> Cc: dev at openvswitch.org
> Subject: Re: [ovs-dev] [PATCH v5 1/1] netdev-dpdk: add dpdk vhost ports
> 
> On Mon, Sep 29, 2014 at 10:10 AM, maryam.tahhan
> <maryam.tahhan at intel.com> wrote:
> > This patch implements the vhost-net offload API.  It adds support for
> > a new port type to userspace datapath called dpdkvhost. This allows
> > KVM
> > (QEMU) to offload the servicing of virtio-net devices to it's
> > associated dpdkvhost port. Instructions for use are in INSTALL.DPDK.
> >
> > This has been tested on Intel multi-core platforms and with clients
> > that have virtio-net interfaces.
> >
> > ver 5:
> >   - rebased against latest master
> > ver 4:
> >   - added eventfd_link.h and eventfd_link.c to EXTRA_DIST in
> utilities/automake.mk
> >   - rebased with master to work with DPDK 1.7 ver 3:
> >   - rebased with master
> > ver 2:
> >   - rebased with master
> >
> > Signed-off-by: maryam.tahhan <maryam.tahhan at intel.com>
> > ---
> Thanks for the patch, I have following comments.
> - NON_PMD_THREAD_TX_QUEUE is not used in the patch.
> - why do we need to limit MAX_BASENAME_SZ to 12
No, I suppose we can make this variable, at the moment we were testing with vhost-net and usvhost-i where I is an integer, but there's no reason we couldn't extend that.
> - dev_basename[] should be configurable at run time or it should be vswitchd
> parameter.
This can be added
> - any reason for liming MAX_PKT_BURST to 32?
 this is the ideal burstsize for a guest running virtio-net driver.
> - netdev_dpdk->type should be a enum type with DPDK and VHOST members.
> I am not sure if you really need type member in netdev_dpdk. you can just
> define separate device ops for DPDK and VHOST of you was to do special
> processing for each device.
> - there is no check on gpa_to_vva() return value.
> - in function virtio_dev_rx() variables can be defined in local block rather than
> in function block. this simplifies code reading.
> - in function virtio_dev_rx() virtio_hdr is always zero, so it can be static variable
> rather than on stack.
> - in function virtio_dev_rx() virtio_hdr should be copied before packet data.
> - in function virtio_dev_rx() can we prefetch next vq->desc before coping
> packet data?
> - ovs-mutex is bit heavy weight, can you use rte-spin-lock
> - can you reverse name of virtio_dev_rx(), virtio_dev_tx(), since this is ovs-
> netdev code we could use name same as ovs context.
> - virtio_dev_tx() is always called from PMD thread, so no need to the check.
> - there is no synchronization for netdev_dpdk->rx_count and tx_count.
> - destroy_device() can use RCU based mechanism rather than polling packet
> count.
> - Currently new_device() assigns post in linear fashion. How does handle
> multiple bridge case where vhost port might belong to available port on
> different bridge?
> - virtio_net_device_ops ops should be set before registering cuse device.
> - destroy function sets virtio_dev to NULL, so no need to check for remove flag,
> ofcourse you need to RCUfy it first.


More information about the dev mailing list