> -----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 > > > <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. > > Best regards, > Tiwei Bie > > > > Regards, > > D. > > > > > > > > Is my above understanding correct? Thanks! > > > > > > Best regards, > > > Tiwei Bie > > >
