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

[Jan Wickbom <jan.wickbom@ericsson.com>]

> -----Original Message-----
> From: Yuanhan Liu [mailto:yuanhan.liu@linux.intel.com]
> Sent: den 6 december 2016 07:57
> To: Jan Wickbom <jan.wickbom@ericsson.com>
> Cc: huawei.xie@intel.com; dev@dpdk.org; Patrik Andersson R
> <patrik.r.andersson@ericsson.com>
> Subject: Re: [PATCH] vhost: allow for many vhost user ports
> 
> 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!

Thanks for your comments! Please see below for some questions/opinions.

> 
> > 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'd like to keep the cast. The function returns int and it's nice to show we deliberately
don't care. No strong opinion though, but we should do the same everywhere. Please
see below.

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

If we remove the cast above, we should remove this one as well.

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