Re: [dpdk-dev] [PATCH 2/6] vhost: add vhost-user client mode

[Victor Kaplansky <vkaplans@redhat.com>]

Adding a flag for a future extension seems fine to me.
Could we manage without adding a flag?
For example, could we always try a client mode for a while at first,
and if unsuccessful, then come up with server mode?

-- Victor

----- Original Message -----
> From: "Yuanhan Liu" <yuanhan.liu@linux.intel.com>
> To: dev@dpdk.org
> Cc: "huawei xie" <huawei.xie@intel.com>, "Yuanhan Liu" <yuanhan.liu@linux.intel.com>
> Sent: Saturday, May 7, 2016 9:40:20 AM
> Subject: [dpdk-dev] [PATCH 2/6] vhost: add vhost-user client mode
> 
> Add a new paramter (flags) to rte_vhost_driver_register(). DPDK
> vhost-user acts as client mode when RTE_VHOST_USER_CLIENT flag
> is set.
> 
> The flags would also allow future extensions without breaking the
> API (again).
> 
> The rest is straingfoward then: allocate a unix socket, and
> bind/listen for server, connect for client.
> 
> Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
> ---
>  drivers/net/vhost/rte_eth_vhost.c            |   2 +-
>  examples/vhost/main.c                        |   2 +-
>  lib/librte_vhost/rte_virtio_net.h            |  11 +-
>  lib/librte_vhost/vhost_user/vhost-net-user.c | 215
>  ++++++++++++++++-----------
>  4 files changed, 142 insertions(+), 88 deletions(-)
> 
> diff --git a/drivers/net/vhost/rte_eth_vhost.c
> b/drivers/net/vhost/rte_eth_vhost.c
> index a9dada5..36697cf 100644
> --- a/drivers/net/vhost/rte_eth_vhost.c
> +++ b/drivers/net/vhost/rte_eth_vhost.c
> @@ -456,7 +456,7 @@ eth_dev_start(struct rte_eth_dev *dev)
>  	int ret = 0;
>  
>  	if (rte_atomic16_cmpset(&internal->once, 0, 1)) {
> -		ret = rte_vhost_driver_register(internal->iface_name);
> +		ret = rte_vhost_driver_register(internal->iface_name, 0);
>  		if (ret)
>  			return ret;
>  	}
> diff --git a/examples/vhost/main.c b/examples/vhost/main.c
> index bbf0d28..6899189 100644
> --- a/examples/vhost/main.c
> +++ b/examples/vhost/main.c
> @@ -1499,7 +1499,7 @@ main(int argc, char *argv[])
>  		rte_vhost_feature_disable(1ULL << VIRTIO_NET_F_MRG_RXBUF);
>  
>  	/* Register vhost(cuse or user) driver to handle vhost messages. */
> -	ret = rte_vhost_driver_register((char *)&dev_basename);
> +	ret = rte_vhost_driver_register(dev_basename, 0);
>  	if (ret != 0)
>  		rte_exit(EXIT_FAILURE, "vhost driver register failure.\n");
>  
> diff --git a/lib/librte_vhost/rte_virtio_net.h
> b/lib/librte_vhost/rte_virtio_net.h
> index 4e50425..c84e7ab 100644
> --- a/lib/librte_vhost/rte_virtio_net.h
> +++ b/lib/librte_vhost/rte_virtio_net.h
> @@ -51,6 +51,8 @@
>  #include <rte_mempool.h>
>  #include <rte_ether.h>
>  
> +#define RTE_VHOST_USER_CLIENT		(1ULL << 0)
> +
>  struct rte_mbuf;
>  
>  #define VHOST_MEMORY_MAX_NREGIONS 8
> @@ -96,11 +98,14 @@ uint64_t rte_vhost_feature_get(void);
>  
>  int rte_vhost_enable_guest_notification(int vid, uint16_t queue_id, int
>  enable);
>  
> -/* Register vhost driver. dev_name could be different for multiple instance
> support. */
> -int rte_vhost_driver_register(const char *dev_name);
> +/**
> + * Register vhost driver. path could be different for multiple
> + * instance support.
> + */
> +int rte_vhost_driver_register(const char *path, uint64_t flags);
>  
>  /* Unregister vhost driver. This is only meaningful to vhost user. */
> -int rte_vhost_driver_unregister(const char *dev_name);
> +int rte_vhost_driver_unregister(const char *path);
>  
>  /* Register callbacks. */
>  int rte_vhost_driver_callback_register(struct virtio_net_device_ops const *
>  const);
> diff --git a/lib/librte_vhost/vhost_user/vhost-net-user.c
> b/lib/librte_vhost/vhost_user/vhost-net-user.c
> index f485a3b..aa98717 100644
> --- a/lib/librte_vhost/vhost_user/vhost-net-user.c
> +++ b/lib/librte_vhost/vhost_user/vhost-net-user.c
> @@ -58,6 +58,7 @@
>  struct vhost_user_socket {
>  	char *path;
>  	int listenfd;
> +	int is_server;
>  };
>  
>  struct vhost_user_connection {
> @@ -75,7 +76,7 @@ struct vhost_user {
>  
>  #define MAX_VIRTIO_BACKLOG 128
>  
> -static void vhost_user_new_connection(int fd, void *data, int *remove);
> +static void vhost_user_server_new_connection(int fd, void *data, int
> *remove);
>  static void vhost_user_msg_handler(int fd, void *dat, int *remove);
>  
>  static struct vhost_user vhost_user = {
> @@ -111,48 +112,6 @@ static const char *vhost_message_str[VHOST_USER_MAX] = {
>  	[VHOST_USER_SEND_RARP]  = "VHOST_USER_SEND_RARP",
>  };
>  
> -/**
> - * Create a unix domain socket, bind to path and listen for connection.
> - * @return
> - *  socket fd or -1 on failure
> - */
> -static int
> -uds_socket(const char *path)
> -{
> -	struct sockaddr_un un;
> -	int sockfd;
> -	int ret;
> -
> -	if (path == NULL)
> -		return -1;
> -
> -	sockfd = socket(AF_UNIX, SOCK_STREAM, 0);
> -	if (sockfd < 0)
> -		return -1;
> -	RTE_LOG(INFO, VHOST_CONFIG, "socket created, fd:%d\n", sockfd);
> -
> -	memset(&un, 0, sizeof(un));
> -	un.sun_family = AF_UNIX;
> -	snprintf(un.sun_path, sizeof(un.sun_path), "%s", path);
> -	ret = bind(sockfd, (struct sockaddr *)&un, sizeof(un));
> -	if (ret == -1) {
> -		RTE_LOG(ERR, VHOST_CONFIG, "fail to bind fd:%d, remove file:%s and try
> again.\n",
> -			sockfd, path);
> -		goto err;
> -	}
> -	RTE_LOG(INFO, VHOST_CONFIG, "bind to %s\n", path);
> -
> -	ret = listen(sockfd, MAX_VIRTIO_BACKLOG);
> -	if (ret == -1)
> -		goto err;
> -
> -	return sockfd;
> -
> -err:
> -	close(sockfd);
> -	return -1;
> -}
> -
>  /* return bytes# of read on success or negative val on failure. */
>  static int
>  read_fd_message(int sockfd, char *buf, int buflen, int *fds, int fd_num)
> @@ -287,32 +246,24 @@ send_vhost_message(int sockfd, struct VhostUserMsg
> *msg)
>  	return ret;
>  }
>  
> -/* call back when there is new vhost-user connection.  */
> +
>  static void
> -vhost_user_new_connection(int fd, void *dat, int *remove __rte_unused)
> +vhost_user_add_connection(int fd, struct vhost_user_socket *vsocket)
>  {
> -	struct vhost_user_socket *vsocket = dat;
> -	int conn_fd;
> -	struct vhost_user_connection *conn;
>  	int vid;
> -	unsigned int size;
> -
> -	conn_fd = accept(fd, NULL, NULL);
> -	RTE_LOG(INFO, VHOST_CONFIG,
> -		"new virtio connection is %d\n", conn_fd);
> -	if (conn_fd < 0)
> -		return;
> +	size_t size;
> +	struct vhost_user_connection *conn;
>  
> -	conn = calloc(1, sizeof(*conn));
> +	conn = malloc(sizeof(*conn));
>  	if (conn == NULL) {
> -		close(conn_fd);
> +		close(fd);
>  		return;
>  	}
>  
>  	vid = vhost_new_device();
>  	if (vid == -1) {
> +		close(fd);
>  		free(conn);
> -		close(conn_fd);
>  		return;
>  	}
>  
> @@ -323,8 +274,21 @@ vhost_user_new_connection(int fd, void *dat, int *remove
> __rte_unused)
>  
>  	conn->vsocket = vsocket;
>  	conn->vid = vid;
> -	fdset_add(&vhost_user.fdset,
> -		conn_fd, vhost_user_msg_handler, NULL, conn);
> +	fdset_add(&vhost_user.fdset, fd, vhost_user_msg_handler, NULL, conn);
> +}
> +
> +/* call back when there is new vhost-user connection from client  */
> +static void
> +vhost_user_server_new_connection(int fd, void *dat, int *remove
> __rte_unused)
> +{
> +	struct vhost_user_socket *vsocket = dat;
> +
> +	fd = accept(fd, NULL, NULL);
> +	if (fd < 0)
> +		return;
> +
> +	RTE_LOG(INFO, VHOST_CONFIG, "new vhost user connection is %d\n", fd);
> +	vhost_user_add_connection(fd, vsocket);
>  }
>  
>  /* callback when there is message on the connfd */
> @@ -452,50 +416,135 @@ vhost_user_msg_handler(int connfd, void *dat, int
> *remove)
>  	}
>  }
>  
> -/**
> - * Creates and initialise the vhost server.
> - */
> -int
> -rte_vhost_driver_register(const char *path)
> +static int
> +create_unix_socket(const char *path, struct sockaddr_un *un, int is_server)
>  {
> -	struct vhost_user_socket *vsocket;
> +	int fd;
>  
> -	pthread_mutex_lock(&vhost_user.mutex);
> +	fd = socket(AF_UNIX, SOCK_STREAM, 0);
> +	if (fd < 0)
> +		return -1;
> +	RTE_LOG(INFO, VHOST_CONFIG, "vhost-user %s: socket created, fd: %d\n",
> +		is_server ? "server" : "client", fd);
>  
> -	if (vhost_user.vsocket_cnt == MAX_VHOST_SOCKET) {
> -		RTE_LOG(ERR, VHOST_CONFIG,
> -			"error: the number of servers reaches maximum\n");
> -		pthread_mutex_unlock(&vhost_user.mutex);
> +	memset(un, 0, sizeof(*un));
> +	un->sun_family = AF_UNIX;
> +	strncpy(un->sun_path, path, sizeof(un->sun_path));
> +
> +	return fd;
> +}
> +
> +static int
> +vhost_user_create_server(struct vhost_user_socket *vsocket)
> +{
> +	int fd;
> +	int ret;
> +	struct sockaddr_un un;
> +	const char *path = vsocket->path;
> +
> +	fd = create_unix_socket(path, &un, vsocket->is_server);
> +	if (fd < 0)
>  		return -1;
> +
> +	ret = bind(fd, (struct sockaddr *)&un, sizeof(un));
> +	if (ret < 0) {
> +		RTE_LOG(ERR, VHOST_CONFIG,
> +			"failed to bind to %s: %s; remove it and try again\n",
> +			path, strerror(errno));
> +		goto err;
>  	}
> +	RTE_LOG(INFO, VHOST_CONFIG, "bind to %s\n", path);
>  
> -	vsocket = calloc(sizeof(struct vhost_user_socket), 1);
> -	if (vsocket == NULL) {
> -		pthread_mutex_unlock(&vhost_user.mutex);
> +	ret = listen(fd, MAX_VIRTIO_BACKLOG);
> +	if (ret < 0)
> +		goto err;
> +
> +	vsocket->listenfd = fd;
> +	fdset_add(&vhost_user.fdset, fd, vhost_user_server_new_connection,
> +		  NULL, vsocket);
> +
> +	return 0;
> +
> +err:
> +	close(fd);
> +	return -1;
> +}
> +
> +static int
> +vhost_user_create_client(struct vhost_user_socket *vsocket)
> +{
> +	int fd;
> +	int ret;
> +	struct sockaddr_un un;
> +	const char *path = vsocket->path;
> +
> +	fd = create_unix_socket(path, &un, vsocket->is_server);
> +	if (fd < 0)
> +		return -1;
> +
> +	ret = connect(fd, (struct sockaddr *)&un, sizeof(un));
> +	if (ret < 0) {
> +		RTE_LOG(ERR, VHOST_CONFIG, "failed to connect to %s: %s\n",
> +			path, strerror(errno));
> +		close(fd);
>  		return -1;
>  	}
>  
> -	vsocket->listenfd = uds_socket(path);
> -	if (vsocket->listenfd < 0) {
> -		free(vsocket);
> -		pthread_mutex_unlock(&vhost_user.mutex);
> +	vhost_user_add_connection(fd, vsocket);
> +
> +	return 0;
> +}
> +
> +/*
> + * Register a new vhost-user socket; here we could act as server
> + * (the default case), or client (when RTE_VHOST_USER_CLIENT) flag
> + * is set.
> + */
> +int
> +rte_vhost_driver_register(const char *path, uint64_t flags)
> +{
> +	int ret = -1;
> +	struct vhost_user_socket *vsocket;
> +
> +	if (!path)
>  		return -1;
> +
> +	pthread_mutex_lock(&vhost_user.mutex);
> +
> +	if (vhost_user.vsocket_cnt == MAX_VHOST_SOCKET) {
> +		RTE_LOG(ERR, VHOST_CONFIG,
> +			"error: the number of vhost sockets reaches maximum\n");
> +		goto out;
>  	}
>  
> +	vsocket = malloc(sizeof(struct vhost_user_socket));
> +	if (!vsocket)
> +		goto out;
> +	memset(vsocket, 0, sizeof(struct vhost_user_socket));
>  	vsocket->path = strdup(path);
>  
> -	fdset_add(&vhost_user.fdset, vsocket->listenfd,
> -		vhost_user_new_connection, NULL, vsocket);
> +	if ((flags & RTE_VHOST_USER_CLIENT) != 0) {
> +		ret = vhost_user_create_client(vsocket);
> +	} else {
> +		vsocket->is_server = 1;
> +		ret = vhost_user_create_server(vsocket);
> +	}
> +	if (ret < 0) {
> +		free(vsocket->path);
> +		free(vsocket);
> +		goto out;
> +	}
>  
>  	vhost_user.vsockets[vhost_user.vsocket_cnt++] = vsocket;
> +
> +out:
>  	pthread_mutex_unlock(&vhost_user.mutex);
>  
> -	return 0;
> +	return ret;
>  }
>  
> -
>  /**
> - * Unregister the specified vhost server
> + * Unregister the specified vhost socket
>   */
>  int
>  rte_vhost_driver_unregister(const char *path)
> --
> 1.9.0
> 
>