[dpdk-dev,01/17] vhost: introduce driver features related APIs
Checks
Commit Message
Introduce few APIs to set/get/enable/disable driver features.
Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
---
lib/librte_vhost/rte_vhost_version.map | 10 ++++
lib/librte_vhost/rte_virtio_net.h | 9 ++++
lib/librte_vhost/socket.c | 90 ++++++++++++++++++++++++++++++++++
3 files changed, 109 insertions(+)
Comments
On 03/03/2017 10:51 AM, Yuanhan Liu wrote:
> Introduce few APIs to set/get/enable/disable driver features.
>
> Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
> ---
> lib/librte_vhost/rte_vhost_version.map | 10 ++++
> lib/librte_vhost/rte_virtio_net.h | 9 ++++
> lib/librte_vhost/socket.c | 90 ++++++++++++++++++++++++++++++++++
> 3 files changed, 109 insertions(+)
Nice!
Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
I wonder whether we could protect from setting/enabling/disabling
features once negotiation is done?
Thanks,
Maxime
On 03/14/2017 10:46 AM, Maxime Coquelin wrote:
>
>
> On 03/03/2017 10:51 AM, Yuanhan Liu wrote:
>> Introduce few APIs to set/get/enable/disable driver features.
>>
>> Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
>> ---
>> lib/librte_vhost/rte_vhost_version.map | 10 ++++
>> lib/librte_vhost/rte_virtio_net.h | 9 ++++
>> lib/librte_vhost/socket.c | 90
>> ++++++++++++++++++++++++++++++++++
>> 3 files changed, 109 insertions(+)
>
> Nice!
> Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>
> I wonder whether we could protect from setting/enabling/disabling
> features once negotiation is done?
Oh, I forgot one comment on this patch.
Since these new functions are part to the API, shouldn't them be
documented?
Maxime
On Tue, Mar 14, 2017 at 10:53:23AM +0100, Maxime Coquelin wrote:
>
>
> On 03/14/2017 10:46 AM, Maxime Coquelin wrote:
> >
> >
> >On 03/03/2017 10:51 AM, Yuanhan Liu wrote:
> >>Introduce few APIs to set/get/enable/disable driver features.
> >>
> >>Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
> >>---
> >> lib/librte_vhost/rte_vhost_version.map | 10 ++++
> >> lib/librte_vhost/rte_virtio_net.h | 9 ++++
> >> lib/librte_vhost/socket.c | 90
> >>++++++++++++++++++++++++++++++++++
> >> 3 files changed, 109 insertions(+)
> >
> >Nice!
> >Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> >
> >I wonder whether we could protect from setting/enabling/disabling
> >features once negotiation is done?
Those APIs won't be able to change the negotiated features. They are
just some interfaces before the vhost-user connection is established.
Ideally, we could/should get rid of the enabling/disabling functions:
let the vhost-user driver to handle the features directly (say, for
vhost-user pmd, we could use vdev options to disable/enable few specific
features). Once that is done, use the rte_vhost_driver_set_features()
set the features once.
The reason I introduced the enable/disable_features() is to keep the
compatability with the builtin vhost-user net driver (virtio_net.c).
If there is a plan to move it into vhost-pmd, they won't be needed.
And I don't think that will happen soon.
> Oh, I forgot one comment on this patch.
> Since these new functions are part to the API, shouldn't them be
> documented?
Yes, indeed, it's also noted in my cover letter.
--yliu
On 03/16/2017 08:08 AM, Yuanhan Liu wrote:
> On Tue, Mar 14, 2017 at 10:53:23AM +0100, Maxime Coquelin wrote:
>>
>>
>> On 03/14/2017 10:46 AM, Maxime Coquelin wrote:
>>>
>>>
>>> On 03/03/2017 10:51 AM, Yuanhan Liu wrote:
>>>> Introduce few APIs to set/get/enable/disable driver features.
>>>>
>>>> Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
>>>> ---
>>>> lib/librte_vhost/rte_vhost_version.map | 10 ++++
>>>> lib/librte_vhost/rte_virtio_net.h | 9 ++++
>>>> lib/librte_vhost/socket.c | 90
>>>> ++++++++++++++++++++++++++++++++++
>>>> 3 files changed, 109 insertions(+)
>>>
>>> Nice!
>>> Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>>>
>>> I wonder whether we could protect from setting/enabling/disabling
>>> features once negotiation is done?
>
> Those APIs won't be able to change the negotiated features. They are
> just some interfaces before the vhost-user connection is established.
Right.
I meant it could return an error is the connection is already
established. Else, the caller might think the feature has been
successfully enabled/disabled, whereas it is not the case.
But this is maybe over-engineering to handle this case.
> Ideally, we could/should get rid of the enabling/disabling functions:
> let the vhost-user driver to handle the features directly (say, for
> vhost-user pmd, we could use vdev options to disable/enable few specific
> features). Once that is done, use the rte_vhost_driver_set_features()
> set the features once.
Ok, but what about vhost lib users and TSO (I'm thinking of OVS).
> The reason I introduced the enable/disable_features() is to keep the
> compatability with the builtin vhost-user net driver (virtio_net.c).
> If there is a plan to move it into vhost-pmd, they won't be needed.
> And I don't think that will happen soon.
I agree with you that would be ideal, but unlikely to happen soon.
>> Oh, I forgot one comment on this patch.
>> Since these new functions are part to the API, shouldn't them be
>> documented?
>
> Yes, indeed, it's also noted in my cover letter.
Oops, I missed that!
Thanks,
Maxime
> --yliu
>
On Thu, Mar 16, 2017 at 10:18:21AM +0100, Maxime Coquelin wrote:
>
>
> On 03/16/2017 08:08 AM, Yuanhan Liu wrote:
> >On Tue, Mar 14, 2017 at 10:53:23AM +0100, Maxime Coquelin wrote:
> >>
> >>
> >>On 03/14/2017 10:46 AM, Maxime Coquelin wrote:
> >>>
> >>>
> >>>On 03/03/2017 10:51 AM, Yuanhan Liu wrote:
> >>>>Introduce few APIs to set/get/enable/disable driver features.
> >>>>
> >>>>Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
> >>>>---
> >>>>lib/librte_vhost/rte_vhost_version.map | 10 ++++
> >>>>lib/librte_vhost/rte_virtio_net.h | 9 ++++
> >>>>lib/librte_vhost/socket.c | 90
> >>>>++++++++++++++++++++++++++++++++++
> >>>>3 files changed, 109 insertions(+)
> >>>
> >>>Nice!
> >>>Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> >>>
> >>>I wonder whether we could protect from setting/enabling/disabling
> >>>features once negotiation is done?
> >
> >Those APIs won't be able to change the negotiated features. They are
> >just some interfaces before the vhost-user connection is established.
>
> Right.
> I meant it could return an error is the connection is already
> established. Else, the caller might think the feature has been
> successfully enabled/disabled, whereas it is not the case.
> But this is maybe over-engineering to handle this case.
>
> >Ideally, we could/should get rid of the enabling/disabling functions:
> >let the vhost-user driver to handle the features directly (say, for
> >vhost-user pmd, we could use vdev options to disable/enable few specific
> >features). Once that is done, use the rte_vhost_driver_set_features()
> >set the features once.
>
> Ok, but what about vhost lib users and TSO (I'm thinking of OVS).
Yes, that's why I kept them :)
--yliu
>
> >The reason I introduced the enable/disable_features() is to keep the
> >compatability with the builtin vhost-user net driver (virtio_net.c).
> >If there is a plan to move it into vhost-pmd, they won't be needed.
> >And I don't think that will happen soon.
>
> I agree with you that would be ideal, but unlikely to happen soon.
>
> >>Oh, I forgot one comment on this patch.
> >>Since these new functions are part to the API, shouldn't them be
> >>documented?
> >
> >Yes, indeed, it's also noted in my cover letter.
>
> Oops, I missed that!
>
> Thanks,
> Maxime
> > --yliu
> >
@@ -30,3 +30,13 @@ DPDK_16.07 {
rte_vhost_get_queue_num;
} DPDK_2.1;
+
+DPDK_17.05 {
+ global:
+
+ rte_vhost_driver_disable_features;
+ rte_vhost_driver_enable_features;
+ rte_vhost_driver_get_features;
+ rte_vhost_driver_set_features;
+
+} DPDK_16.07;
@@ -94,6 +94,15 @@ struct virtio_net_device_ops {
/* Unregister vhost driver. This is only meaningful to vhost user. */
int rte_vhost_driver_unregister(const char *path);
+/**
+ * Set feature bits the vhost driver supports.
+ */
+int rte_vhost_driver_set_features(const char *path, uint64_t features);
+uint64_t rte_vhost_driver_get_features(const char *path);
+
+int rte_vhost_driver_enable_features(const char *path, uint64_t features);
+int rte_vhost_driver_disable_features(const char *path, uint64_t features);
+
/* Register callbacks. */
int rte_vhost_driver_callback_register(struct virtio_net_device_ops const * const);
/* Start vhost driver session blocking loop. */
@@ -63,6 +63,16 @@ struct vhost_user_socket {
bool is_server;
bool reconnect;
bool dequeue_zero_copy;
+
+ /*
+ * The "supported_features" indicates the feature bits the
+ * vhost driver supports. The "features" indicates the feature
+ * bits after the rte_vhost_driver_features_disable/enable().
+ * It is also the final feature bits used for vhost-user
+ * features negotiation.
+ */
+ uint64_t supported_features;
+ uint64_t features;
};
struct vhost_user_connection {
@@ -475,6 +485,86 @@ struct vhost_user_reconnect_list {
return 0;
}
+static struct vhost_user_socket *
+find_vhost_user_socket(const char *path)
+{
+ int i;
+
+ for (i = 0; i < vhost_user.vsocket_cnt; i++) {
+ struct vhost_user_socket *vsocket = vhost_user.vsockets[i];
+
+ if (!strcmp(vsocket->path, path))
+ return vsocket;
+ }
+
+ return NULL;
+}
+
+int
+rte_vhost_driver_disable_features(const char *path, uint64_t features)
+{
+ struct vhost_user_socket *vsocket;
+
+ pthread_mutex_lock(&vhost_user.mutex);
+ vsocket = find_vhost_user_socket(path);
+ if (vsocket)
+ vsocket->features &= ~features;
+ pthread_mutex_unlock(&vhost_user.mutex);
+
+ return vsocket ? 0 : -1;
+}
+
+int
+rte_vhost_driver_enable_features(const char *path, uint64_t features)
+{
+ struct vhost_user_socket *vsocket;
+
+ pthread_mutex_lock(&vhost_user.mutex);
+ vsocket = find_vhost_user_socket(path);
+ if (vsocket) {
+ if ((vsocket->supported_features & features) != features) {
+ /*
+ * trying to enable features the driver doesn't
+ * support.
+ */
+ pthread_mutex_unlock(&vhost_user.mutex);
+ return -1;
+ }
+ vsocket->features |= features;
+ }
+ pthread_mutex_unlock(&vhost_user.mutex);
+
+ return vsocket ? 0 : -1;
+}
+
+int
+rte_vhost_driver_set_features(const char *path, uint64_t features)
+{
+ struct vhost_user_socket *vsocket;
+
+ pthread_mutex_lock(&vhost_user.mutex);
+ vsocket = find_vhost_user_socket(path);
+ if (vsocket) {
+ vsocket->supported_features = features;
+ vsocket->features = features;
+ }
+ pthread_mutex_unlock(&vhost_user.mutex);
+
+ return vsocket ? 0 : -1;
+}
+
+uint64_t
+rte_vhost_driver_get_features(const char *path)
+{
+ struct vhost_user_socket *vsocket;
+
+ pthread_mutex_lock(&vhost_user.mutex);
+ vsocket = find_vhost_user_socket(path);
+ pthread_mutex_unlock(&vhost_user.mutex);
+
+ return vsocket ? vsocket->features : (uint64_t)-1;
+}
+
/*
* Register a new vhost-user socket; here we could act as server
* (the default case), or client (when RTE_VHOST_USER_CLIENT) flag