[35/40] net/virtio: make server mode blocking
Checks
Commit Message
This patch makes the Vhost-user backend server mode
blocking at init, waiting for the client connection.
The goal is to make the driver more reliable, as without
waiting for client connection, the Virtio driver has to
assume the Vhost-user backend will support all the
features it has advertized, which could lead to undefined
behaviour.
For example, without this patch, if the user enables packed
ring Virtio feature but the backend does not support it,
the ring initialized by the driver will not be compatible
with the backend.
Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
drivers/net/virtio/virtio_user/vhost_user.c | 9 +-
.../net/virtio/virtio_user/virtio_user_dev.c | 118 +++++++-----------
drivers/net/virtio/virtio_user_ethdev.c | 5 -
3 files changed, 54 insertions(+), 78 deletions(-)
Comments
Hi Maxime,
> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> Sent: Monday, December 21, 2020 5:14 AM
> To: dev@dpdk.org; Xia, Chenbo <chenbo.xia@intel.com>; olivier.matz@6wind.com;
> amorenoz@redhat.com; david.marchand@redhat.com
> Cc: Maxime Coquelin <maxime.coquelin@redhat.com>
> Subject: [PATCH 35/40] net/virtio: make server mode blocking
>
> This patch makes the Vhost-user backend server mode
> blocking at init, waiting for the client connection.
>
> The goal is to make the driver more reliable, as without
> waiting for client connection, the Virtio driver has to
> assume the Vhost-user backend will support all the
> features it has advertized, which could lead to undefined
> behaviour.
>
> For example, without this patch, if the user enables packed
> ring Virtio feature but the backend does not support it,
> the ring initialized by the driver will not be compatible
> with the backend.
>
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> ---
> drivers/net/virtio/virtio_user/vhost_user.c | 9 +-
> .../net/virtio/virtio_user/virtio_user_dev.c | 118 +++++++-----------
> drivers/net/virtio/virtio_user_ethdev.c | 5 -
> 3 files changed, 54 insertions(+), 78 deletions(-)
>
> diff --git a/drivers/net/virtio/virtio_user/vhost_user.c
> b/drivers/net/virtio/virtio_user/vhost_user.c
> index a57106a468..94a33326ae 100644
> --- a/drivers/net/virtio/virtio_user/vhost_user.c
> +++ b/drivers/net/virtio/virtio_user/vhost_user.c
> @@ -677,6 +677,14 @@ virtio_user_start_server(struct virtio_user_dev *dev,
> struct sockaddr_un *un)
> if (ret < 0)
> return -1;
>
> + PMD_DRV_LOG(NOTICE, "(%s) waiting for client connection...", dev->path);
> + dev->vhostfd = accept(fd, NULL, NULL);
> + if (dev->vhostfd < 0) {
> + PMD_DRV_LOG(ERR, "Failed to accept initial client connection (%s)",
> + strerror(errno));
> + return -1;
> + }
> +
> flag = fcntl(fd, F_GETFL);
> if (fcntl(fd, F_SETFL, flag | O_NONBLOCK) < 0) {
> PMD_DRV_LOG(ERR, "fcntl failed, %s", strerror(errno));
> @@ -721,7 +729,6 @@ vhost_user_setup(struct virtio_user_dev *dev)
> close(fd);
> return -1;
> }
> - dev->vhostfd = -1;
> } else {
> if (connect(fd, (struct sockaddr *)&un, sizeof(un)) < 0) {
> PMD_DRV_LOG(ERR, "connect error, %s", strerror(errno));
> diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.c
> b/drivers/net/virtio/virtio_user/virtio_user_dev.c
> index b92b7f7aae..19d59d401e 100644
> --- a/drivers/net/virtio/virtio_user/virtio_user_dev.c
> +++ b/drivers/net/virtio/virtio_user/virtio_user_dev.c
> @@ -144,10 +144,6 @@ virtio_user_dev_set_features(struct virtio_user_dev *dev)
>
> pthread_mutex_lock(&dev->mutex);
>
> - if (dev->backend_type == VIRTIO_USER_BACKEND_VHOST_USER &&
> - dev->vhostfd < 0)
> - goto error;
> -
> /* Step 0: tell vhost to create queues */
> if (virtio_user_queue_setup(dev, virtio_user_create_queue) < 0)
> goto error;
> @@ -190,11 +186,6 @@ virtio_user_start_device(struct virtio_user_dev *dev)
> rte_mcfg_mem_read_lock();
> pthread_mutex_lock(&dev->mutex);
>
> - /* Vhost-user client not connected yet, will start later */
> - if (dev->backend_type == VIRTIO_USER_BACKEND_VHOST_USER &&
> - dev->vhostfd < 0)
> - goto out;
> -
> /* Step 2: share memory regions */
> ret = dev->ops->set_memory_table(dev);
> if (ret < 0)
> @@ -213,7 +204,7 @@ virtio_user_start_device(struct virtio_user_dev *dev)
> goto error;
>
> dev->started = true;
> -out:
> +
> pthread_mutex_unlock(&dev->mutex);
> rte_mcfg_mem_read_unlock();
>
> @@ -421,36 +412,36 @@ virtio_user_dev_setup(struct virtio_user_dev *dev)
> PMD_DRV_LOG(ERR, "Server mode only supports vhost-user!");
> return -1;
> }
> + }
> +
> + if (dev->backend_type == VIRTIO_USER_BACKEND_VHOST_USER) {
> dev->ops = &virtio_ops_user;
> - } else {
> - if (dev->backend_type == VIRTIO_USER_BACKEND_VHOST_USER) {
> - dev->ops = &virtio_ops_user;
> - } else if (dev->backend_type ==
> - VIRTIO_USER_BACKEND_VHOST_KERNEL) {
> - dev->ops = &virtio_ops_kernel;
> -
> - dev->vhostfds = malloc(dev->max_queue_pairs *
> - sizeof(int));
> - dev->tapfds = malloc(dev->max_queue_pairs *
> - sizeof(int));
> - if (!dev->vhostfds || !dev->tapfds) {
> - PMD_INIT_LOG(ERR, "(%s) Failed to allocate FDs", dev-
> >path);
> - return -1;
If tapfds failed on malloc, this will lead to vhostfds not freed?
And, should we free the fds when errors happen later in this function?
Thanks!
Chenbo
> - }
> -
> - for (q = 0; q < dev->max_queue_pairs; ++q) {
> - dev->vhostfds[q] = -1;
> - dev->tapfds[q] = -1;
> - }
> - } else if (dev->backend_type ==
> - VIRTIO_USER_BACKEND_VHOST_VDPA) {
> - dev->ops = &virtio_ops_vdpa;
> - } else {
> - PMD_DRV_LOG(ERR, "(%s) Unknown backend type", dev->path);
On 1/7/21 4:20 AM, Xia, Chenbo wrote:
> Hi Maxime,
>
>> -----Original Message-----
>> From: Maxime Coquelin <maxime.coquelin@redhat.com>
>> Sent: Monday, December 21, 2020 5:14 AM
>> To: dev@dpdk.org; Xia, Chenbo <chenbo.xia@intel.com>; olivier.matz@6wind.com;
>> amorenoz@redhat.com; david.marchand@redhat.com
>> Cc: Maxime Coquelin <maxime.coquelin@redhat.com>
>> Subject: [PATCH 35/40] net/virtio: make server mode blocking
>>
>> This patch makes the Vhost-user backend server mode
>> blocking at init, waiting for the client connection.
>>
>> The goal is to make the driver more reliable, as without
>> waiting for client connection, the Virtio driver has to
>> assume the Vhost-user backend will support all the
>> features it has advertized, which could lead to undefined
>> behaviour.
>>
>> For example, without this patch, if the user enables packed
>> ring Virtio feature but the backend does not support it,
>> the ring initialized by the driver will not be compatible
>> with the backend.
>>
>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>> ---
>> drivers/net/virtio/virtio_user/vhost_user.c | 9 +-
>> .../net/virtio/virtio_user/virtio_user_dev.c | 118 +++++++-----------
>> drivers/net/virtio/virtio_user_ethdev.c | 5 -
>> 3 files changed, 54 insertions(+), 78 deletions(-)
>>
>> diff --git a/drivers/net/virtio/virtio_user/vhost_user.c
>> b/drivers/net/virtio/virtio_user/vhost_user.c
>> index a57106a468..94a33326ae 100644
>> --- a/drivers/net/virtio/virtio_user/vhost_user.c
>> +++ b/drivers/net/virtio/virtio_user/vhost_user.c
>> @@ -677,6 +677,14 @@ virtio_user_start_server(struct virtio_user_dev *dev,
>> struct sockaddr_un *un)
>> if (ret < 0)
>> return -1;
>>
>> + PMD_DRV_LOG(NOTICE, "(%s) waiting for client connection...", dev->path);
>> + dev->vhostfd = accept(fd, NULL, NULL);
>> + if (dev->vhostfd < 0) {
>> + PMD_DRV_LOG(ERR, "Failed to accept initial client connection (%s)",
>> + strerror(errno));
>> + return -1;
>> + }
>> +
>> flag = fcntl(fd, F_GETFL);
>> if (fcntl(fd, F_SETFL, flag | O_NONBLOCK) < 0) {
>> PMD_DRV_LOG(ERR, "fcntl failed, %s", strerror(errno));
>> @@ -721,7 +729,6 @@ vhost_user_setup(struct virtio_user_dev *dev)
>> close(fd);
>> return -1;
>> }
>> - dev->vhostfd = -1;
>> } else {
>> if (connect(fd, (struct sockaddr *)&un, sizeof(un)) < 0) {
>> PMD_DRV_LOG(ERR, "connect error, %s", strerror(errno));
>> diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.c
>> b/drivers/net/virtio/virtio_user/virtio_user_dev.c
>> index b92b7f7aae..19d59d401e 100644
>> --- a/drivers/net/virtio/virtio_user/virtio_user_dev.c
>> +++ b/drivers/net/virtio/virtio_user/virtio_user_dev.c
>> @@ -144,10 +144,6 @@ virtio_user_dev_set_features(struct virtio_user_dev *dev)
>>
>> pthread_mutex_lock(&dev->mutex);
>>
>> - if (dev->backend_type == VIRTIO_USER_BACKEND_VHOST_USER &&
>> - dev->vhostfd < 0)
>> - goto error;
>> -
>> /* Step 0: tell vhost to create queues */
>> if (virtio_user_queue_setup(dev, virtio_user_create_queue) < 0)
>> goto error;
>> @@ -190,11 +186,6 @@ virtio_user_start_device(struct virtio_user_dev *dev)
>> rte_mcfg_mem_read_lock();
>> pthread_mutex_lock(&dev->mutex);
>>
>> - /* Vhost-user client not connected yet, will start later */
>> - if (dev->backend_type == VIRTIO_USER_BACKEND_VHOST_USER &&
>> - dev->vhostfd < 0)
>> - goto out;
>> -
>> /* Step 2: share memory regions */
>> ret = dev->ops->set_memory_table(dev);
>> if (ret < 0)
>> @@ -213,7 +204,7 @@ virtio_user_start_device(struct virtio_user_dev *dev)
>> goto error;
>>
>> dev->started = true;
>> -out:
>> +
>> pthread_mutex_unlock(&dev->mutex);
>> rte_mcfg_mem_read_unlock();
>>
>> @@ -421,36 +412,36 @@ virtio_user_dev_setup(struct virtio_user_dev *dev)
>> PMD_DRV_LOG(ERR, "Server mode only supports vhost-user!");
>> return -1;
>> }
>> + }
>> +
>> + if (dev->backend_type == VIRTIO_USER_BACKEND_VHOST_USER) {
>> dev->ops = &virtio_ops_user;
>> - } else {
>> - if (dev->backend_type == VIRTIO_USER_BACKEND_VHOST_USER) {
>> - dev->ops = &virtio_ops_user;
>> - } else if (dev->backend_type ==
>> - VIRTIO_USER_BACKEND_VHOST_KERNEL) {
>> - dev->ops = &virtio_ops_kernel;
>> -
>> - dev->vhostfds = malloc(dev->max_queue_pairs *
>> - sizeof(int));
>> - dev->tapfds = malloc(dev->max_queue_pairs *
>> - sizeof(int));
>> - if (!dev->vhostfds || !dev->tapfds) {
>> - PMD_INIT_LOG(ERR, "(%s) Failed to allocate FDs", dev-
>>> path);
>> - return -1;
>
> If tapfds failed on malloc, this will lead to vhostfds not freed?
>
> And, should we free the fds when errors happen later in this function?
Certainly, but that it is not the purpose of this patch.
It is being moved to vhost-kernel in later patch, I'll check there if it
is freed properly.
Thanks,
Maxime
> Thanks!
> Chenbo
>
>> - }
>> -
>> - for (q = 0; q < dev->max_queue_pairs; ++q) {
>> - dev->vhostfds[q] = -1;
>> - dev->tapfds[q] = -1;
>> - }
>> - } else if (dev->backend_type ==
>> - VIRTIO_USER_BACKEND_VHOST_VDPA) {
>> - dev->ops = &virtio_ops_vdpa;
>> - } else {
>> - PMD_DRV_LOG(ERR, "(%s) Unknown backend type", dev->path);
@@ -677,6 +677,14 @@ virtio_user_start_server(struct virtio_user_dev *dev, struct sockaddr_un *un)
if (ret < 0)
return -1;
+ PMD_DRV_LOG(NOTICE, "(%s) waiting for client connection...", dev->path);
+ dev->vhostfd = accept(fd, NULL, NULL);
+ if (dev->vhostfd < 0) {
+ PMD_DRV_LOG(ERR, "Failed to accept initial client connection (%s)",
+ strerror(errno));
+ return -1;
+ }
+
flag = fcntl(fd, F_GETFL);
if (fcntl(fd, F_SETFL, flag | O_NONBLOCK) < 0) {
PMD_DRV_LOG(ERR, "fcntl failed, %s", strerror(errno));
@@ -721,7 +729,6 @@ vhost_user_setup(struct virtio_user_dev *dev)
close(fd);
return -1;
}
- dev->vhostfd = -1;
} else {
if (connect(fd, (struct sockaddr *)&un, sizeof(un)) < 0) {
PMD_DRV_LOG(ERR, "connect error, %s", strerror(errno));
@@ -144,10 +144,6 @@ virtio_user_dev_set_features(struct virtio_user_dev *dev)
pthread_mutex_lock(&dev->mutex);
- if (dev->backend_type == VIRTIO_USER_BACKEND_VHOST_USER &&
- dev->vhostfd < 0)
- goto error;
-
/* Step 0: tell vhost to create queues */
if (virtio_user_queue_setup(dev, virtio_user_create_queue) < 0)
goto error;
@@ -190,11 +186,6 @@ virtio_user_start_device(struct virtio_user_dev *dev)
rte_mcfg_mem_read_lock();
pthread_mutex_lock(&dev->mutex);
- /* Vhost-user client not connected yet, will start later */
- if (dev->backend_type == VIRTIO_USER_BACKEND_VHOST_USER &&
- dev->vhostfd < 0)
- goto out;
-
/* Step 2: share memory regions */
ret = dev->ops->set_memory_table(dev);
if (ret < 0)
@@ -213,7 +204,7 @@ virtio_user_start_device(struct virtio_user_dev *dev)
goto error;
dev->started = true;
-out:
+
pthread_mutex_unlock(&dev->mutex);
rte_mcfg_mem_read_unlock();
@@ -421,36 +412,36 @@ virtio_user_dev_setup(struct virtio_user_dev *dev)
PMD_DRV_LOG(ERR, "Server mode only supports vhost-user!");
return -1;
}
+ }
+
+ if (dev->backend_type == VIRTIO_USER_BACKEND_VHOST_USER) {
dev->ops = &virtio_ops_user;
- } else {
- if (dev->backend_type == VIRTIO_USER_BACKEND_VHOST_USER) {
- dev->ops = &virtio_ops_user;
- } else if (dev->backend_type ==
- VIRTIO_USER_BACKEND_VHOST_KERNEL) {
- dev->ops = &virtio_ops_kernel;
-
- dev->vhostfds = malloc(dev->max_queue_pairs *
- sizeof(int));
- dev->tapfds = malloc(dev->max_queue_pairs *
- sizeof(int));
- if (!dev->vhostfds || !dev->tapfds) {
- PMD_INIT_LOG(ERR, "(%s) Failed to allocate FDs", dev->path);
- return -1;
- }
-
- for (q = 0; q < dev->max_queue_pairs; ++q) {
- dev->vhostfds[q] = -1;
- dev->tapfds[q] = -1;
- }
- } else if (dev->backend_type ==
- VIRTIO_USER_BACKEND_VHOST_VDPA) {
- dev->ops = &virtio_ops_vdpa;
- } else {
- PMD_DRV_LOG(ERR, "(%s) Unknown backend type", dev->path);
+ } else if (dev->backend_type ==
+ VIRTIO_USER_BACKEND_VHOST_KERNEL) {
+ dev->ops = &virtio_ops_kernel;
+
+ dev->vhostfds = malloc(dev->max_queue_pairs *
+ sizeof(int));
+ dev->tapfds = malloc(dev->max_queue_pairs *
+ sizeof(int));
+ if (!dev->vhostfds || !dev->tapfds) {
+ PMD_INIT_LOG(ERR, "(%s) Failed to allocate FDs", dev->path);
return -1;
}
+
+ for (q = 0; q < dev->max_queue_pairs; ++q) {
+ dev->vhostfds[q] = -1;
+ dev->tapfds[q] = -1;
+ }
+ } else if (dev->backend_type ==
+ VIRTIO_USER_BACKEND_VHOST_VDPA) {
+ dev->ops = &virtio_ops_vdpa;
+ } else {
+ PMD_DRV_LOG(ERR, "(%s) Unknown backend type", dev->path);
+ return -1;
}
+
if (dev->ops->setup(dev) < 0) {
PMD_INIT_LOG(ERR, "(%s) Failed to setup backend\n", dev->path);
return -1;
@@ -533,52 +524,35 @@ virtio_user_dev_init(struct virtio_user_dev *dev, char *path, int queues,
dev->unsupported_features |=
(1ULL << VHOST_USER_F_PROTOCOL_FEATURES);
- if (!dev->is_server) {
- if (dev->ops->set_owner(dev) < 0) {
- PMD_INIT_LOG(ERR, "(%s) Failed to set backend owner", dev->path);
- return -1;
- }
+ if (dev->ops->set_owner(dev) < 0) {
+ PMD_INIT_LOG(ERR, "(%s) Failed to set backend owner", dev->path);
+ return -1;
+ }
+
+ if (dev->ops->get_features(dev, &dev->device_features) < 0) {
+ PMD_INIT_LOG(ERR, "(%s) Failed to get backend features", dev->path);
+ return -1;
+ }
- if (dev->ops->get_features(dev, &dev->device_features) < 0) {
- PMD_INIT_LOG(ERR, "(%s) Failed to get backend features", dev->path);
+ if (dev->device_features & (1ULL << VHOST_USER_F_PROTOCOL_FEATURES)) {
+ if (dev->ops->get_protocol_features(dev, &protocol_features)) {
+ PMD_INIT_LOG(ERR, "(%s) Failed to get backend protocol features",
+ dev->path);
return -1;
}
- if (dev->device_features & (1ULL << VHOST_USER_F_PROTOCOL_FEATURES)) {
- if (dev->ops->get_protocol_features(dev, &protocol_features)) {
- PMD_INIT_LOG(ERR, "(%s) Failed to get backend protocol features",
- dev->path);
- return -1;
- }
-
- dev->protocol_features &= protocol_features;
-
- if (dev->ops->set_protocol_features(dev, dev->protocol_features)) {
- PMD_INIT_LOG(ERR, "(%s) Failed to set backend protocol features",
- dev->path);
- return -1;
- }
+ dev->protocol_features &= protocol_features;
- if (!(dev->protocol_features & (1ULL << VHOST_USER_PROTOCOL_F_MQ)))
- dev->unsupported_features |= (1ull << VIRTIO_NET_F_MQ);
+ if (dev->ops->set_protocol_features(dev, dev->protocol_features)) {
+ PMD_INIT_LOG(ERR, "(%s) Failed to set backend protocol features",
+ dev->path);
+ return -1;
}
- } else {
- /* We just pretend vhost-user can support all these features.
- * Note that this could be problematic that if some feature is
- * negotiated but not supported by the vhost-user which comes
- * later.
- */
- dev->device_features = VIRTIO_USER_SUPPORTED_FEATURES;
- /* We cannot assume VHOST_USER_PROTOCOL_F_STATUS is supported
- * until it's negotiated
- */
- dev->protocol_features &=
- ~(1ULL << VHOST_USER_PROTOCOL_F_STATUS);
+ if (!(dev->protocol_features & (1ULL << VHOST_USER_PROTOCOL_F_MQ)))
+ dev->unsupported_features |= (1ull << VIRTIO_NET_F_MQ);
}
-
-
if (!mrg_rxbuf)
dev->unsupported_features |= (1ull << VIRTIO_NET_F_MRG_RXBUF);
@@ -174,11 +174,6 @@ virtio_user_delayed_handler(void *param)
if (dev->vhostfd >= 0) {
close(dev->vhostfd);
dev->vhostfd = -1;
- /* Until the featuers are negotiated again, don't assume
- * the backend supports VHOST_USER_PROTOCOL_F_STATUS
- */
- dev->protocol_features &=
- ~(1ULL << VHOST_USER_PROTOCOL_F_STATUS);
}
eth_dev->intr_handle->fd = dev->listenfd;
rte_intr_callback_register(eth_dev->intr_handle,