Re: [dpdk-dev] [RFC PATCH v2] vhost: Add VHOST PMD

[Bruce Richardson <bruce.richardson@intel.com>]

On Wed, Oct 21, 2015 at 09:25:12AM +0300, Panu Matilainen wrote:
> On 10/21/2015 07:35 AM, Tetsuya Mukawa wrote:
> >On 2015/10/19 22:27, Richardson, Bruce wrote:
> >>>-----Original Message-----
> >>>From: Panu Matilainen [mailto:pmatilai@redhat.com]
> >>>Sent: Monday, October 19, 2015 2:26 PM
> >>>To: Tetsuya Mukawa <mukawa@igel.co.jp>; Richardson, Bruce
> >>><bruce.richardson@intel.com>; Loftus, Ciara <ciara.loftus@intel.com>
> >>>Cc: dev@dpdk.org; ann.zhuangyanying@huawei.com
> >>>Subject: Re: [dpdk-dev] [RFC PATCH v2] vhost: Add VHOST PMD
> >>>
> >>>On 10/19/2015 01:50 PM, Tetsuya Mukawa wrote:
> >>>>On 2015/10/19 18:45, Bruce Richardson wrote:
> >>>>>On Mon, Oct 19, 2015 at 10:32:50AM +0100, Loftus, Ciara wrote:
> >>>>>>>On 2015/10/16 21:52, Bruce Richardson wrote:
> >>>>>>>>On Mon, Aug 31, 2015 at 12:55:26PM +0900, Tetsuya Mukawa wrote:
> >>>>>>>>>The patch introduces a new PMD. This PMD is implemented as thin
> >>>>>>>wrapper
> >>>>>>>>>of librte_vhost. It means librte_vhost is also needed to compile
> >>>the PMD.
> >>>>>>>>>The PMD can have 'iface' parameter like below to specify a path
> >>>>>>>>>to
> >>>>>>>connect
> >>>>>>>>>to a virtio-net device.
> >>>>>>>>>
> >>>>>>>>>$ ./testpmd -c f -n 4 --vdev 'eth_vhost0,iface=/tmp/sock0' -- -i
> >>>>>>>>>
> >>>>>>>>>To connect above testpmd, here is qemu command example.
> >>>>>>>>>
> >>>>>>>>>$ qemu-system-x86_64 \
> >>>>>>>>>          <snip>
> >>>>>>>>>          -chardev socket,id=chr0,path=/tmp/sock0 \
> >>>>>>>>>          -netdev vhost-user,id=net0,chardev=chr0,vhostforce \
> >>>>>>>>>          -device virtio-net-pci,netdev=net0
> >>>>>>>>>
> >>>>>>>>>Signed-off-by: Tetsuya Mukawa <mukawa@igel.co.jp>
> >>>>>>>>With this PMD in place, is there any need to keep the existing
> >>>>>>>>vhost library around as a separate entity? Can the existing
> >>>>>>>>library be
> >>>>>>>subsumed/converted into
> >>>>>>>>a standard PMD?
> >>>>>>>>
> >>>>>>>>/Bruce
> >>>>>>>Hi Bruce,
> >>>>>>>
> >>>>>>>I concern about whether the PMD has all features of librte_vhost,
> >>>>>>>because librte_vhost provides more features and freedom than ethdev
> >>>>>>>API provides.
> >>>>>>>In some cases, user needs to choose limited implementation without
> >>>>>>>librte_vhost.
> >>>>>>>I am going to eliminate such cases while implementing the PMD.
> >>>>>>>But I don't have strong belief that we can remove librte_vhost now.
> >>>>>>>
> >>>>>>>So how about keeping current separation in next DPDK?
> >>>>>>>I guess people will try to replace librte_vhost to vhost PMD,
> >>>>>>>because apparently using ethdev APIs will be useful in many cases.
> >>>>>>>And we will get feedbacks like "vhost PMD needs to support like this
> >>>usage".
> >>>>>>>(Or we will not have feedbacks, but it's also OK.) Then, we will be
> >>>>>>>able to merge librte_vhost and vhost PMD.
> >>>>>>I agree with the above. One the concerns I had when reviewing the
> >>>patch was that the PMD removes some freedom that is available with the
> >>>library. Eg. Ability to implement the new_device and destroy_device
> >>>callbacks. If using the PMD you are constrained to the implementations of
> >>>these in the PMD driver, but if using librte_vhost, you can implement your
> >>>own with whatever functionality you like - a good example of this can be
> >>>seen in the vhost sample app.
> >>>>>>On the other hand, the PMD is useful in that it removes a lot of
> >>>complexity for the user and may work for some more general use cases. So I
> >>>would be in favour of having both options available too.
> >>>>>>Ciara
> >>>>>>
> >>>>>Thanks.
> >>>>>However, just because the libraries are merged does not mean that you
> >>>>>need be limited by PMD functionality. Many PMDs provide additional
> >>>>>library-specific functions over and above their PMD capabilities. The
> >>>>>bonded PMD is a good example here, as it has a whole set of extra
> >>>>>functions to create and manipulate bonded devices - things that are
> >>>>>obviously not part of the general ethdev API. Other vPMDs similarly
> >>>include functions to allow them to be created on the fly too.
> >>>>>regards,
> >>>>>/Bruce
> >>>>Hi Bruce,
> >>>>
> >>>>I appreciate for showing a good example. I haven't noticed the PMD.
> >>>>I will check the bonding PMD, and try to remove librte_vhost without
> >>>>losing freedom and features of the library.
> >>>Hi,
> >>>
> >>>Just a gentle reminder - if you consider removing (even if by just
> >>>replacing/renaming) an entire library, it needs to happen the ABI
> >>>deprecation process.
> >>>
> >>>It seems obvious enough. But for all the ABI policing here, somehow we all
> >>>failed to notice the two compatibility breaking rename-elephants in the
> >>>room during 2.1 development:
> >>>- libintel_dpdk was renamed to libdpdk
> >>>- librte_pmd_virtio_uio was renamed to librte_pmd_virtio
> >>>
> >>>Of course these cases are easy to work around with symlinks, and are
> >>>unrelated to the matter at hand. Just wanting to make sure such things
> >>>dont happen again.
> >>>
> >>>	- Panu -
> >>Still doesn't hurt to remind us, Panu! Thanks. :-)
> >
> >Hi,
> >
> >Thanks for reminder. I've checked the DPDK documentation.
> >I will submit deprecation notice to follow DPDK deprecation process.
> >(Probably we will be able to remove vhost library in DPDK-2.3 or later.)
> >
> >BTW, I will merge vhost library and PMD like below.
> >Step1. Move vhost library under vhost PMD.
> >Step2. Rename current APIs.
> >Step3. Add a function to get a pointer of "struct virtio_net device" by
> >a portno.
> >
> >Last steps allows us to be able to convert a portno to the pointer of
> >corresponding vrtio_net device.
> >And we can still use features and freedom vhost library APIs provided.
> 
> Just wondering, is that *really* worth the price of breaking every single
> vhost library user out there?
> 
> I mean, this is not about removing some bitrotten function or two which
> nobody cares about anymore but removing (by renaming) one of the more widely
> (AFAICS) used libraries and its entire API.
> 
> If current APIs are kept then compatibility is largely a matter of planting
> a strategic symlink or two, but it might make the API look inconsistent.
> 
> But just wondering about the benefit of this merge thing, compared to just
> adding a vhost pmd and leaving the library be. The ABI process is not there
> to make life miserable for DPDK developers, its there to help make DPDK
> nicer for *other* developers. And the first and the foremost rule is simply:
> dont break backwards compatibility. Not unless there's a damn good reason to
> doing so, and I fail to see that reason here.
> 
> 	- Panu -
>
Good question, and I'll accept that maybe it's not worth doing. I'm not that
much of an expert on the internals and APIs of vhost library.

However, the merge I was looking for was more from a code locality point
of view, to have all the vhost code in one directory (under drivers/net),
than spread across multiple ones. What API's need to be deprecated
or not as part of that work, is a separate question, and so in theory we could
create a combined vhost library that does not deprecate anything (though to
avoid a build-up of technical debt, we'll probably want to deprecate some 
functions).

I'll leave it up to the vhost experts do decide what's best, but for me, any
library that handles transmission and reception of packets outside of a DPDK
app should be a PMD library using ethdev rx/tx burst routines, and located
under drivers/net. (KNI is another obvious target for such a move and conversion).

Regards,
/Bruce