[dpdk-dev] [PATCH 0/2] add pipe event for optimizing negotiating

[dpdk-dev] [PATCH 0/2] add pipe event for optimizing negotiating

[xiangxia.m.yue]

From: Tonghao Zhang <xiangxia.m.yue@gmail.com>

These patches depend on previous patches.
1. http://dpdk.org/dev/patchwork/patch/36440/
2. http://dpdk.org/dev/patchwork/patch/36441/

Tonghao Zhang (2):
  vhost: move stdbool header file to vhost.h
  vhost: add pipe event for optimizing negotiating

 lib/librte_vhost/fd_man.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++
 lib/librte_vhost/fd_man.h | 16 ++++++++++++++++
 lib/librte_vhost/socket.c | 15 ++++++++++++++-
 lib/librte_vhost/vhost.h  |  1 +
 4 files changed, 80 insertions(+), 1 deletion(-)

-- 
1.8.3.1

[dpdk-dev] [PATCH 1/2] vhost: move stdbool header file to vhost.h

[xiangxia.m.yue]

From: Tonghao Zhang <xiangxia.m.yue@gmail.com>

The vhost.h file uses bool type, but not include stdbool
header file. If other c files include vhost.h directly,
there will be a compile error.

This patch will be used in the next patch.

Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
---
 lib/librte_vhost/socket.c | 1 -
 lib/librte_vhost/vhost.h  | 1 +
 2 files changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/librte_vhost/socket.c b/lib/librte_vhost/socket.c
index 1352339..95bed78 100644
--- a/lib/librte_vhost/socket.c
+++ b/lib/librte_vhost/socket.c
@@ -4,7 +4,6 @@
 
 #include <stdint.h>
 #include <stdio.h>
-#include <stdbool.h>
 #include <limits.h>
 #include <stdlib.h>
 #include <unistd.h>
diff --git a/lib/librte_vhost/vhost.h b/lib/librte_vhost/vhost.h
index d947bc9..37edb48 100644
--- a/lib/librte_vhost/vhost.h
+++ b/lib/librte_vhost/vhost.h
@@ -6,6 +6,7 @@
 #define _VHOST_NET_CDEV_H_
 #include <stdint.h>
 #include <stdio.h>
+#include <stdbool.h>
 #include <sys/types.h>
 #include <sys/queue.h>
 #include <unistd.h>
-- 
1.8.3.1

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

[xiangxia.m.yue]

From: Tonghao Zhang <xiangxia.m.yue@gmail.com>

When vhost—user connects qemu successfully, dpdk will call
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"
 
 #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

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

[Tan, Jianfeng]

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

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

[Tonghao Zhang]

On Thu, Mar 29, 2018 at 3:32 PM, Tan, Jianfeng <jianfeng.tan@intel.com> wrote:
> 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: "-"
v2 will update it. :)

>> 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.

Hi Jianfeng, thanks for your review. In the fdset_pipe_init function,
I call the RTE_LOG with VHOST_CONFIG, when init pipe not successfully.
So I included the vhost 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
>

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

[Maxime Coquelin]

Hi Xiangxia,

On 03/29/2018 01:24 PM, Tonghao Zhang wrote:
> On Thu, Mar 29, 2018 at 3:32 PM, Tan, Jianfeng <jianfeng.tan@intel.com> wrote:
>> 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: "-"
> v2 will update it. :)
> 
>>> 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.
> 
> Hi Jianfeng, thanks for your review. In the fdset_pipe_init function,
> I call the RTE_LOG with VHOST_CONFIG, when init pipe not successfully.
> So I included the vhost header file.

Maybe better to create VHOST_SOCKET in socket.c, than including vhost.h 
that creates a layer violation:

#define RTE_LOGTYPE_VHOST_SOCKET RTE_LOGTYPE_USER1

Is it Ok for you? I so no need to resend, I can handle the change when
applying.

Thanks,
Maxime
> 
> 
>> 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
>>

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

[Tonghao Zhang]

