On 1/7/21 4:20 AM, Xia, Chenbo wrote:
> Hi Maxime,
>
>> -----Original Message-----
>> From: Maxime Coquelin <maxime.coque...@redhat.com>
>> Sent: Monday, December 21, 2020 5:14 AM
>> To: dev@dpdk.org; Xia, Chenbo <chenbo....@intel.com>; olivier.m...@6wind.com;
>> amore...@redhat.com; david.march...@redhat.com
>> Cc: Maxime Coquelin <maxime.coque...@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.coque...@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);
