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

[Tiwei Bie <tiwei.bie@intel.com>]

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.

Best regards,
Tiwei Bie


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