Re: [dpdk-dev] [RFC v3 0/7] vhost2: new librte_vhost2 proposal

[Maxime Coquelin <maxime.coquelin@redhat.com>]

On 06/26/2018 11:14 AM, Tiwei Bie wrote:
> On Tue, Jun 26, 2018 at 04:47:33PM +0800, Stojaczyk, DariuszX wrote:
>>> -----Original Message-----
>>> From: Bie, Tiwei
>>> Sent: Tuesday, June 26, 2018 10:22 AM
>>> To: Stojaczyk, DariuszX <dariuszx.stojaczyk@intel.com>
>>> Cc: Dariusz Stojaczyk <darek.stojaczyk@gmail.com>; dev@dpdk.org; Maxime
>>> Coquelin <maxime.coquelin@redhat.com>; Tetsuya Mukawa
>>> <mtetsuyah@gmail.com>; Stefan Hajnoczi <stefanha@redhat.com>; Thomas
>>> Monjalon <thomas@monjalon.net>; yliu@fridaylinux.org; Harris, James R
>>> <james.r.harris@intel.com>; Kulasek, TomaszX <tomaszx.kulasek@intel.com>;
>>> Wodkowski, PawelX <pawelx.wodkowski@intel.com>
>>> Subject: Re: [dpdk-dev] [RFC v3 0/7] vhost2: new librte_vhost2 proposal
>>>
>>> On Mon, Jun 25, 2018 at 08:17:08PM +0800, Stojaczyk, DariuszX wrote:
>>>>> -----Original Message-----
>>>>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Tiwei Bie
>>>>> Sent: Monday, June 25, 2018 1:02 PM
>>>>> <snip>
>>>>>
>>>>> Hi Dariusz,
>>>>>
>>>>
>>>> Hi Tiwei,
>>>>
>>>>> Thank you for putting efforts in making the DPDK
>>>>> vhost more generic!
>>>>>
>>>>>  From my understanding, your proposal is that:
>>>>>
>>>>> 1) Introduce rte_vhost2 to provide the APIs which
>>>>>     allow users to implement vhost backends like
>>>>>     SCSI, net, crypto, ..
>>>>>
>>>>
>>>> That's right.
>>>>
>>>>> 2) Refactor the existing rte_vhost to use rte_vhost2.
>>>>>     The rte_vhost will still provide below existing
>>>>>     sets of APIs:
>>>>>      1. The APIs which allow users to implement
>>>>>         external vhost backends (these APIs were
>>>>>         designed for SPDK previously)
>>>>>      2. The APIs provided by the net backend
>>>>>      3. The APIs provided by the crypto backend
>>>>>     And above APIs in rte_vhost won't be changed.
>>>>
>>>> That's correct. Rte_vhost would register its own rte_vhost2_tgt_ops
>>> underneath and will call existing vhost_device_ops for e.g. starting the device
>>> once all queues are started.
>>>
>>> Currently I have below concerns and questions:
>>>
>>> - The rte_vhost's problem is still there. Even though
>>>    rte_vhost2 is introduced, the net and crypto backends
>>>    in rte_vhost won't benefit from the new callbacks.
>>>
>>>    The existing rte_vhost in DPDK not only provides the
>>>    APIs for DPDK applications to implement the external
>>>    backends. But also provides high performance net and
>>>    crypto backends implementation (maybe more in the
>>>    future). So it's important that besides the DPDK
>>>    applications which implement their external backends,
>>>    the DPDK applications which use the builtin backends
>>>    will also benefit from the new callbacks.
>>>
>>>    So we should have a clear plan on how will the legacy
>>>    callbacks in rte_vhost be dealt with in the next step.
>>>
>>>    Besides, the new library's name is a bit misleading.
>>>    It makes the existing rte_vhost library sound like an
>>>    obsolete library. But actually the existing rte_vhost
>>>    isn't an obsolete library. It will still provide the
>>>    net and crypto backends. So if we want to introduce
>>>    this new library, we should give it a better name.
>>>
>>> - It's possible to solve rte_vhost's problem you met
>>>    by refactoring the existing vhost library directly
>>>    instead of re-implementing a new vhost library from
>>>    scratch and keeping the old one's problem as is.
>>>
>>>    In this way, it will solve the problem you met and
>>>    also solve the problem for rte_vhost. Why not go
>>>    this way? Something like:
>>>
>>>    Below is the existing callbacks set in rte_vhost.h:
>>>
>>>    /**
>>>     * Device and vring operations.
>>>     */
>>>    struct vhost_device_ops {
>>>            ......
>>>    };
>>>
>>>    It's a legacy implementation, and doesn't really
>>>    follow the DPDK API design (e.g. no rte_ prefix).
>>>    We can design and implement a new message handling
>>>    and a new set of callbacks for rte_vhost to solve
>>>    the problem you met without changing the old one.
>>>    Something like:
>>>
>>>    struct rte_vhost_device_ops {
>>>            ......
>>>    }
>>>
>>>    int
>>>    vhost_user_msg_handler(struct vhost_dev *vdev, struct vhost_user_msg
>>> *msg)
>>>    {
>>>            ......
>>>
>>>            if (!vdev->is_using_new_device_ops) {
>>>                    // Call the existing message handler
>>>                    return vhost_user_msg_handler_legacy(vdev, msg);
>>>            }
>>>
>>>            // Implement the new logic here
>>>            ......
>>>    }
>>>
>>>    A vhost application is allowed to register only struct
>>>    rte_vhost_device_ops or struct vhost_device_ops (which
>>>    should be deprecated in the future). The two ops cannot
>>>    be registered at the same time.
>>>
>>>    The existing applications could use the old ops. And
>>>    if an application registers struct rte_vhost_device_ops,
>>>    the new callbacks and message handler will be used.
>>
>> Please notice that some features like vIOMMU are not even a part of the public rte_vhost API. Only vhost-net benefits from vIOMMU right now. Separating vhost-net from a generic vhost library (rte_vhost2) would avoid making such design mistakes in future. What's the point of having a single rte_vhost library, if some vhost-user features are only implemented for vhost-net.
> 
> These APIs can be safely added at any time.
> And we can also ask developers to add public
> APIs if it matters when adding new features
> in the future. I don't think it's a big
> problem.

+1, I don't think it is a problem.
It is better to have it internal only at the beginning than having to
break the API.

Thanks,
Maxime
> Best regards,
> Tiwei Bie
> 
>>
>>>
>>> Best regards,
>>> Tiwei Bie
>>>
>>>
>>>> Regards,
>>>> D.
>>>>
>>>>>
>>>>> Is my above understanding correct? Thanks!
>>>>>
>>>>> Best regards,
>>>>> Tiwei Bie
>>>>>