Re: [dpdk-dev] [PATCH v5] net/virtio-user: add support for server mode

[Tiwei Bie <tiwei.bie@intel.com>]

On Thu, Apr 05, 2018 at 01:17:53AM +0800, zhiyong.yang@intel.com wrote:
[...]
> +static int
> +virtio_user_start_server(struct virtio_user_dev *dev, struct sockaddr_un *un)
> +{
> +	int ret;
> +	int flag;
> +	int fd = dev->listenfd;
> +
> +	ret = bind(fd, (struct sockaddr *)un, sizeof(*un));
> +	if (ret < 0) {
> +		PMD_DRV_LOG(ERR, "failed to bind to %s: %s; remove it and try again\n",
> +			    dev->path, strerror(errno));
> +		goto err;
> +	}
> +	ret = listen(fd, MAX_VIRTIO_USER_BACKLOG);
> +	if (ret < 0)
> +		goto err;
> +
> +	flag = fcntl(fd, F_GETFL);
> +	fcntl(fd, F_SETFL, flag | O_NONBLOCK);
> +	dev->vhostfd = -1;
> +
> +	return 0;
> +err:
> +	close(dev->listenfd);

The dev->listenfd isn't created in this function, maybe it's
better to avoid closing this file in this function.

> +	return -1;
> +}
> +
>  /**
>   * Set up environment to talk with a vhost user backend.
>   *
> @@ -390,6 +418,7 @@ vhost_user_setup(struct virtio_user_dev *dev)
>  {
>  	int fd;
>  	int flag;
> +	int ret = 0;
>  	struct sockaddr_un un;
>  
>  	fd = socket(AF_UNIX, SOCK_STREAM, 0);
> @@ -405,14 +434,20 @@ vhost_user_setup(struct virtio_user_dev *dev)
>  	memset(&un, 0, sizeof(un));
>  	un.sun_family = AF_UNIX;
>  	snprintf(un.sun_path, sizeof(un.sun_path), "%s", dev->path);
> -	if (connect(fd, (struct sockaddr *)&un, sizeof(un)) < 0) {
> -		PMD_DRV_LOG(ERR, "connect error, %s", strerror(errno));
> -		close(fd);
> -		return -1;
> +
> +	if (dev->is_server) {
> +		dev->listenfd = fd;
> +		ret = virtio_user_start_server(dev, &un);
> +	} else {

Maybe it's better to keep the style consistent. How
about something like this:

	if (dev->is_server) {
		if (virtio_user_start_server(fd, &un) < 0) {
			PMD_DRV_LOG(ERR, some messages...);
			close(fd);
			return -1;
		}
		dev->listenfd = fd;
		dev->vhostfd = -1;
	} else {

> +		dev->vhostfd = fd;
> +		if (connect(fd, (struct sockaddr *)&un, sizeof(un)) < 0) {
> +			PMD_DRV_LOG(ERR, "connect error, %s", strerror(errno));
> +			close(fd);
> +			return -1;
> +		}
>  	}
>  
> -	dev->vhostfd = fd;
> -	return 0;
> +	return ret;
>  }
>  
>  static int
> diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.c b/drivers/net/virtio/virtio_user/virtio_user_dev.c
> index f90fee9e5..45e324679 100644
> --- a/drivers/net/virtio/virtio_user/virtio_user_dev.c
> +++ b/drivers/net/virtio/virtio_user/virtio_user_dev.c
> @@ -254,7 +254,8 @@ virtio_user_fill_intr_handle(struct virtio_user_dev *dev)
>  	eth_dev->intr_handle->fd = -1;
>  	if (dev->vhostfd >= 0)
>  		eth_dev->intr_handle->fd = dev->vhostfd;
> -

Maybe it's better to keep this empty line (keep it before the return 0).

> +	else if (dev->is_server)
> +		eth_dev->intr_handle->fd = dev->listenfd;
>  	return 0;
>  }
>  
> @@ -267,24 +268,29 @@ virtio_user_dev_setup(struct virtio_user_dev *dev)
>  	dev->vhostfds = NULL;
>  	dev->tapfds = NULL;
>  
> -	if (is_vhost_user_by_type(dev->path)) {
> -		dev->ops = &ops_user;
> +	if (dev->is_server) {
> +		dev->ops = &ops_user;/* server mode only supports vhost user */
>  	} else {
> -		dev->ops = &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, "Failed to malloc");
> -			return -1;
> -		}
> -
> -		for (q = 0; q < dev->max_queue_pairs; ++q) {
> -			dev->vhostfds[q] = -1;
> -			dev->tapfds[q] = -1;
> +		if (is_vhost_user_by_type(dev->path)) {
> +			dev->ops = &ops_user;
> +		} else {
> +			dev->ops = &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, "Failed to malloc");
> +				return -1;
> +			}
> +
> +			for (q = 0; q < dev->max_queue_pairs; ++q) {
> +				dev->vhostfds[q] = -1;
> +				dev->tapfds[q] = -1;
> +			}
>  		}
>  	}
> -