On Fri, Mar 30, 2018 at 3:57 PM, Maxime Coquelin
<maxime.coquelin@redhat.com> wrote:
> Hi Xiangxia,
>
>
> On 03/29/2018 01:24 PM, Tonghao Zhang wrote:
>>
>> On Thu, Mar 29, 2018 at 3:32 PM, Tan, Jianfeng <jianfeng.tan@intel.com>
>> wrote:
>>>
>>> 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: "-"
>>
>> v2 will update it. :)
>>
>>>> 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.
>>
>>
>> Hi Jianfeng, thanks for your review. In the fdset_pipe_init function,
>> I call the RTE_LOG with VHOST_CONFIG, when init pipe not successfully.
>> So I included the vhost header file.
>
>
> Maybe better to create VHOST_SOCKET in socket.c, than including vhost.h that
> creates a layer violation:
>
> #define RTE_LOGTYPE_VHOST_SOCKET RTE_LOGTYPE_USER1
>
> Is it Ok for you? I so no need to resend, I can handle the change when
> applying.
yes, thanks.

>
> Thanks,
> Maxime
>
>>
>>
>>> 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
>>>
>>>
>

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

[Maxime Coquelin]

On 03/30/2018 10:07 AM, Tonghao Zhang wrote:
> On Fri, Mar 30, 2018 at 3:57 PM, Maxime Coquelin
> <maxime.coquelin@redhat.com> wrote:
>> Hi Xiangxia,
>>
>>
>> On 03/29/2018 01:24 PM, Tonghao Zhang wrote:
>>>
>>> On Thu, Mar 29, 2018 at 3:32 PM, Tan, Jianfeng <jianfeng.tan@intel.com>
>>> wrote:
>>>>
>>>> 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: "-"
>>>
>>> v2 will update it. :)
>>>
>>>>> 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.
>>>
>>>
>>> Hi Jianfeng, thanks for your review. In the fdset_pipe_init function,
>>> I call the RTE_LOG with VHOST_CONFIG, when init pipe not successfully.
>>> So I included the vhost header file.
>>
>>
>> Maybe better to create VHOST_SOCKET in socket.c, than including vhost.h that
>> creates a layer violation:
>>
>> #define RTE_LOGTYPE_VHOST_SOCKET RTE_LOGTYPE_USER1
>>
>> Is it Ok for you? I so no need to resend, I can handle the change when
>> applying.
> yes, thanks.

Great, so with above change:

Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Thanks,
Maxime
>>
>> Thanks,
>> Maxime
>>
>>>
>>>
>>>> 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
>>>>
>>>>
>>

Re: [dpdk-dev] [PATCH 1/2] vhost: move stdbool header file to vhost.h

[Maxime Coquelin]

On 03/28/2018 07:49 AM, xiangxia.m.yue@gmail.com wrote:
> From: Tonghao Zhang <xiangxia.m.yue@gmail.com>
> 
> The vhost.h file uses bool type, but not include stdbool
> header file. If other c files include vhost.h directly,
> there will be a compile error.
> 
> This patch will be used in the next patch.
> 
> Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
> ---
>   lib/librte_vhost/socket.c | 1 -
>   lib/librte_vhost/vhost.h  | 1 +
>   2 files changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Maxime Cqouelin <maxime.coquelin@redhat.com>

Thanks,
Maxime

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

[Maxime Coquelin]

On 03/28/2018 07:49 AM, xiangxia.m.yue@gmail.com wrote:
> From: Tonghao Zhang <xiangxia.m.yue@gmail.com>
> 
> These patches depend on previous patches.
> 1. http://dpdk.org/dev/patchwork/patch/36440/
> 2. http://dpdk.org/dev/patchwork/patch/36441/
> 
> Tonghao Zhang (2):
>    vhost: move stdbool header file to vhost.h
>    vhost: add pipe event for optimizing negotiating
> 
>   lib/librte_vhost/fd_man.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++
>   lib/librte_vhost/fd_man.h | 16 ++++++++++++++++
>   lib/librte_vhost/socket.c | 15 ++++++++++++++-
>   lib/librte_vhost/vhost.h  |  1 +
>   4 files changed, 80 insertions(+), 1 deletion(-)
> 

Series applied to dpdk-next-virtio/master with suggested changes.

Thanks,
Maxime