Re: [dpdk-dev] [PATCH] vhost: allow for many vhost user ports

[Yuanhan Liu <yuanhan.liu@linux.intel.com>]

On Thu, Dec 01, 2016 at 04:26:50PM +0100, Jan Wickbom wrote:
> Currently select() is used to monitor file descriptors for vhostuser
> ports. This limits the number of ports possible to create since the
> fd number is used as index in the fd_set and we have seen fds > 1023.

Yeah, it's a known issue for a while. Thanks for making a patch to fix
that!

> This patch changes select() to poll(). This way we can keep an
> packed (pollfd) array for the fds, e.g. as many fds as the size of
> the array.
> 
> Also see:
> http://dpdk.org/ml/archives/dev/2016-April/037024.html
> 
...
> +/**
> + * Adjusts the highest index populated in the array of fds

This function only shrinks (but not extends) the fdset array, so why
not naming it to something like "fdset_shrink".

> + * @return
> + *   Index of highest position populated + 1.

I think it's clearer to say "The new size of fdset".

  
> @@ -189,7 +206,7 @@
>  			pfdset->fd[i].fd = -1;
>  			pfdset->fd[i].rcb = pfdset->fd[i].wcb = NULL;
>  			pfdset->fd[i].dat = NULL;
> -			pfdset->num--;
> +			(void) fdset_adjust_num(pfdset);

Unncessary cast.

>  			i = -1;
>  		}
>  		pthread_mutex_unlock(&pfdset->fd_mutex);
> @@ -211,12 +228,12 @@
>  
>  	pfdset->fd[index].fd = -1;
>  	pfdset->fd[index].rcb = pfdset->fd[index].wcb = NULL;
> -	pfdset->fd[index].dat = NULL;
> -	pfdset->num--;
> +	(void) fdset_adjust_num(pfdset);
>  
>  	pthread_mutex_unlock(&pfdset->fd_mutex);
>  }
>  
> +
>  /**
>   * 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.
> @@ -229,44 +246,48 @@
>  void
>  fdset_event_dispatch(struct fdset *pfdset)
>  {
> -	fd_set rfds, wfds;
> -	int i, maxfds;
> +	int i;
>  	struct fdentry *pfdentry;
> -	int num = MAX_FDS;
> +	int numfds;
>  	fd_cb rcb, wcb;
>  	void *dat;
>  	int fd;
>  	int remove1, remove2;
>  	int ret;
> +	int handled;
>  
>  	if (pfdset == NULL)
>  		return;
>  
> +	struct pollfd * const rwfds =
> +		rte_malloc("struct pollfd", MAX_FDS * sizeof(*rwfds), 0);

There is neither a NULL check, nor a free after the usage. Since it's
a fixed size at compile time (MAX_FDS), you might want to define a
static array for that.

	--yliu