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

[Tetsuya Mukawa]

On 2015/10/21 19:22, Bruce Richardson wrote:
> 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 at redhat.com]
>>>>> Sent: Monday, October 19, 2015 2:26 PM
>>>>> To: Tetsuya Mukawa <mukawa at igel.co.jp>; Richardson, Bruce
>>>>> <bruce.richardson at intel.com>; Loftus, Ciara <ciara.loftus at intel.com>
>>>>> Cc: dev at dpdk.org; ann.zhuangyanying at 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 at 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
>

Hi,

I have submitted latest patches.
I will keep vhost library until we will have agreement to merge it to
vhost PMD.

Regards,
Testuya