From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) by dpdk.org (Postfix) with ESMTP id ACE822BDF for ; Tue, 27 Feb 2018 18:51:53 +0100 (CET) Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.rdu2.redhat.com [10.11.54.4]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 05F6A814DE8B; Tue, 27 Feb 2018 17:51:53 +0000 (UTC) Received: from [10.36.112.12] (unknown [10.36.112.12]) by smtp.corp.redhat.com (Postfix) with ESMTPS id D6BE02026E04; Tue, 27 Feb 2018 17:51:50 +0000 (UTC) To: Zhiyong Yang , dev@dpdk.org, yliu@fridaylinux.org, jianfeng.tan@intel.com, tiwei.bie@intel.com, zhihong.wang@intel.com Cc: dong1.wang@intel.com References: <20180214145330.4679-1-zhiyong.yang@intel.com> <20180214145330.4679-2-zhiyong.yang@intel.com> From: Maxime Coquelin Message-ID: <05ac437d-fb8d-30b9-a3fc-a59b648a5a2a@redhat.com> Date: Tue, 27 Feb 2018 18:51:48 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.6.0 MIME-Version: 1.0 In-Reply-To: <20180214145330.4679-2-zhiyong.yang@intel.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit X-Scanned-By: MIMEDefang 2.78 on 10.11.54.4 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.8]); Tue, 27 Feb 2018 17:51:53 +0000 (UTC) X-Greylist: inspected by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.8]); Tue, 27 Feb 2018 17:51:53 +0000 (UTC) for IP:'10.11.54.4' DOMAIN:'int-mx04.intmail.prod.int.rdu2.redhat.com' HELO:'smtp.corp.redhat.com' FROM:'maxime.coquelin@redhat.com' RCPT:'' Subject: Re: [dpdk-dev] [PATCH 1/4] vhost: move fdset functions from fd_man.c to fd_man.h X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 27 Feb 2018 17:51:54 -0000 Hi Zhiyong, On 02/14/2018 03:53 PM, Zhiyong Yang wrote: > The patch moves fdset related funcitons from fd_man.c to fd_man.h in > order to reuse these funcitons from the perspective of PMDs. > > Signed-off-by: Zhiyong Yang > --- > lib/librte_vhost/Makefile | 3 +- > lib/librte_vhost/fd_man.c | 274 ---------------------------------------------- > lib/librte_vhost/fd_man.h | 258 +++++++++++++++++++++++++++++++++++++++++-- > 3 files changed, 253 insertions(+), 282 deletions(-) > delete mode 100644 lib/librte_vhost/fd_man.c I disagree with the patch. It is a good thing to reuse the code, but to do it, you need to extend the vhost lib API. New API need to be prefixed with rte_vhost_, and be declared in rte_vhost.h. And no need to move the functions from the .c to the .h file, as it moreover makes you inline them, which is not necessary here. > diff --git a/lib/librte_vhost/Makefile b/lib/librte_vhost/Makefile > index 5d6c6abae..e201df79c 100644 > --- a/lib/librte_vhost/Makefile > +++ b/lib/librte_vhost/Makefile > @@ -21,10 +21,11 @@ endif > LDLIBS += -lrte_eal -lrte_mempool -lrte_mbuf -lrte_ethdev -lrte_net > > # all source are stored in SRCS-y > -SRCS-$(CONFIG_RTE_LIBRTE_VHOST) := fd_man.c iotlb.c socket.c vhost.c \ > +SRCS-$(CONFIG_RTE_LIBRTE_VHOST) := iotlb.c socket.c vhost.c \ > vhost_user.c virtio_net.c > > # install includes > SYMLINK-$(CONFIG_RTE_LIBRTE_VHOST)-include += rte_vhost.h > +SYMLINK-$(CONFIG_RTE_LIBRTE_VHOST)-include += fd_man.h > > include $(RTE_SDK)/mk/rte.lib.mk > diff --git a/lib/librte_vhost/fd_man.c b/lib/librte_vhost/fd_man.c > deleted file mode 100644 > index 181711c2a..000000000 > --- a/lib/librte_vhost/fd_man.c > +++ /dev/null > @@ -1,274 +0,0 @@ > -/* SPDX-License-Identifier: BSD-3-Clause > - * Copyright(c) 2010-2014 Intel Corporation > - */ > - > -#include > -#include > -#include > -#include > -#include > -#include > -#include > -#include > - > -#include > -#include > - > -#include "fd_man.h" > - > -#define FDPOLLERR (POLLERR | POLLHUP | POLLNVAL) > - > -static int > -get_last_valid_idx(struct fdset *pfdset, int last_valid_idx) > -{ > - int i; > - > - for (i = last_valid_idx; i >= 0 && pfdset->fd[i].fd == -1; i--) > - ; > - > - return i; > -} > - > -static void > -fdset_move(struct fdset *pfdset, int dst, int src) > -{ > - pfdset->fd[dst] = pfdset->fd[src]; > - pfdset->rwfds[dst] = pfdset->rwfds[src]; > -} > - > -static void > -fdset_shrink_nolock(struct fdset *pfdset) > -{ > - int i; > - int last_valid_idx = get_last_valid_idx(pfdset, pfdset->num - 1); > - > - for (i = 0; i < last_valid_idx; i++) { > - if (pfdset->fd[i].fd != -1) > - continue; > - > - fdset_move(pfdset, i, last_valid_idx); > - last_valid_idx = get_last_valid_idx(pfdset, last_valid_idx - 1); > - } > - pfdset->num = last_valid_idx + 1; > -} > - > -/* > - * Find deleted fd entries and remove them > - */ > -static void > -fdset_shrink(struct fdset *pfdset) > -{ > - pthread_mutex_lock(&pfdset->fd_mutex); > - fdset_shrink_nolock(pfdset); > - pthread_mutex_unlock(&pfdset->fd_mutex); > -} > - > -/** > - * Returns the index in the fdset for a given fd. > - * @return > - * index for the fd, or -1 if fd isn't in the fdset. > - */ > -static int > -fdset_find_fd(struct fdset *pfdset, int fd) > -{ > - int i; > - > - for (i = 0; i < pfdset->num && pfdset->fd[i].fd != fd; i++) > - ; > - > - return i == pfdset->num ? -1 : i; > -} > - > -static void > -fdset_add_fd(struct fdset *pfdset, int idx, int fd, > - fd_cb rcb, fd_cb wcb, void *dat) > -{ > - struct fdentry *pfdentry = &pfdset->fd[idx]; > - struct pollfd *pfd = &pfdset->rwfds[idx]; > - > - pfdentry->fd = fd; > - pfdentry->rcb = rcb; > - pfdentry->wcb = wcb; > - pfdentry->dat = dat; > - > - pfd->fd = fd; > - pfd->events = rcb ? POLLIN : 0; > - pfd->events |= wcb ? POLLOUT : 0; > - pfd->revents = 0; > -} > - > -void > -fdset_init(struct fdset *pfdset) > -{ > - int i; > - > - if (pfdset == NULL) > - return; > - > - for (i = 0; i < MAX_FDS; i++) { > - pfdset->fd[i].fd = -1; > - pfdset->fd[i].dat = NULL; > - } > - pfdset->num = 0; > -} > - > -/** > - * Register the fd in the fdset with read/write handler and context. > - */ > -int > -fdset_add(struct fdset *pfdset, int fd, fd_cb rcb, fd_cb wcb, void *dat) > -{ > - int i; > - > - if (pfdset == NULL || fd == -1) > - return -1; > - > - pthread_mutex_lock(&pfdset->fd_mutex); > - i = pfdset->num < MAX_FDS ? pfdset->num++ : -1; > - if (i == -1) { > - fdset_shrink_nolock(pfdset); > - i = pfdset->num < MAX_FDS ? pfdset->num++ : -1; > - if (i == -1) { > - pthread_mutex_unlock(&pfdset->fd_mutex); > - return -2; > - } > - } > - > - fdset_add_fd(pfdset, i, fd, rcb, wcb, dat); > - pthread_mutex_unlock(&pfdset->fd_mutex); > - > - return 0; > -} > - > -/** > - * Unregister the fd from the fdset. > - * Returns context of a given fd or NULL. > - */ > -void * > -fdset_del(struct fdset *pfdset, int fd) > -{ > - int i; > - void *dat = NULL; > - > - if (pfdset == NULL || fd == -1) > - return NULL; > - > - do { > - pthread_mutex_lock(&pfdset->fd_mutex); > - > - i = fdset_find_fd(pfdset, fd); > - if (i != -1 && pfdset->fd[i].busy == 0) { > - /* busy indicates r/wcb is executing! */ > - dat = pfdset->fd[i].dat; > - pfdset->fd[i].fd = -1; > - pfdset->fd[i].rcb = pfdset->fd[i].wcb = NULL; > - pfdset->fd[i].dat = NULL; > - i = -1; > - } > - pthread_mutex_unlock(&pfdset->fd_mutex); > - } while (i != -1); > - > - return dat; > -} > - > - > -/** > - * This functions runs in infinite blocking loop until there is no fd in > - * pfdset. It calls corresponding r/w handler if there is event on the fd. > - * > - * Before the callback is called, we set the flag to busy status; If other > - * thread(now rte_vhost_driver_unregister) calls fdset_del concurrently, it > - * will wait until the flag is reset to zero(which indicates the callback is > - * finished), then it could free the context after fdset_del. > - */ > -void * > -fdset_event_dispatch(void *arg) > -{ > - int i; > - struct pollfd *pfd; > - struct fdentry *pfdentry; > - fd_cb rcb, wcb; > - void *dat; > - int fd, numfds; > - int remove1, remove2; > - int need_shrink; > - struct fdset *pfdset = arg; > - int val; > - > - if (pfdset == NULL) > - return NULL; > - > - while (1) { > - > - /* > - * When poll is blocked, other threads might unregister > - * listenfds from and register new listenfds into fdset. > - * When poll returns, the entries for listenfds in the fdset > - * might have been updated. It is ok if there is unwanted call > - * for new listenfds. > - */ > - pthread_mutex_lock(&pfdset->fd_mutex); > - numfds = pfdset->num; > - pthread_mutex_unlock(&pfdset->fd_mutex); > - > - val = poll(pfdset->rwfds, numfds, 1000 /* millisecs */); > - if (val < 0) > - continue; > - > - need_shrink = 0; > - for (i = 0; i < numfds; i++) { > - pthread_mutex_lock(&pfdset->fd_mutex); > - > - pfdentry = &pfdset->fd[i]; > - fd = pfdentry->fd; > - pfd = &pfdset->rwfds[i]; > - > - if (fd < 0) { > - need_shrink = 1; > - pthread_mutex_unlock(&pfdset->fd_mutex); > - continue; > - } > - > - if (!pfd->revents) { > - pthread_mutex_unlock(&pfdset->fd_mutex); > - continue; > - } > - > - remove1 = remove2 = 0; > - > - rcb = pfdentry->rcb; > - wcb = pfdentry->wcb; > - dat = pfdentry->dat; > - pfdentry->busy = 1; > - > - pthread_mutex_unlock(&pfdset->fd_mutex); > - > - if (rcb && pfd->revents & (POLLIN | FDPOLLERR)) > - rcb(fd, dat, &remove1); > - if (wcb && pfd->revents & (POLLOUT | FDPOLLERR)) > - wcb(fd, dat, &remove2); > - pfdentry->busy = 0; > - /* > - * fdset_del needs to check busy flag. > - * We don't allow fdset_del to be called in callback > - * directly. > - */ > - /* > - * When we are to clean up the fd from fdset, > - * because the fd is closed in the cb, > - * the old fd val could be reused by when creates new > - * listen fd in another thread, we couldn't call > - * fd_set_del. > - */ > - if (remove1 || remove2) { > - pfdentry->fd = -1; > - need_shrink = 1; > - } > - } > - > - if (need_shrink) > - fdset_shrink(pfdset); > - } > - > - return NULL; > -} > diff --git a/lib/librte_vhost/fd_man.h b/lib/librte_vhost/fd_man.h > index 3a9276c3c..b1c628251 100644 > --- a/lib/librte_vhost/fd_man.h > +++ b/lib/librte_vhost/fd_man.h > @@ -4,11 +4,11 @@ > > #ifndef _FD_MAN_H_ > #define _FD_MAN_H_ > -#include > -#include > #include > +#include > > #define MAX_FDS 1024 > +#define FDPOLLERR (POLLERR | POLLHUP | POLLNVAL) > > typedef void (*fd_cb)(int fd, void *dat, int *remove); > > @@ -27,14 +27,258 @@ struct fdset { > int num; /* current fd number of this fdset */ > }; > > +static inline int > +get_last_valid_idx(struct fdset *pfdset, int last_valid_idx) > +{ > + int i; > > -void fdset_init(struct fdset *pfdset); > + for (i = last_valid_idx; i >= 0 && pfdset->fd[i].fd == -1; i--) > + ; > > -int fdset_add(struct fdset *pfdset, int fd, > - fd_cb rcb, fd_cb wcb, void *dat); > + return i; > +} > > -void *fdset_del(struct fdset *pfdset, int fd); > +static inline void > +fdset_move(struct fdset *pfdset, int dst, int src) > +{ > + pfdset->fd[dst] = pfdset->fd[src]; > + pfdset->rwfds[dst] = pfdset->rwfds[src]; > +} > > -void *fdset_event_dispatch(void *arg); > +static inline void > +fdset_shrink_nolock(struct fdset *pfdset) > +{ > + int i; > + int last_valid_idx = get_last_valid_idx(pfdset, pfdset->num - 1); > + > + for (i = 0; i < last_valid_idx; i++) { > + if (pfdset->fd[i].fd != -1) > + continue; > + > + fdset_move(pfdset, i, last_valid_idx); > + last_valid_idx = get_last_valid_idx(pfdset, last_valid_idx - 1); > + } > + pfdset->num = last_valid_idx + 1; > +} > + > +/* > + * Find deleted fd entries and remove them > + */ > +static inline void > +fdset_shrink(struct fdset *pfdset) > +{ > + pthread_mutex_lock(&pfdset->fd_mutex); > + fdset_shrink_nolock(pfdset); > + pthread_mutex_unlock(&pfdset->fd_mutex); > +} > + > +/** > + * Returns the index in the fdset for a given fd. > + * @return > + * index for the fd, or -1 if fd isn't in the fdset. > + */ > +static inline int > +fdset_find_fd(struct fdset *pfdset, int fd) > +{ > + int i; > + > + for (i = 0; i < pfdset->num && pfdset->fd[i].fd != fd; i++) > + ; > + > + return i == pfdset->num ? -1 : i; > +} > + > +static inline void > +fdset_add_fd(struct fdset *pfdset, int idx, int fd, > + fd_cb rcb, fd_cb wcb, void *dat) > +{ > + struct fdentry *pfdentry = &pfdset->fd[idx]; > + struct pollfd *pfd = &pfdset->rwfds[idx]; > + > + pfdentry->fd = fd; > + pfdentry->rcb = rcb; > + pfdentry->wcb = wcb; > + pfdentry->dat = dat; > + > + pfd->fd = fd; > + pfd->events = rcb ? POLLIN : 0; > + pfd->events |= wcb ? POLLOUT : 0; > + pfd->revents = 0; > +} > + > +static inline void > +fdset_init(struct fdset *pfdset) > +{ > + int i; > + > + if (pfdset == NULL) > + return; > + > + for (i = 0; i < MAX_FDS; i++) { > + pfdset->fd[i].fd = -1; > + pfdset->fd[i].dat = NULL; > + } > + pfdset->num = 0; > +} > + > +/** > + * Register the fd in the fdset with read/write handler and context. > + */ > +static inline int > +fdset_add(struct fdset *pfdset, int fd, fd_cb rcb, fd_cb wcb, void *dat) > +{ > + int i; > + > + if (pfdset == NULL || fd == -1) > + return -1; > + > + pthread_mutex_lock(&pfdset->fd_mutex); > + i = pfdset->num < MAX_FDS ? pfdset->num++ : -1; > + if (i == -1) { > + fdset_shrink_nolock(pfdset); > + i = pfdset->num < MAX_FDS ? pfdset->num++ : -1; > + if (i == -1) { > + pthread_mutex_unlock(&pfdset->fd_mutex); > + return -2; > + } > + } > + > + fdset_add_fd(pfdset, i, fd, rcb, wcb, dat); > + pthread_mutex_unlock(&pfdset->fd_mutex); > + > + return 0; > +} > + > +/** > + * Unregister the fd from the fdset. > + * Returns context of a given fd or NULL. > + */ > +static inline void * > +fdset_del(struct fdset *pfdset, int fd) > +{ > + int i; > + void *dat = NULL; > + > + if (pfdset == NULL || fd == -1) > + return NULL; > + > + do { > + pthread_mutex_lock(&pfdset->fd_mutex); > + > + i = fdset_find_fd(pfdset, fd); > + if (i != -1 && pfdset->fd[i].busy == 0) { > + /* busy indicates r/wcb is executing! */ > + dat = pfdset->fd[i].dat; > + pfdset->fd[i].fd = -1; > + pfdset->fd[i].rcb = pfdset->fd[i].wcb = NULL; > + pfdset->fd[i].dat = NULL; > + i = -1; > + } > + pthread_mutex_unlock(&pfdset->fd_mutex); > + } while (i != -1); > + > + return dat; > +} > + > +/** > + * This functions runs in infinite blocking loop until there is no fd in > + * pfdset. It calls corresponding r/w handler if there is event on the fd. > + * > + * Before the callback is called, we set the flag to busy status; If other > + * thread(now rte_vhost_driver_unregister) calls fdset_del concurrently, it > + * will wait until the flag is reset to zero(which indicates the callback is > + * finished), then it could free the context after fdset_del. > + */ > +static inline void * > +fdset_event_dispatch(void *arg) > +{ > + int i; > + struct pollfd *pfd; > + struct fdentry *pfdentry; > + fd_cb rcb, wcb; > + void *dat; > + int fd, numfds; > + int remove1, remove2; > + int need_shrink; > + struct fdset *pfdset = arg; > + int val; > + > + if (pfdset == NULL) > + return NULL; > + > + while (1) { > + > + /* > + * When poll is blocked, other threads might unregister > + * listenfds from and register new listenfds into fdset. > + * When poll returns, the entries for listenfds in the fdset > + * might have been updated. It is ok if there is unwanted call > + * for new listenfds. > + */ > + pthread_mutex_lock(&pfdset->fd_mutex); > + numfds = pfdset->num; > + pthread_mutex_unlock(&pfdset->fd_mutex); > + > + val = poll(pfdset->rwfds, numfds, 1000 /* millisecs */); > + if (val < 0) > + continue; > + > + need_shrink = 0; > + for (i = 0; i < numfds; i++) { > + pthread_mutex_lock(&pfdset->fd_mutex); > + > + pfdentry = &pfdset->fd[i]; > + fd = pfdentry->fd; > + pfd = &pfdset->rwfds[i]; > + > + if (fd < 0) { > + need_shrink = 1; > + pthread_mutex_unlock(&pfdset->fd_mutex); > + continue; > + } > + > + if (!pfd->revents) { > + pthread_mutex_unlock(&pfdset->fd_mutex); > + continue; > + } > + > + remove1 = remove2 = 0; > + > + rcb = pfdentry->rcb; > + wcb = pfdentry->wcb; > + dat = pfdentry->dat; > + pfdentry->busy = 1; > + > + pthread_mutex_unlock(&pfdset->fd_mutex); > + > + if (rcb && pfd->revents & (POLLIN | FDPOLLERR)) > + rcb(fd, dat, &remove1); > + if (wcb && pfd->revents & (POLLOUT | FDPOLLERR)) > + wcb(fd, dat, &remove2); > + pfdentry->busy = 0; > + /* > + * fdset_del needs to check busy flag. > + * We don't allow fdset_del to be called in callback > + * directly. > + */ > + /* > + * When we are to clean up the fd from fdset, > + * because the fd is closed in the cb, > + * the old fd val could be reused by when creates new > + * listen fd in another thread, we couldn't call > + * fd_set_del. > + */ > + if (remove1 || remove2) { > + pfdentry->fd = -1; > + need_shrink = 1; > + } > + } > + > + if (need_shrink) > + fdset_shrink(pfdset); > + } > + > + return NULL; > +} > > #endif >