Re: [dpdk-dev] [PATCH 2/2] vhost: add pipe event for optimizing negotiating

["Tan, Jianfeng" <jianfeng.tan@intel.com>]

Hi Xiangxia,

> -----Original Message-----
> From: xiangxia.m.yue@gmail.com [mailto:xiangxia.m.yue@gmail.com]
> Sent: Wednesday, March 28, 2018 1:49 PM
> To: Tan, Jianfeng
> Cc: dev@dpdk.org; Tonghao Zhang
> Subject: [PATCH 2/2] vhost: add pipe event for optimizing negotiating
> 
> From: Tonghao Zhang <xiangxia.m.yue@gmail.com>
> 
> When vhost—user connects qemu successfully, dpdk will call

Typo: "-"

> the vhost_user_add_connection to add unix socket fd to poll.
> And fdset_add only set the socket fd to a fdentry while poll
> may sleep now. In a general case, this is no problem. But if
> we use hot update for vhost-user, most downtime of VMs network
> is 750+ms. This patch adds pipe event, so after connections are
> ok, dpdk rebuild the poll immediately. With this patch, the
> most downtime is 20~30ms.
> 
> Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
> ---
>  lib/librte_vhost/fd_man.c | 49
> +++++++++++++++++++++++++++++++++++++++++++++++
>  lib/librte_vhost/fd_man.h | 16 ++++++++++++++++
>  lib/librte_vhost/socket.c | 14 ++++++++++++++
>  3 files changed, 79 insertions(+)
> 
> diff --git a/lib/librte_vhost/fd_man.c b/lib/librte_vhost/fd_man.c
> index 181711c..7716757 100644
> --- a/lib/librte_vhost/fd_man.c
> +++ b/lib/librte_vhost/fd_man.c
> @@ -15,6 +15,7 @@
>  #include <rte_log.h>
> 
>  #include "fd_man.h"
> +#include "vhost.h"

This is a nice finding and solution, however, I don't think we shall include vhost header file in fd related files. Actually, I did not find out why you need to include this header file.

Thanks,
Jianfeng

> 
>  #define FDPOLLERR (POLLERR | POLLHUP | POLLNVAL)
> 
> @@ -272,3 +273,51 @@
> 
>  	return NULL;
>  }
> +
> +static void
> +fdset_pipe_read_cb(int readfd, void *dat __rte_unused,
> +		   int *remove __rte_unused)
> +{
> +	char charbuf[16];
> +	read(readfd, charbuf, sizeof(charbuf));
> +}
> +
> +void
> +fdset_pipe_uninit(struct fdset *fdset)
> +{
> +	fdset_del(fdset, fdset->u.readfd);
> +	close(fdset->u.readfd);
> +	close(fdset->u.writefd);
> +}
> +
> +int
> +fdset_pipe_init(struct fdset *fdset)
> +{
> +	int ret;
> +
> +	if (pipe(fdset->u.pipefd) < 0) {
> +		RTE_LOG(ERR, VHOST_CONFIG,
> +			"failed to create pipe for vhost fdset\n");
> +		return -1;
> +	}
> +
> +	ret = fdset_add(fdset, fdset->u.readfd,
> +			fdset_pipe_read_cb, NULL, NULL);
> +
> +	if (ret < 0) {
> +		RTE_LOG(ERR, VHOST_CONFIG,
> +			"failed to add pipe readfd %d into vhost server
> fdset\n",
> +			fdset->u.readfd);
> +
> +		fdset_pipe_uninit(fdset);
> +		return -1;
> +	}
> +
> +	return 0;
> +}
> +
> +void
> +fdset_pipe_notify(struct fdset *fdset)
> +{
> +	write(fdset->u.writefd, "1", 1);
> +}
> diff --git a/lib/librte_vhost/fd_man.h b/lib/librte_vhost/fd_man.h
> index 3a9276c..76a42fb 100644
> --- a/lib/librte_vhost/fd_man.h
> +++ b/lib/librte_vhost/fd_man.h
> @@ -25,6 +25,16 @@ struct fdset {
>  	struct fdentry fd[MAX_FDS];
>  	pthread_mutex_t fd_mutex;
>  	int num;	/* current fd number of this fdset */
> +
> +	union pipefds {
> +		struct {
> +			int pipefd[2];
> +		};
> +		struct {
> +			int readfd;
> +			int writefd;
> +		};
> +	} u;
>  };
> 
> 
> @@ -37,4 +47,10 @@ int fdset_add(struct fdset *pfdset, int fd,
> 
>  void *fdset_event_dispatch(void *arg);
> 
> +int fdset_pipe_init(struct fdset *fdset);
> +
> +void fdset_pipe_uninit(struct fdset *fdset);
> +
> +void fdset_pipe_notify(struct fdset *fdset);
> +
>  #endif
> diff --git a/lib/librte_vhost/socket.c b/lib/librte_vhost/socket.c
> index 95bed78..795239c 100644
> --- a/lib/librte_vhost/socket.c
> +++ b/lib/librte_vhost/socket.c
> @@ -231,6 +231,8 @@ struct vhost_user {
>  	pthread_mutex_lock(&vsocket->conn_mutex);
>  	TAILQ_INSERT_TAIL(&vsocket->conn_list, conn, next);
>  	pthread_mutex_unlock(&vsocket->conn_mutex);
> +
> +	fdset_pipe_notify(&vhost_user.fdset);
>  	return;
> 
>  err:
> @@ -829,11 +831,23 @@ struct vhost_device_ops const *
>  		return -1;
> 
>  	if (fdset_tid == 0) {
> +		/**
> +		 * create a pipe which will be waited by poll and notified to
> +		 * rebuild the wait list of poll.
> +		 */
> +		if (fdset_pipe_init(&vhost_user.fdset) < 0) {
> +			RTE_LOG(ERR, VHOST_CONFIG,
> +				"failed to create pipe for vhost fdset\n");
> +			return -1;
> +		}
> +
>  		int ret = pthread_create(&fdset_tid, NULL, fdset_event_dispatch,
>  				     &vhost_user.fdset);
>  		if (ret != 0) {
>  			RTE_LOG(ERR, VHOST_CONFIG,
>  				"failed to create fdset handling thread");
> +
> +			fdset_pipe_uninit(&vhost_user.fdset);
>  			return -1;
>  		} else {
>  			snprintf(thread_name, RTE_MAX_THREAD_NAME_LEN,
> --
> 1.8.3.1