From mboxrd@z Thu Jan  1 00:00:00 1970
From: Tiwei Bie <tiwei.bie@intel.com>
Subject: Re: [RFC v3 0/7] vhost2: new librte_vhost2 proposal
Date: Tue, 26 Jun 2018 17:14:28 +0800
Message-ID: <20180626091428.GA20198@debian>
References: <1526648465-62579-1-git-send-email-dariuszx.stojaczyk@intel.com>
 <20180607151227.23660-1-darek.stojaczyk@gmail.com>
 <20180625110146.GA18211@debian>
 <FBE7E039FA50BF47A673AD0BD3CD56A846172026@HASMSX105.ger.corp.intel.com>
 <20180626082226.GA15665@debian>
 <FBE7E039FA50BF47A673AD0BD3CD56A846172E58@HASMSX105.ger.corp.intel.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=utf-8
Cc: Dariusz Stojaczyk <darek.stojaczyk@gmail.com>,
 "dev@dpdk.org" <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" <yliu@fridaylinux.org>,
 "Harris, James R" <james.r.harris@intel.com>,
 "Kulasek, TomaszX" <tomaszx.kulasek@intel.com>,
 "Wodkowski, PawelX" <pawelx.wodkowski@intel.com>
To: "Stojaczyk, DariuszX" <dariuszx.stojaczyk@intel.com>
Return-path: <dev-bounces@dpdk.org>
Received: from mga17.intel.com (mga17.intel.com [192.55.52.151])
 by dpdk.org (Postfix) with ESMTP id A3D565B2C
 for <dev@dpdk.org>; Tue, 26 Jun 2018 11:14:28 +0200 (CEST)
Content-Disposition: inline
In-Reply-To: <FBE7E039FA50BF47A673AD0BD3CD56A846172E58@HASMSX105.ger.corp.intel.com>
List-Id: DPDK patches and discussions <dev.dpdk.org>
List-Unsubscribe: <https://mails.dpdk.org/options/dev>,
 <mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://mails.dpdk.org/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <https://mails.dpdk.org/listinfo/dev>,
 <mailto:dev-request@dpdk.org?subject=subscribe>
Errors-To: dev-bounces@dpdk.org
Sender: "dev" <dev-bounces@dpdk.org>

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.

Best regards,
Tiwei Bie

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