Re: [dpdk-dev] [PATCH 1/4] vhost: move fdset functions from fd_man.c to fd_man.h

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

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