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.stojac...@intel.com>
Cc: Dariusz Stojaczyk <darek.stojac...@gmail.com>; dev@dpdk.org; Maxime
Coquelin <maxime.coque...@redhat.com>; Tetsuya Mukawa
<mtetsu...@gmail.com>; Stefan Hajnoczi <stefa...@redhat.com>; Thomas
Monjalon <tho...@monjalon.net>; y...@fridaylinux.org; Harris, James R
<james.r.har...@intel.com>; Kulasek, TomaszX <tomaszx.kula...@intel.com>;
Wodkowski, PawelX <pawelx.wodkow...@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-boun...@dpdk.org] On Behalf Of Tiwei Bie
Sent: Monday, June 25, 2018 1:02 PM

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 {

   vhost_user_msg_handler(struct vhost_dev *vdev, struct vhost_user_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

+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.

Best regards,
Tiwei Bie

Best regards,
Tiwei Bie


Is my above understanding correct? Thanks!

Best regards,
Tiwei Bie

Reply via email to