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.brocc...@oracle.com> so this needs to be addressed also.
Thanks Maryam > -----Original Message----- > From: Pravin Shelar [mailto:pshe...@nicira.com] > Sent: Tuesday, October 21, 2014 12:00 AM > To: Tahhan, Maryam > Cc: dev@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.tah...@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.tah...@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. _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
