[dpdk-dev] [PATCH 35/40] net/virtio: make server mode blocking

[Maxime Coquelin]

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