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

[Maxime Coquelin <maxime.coquelin@redhat.com>]

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