[dpdk-dev] [PATCH 1/4] vhost: move fdset functions from fd_man.c to fd_man.h
Maxime Coquelin
maxime.coquelin at redhat.com
Tue Feb 27 18:51:48 CET 2018
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 <zhiyong.yang at intel.com>
> ---
> 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 <stdint.h>
> -#include <stdio.h>
> -#include <stdlib.h>
> -#include <sys/socket.h>
> -#include <sys/time.h>
> -#include <sys/types.h>
> -#include <unistd.h>
> -#include <string.h>
> -
> -#include <rte_common.h>
> -#include <rte_log.h>
> -
> -#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 <stdint.h>
> -#include <pthread.h>
> #include <poll.h>
> +#include <stdio.h>
>
> #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
>
More information about the dev
mailing list