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