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

[Tonghao Zhang <xiangxia.m.yue@gmail.com>]

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