There is no need to remove this empty line.

>  	if (dev->ops->setup(dev) < 0)
>  		return -1;
>  
> @@ -337,16 +343,21 @@ virtio_user_dev_init(struct virtio_user_dev *dev, char *path, int queues,
>  		return -1;
>  	}
>  
> -	if (dev->ops->send_request(dev, VHOST_USER_SET_OWNER, NULL) < 0) {
> -		PMD_INIT_LOG(ERR, "set_owner fails: %s", strerror(errno));
> -		return -1;
> -	}
> +	if (dev->vhostfd >= 0) {
> +		if (dev->ops->send_request(dev, VHOST_USER_SET_OWNER, NULL) < 0) {
> +			PMD_INIT_LOG(ERR, "set_owner fails: %s", strerror(errno));
> +			return -1;
> +		}
>  
> -	if (dev->ops->send_request(dev, VHOST_USER_GET_FEATURES,
> -			    &dev->device_features) < 0) {
> -		PMD_INIT_LOG(ERR, "get_features failed: %s", strerror(errno));
> -		return -1;
> +		if (dev->ops->send_request(dev, VHOST_USER_GET_FEATURES,
> +				&dev->device_features) < 0) {
> +			PMD_INIT_LOG(ERR, "get_features failed: %s", strerror(errno));
> +			return -1;
> +		}
> +	} else {
> +		dev->device_features = VIRTIO_USER_SUPPORTED_FEATURES;

If the backend doesn't support e.g. VIRTIO_RING_F_INDIRECT_DESC.
Will it cause any problem?

>  	}
> +
>  	if (dev->mac_specified)
>  		dev->device_features |= (1ull << VIRTIO_NET_F_MAC);
>  
> @@ -388,6 +399,11 @@ virtio_user_dev_uninit(struct virtio_user_dev *dev)
>  
>  	close(dev->vhostfd);
>  
> +	if (dev->is_server && dev->listenfd >= 0) {
> +		close(dev->listenfd);
> +		dev->listenfd = -1;
> +	}
> +
>  	if (dev->vhostfds) {
>  		for (i = 0; i < dev->max_queue_pairs; ++i)
>  			close(dev->vhostfds[i]);
> @@ -396,6 +412,9 @@ virtio_user_dev_uninit(struct virtio_user_dev *dev)
>  	}
>  
>  	free(dev->ifname);
> +
> +	if (dev->is_server)
> +		unlink(dev->path);
>  }
[...]
>  
>  static int
>  get_string_arg(const char *key __rte_unused,
> @@ -378,10 +438,12 @@ virtio_user_pmd_probe(struct rte_vdev_device *dev)
>  	uint64_t queues = VIRTIO_USER_DEF_Q_NUM;
>  	uint64_t cq = VIRTIO_USER_DEF_CQ_EN;
>  	uint64_t queue_size = VIRTIO_USER_DEF_Q_SZ;
> +	uint64_t server_mode = VIRTIO_USER_DEF_SERVER_MODE;
>  	char *path = NULL;
>  	char *ifname = NULL;
>  	char *mac_addr = NULL;
>  	int ret = -1;
> +	struct virtio_user_dev *vu_dev = NULL;

Maybe it's better to move the definition of vu_dev after
eth_dev. And there isn't no need to initialize it.

Thanks

>  
>  	kvlist = rte_kvargs_parse(rte_vdev_device_args(dev), valid_args);
>  	if (!kvlist) {
> @@ -445,6 +507,15 @@ virtio_user_pmd_probe(struct rte_vdev_device *dev)
>  		}
>  	}
>  
> +	if (rte_kvargs_count(kvlist, VIRTIO_USER_ARG_SERVER_MODE) == 1) {
> +		if (rte_kvargs_process(kvlist, VIRTIO_USER_ARG_SERVER_MODE,
> +				       &get_integer_arg, &server_mode) < 0) {
> +			PMD_INIT_LOG(ERR, "error to parse %s",
> +				     VIRTIO_USER_ARG_SERVER_MODE);
> +			goto end;
> +		}
> +	}
> +
>  	if (rte_kvargs_count(kvlist, VIRTIO_USER_ARG_CQ_NUM) == 1) {
>  		if (rte_kvargs_process(kvlist, VIRTIO_USER_ARG_CQ_NUM,
>  				       &get_integer_arg, &cq) < 0) {
> @@ -476,6 +547,11 @@ virtio_user_pmd_probe(struct rte_vdev_device *dev)
>  		}
>  
>  		hw = eth_dev->data->dev_private;
> +		vu_dev = virtio_user_get_dev(hw);
> +		if (server_mode == 1)
> +			vu_dev->is_server = true;
> +		else
> +			vu_dev->is_server = false;
>  		if (virtio_user_dev_init(hw->virtio_user_dev, path, queues, cq,
>  				 queue_size, mac_addr, &ifname) < 0) {
>  			PMD_INIT_LOG(ERR, "virtio_user_dev_init fails");
> -- 
> 2.14.3
>