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

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

[Jan Wickbom]

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

Signed-off-by: Jan Wickbom <jan.wickbom@ericsson.com>
Reported-by: Patrik Andersson <patrik.r.andersson@ericsson.com>
---
 lib/librte_vhost/fd_man.c | 140 +++++++++++++++++++++++++++-------------------
 lib/librte_vhost/fd_man.h |   2 +-
 2 files changed, 83 insertions(+), 59 deletions(-)

diff --git a/lib/librte_vhost/fd_man.c b/lib/librte_vhost/fd_man.c
index 2d3eeb7..dc4aa3f 100644
--- a/lib/librte_vhost/fd_man.c
+++ b/lib/librte_vhost/fd_man.c
@@ -35,16 +35,40 @@
 #include <stdio.h>
 #include <stdlib.h>
 #include <sys/socket.h>
-#include <sys/select.h>
 #include <sys/time.h>
 #include <sys/types.h>
+#include <poll.h>
 #include <unistd.h>
 
 #include <rte_common.h>
+#include <rte_malloc.h>
 #include <rte_log.h>
 
 #include "fd_man.h"
 
+#define FDPOLLERR (POLLERR | POLLHUP | POLLNVAL)
+
+
+/**
+ * Adjusts the highest index populated in the array of fds
+ * @return
+ *   Index of highest position populated + 1.
+ */
+static int
+fdset_adjust_num(struct fdset *pfdset)
+{
+	int idx;
+
+	for (idx = pfdset->num - 1;
+	     idx >= 0 && pfdset->fd[idx].fd == -1;
+	     idx--)
+		;
+
+	pfdset->num = idx + 1;
+
+	return pfdset->num;
+}
+
 /**
  * Returns the index in the fdset for a given fd.
  * If fd is -1, it means to search for a free entry.
@@ -56,72 +80,62 @@
 {
 	int i;
 
-	if (pfdset == NULL)
-		return -1;
-
-	for (i = 0; i < MAX_FDS && pfdset->fd[i].fd != fd; i++)
+	for (i = 0; i < pfdset->num && pfdset->fd[i].fd != fd; i++)
 		;
 
-	return i ==  MAX_FDS ? -1 : i;
+	return i == pfdset->num ? -1 : i;
 }
 
 static int
 fdset_find_free_slot(struct fdset *pfdset)
 {
-	return fdset_find_fd(pfdset, -1);
+	if (pfdset->num < MAX_FDS)
+		return pfdset->num;
+	else
+		return fdset_find_fd(pfdset, -1);
 }
 
-static int
-fdset_add_fd(struct fdset  *pfdset, int idx, int fd,
+static void
+fdset_add_fd(struct fdset *pfdset, int idx, int fd,
 	fd_cb rcb, fd_cb wcb, void *dat)
 {
 	struct fdentry *pfdentry;
 
-	if (pfdset == NULL || idx >= MAX_FDS || fd >= FD_SETSIZE)
-		return -1;
-
 	pfdentry = &pfdset->fd[idx];
 	pfdentry->fd = fd;
 	pfdentry->rcb = rcb;
 	pfdentry->wcb = wcb;
 	pfdentry->dat = dat;
-
-	return 0;
 }
 
 /**
- * Fill the read/write fd_set with the fds in the fdset.
+ * Compact the fdset and fill the read/write fds with the fds in the fdset.
  * @return
- *  the maximum fds filled in the read/write fd_set.
+ *  the number of fds filled in the read/write fds.
  */
 static int
-fdset_fill(fd_set *rfset, fd_set *wfset, struct fdset *pfdset)
+fdset_fill(struct pollfd *rwfds, struct fdset *pfdset)
 {
 	struct fdentry *pfdentry;
-	int i, maxfds = -1;
-	int num = MAX_FDS;
-
-	if (pfdset == NULL)
-		return -1;
+	int i;
+	int num;
 
-	for (i = 0; i < num; i++) {
+	for (i = 0, num = pfdset->num; i < num; i++) {
 		pfdentry = &pfdset->fd[i];
-		if (pfdentry->fd != -1) {
-			int added = 0;
-			if (pfdentry->rcb && rfset) {
-				FD_SET(pfdentry->fd, rfset);
-				added = 1;
-			}
-			if (pfdentry->wcb && wfset) {
-				FD_SET(pfdentry->fd, wfset);
-				added = 1;
-			}
-			if (added)
-				maxfds = pfdentry->fd < maxfds ?
-					maxfds : pfdentry->fd;
+
+		if (pfdentry->fd < 0) {
+			/* Hole in the list. Move the last one here */
+
+			*pfdentry = pfdset->fd[num - 1];
+			pfdset->fd[num - 1].fd = -1;
+			num = fdset_adjust_num(pfdset);
 		}
+		rwfds[i].fd = pfdentry->fd;
+		rwfds[i].events = pfdentry->rcb ? POLLIN : 0;
+		rwfds[i].events |= pfdentry->wcb ? POLLOUT : 0;
 	}
-	return maxfds;
+
+	return i;
 }
 
 void
@@ -132,6 +146,8 @@
 	if (pfdset == NULL)
 		return;
 
+	pthread_mutex_init(&pfdset->fd_mutex, NULL);
+
 	for (i = 0; i < MAX_FDS; i++) {
 		pfdset->fd[i].fd = -1;
 		pfdset->fd[i].dat = NULL;
@@ -152,14 +168,15 @@
 
 	pthread_mutex_lock(&pfdset->fd_mutex);
 
-	/* Find a free slot in the list. */
 	i = fdset_find_free_slot(pfdset);
-	if (i == -1 || fdset_add_fd(pfdset, i, fd, rcb, wcb, dat) < 0) {
+	if (i == -1) {
 		pthread_mutex_unlock(&pfdset->fd_mutex);
 		return -2;
 	}
 
-	pfdset->num++;
+	fdset_add_fd(pfdset, i, fd, rcb, wcb, dat);
+	if (i == pfdset->num)
+		pfdset->num++;
 
 	pthread_mutex_unlock(&pfdset->fd_mutex);
 
@@ -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);
 			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);
+
 	while (1) {
-		struct timeval tv;
-		tv.tv_sec = 1;
-		tv.tv_usec = 0;
-		FD_ZERO(&rfds);
-		FD_ZERO(&wfds);
 		pthread_mutex_lock(&pfdset->fd_mutex);
 
-		maxfds = fdset_fill(&rfds, &wfds, pfdset);
+		numfds = fdset_fill(rwfds, pfdset);
 
 		pthread_mutex_unlock(&pfdset->fd_mutex);
 
 		/*
-		 * When select is blocked, other threads might unregister
+		 * When poll is blocked, other threads might unregister
 		 * listenfds from and register new listenfds into fdset.
-		 * When select returns, the entries for listenfds in the 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.
 		 */
-		ret = select(maxfds + 1, &rfds, &wfds, NULL, &tv);
+		ret = poll(rwfds, numfds, 1000 /* millisecs */);
+
 		if (ret <= 0)
 			continue;
 
-		for (i = 0; i < num; i++) {
+		for (i = handled = 0; i < numfds && handled < ret; i++) {
+			if (!rwfds[i].revents)
+				continue;
+
+			handled++;
 			remove1 = remove2 = 0;
+
 			pthread_mutex_lock(&pfdset->fd_mutex);
 			pfdentry = &pfdset->fd[i];
 			fd = pfdentry->fd;
@@ -275,9 +296,12 @@
 			dat = pfdentry->dat;
 			pfdentry->busy = 1;
 			pthread_mutex_unlock(&pfdset->fd_mutex);
-			if (fd >= 0 && FD_ISSET(fd, &rfds) && rcb)
+
+			if (fd >= 0 && rcb &&
+			    rwfds[i].revents & (POLLIN | FDPOLLERR))
 				rcb(fd, dat, &remove1);
-			if (fd >= 0 && FD_ISSET(fd, &wfds) && wcb)
+			if (fd >= 0 && wcb &&
+			    rwfds[i].revents & (POLLOUT | FDPOLLERR))
 				wcb(fd, dat, &remove2);
 			pfdentry->busy = 0;
 			/*
diff --git a/lib/librte_vhost/fd_man.h b/lib/librte_vhost/fd_man.h
index bd66ed1..b5ba688 100644
--- a/lib/librte_vhost/fd_man.h
+++ b/lib/librte_vhost/fd_man.h
@@ -51,7 +51,7 @@ struct fdentry {
 struct fdset {
 	struct fdentry fd[MAX_FDS];
 	pthread_mutex_t fd_mutex;
-	int num;	/* current fd number of this fdset */
+	int num;	/* highest index occupied in fd array + 1 */
 };
 
 
-- 
1.9.1

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

[Yuanhan Liu]

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

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

[Jan Wickbom]

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

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

[Yuanhan Liu]

On Tue, Dec 06, 2016 at 11:42:36AM +0000, Jan Wickbom wrote:
> > > @@ -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.

I know your point, but it's not needed.

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

Yes.

	--yliu

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

[Yuanhan Liu]

On Thu, Dec 01, 2016 at 04:26:50PM +0100, Jan Wickbom wrote:
>  static int
> -fdset_fill(fd_set *rfset, fd_set *wfset, struct fdset *pfdset)
> +fdset_fill(struct pollfd *rwfds, struct fdset *pfdset)
>  {
>  	struct fdentry *pfdentry;
> -	int i, maxfds = -1;
> -	int num = MAX_FDS;
> -
> -	if (pfdset == NULL)
> -		return -1;
> +	int i;
> +	int num;
>  
> -	for (i = 0; i < num; i++) {
> +	for (i = 0, num = pfdset->num; i < num; i++) {
>  		pfdentry = &pfdset->fd[i];
> -		if (pfdentry->fd != -1) {
> -			int added = 0;
> -			if (pfdentry->rcb && rfset) {
> -				FD_SET(pfdentry->fd, rfset);
> -				added = 1;
> -			}
> -			if (pfdentry->wcb && wfset) {
> -				FD_SET(pfdentry->fd, wfset);
> -				added = 1;
> -			}
> -			if (added)
> -				maxfds = pfdentry->fd < maxfds ?
> -					maxfds : pfdentry->fd;
> +
> +		if (pfdentry->fd < 0) {
> +			/* Hole in the list. Move the last one here */
> +
> +			*pfdentry = pfdset->fd[num - 1];
> +			pfdset->fd[num - 1].fd = -1;
> +			num = fdset_adjust_num(pfdset);
>  		}
> +		rwfds[i].fd = pfdentry->fd;
> +		rwfds[i].events = pfdentry->rcb ? POLLIN : 0;
> +		rwfds[i].events |= pfdentry->wcb ? POLLOUT : 0;

Another thing is we don't have to re-init this rwfds array again
and again. Instead, we could 

- set it up correctly when fdset_add is invoked: set the fd and
  events.

- reset revents when it's been handled at fdset_event_dispatch().

- swap with the last one and shrink the array on fd delete

Could you make a follow up patch for that?

Thanks.

	--yliu

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

[Jan Wickbom]

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

Signed-off-by: Jan Wickbom <jan.wickbom@ericsson.com>
Reported-by: Patrik Andersson <patrik.r.andersson@ericsson.com>
---

v2:
* removed unnecessary casts
* static array replacing allocated memory

 lib/librte_vhost/fd_man.c | 142 +++++++++++++++++++++++++++-------------------
 lib/librte_vhost/fd_man.h |   2 +-
 2 files changed, 85 insertions(+), 59 deletions(-)

diff --git a/lib/librte_vhost/fd_man.c b/lib/librte_vhost/fd_man.c
index 2d3eeb7..3c743e3 100644
--- a/lib/librte_vhost/fd_man.c
+++ b/lib/librte_vhost/fd_man.c
@@ -35,16 +35,43 @@
 #include <stdio.h>
 #include <stdlib.h>
 #include <sys/socket.h>
-#include <sys/select.h>
 #include <sys/time.h>
 #include <sys/types.h>
+#include <poll.h>
 #include <unistd.h>
+#include <string.h>
 
 #include <rte_common.h>
+#include <rte_malloc.h>
 #include <rte_log.h>
 
 #include "fd_man.h"
 
+#define FDPOLLERR (POLLERR | POLLHUP | POLLNVAL)
+
+
+static struct pollfd rwfds[MAX_FDS];
+
+/**
+ * Adjusts the highest index populated in the array of fds
+ * @return
+ *   The new size of fdset.
+ */
+static int
+fdset_shrink(struct fdset *pfdset)
+{
+	int idx;
+
+	for (idx = pfdset->num - 1;
+	     idx >= 0 && pfdset->fd[idx].fd == -1;
+	     idx--)
+		;
+
+	pfdset->num = idx + 1;
+
+	return pfdset->num;
+}
+
 /**
  * Returns the index in the fdset for a given fd.
  * If fd is -1, it means to search for a free entry.
@@ -56,72 +83,62 @@
 {
 	int i;
 
-	if (pfdset == NULL)
-		return -1;
-
-	for (i = 0; i < MAX_FDS && pfdset->fd[i].fd != fd; i++)
+	for (i = 0; i < pfdset->num && pfdset->fd[i].fd != fd; i++)
 		;
 
-	return i ==  MAX_FDS ? -1 : i;
+	return i == pfdset->num ? -1 : i;
 }
 
 static int
 fdset_find_free_slot(struct fdset *pfdset)
 {
-	return fdset_find_fd(pfdset, -1);
+	if (pfdset->num < MAX_FDS)
+		return pfdset->num;
+	else
+		return fdset_find_fd(pfdset, -1);
 }
 
-static int
-fdset_add_fd(struct fdset  *pfdset, int idx, int fd,
+static void
+fdset_add_fd(struct fdset *pfdset, int idx, int fd,
 	fd_cb rcb, fd_cb wcb, void *dat)
 {
 	struct fdentry *pfdentry;
 
-	if (pfdset == NULL || idx >= MAX_FDS || fd >= FD_SETSIZE)
-		return -1;
-
 	pfdentry = &pfdset->fd[idx];
 	pfdentry->fd = fd;
 	pfdentry->rcb = rcb;
 	pfdentry->wcb = wcb;
 	pfdentry->dat = dat;
-
-	return 0;
 }
 
 /**
- * Fill the read/write fd_set with the fds in the fdset.
+ * Compact the fdset and fill the read/write fds with the fds in the fdset.
  * @return
- *  the maximum fds filled in the read/write fd_set.
+ *  the number of fds filled in the read/write fds.
  */
 static int
-fdset_fill(fd_set *rfset, fd_set *wfset, struct fdset *pfdset)
+fdset_fill(struct pollfd *rwfds, struct fdset *pfdset)
 {
 	struct fdentry *pfdentry;
-	int i, maxfds = -1;
-	int num = MAX_FDS;
-
-	if (pfdset == NULL)
-		return -1;
+	int i;
+	int num;
 
-	for (i = 0; i < num; i++) {
+	for (i = 0, num = pfdset->num; i < num; i++) {
 		pfdentry = &pfdset->fd[i];
-		if (pfdentry->fd != -1) {
-			int added = 0;
-			if (pfdentry->rcb && rfset) {
-				FD_SET(pfdentry->fd, rfset);
-				added = 1;
-			}
-			if (pfdentry->wcb && wfset) {
-				FD_SET(pfdentry->fd, wfset);
-				added = 1;
-			}
-			if (added)
-				maxfds = pfdentry->fd < maxfds ?
-					maxfds : pfdentry->fd;
+
+		if (pfdentry->fd < 0) {
+			/* Hole in the list. Move the last one here */
+
+			*pfdentry = pfdset->fd[num - 1];
+			pfdset->fd[num - 1].fd = -1;
+			num = fdset_shrink(pfdset);
 		}
+		rwfds[i].fd = pfdentry->fd;
+		rwfds[i].events = pfdentry->rcb ? POLLIN : 0;
+		rwfds[i].events |= pfdentry->wcb ? POLLOUT : 0;
 	}
-	return maxfds;
+
+	return i;
 }
 
 void
@@ -132,6 +149,8 @@
 	if (pfdset == NULL)
 		return;
 
+	pthread_mutex_init(&pfdset->fd_mutex, NULL);
+
 	for (i = 0; i < MAX_FDS; i++) {
 		pfdset->fd[i].fd = -1;
 		pfdset->fd[i].dat = NULL;
@@ -152,14 +171,15 @@
 
 	pthread_mutex_lock(&pfdset->fd_mutex);
 
-	/* Find a free slot in the list. */
 	i = fdset_find_free_slot(pfdset);
-	if (i == -1 || fdset_add_fd(pfdset, i, fd, rcb, wcb, dat) < 0) {
+	if (i == -1) {
 		pthread_mutex_unlock(&pfdset->fd_mutex);
 		return -2;
 	}
 
-	pfdset->num++;
+	fdset_add_fd(pfdset, i, fd, rcb, wcb, dat);
+	if (i == pfdset->num)
+		pfdset->num++;
 
 	pthread_mutex_unlock(&pfdset->fd_mutex);
 
@@ -189,7 +209,7 @@
 			pfdset->fd[i].fd = -1;
 			pfdset->fd[i].rcb = pfdset->fd[i].wcb = NULL;
 			pfdset->fd[i].dat = NULL;
-			pfdset->num--;
+			fdset_shrink(pfdset);
 			i = -1;
 		}
 		pthread_mutex_unlock(&pfdset->fd_mutex);
@@ -211,12 +231,12 @@
 
 	pfdset->fd[index].fd = -1;
 	pfdset->fd[index].rcb = pfdset->fd[index].wcb = NULL;
-	pfdset->fd[index].dat = NULL;
-	pfdset->num--;
+	fdset_shrink(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 +249,47 @@
 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;
 
+	memset(rwfds, 0, sizeof(rwfds));
+
 	while (1) {
-		struct timeval tv;
-		tv.tv_sec = 1;
-		tv.tv_usec = 0;
-		FD_ZERO(&rfds);
-		FD_ZERO(&wfds);
 		pthread_mutex_lock(&pfdset->fd_mutex);
 
-		maxfds = fdset_fill(&rfds, &wfds, pfdset);
+		numfds = fdset_fill(rwfds, pfdset);
 
 		pthread_mutex_unlock(&pfdset->fd_mutex);
 
 		/*
-		 * When select is blocked, other threads might unregister
+		 * When poll is blocked, other threads might unregister
 		 * listenfds from and register new listenfds into fdset.
-		 * When select returns, the entries for listenfds in the 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.
 		 */
-		ret = select(maxfds + 1, &rfds, &wfds, NULL, &tv);
+		ret = poll(rwfds, numfds, 1000 /* millisecs */);
+
 		if (ret <= 0)
 			continue;
 
-		for (i = 0; i < num; i++) {
+		for (i = handled = 0; i < numfds && handled < ret; i++) {
+			if (!rwfds[i].revents)
+				continue;
+
+			handled++;
 			remove1 = remove2 = 0;
+
 			pthread_mutex_lock(&pfdset->fd_mutex);
 			pfdentry = &pfdset->fd[i];
 			fd = pfdentry->fd;
@@ -275,9 +298,12 @@
 			dat = pfdentry->dat;
 			pfdentry->busy = 1;
 			pthread_mutex_unlock(&pfdset->fd_mutex);
-			if (fd >= 0 && FD_ISSET(fd, &rfds) && rcb)
+
+			if (fd >= 0 && rcb &&
+			    rwfds[i].revents & (POLLIN | FDPOLLERR))
 				rcb(fd, dat, &remove1);
-			if (fd >= 0 && FD_ISSET(fd, &wfds) && wcb)
+			if (fd >= 0 && wcb &&
+			    rwfds[i].revents & (POLLOUT | FDPOLLERR))
 				wcb(fd, dat, &remove2);
 			pfdentry->busy = 0;
 			/*
diff --git a/lib/librte_vhost/fd_man.h b/lib/librte_vhost/fd_man.h
index bd66ed1..b5ba688 100644
--- a/lib/librte_vhost/fd_man.h
+++ b/lib/librte_vhost/fd_man.h
@@ -51,7 +51,7 @@ struct fdentry {
 struct fdset {
 	struct fdentry fd[MAX_FDS];
 	pthread_mutex_t fd_mutex;
-	int num;	/* current fd number of this fdset */
+	int num;	/* highest index occupied in fd array + 1 */
 };
 
 
-- 
1.9.1

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

[Jan Wickbom]

> -----Original Message-----
> From: Yuanhan Liu [mailto:yuanhan.liu@linux.intel.com]
> Sent: den 7 december 2016 11:13
> To: Jan Wickbom <jan.wickbom@ericsson.com>
> Cc: 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:
> >  static int
> > -fdset_fill(fd_set *rfset, fd_set *wfset, struct fdset *pfdset)
> > +fdset_fill(struct pollfd *rwfds, struct fdset *pfdset)
> >  {
> >  	struct fdentry *pfdentry;
> > -	int i, maxfds = -1;
> > -	int num = MAX_FDS;
> > -
> > -	if (pfdset == NULL)
> > -		return -1;
> > +	int i;
> > +	int num;
> >
> > -	for (i = 0; i < num; i++) {
> > +	for (i = 0, num = pfdset->num; i < num; i++) {
> >  		pfdentry = &pfdset->fd[i];
> > -		if (pfdentry->fd != -1) {
> > -			int added = 0;
> > -			if (pfdentry->rcb && rfset) {
> > -				FD_SET(pfdentry-
> >fd, rfset);
> > -				added = 1;
> > -			}
> > -			if (pfdentry->wcb && wfset) {
> > -				FD_SET(pfdentry-
> >fd, wfset);
> > -				added = 1;
> > -			}
> > -			if (added)
> > -				maxfds = pfdentry-
> >fd < maxfds ?
> > -
> 	maxfds : pfdentry->fd;
> > +
> > +		if (pfdentry->fd < 0) {
> > +			/* Hole in the list. Move the last
> one here */
> > +
> > +			*pfdentry = pfdset->fd[num - 1];
> > +			pfdset->fd[num - 1].fd = -1;
> > +			num =
> fdset_adjust_num(pfdset);
> >  		}
> > +		rwfds[i].fd = pfdentry->fd;
> > +		rwfds[i].events = pfdentry->rcb ? POLLIN : 0;
> > +		rwfds[i].events |= pfdentry->wcb ? POLLOUT :
> 0;
> 
> Another thing is we don't have to re-init this rwfds array again
> and again. Instead, we could
> 
> - set it up correctly when fdset_add is invoked: set the fd and
>   events.
> 
> - reset revents when it's been handled at fdset_event_dispatch().
> 
> - swap with the last one and shrink the array on fd delete
> 
> Could you make a follow up patch for that?

I don't see how that could easily be done. The loop index, i, is a direct reference between
an entry in the rwfds array and an entry in the pfdset array. It should stay like that while we are
hanging in the poll(). If  an entry in the pfdset array is removed while we are hanging in the poll()
and we then immediately replaces it with the last entry in the array we will end up in trouble if the
revent gets set for the "replaced" index. The direct reference is gone.
Or am I missing something?
/jaw
> 
> Thanks.
> 
> 	--yliu

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

[Yuanhan Liu]

On Wed, Dec 07, 2016 at 01:23:48PM +0000, Jan Wickbom wrote:
> > On Thu, Dec 01, 2016 at 04:26:50PM +0100, Jan Wickbom wrote:
> > >  static int
> > > -fdset_fill(fd_set *rfset, fd_set *wfset, struct fdset *pfdset)
> > > +fdset_fill(struct pollfd *rwfds, struct fdset *pfdset)
...
> > > +		rwfds[i].fd = pfdentry->fd;
> > > +		rwfds[i].events = pfdentry->rcb ? POLLIN : 0;
> > > +		rwfds[i].events |= pfdentry->wcb ? POLLOUT :
> > 0;
> > 
> > Another thing is we don't have to re-init this rwfds array again
> > and again. Instead, we could
> > 
> > - set it up correctly when fdset_add is invoked: set the fd and
> >   events.
> > 
> > - reset revents when it's been handled at fdset_event_dispatch().
> > 
> > - swap with the last one and shrink the array on fd delete
> > 
> > Could you make a follow up patch for that?
> 
> I don't see how that could easily be done. The loop index, i, is a direct reference between
> an entry in the rwfds array and an entry in the pfdset array. It should stay like that while we are
> hanging in the poll(). If  an entry in the pfdset array is removed while we are hanging in the poll()
> and we then immediately replaces it with the last entry in the array we will end up in trouble if the
> revent gets set for the "replaced" index. The direct reference is gone.
> Or am I missing something?

Yes, we should not shrink the rwfds during the poll, but we could later, at
the end of the while() loop.

Talking about that, you should not invoke fdset_shrink() inside fdset_del(),
since it could be in the poll stage.

	--yliu

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

[Jan Wickbom]

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

Signed-off-by: Jan Wickbom <jan.wickbom@ericsson.com>
Reported-by: Patrik Andersson <patrik.r.andersson@ericsson.com>
---

v3:
* removed unnecessary include
* removed fdset_fill, made it functionally part of poll loop

v2:
* removed unnecessary casts
* static array replacing allocated memory

 lib/librte_vhost/fd_man.c | 194 +++++++++++++++++++++++++---------------------
 lib/librte_vhost/fd_man.h |   2 +-
 2 files changed, 105 insertions(+), 91 deletions(-)

diff --git a/lib/librte_vhost/fd_man.c b/lib/librte_vhost/fd_man.c
index 2d3eeb7..c360d07 100644
--- a/lib/librte_vhost/fd_man.c
+++ b/lib/librte_vhost/fd_man.c
@@ -35,16 +35,40 @@
 #include <stdio.h>
 #include <stdlib.h>
 #include <sys/socket.h>
-#include <sys/select.h>
 #include <sys/time.h>
 #include <sys/types.h>
+#include <poll.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 struct pollfd rwfds[MAX_FDS];
+
+/**
+ * Adjusts the highest index populated in the array of fds
+ * @return
+ *   The new size of fdset.
+ */
+static void
+fdset_shrink(struct fdset *pfdset)
+{
+	int idx;
+
+	for (idx = pfdset->num - 1;
+	     idx >= 0 && pfdset->fd[idx].fd == -1;
+	     idx--)
+		;
+
+	pfdset->num = idx + 1;
+}
+
 /**
  * Returns the index in the fdset for a given fd.
  * If fd is -1, it means to search for a free entry.
@@ -56,72 +80,32 @@
 {
 	int i;
 
-	if (pfdset == NULL)
-		return -1;
-
-	for (i = 0; i < MAX_FDS && pfdset->fd[i].fd != fd; i++)
+	for (i = 0; i < pfdset->num && pfdset->fd[i].fd != fd; i++)
 		;
 
-	return i ==  MAX_FDS ? -1 : i;
+	return i == pfdset->num ? -1 : i;
 }
 
 static int
 fdset_find_free_slot(struct fdset *pfdset)
 {
-	return fdset_find_fd(pfdset, -1);
+	if (pfdset->num < MAX_FDS)
+		return pfdset->num;
+	else
+		return fdset_find_fd(pfdset, -1);
 }
 
-static int
-fdset_add_fd(struct fdset  *pfdset, int idx, int fd,
+static void
+fdset_add_fd(struct fdset *pfdset, int idx, int fd,
 	fd_cb rcb, fd_cb wcb, void *dat)
 {
 	struct fdentry *pfdentry;
 
-	if (pfdset == NULL || idx >= MAX_FDS || fd >= FD_SETSIZE)
-		return -1;
-
 	pfdentry = &pfdset->fd[idx];
 	pfdentry->fd = fd;
 	pfdentry->rcb = rcb;
 	pfdentry->wcb = wcb;
 	pfdentry->dat = dat;
-
-	return 0;
-}
-
-/**
- * Fill the read/write fd_set with the fds in the fdset.
- * @return
- *  the maximum fds filled in the read/write fd_set.
- */
-static int
-fdset_fill(fd_set *rfset, fd_set *wfset, struct fdset *pfdset)
-{
-	struct fdentry *pfdentry;
-	int i, maxfds = -1;
-	int num = MAX_FDS;
-
-	if (pfdset == NULL)
-		return -1;
-
-	for (i = 0; i < num; i++) {
-		pfdentry = &pfdset->fd[i];
-		if (pfdentry->fd != -1) {
-			int added = 0;
-			if (pfdentry->rcb && rfset) {
-				FD_SET(pfdentry->fd, rfset);
-				added = 1;
-			}
-			if (pfdentry->wcb && wfset) {
-				FD_SET(pfdentry->fd, wfset);
-				added = 1;
-			}
-			if (added)
-				maxfds = pfdentry->fd < maxfds ?
-					maxfds : pfdentry->fd;
-		}
-	}
-	return maxfds;
 }
 
 void
@@ -132,6 +116,8 @@
 	if (pfdset == NULL)
 		return;
 
+	pthread_mutex_init(&pfdset->fd_mutex, NULL);
+
 	for (i = 0; i < MAX_FDS; i++) {
 		pfdset->fd[i].fd = -1;
 		pfdset->fd[i].dat = NULL;
@@ -152,14 +138,15 @@
 
 	pthread_mutex_lock(&pfdset->fd_mutex);
 
-	/* Find a free slot in the list. */
 	i = fdset_find_free_slot(pfdset);
-	if (i == -1 || fdset_add_fd(pfdset, i, fd, rcb, wcb, dat) < 0) {
+	if (i == -1) {
 		pthread_mutex_unlock(&pfdset->fd_mutex);
 		return -2;
 	}
 
-	pfdset->num++;
+	fdset_add_fd(pfdset, i, fd, rcb, wcb, dat);
+	if (i == pfdset->num)
+		pfdset->num++;
 
 	pthread_mutex_unlock(&pfdset->fd_mutex);
 
@@ -189,7 +176,7 @@
 			pfdset->fd[i].fd = -1;
 			pfdset->fd[i].rcb = pfdset->fd[i].wcb = NULL;
 			pfdset->fd[i].dat = NULL;
-			pfdset->num--;
+			fdset_shrink(pfdset);
 			i = -1;
 		}
 		pthread_mutex_unlock(&pfdset->fd_mutex);
@@ -198,25 +185,26 @@
 	return dat;
 }
 
+
 /**
- *  Unregister the fd at the specified slot from the fdset.
+ *  Moves the fd from last slot to specified slot, including
+ *  corresponding pollfd
  */
 static void
-fdset_del_slot(struct fdset *pfdset, int index)
+fdset_move_last(struct fdset *pfdset, int idx)
 {
-	if (pfdset == NULL || index < 0 || index >= MAX_FDS)
-		return;
-
-	pthread_mutex_lock(&pfdset->fd_mutex);
+	int last_idx = pfdset->num - 1;
 
-	pfdset->fd[index].fd = -1;
-	pfdset->fd[index].rcb = pfdset->fd[index].wcb = NULL;
-	pfdset->fd[index].dat = NULL;
-	pfdset->num--;
+	if (idx < last_idx) {
+		pfdset->fd[idx] = pfdset->fd[last_idx];
+		pfdset->fd[last_idx].fd = -1;
 
-	pthread_mutex_unlock(&pfdset->fd_mutex);
+		rwfds[idx] = rwfds[last_idx];
+		rwfds[last_idx].revents = 0;
+	}
 }
 
+
 /**
  * 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,55 +217,71 @@
 void
 fdset_event_dispatch(struct fdset *pfdset)
 {
-	fd_set rfds, wfds;
-	int i, maxfds;
+	int i;
 	struct fdentry *pfdentry;
-	int num = MAX_FDS;
 	fd_cb rcb, wcb;
 	void *dat;
 	int fd;
 	int remove1, remove2;
-	int ret;
 
 	if (pfdset == NULL)
 		return;
 
-	while (1) {
-		struct timeval tv;
-		tv.tv_sec = 1;
-		tv.tv_usec = 0;
-		FD_ZERO(&rfds);
-		FD_ZERO(&wfds);
-		pthread_mutex_lock(&pfdset->fd_mutex);
-
-		maxfds = fdset_fill(&rfds, &wfds, pfdset);
-
-		pthread_mutex_unlock(&pfdset->fd_mutex);
+	memset(rwfds, 0, sizeof(rwfds));
 
+	while (1) {
 		/*
-		 * When select is blocked, other threads might unregister
+		 * When poll is blocked, other threads might unregister
 		 * listenfds from and register new listenfds into fdset.
-		 * When select returns, the entries for listenfds in the 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.
 		 */
-		ret = select(maxfds + 1, &rfds, &wfds, NULL, &tv);
-		if (ret <= 0)
-			continue;
+		poll(rwfds, pfdset->num, 1000 /* millisecs */);
 
-		for (i = 0; i < num; i++) {
-			remove1 = remove2 = 0;
+		for (i = 0; i < pfdset->num; ) {
 			pthread_mutex_lock(&pfdset->fd_mutex);
+
 			pfdentry = &pfdset->fd[i];
 			fd = pfdentry->fd;
+
+			if (fd < 0) {
+				/* Removed during poll */
+
+				fdset_move_last(pfdset, i);
+				fdset_shrink(pfdset);
+
+				pthread_mutex_unlock(&pfdset->fd_mutex);
+
+				continue;
+			}
+
+			if (!rwfds[i].revents) {
+				/* No revents, maybe added during poll */
+
+				rwfds[i].fd = fd;
+				rwfds[i].events = pfdentry->rcb ? POLLIN : 0;
+				rwfds[i].events |= pfdentry->wcb ? POLLOUT : 0;
+				pthread_mutex_unlock(&pfdset->fd_mutex);
+
+				i++;
+				continue;
+			}
+
+			/* Valid fd, and at least one revent ... */
+
+			remove1 = remove2 = 0;
+
 			rcb = pfdentry->rcb;
 			wcb = pfdentry->wcb;
 			dat = pfdentry->dat;
 			pfdentry->busy = 1;
+
 			pthread_mutex_unlock(&pfdset->fd_mutex);
-			if (fd >= 0 && FD_ISSET(fd, &rfds) && rcb)
+
+			if (rcb && rwfds[i].revents & (POLLIN | FDPOLLERR))
 				rcb(fd, dat, &remove1);
-			if (fd >= 0 && FD_ISSET(fd, &wfds) && wcb)
+			if (wcb && rwfds[i].revents & (POLLOUT | FDPOLLERR))
 				wcb(fd, dat, &remove2);
 			pfdentry->busy = 0;
 			/*
@@ -292,8 +296,18 @@
 			 * listen fd in another thread, we couldn't call
 			 * fd_set_del.
 			 */
-			if (remove1 || remove2)
-				fdset_del_slot(pfdset, i);
+			if (remove1 || remove2) {
+				pthread_mutex_lock(&pfdset->fd_mutex);
+
+				fdset_move_last(pfdset, i);
+				fdset_shrink(pfdset);
+
+				pthread_mutex_unlock(&pfdset->fd_mutex);
+
+				continue;
+			}
+
+			i++;
 		}
 	}
 }
diff --git a/lib/librte_vhost/fd_man.h b/lib/librte_vhost/fd_man.h
index bd66ed1..b5ba688 100644
--- a/lib/librte_vhost/fd_man.h
+++ b/lib/librte_vhost/fd_man.h
@@ -51,7 +51,7 @@ struct fdentry {
 struct fdset {
 	struct fdentry fd[MAX_FDS];
 	pthread_mutex_t fd_mutex;
-	int num;	/* current fd number of this fdset */
+	int num;	/* highest index occupied in fd array + 1 */
 };
 
 
-- 
1.9.1

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

[Jan Wickbom]

> -----Original Message-----
> From: Yuanhan Liu [mailto:yuanhan.liu@linux.intel.com]
> Sent: den 8 december 2016 06:51
> To: Jan Wickbom <jan.wickbom@ericsson.com>
> Cc: dev@dpdk.org; Patrik Andersson R <patrik.r.andersson@ericsson.com>
> Subject: Re: [PATCH] vhost: allow for many vhost user ports
> 
> On Wed, Dec 07, 2016 at 01:23:48PM +0000, Jan Wickbom wrote:
> > > On Thu, Dec 01, 2016 at 04:26:50PM +0100, Jan Wickbom wrote:
> > > >  static int
> > > > -fdset_fill(fd_set *rfset, fd_set *wfset, struct fdset *pfdset)
> > > > +fdset_fill(struct pollfd *rwfds, struct fdset *pfdset)
> ...
> > > > +		rwfds[i].fd = pfdentry->fd;
> > > > +		rwfds[i].events = pfdentry->rcb ? POLLIN : 0;
> > > > +		rwfds[i].events |= pfdentry->wcb ? POLLOUT :
> > > 0;
> > >
> > > Another thing is we don't have to re-init this rwfds array again
> > > and again. Instead, we could
> > >
> > > - set it up correctly when fdset_add is invoked: set the fd and
> > >   events.
> > >
> > > - reset revents when it's been handled at fdset_event_dispatch().
> > >
> > > - swap with the last one and shrink the array on fd delete
> > >
> > > Could you make a follow up patch for that?
> >
> > I don't see how that could easily be done. The loop index, i, is a direct
> reference between
> > an entry in the rwfds array and an entry in the pfdset array. It should stay
> like that while we are
> > hanging in the poll(). If  an entry in the pfdset array is removed while we
> are hanging in the poll()
> > and we then immediately replaces it with the last entry in the array we will
> end up in trouble if the
> > revent gets set for the "replaced" index. The direct reference is gone.
> > Or am I missing something?
> 
> Yes, we should not shrink the rwfds during the poll, but we could later, at
> the end of the while() loop.

Did something in v3, please have a look.

> 
> Talking about that, you should not invoke fdset_shrink() inside fdset_del(),
> since it could be in the poll stage.

Thanks! Forgot this! Will send a v4

> 
> 	--yliu

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

[Yuanhan Liu]

On Mon, Dec 12, 2016 at 05:50:34PM +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.
> 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
> 
> Signed-off-by: Jan Wickbom <jan.wickbom@ericsson.com>
> Reported-by: Patrik Andersson <patrik.r.andersson@ericsson.com>
...
> +static struct pollfd rwfds[MAX_FDS];

Though it's unlikely, but just assume we have multiple instance of
fdset_event_dispatch(pfdset), a global rwfds will not work.

Thought twice, and it's better to put it into the fdset struct:

    struct fdset {
            struct fdentry fd[MAX_FDS];
            struct pollfd rwfds[MAX_FDS];
            ...

>  /**
>   * 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,55 +217,71 @@
>  void
>  fdset_event_dispatch(struct fdset *pfdset)
>  {
> -	fd_set rfds, wfds;
> -	int i, maxfds;
> +	int i;
>  	struct fdentry *pfdentry;
> -	int num = MAX_FDS;
>  	fd_cb rcb, wcb;
>  	void *dat;
>  	int fd;
>  	int remove1, remove2;
> -	int ret;
>  
>  	if (pfdset == NULL)
>  		return;
>  
> -	while (1) {
> -		struct timeval tv;
> -		tv.tv_sec = 1;
> -		tv.tv_usec = 0;
> -		FD_ZERO(&rfds);
> -		FD_ZERO(&wfds);
> -		pthread_mutex_lock(&pfdset->fd_mutex);
> -
> -		maxfds = fdset_fill(&rfds, &wfds, pfdset);
> -
> -		pthread_mutex_unlock(&pfdset->fd_mutex);
> +	memset(rwfds, 0, sizeof(rwfds));
>  
> +	while (1) {
>  		/*
> -		 * When select is blocked, other threads might unregister
> +		 * When poll is blocked, other threads might unregister
>  		 * listenfds from and register new listenfds into fdset.
> -		 * When select returns, the entries for listenfds in the 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.
>  		 */
> -		ret = select(maxfds + 1, &rfds, &wfds, NULL, &tv);
> -		if (ret <= 0)
> -			continue;
> +		poll(rwfds, pfdset->num, 1000 /* millisecs */);
>  
> -		for (i = 0; i < num; i++) {
> -			remove1 = remove2 = 0;
> +		for (i = 0; i < pfdset->num; ) {
>  			pthread_mutex_lock(&pfdset->fd_mutex);
> +
>  			pfdentry = &pfdset->fd[i];
>  			fd = pfdentry->fd;
> +
> +			if (fd < 0) {
> +				/* Removed during poll */
> +
> +				fdset_move_last(pfdset, i);
> +				fdset_shrink(pfdset);
> +
> +				pthread_mutex_unlock(&pfdset->fd_mutex);
> +
> +				continue;
> +			}
> +
> +			if (!rwfds[i].revents) {
> +				/* No revents, maybe added during poll */
> +
> +				rwfds[i].fd = fd;
> +				rwfds[i].events = pfdentry->rcb ? POLLIN : 0;
> +				rwfds[i].events |= pfdentry->wcb ? POLLOUT : 0;
> +				pthread_mutex_unlock(&pfdset->fd_mutex);
> +
> +				i++;
> +				continue;

I think it's error-prone to manipulate the rwfds here. Besides, it
registers an fd repeatedly.

The way I think of is:

- set rwfds[i] at fdset_add().

  This also simply makes sure that pfdset->rwfds[i] and pfdset->fd[i] is
  correctly bond.

	fdset_add(fdset, fd, ...) {
		lock();

		i = fdset_find_free_slot(..);

		pfdset->fd[i]->fd  = fd;
		pfdset->fd[i]->rcb = rcb;
		pfdset->fd[i]->...;

		pfdset->rwfds[i]->fd = fd;
		pfdset->rwfds[i]->events  = ...;
		pfdset->rwfds[i]->revents = 0;
	}


- set pfdset->fd[i]->fd = -1 on fdset_del. Note we should not decrease
  'num' here, as we may be at poll.

	fdset_del(fdset, fd) {
		lock();

		i = fdset_find_fd(pfdset, fd);
		pfdset->fd[i]->fd = -1;

		...
	}
		
		

- log down pfdset->num before poll, because 'num' may increase during poll.

  I think it's optional, since even 'num' is increased during poll, it just
  leads to few more rwfds entries will be accessed. But it's not tracked by
  kernel, and revents is initiated with 0, that there is no issue.


- shrink the fdset and rwfds (together) for those removed entries, __outside__
  the for loop after poll.

Works to you?

	--yliu

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

[Jan Wickbom]

> -----Original Message-----
> From: Yuanhan Liu [mailto:yuanhan.liu@linux.intel.com]
> Sent: den 13 december 2016 10:15
> To: Jan Wickbom <jan.wickbom@ericsson.com>
> Cc: dev@dpdk.org; Patrik Andersson R <patrik.r.andersson@ericsson.com>
> Subject: Re: [PATCH v3] vhost: allow for many vhost user ports
> 
> On Mon, Dec 12, 2016 at 05:50:34PM +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.
> > 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
> >
> > Signed-off-by: Jan Wickbom <jan.wickbom@ericsson.com>
> > Reported-by: Patrik Andersson <patrik.r.andersson@ericsson.com>
> ...
> > +static struct pollfd rwfds[MAX_FDS];
> 
> Though it's unlikely, but just assume we have multiple instance of
> fdset_event_dispatch(pfdset), a global rwfds will not work.
> 
> Thought twice, and it's better to put it into the fdset struct:
> 
>     struct fdset {
>             struct fdentry fd[MAX_FDS];
>             struct pollfd rwfds[MAX_FDS];
>             ...
> 

Done


> >  /**
> >   * 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,55 +217,71 @@
> >  void
> >  fdset_event_dispatch(struct fdset *pfdset)
> >  {
> > -	fd_set rfds, wfds;
> > -	int i, maxfds;
> > +	int i;
> >  	struct fdentry *pfdentry;
> > -	int num = MAX_FDS;
> >  	fd_cb rcb, wcb;
> >  	void *dat;
> >  	int fd;
> >  	int remove1, remove2;
> > -	int ret;
> >
> >  	if (pfdset == NULL)
> >  		return;
> >
> > -	while (1) {
> > -		struct timeval tv;
> > -		tv.tv_sec = 1;
> > -		tv.tv_usec = 0;
> > -		FD_ZERO(&rfds);
> > -		FD_ZERO(&wfds);
> > -		pthread_mutex_lock(&pfdset->fd_mutex);
> > -
> > -		maxfds = fdset_fill(&rfds, &wfds, pfdset);
> > -
> > -		pthread_mutex_unlock(&pfdset->fd_mutex);
> > +	memset(rwfds, 0, sizeof(rwfds));
> >
> > +	while (1) {
> >  		/*
> > -		 * When select is blocked, other threads might
> unregister
> > +		 * When poll is blocked, other threads might
> unregister
> >  		 * listenfds from and register new listenfds into
> fdset.
> > -		 * When select returns, the entries for listenfds
> in the 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.
> >  		 */
> > -		ret = select(maxfds + 1, &rfds, &wfds, NULL,
> &tv);
> > -		if (ret <= 0)
> > -			continue;
> > +		poll(rwfds, pfdset->num, 1000 /* millisecs */);
> >
> > -		for (i = 0; i < num; i++) {
> > -			remove1 = remove2 = 0;
> > +		for (i = 0; i < pfdset->num; ) {
> >  			pthread_mutex_lock(&pfdset-
> >fd_mutex);
> > +
> >  			pfdentry = &pfdset->fd[i];
> >  			fd = pfdentry->fd;
> > +
> > +			if (fd < 0) {
> > +				/* Removed during
> poll */
> > +
> > +
> 	fdset_move_last(pfdset, i);
> > +
> 	fdset_shrink(pfdset);
> > +
> > +
> 	pthread_mutex_unlock(&pfdset->fd_mutex);
> > +
> > +				continue;
> > +			}
> > +
> > +			if (!rwfds[i].revents) {
> > +				/* No revents,
> maybe added during poll */
> > +
> > +				rwfds[i].fd = fd;
> > +				rwfds[i].events =
> pfdentry->rcb ? POLLIN : 0;
> > +				rwfds[i].events |=
> pfdentry->wcb ? POLLOUT : 0;
> > +
> 	pthread_mutex_unlock(&pfdset->fd_mutex);
> > +
> > +				i++;
> > +				continue;
> 
> I think it's error-prone to manipulate the rwfds here. Besides, it
> registers an fd repeatedly.
> 
> The way I think of is:
> 
> - set rwfds[i] at fdset_add().
> 
>   This also simply makes sure that pfdset->rwfds[i] and pfdset->fd[i] is
>   correctly bond.
> 
> 	fdset_add(fdset, fd, ...) {
> 		lock();
> 
> 		i = fdset_find_free_slot(..);
> 
> 		pfdset->fd[i]->fd  = fd;
> 		pfdset->fd[i]->rcb = rcb;
> 		pfdset->fd[i]->...;
> 
> 		pfdset->rwfds[i]->fd = fd;
> 		pfdset->rwfds[i]->events  = ...;
> 		pfdset->rwfds[i]->revents = 0;
> 	}
> 
> 
> - set pfdset->fd[i]->fd = -1 on fdset_del. Note we should not decrease
>   'num' here, as we may be at poll.
> 
> 	fdset_del(fdset, fd) {
> 		lock();
> 
> 		i = fdset_find_fd(pfdset, fd);
> 		pfdset->fd[i]->fd = -1;
> 
> 		...
> 	}
> 
> 
> 
> - log down pfdset->num before poll, because 'num' may increase during poll.
> 
>   I think it's optional, since even 'num' is increased during poll, it just
>   leads to few more rwfds entries will be accessed. But it's not tracked by
>   kernel, and revents is initiated with 0, that there is no issue.
> 
> 
> - shrink the fdset and rwfds (together) for those removed entries,
> __outside__
>   the for loop after poll.
> 
> Works to you?

I did quite of a rework of this yesterday before reading your mail. I think
That should work, please have a look.
V4 coming in a minute
> 
> 	--yliu

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

[Jan Wickbom]

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

Signed-off-by: Jan Wickbom <jan.wickbom@ericsson.com>
Reported-by: Patrik Andersson <patrik.r.andersson@ericsson.com>
---

v4:
* fdset_del can not shrink the array. Took care of that in sveral places
* Moved rwfds[] into struct fdset

v3:
* removed unnecessary include
* removed fdset_fill, made it functionally part of poll loop

v2:
* removed unnecessary casts
* static array replacing allocated memory

 lib/librte_vhost/fd_man.c | 226 ++++++++++++++++++++++++++--------------------
 lib/librte_vhost/fd_man.h |   4 +-
 2 files changed, 131 insertions(+), 99 deletions(-)

diff --git a/lib/librte_vhost/fd_man.c b/lib/librte_vhost/fd_man.c
index 2d3eeb7..d7c775d 100644
--- a/lib/librte_vhost/fd_man.c
+++ b/lib/librte_vhost/fd_man.c
@@ -35,16 +35,38 @@
 #include <stdio.h>
 #include <stdlib.h>
 #include <sys/socket.h>
-#include <sys/select.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)
+
+/**
+ * Adjusts the highest index populated in the array of fds
+ * @return
+ *   The new size of fdset.
+ */
+static int
+fdset_shrink(struct fdset *pfdset)
+{
+	int idx;
+
+	for (idx = pfdset->num - 1;
+	     idx >= 0 && pfdset->fd[idx].fd == -1;
+	     idx--)
+		;
+
+	pfdset->num = idx + 1;
+
+	return pfdset->num;
+}
+
 /**
  * Returns the index in the fdset for a given fd.
  * If fd is -1, it means to search for a free entry.
@@ -56,72 +78,28 @@
 {
 	int i;
 
-	if (pfdset == NULL)
-		return -1;
-
-	for (i = 0; i < MAX_FDS && pfdset->fd[i].fd != fd; i++)
+	for (i = 0; i < pfdset->num && pfdset->fd[i].fd != fd; i++)
 		;
 
-	return i ==  MAX_FDS ? -1 : i;
-}
-
-static int
-fdset_find_free_slot(struct fdset *pfdset)
-{
-	return fdset_find_fd(pfdset, -1);
+	return i == pfdset->num ? -1 : i;
 }
 
-static int
-fdset_add_fd(struct fdset  *pfdset, int idx, int fd,
+static void
+fdset_add_fd(struct fdset *pfdset, int idx, int fd,
 	fd_cb rcb, fd_cb wcb, void *dat)
 {
-	struct fdentry *pfdentry;
-
-	if (pfdset == NULL || idx >= MAX_FDS || fd >= FD_SETSIZE)
-		return -1;
+	struct fdentry *pfdentry = &pfdset->fd[idx];
+	struct pollfd *pfd = &pfdset->rwfds[idx];
 
-	pfdentry = &pfdset->fd[idx];
 	pfdentry->fd = fd;
 	pfdentry->rcb = rcb;
 	pfdentry->wcb = wcb;
 	pfdentry->dat = dat;
 
-	return 0;
-}
-
-/**
- * Fill the read/write fd_set with the fds in the fdset.
- * @return
- *  the maximum fds filled in the read/write fd_set.
- */
-static int
-fdset_fill(fd_set *rfset, fd_set *wfset, struct fdset *pfdset)
-{
-	struct fdentry *pfdentry;
-	int i, maxfds = -1;
-	int num = MAX_FDS;
-
-	if (pfdset == NULL)
-		return -1;
-
-	for (i = 0; i < num; i++) {
-		pfdentry = &pfdset->fd[i];
-		if (pfdentry->fd != -1) {
-			int added = 0;
-			if (pfdentry->rcb && rfset) {
-				FD_SET(pfdentry->fd, rfset);
-				added = 1;
-			}
-			if (pfdentry->wcb && wfset) {
-				FD_SET(pfdentry->fd, wfset);
-				added = 1;
-			}
-			if (added)
-				maxfds = pfdentry->fd < maxfds ?
-					maxfds : pfdentry->fd;
-		}
-	}
-	return maxfds;
+	pfd->fd = fd;
+	pfd->events = rcb ? POLLIN : 0;
+	pfd->events |= wcb ? POLLOUT : 0;
+	pfd->revents = 0;
 }
 
 void
@@ -132,11 +110,15 @@
 	if (pfdset == NULL)
 		return;
 
+	pthread_mutex_init(&pfdset->fd_mutex, NULL);
+
 	for (i = 0; i < MAX_FDS; i++) {
 		pfdset->fd[i].fd = -1;
 		pfdset->fd[i].dat = NULL;
 	}
 	pfdset->num = 0;
+
+	memset(pfdset->rwfds, 0, sizeof(pfdset->rwfds));
 }
 
 /**
@@ -152,14 +134,14 @@
 
 	pthread_mutex_lock(&pfdset->fd_mutex);
 
-	/* Find a free slot in the list. */
-	i = fdset_find_free_slot(pfdset);
-	if (i == -1 || fdset_add_fd(pfdset, i, fd, rcb, wcb, dat) < 0) {
+	i = pfdset->num < MAX_FDS ? pfdset->num++ : -1;
+
+	if (i == -1) {
 		pthread_mutex_unlock(&pfdset->fd_mutex);
 		return -2;
 	}
 
-	pfdset->num++;
+	fdset_add_fd(pfdset, i, fd, rcb, wcb, dat);
 
 	pthread_mutex_unlock(&pfdset->fd_mutex);
 
@@ -189,7 +171,6 @@
 			pfdset->fd[i].fd = -1;
 			pfdset->fd[i].rcb = pfdset->fd[i].wcb = NULL;
 			pfdset->fd[i].dat = NULL;
-			pfdset->num--;
 			i = -1;
 		}
 		pthread_mutex_unlock(&pfdset->fd_mutex);
@@ -198,25 +179,26 @@
 	return dat;
 }
 
+
 /**
- *  Unregister the fd at the specified slot from the fdset.
+ *  Moves the fd from last slot to specified slot, including
+ *  corresponding pollfd
  */
 static void
-fdset_del_slot(struct fdset *pfdset, int index)
+fdset_move_last(struct fdset *pfdset, int idx)
 {
-	if (pfdset == NULL || index < 0 || index >= MAX_FDS)
-		return;
-
-	pthread_mutex_lock(&pfdset->fd_mutex);
+	int last_idx = pfdset->num - 1;
 
-	pfdset->fd[index].fd = -1;
-	pfdset->fd[index].rcb = pfdset->fd[index].wcb = NULL;
-	pfdset->fd[index].dat = NULL;
-	pfdset->num--;
+	if (idx < last_idx) {
+		pfdset->fd[idx] = pfdset->fd[last_idx];
+		pfdset->fd[last_idx].fd = -1;
 
-	pthread_mutex_unlock(&pfdset->fd_mutex);
+		pfdset->rwfds[idx] = pfdset->rwfds[last_idx];
+		pfdset->rwfds[last_idx].revents = 0;
+	}
 }
 
+
 /**
  * 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,55 +211,75 @@
 void
 fdset_event_dispatch(struct fdset *pfdset)
 {
-	fd_set rfds, wfds;
-	int i, maxfds;
+	int i;
+	struct pollfd *pfd;
 	struct fdentry *pfdentry;
-	int num = MAX_FDS;
 	fd_cb rcb, wcb;
 	void *dat;
-	int fd;
+	int fd, numfds;
 	int remove1, remove2;
-	int ret;
 
 	if (pfdset == NULL)
 		return;
 
 	while (1) {
-		struct timeval tv;
-		tv.tv_sec = 1;
-		tv.tv_usec = 0;
-		FD_ZERO(&rfds);
-		FD_ZERO(&wfds);
-		pthread_mutex_lock(&pfdset->fd_mutex);
-
-		maxfds = fdset_fill(&rfds, &wfds, pfdset);
-
-		pthread_mutex_unlock(&pfdset->fd_mutex);
-
 		/*
-		 * When select is blocked, other threads might unregister
+		 * When poll is blocked, other threads might unregister
 		 * listenfds from and register new listenfds into fdset.
-		 * When select returns, the entries for listenfds in the 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.
 		 */
-		ret = select(maxfds + 1, &rfds, &wfds, NULL, &tv);
-		if (ret <= 0)
-			continue;
+		pthread_mutex_lock(&pfdset->fd_mutex);
 
-		for (i = 0; i < num; i++) {
-			remove1 = remove2 = 0;
+		numfds = pfdset->num;
+
+		pthread_mutex_unlock(&pfdset->fd_mutex);
+
+		poll(pfdset->rwfds, numfds, 1000 /* millisecs */);
+
+		for (i = 0; i < numfds; ) {
 			pthread_mutex_lock(&pfdset->fd_mutex);
+
 			pfdentry = &pfdset->fd[i];
 			fd = pfdentry->fd;
+			pfd = &pfdset->rwfds[i];
+
+			if (fd < 0) {
+				/* Removed during poll */
+				/* shrink first, last migth be deleted*/
+
+				fdset_shrink(pfdset);
+				fdset_move_last(pfdset, i);
+				fdset_shrink(pfdset);
+
+				pthread_mutex_unlock(&pfdset->fd_mutex);
+
+				continue;
+			}
+
+			if (!pfd->revents) {
+
+				pthread_mutex_unlock(&pfdset->fd_mutex);
+
+				i++;
+				continue;
+			}
+
+			/* Valid fd, and at least one revent ... */
+
+			remove1 = remove2 = 0;
+
 			rcb = pfdentry->rcb;
 			wcb = pfdentry->wcb;
 			dat = pfdentry->dat;
 			pfdentry->busy = 1;
+
 			pthread_mutex_unlock(&pfdset->fd_mutex);
-			if (fd >= 0 && FD_ISSET(fd, &rfds) && rcb)
+
+			if (rcb && pfd->revents & (POLLIN | FDPOLLERR))
 				rcb(fd, dat, &remove1);
-			if (fd >= 0 && FD_ISSET(fd, &wfds) && wcb)
+			if (wcb && pfd->revents & (POLLOUT | FDPOLLERR))
 				wcb(fd, dat, &remove2);
 			pfdentry->busy = 0;
 			/*
@@ -292,8 +294,36 @@
 			 * listen fd in another thread, we couldn't call
 			 * fd_set_del.
 			 */
-			if (remove1 || remove2)
-				fdset_del_slot(pfdset, i);
+			if (remove1 || remove2) {
+				pthread_mutex_lock(&pfdset->fd_mutex);
+
+				/* shrink first, last migth be deleted*/
+				fdset_shrink(pfdset);
+				fdset_move_last(pfdset, i);
+				fdset_shrink(pfdset);
+
+				pthread_mutex_unlock(&pfdset->fd_mutex);
+
+				continue;
+			}
+
+			i++;
 		}
+
+		/* fdset_del do not shrink, pack eventual remainings of array */
+		pthread_mutex_lock(&pfdset->fd_mutex);
+
+		fdset_shrink(pfdset);
+
+		for ( ; i < pfdset->num; i++) {
+			pfdentry = &pfdset->fd[i];
+
+			if (pfdentry->fd < 0) {
+				fdset_move_last(pfdset, i);
+				fdset_shrink(pfdset);
+			}
+		}
+
+		pthread_mutex_unlock(&pfdset->fd_mutex);
 	}
 }
diff --git a/lib/librte_vhost/fd_man.h b/lib/librte_vhost/fd_man.h
index bd66ed1..03e7881 100644
--- a/lib/librte_vhost/fd_man.h
+++ b/lib/librte_vhost/fd_man.h
@@ -35,6 +35,7 @@
 #define _FD_MAN_H_
 #include <stdint.h>
 #include <pthread.h>
+#include <poll.h>
 
 #define MAX_FDS 1024
 
@@ -49,9 +50,10 @@ struct fdentry {
 };
 
 struct fdset {
+	struct pollfd rwfds[MAX_FDS];
 	struct fdentry fd[MAX_FDS];
 	pthread_mutex_t fd_mutex;
-	int num;	/* current fd number of this fdset */
+	int num;	/* highest index occupied in fd array + 1 */
 };
 
 
-- 
1.9.1

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

[Yuanhan Liu]

On Tue, Dec 13, 2016 at 02:19:47PM +0100, Jan Wickbom wrote:
> +
> +		poll(pfdset->rwfds, numfds, 1000 /* millisecs */);
> +
> +		for (i = 0; i < numfds; ) {
>  			pthread_mutex_lock(&pfdset->fd_mutex);
> +
>  			pfdentry = &pfdset->fd[i];
>  			fd = pfdentry->fd;
> +			pfd = &pfdset->rwfds[i];
> +
> +			if (fd < 0) {
> +				/* Removed during poll */
> +				/* shrink first, last migth be deleted*/
> +
> +				fdset_shrink(pfdset);
> +				fdset_move_last(pfdset, i);
> +				fdset_shrink(pfdset);
> +
> +				pthread_mutex_unlock(&pfdset->fd_mutex);

This patch looks better, but as said, I will not do shrink in the
event processing loop: I would simply set a flag, something like
"need_shrink" here, and do the shrink outside this for loop.


> +			if (remove1 || remove2) {
> +				pthread_mutex_lock(&pfdset->fd_mutex);
> +
> +				/* shrink first, last migth be deleted*/
> +				fdset_shrink(pfdset);
> +				fdset_move_last(pfdset, i);
> +				fdset_shrink(pfdset);
> +
> +				pthread_mutex_unlock(&pfdset->fd_mutex);
> +
> +				continue;

Same here: just sets a flag.

> +			}
> +
> +			i++;
>  		}
> +
> +		/* fdset_del do not shrink, pack eventual remainings of array */
> +		pthread_mutex_lock(&pfdset->fd_mutex);
> +
> +		fdset_shrink(pfdset);
> +
> +		for ( ; i < pfdset->num; i++) {
> +			pfdentry = &pfdset->fd[i];
> +
> +			if (pfdentry->fd < 0) {
> +				fdset_move_last(pfdset, i);
> +				fdset_shrink(pfdset);
> +			}
> +		}

And yes, do the shrink here (when the shrink flag is set). But I would
simply call fdset_shrink() here (without the for loop), and let the
fdset_shrink() to handle the details: it could either be a swap with
last __valid__ entry, or simply a memmov.

That said, if you prefer to choose fdset_move_last(), fine, invoke it
inside fdset_shrink() then. Let fdset_shrink be able to remove an fd
in the middle.

> +
> +		pthread_mutex_unlock(&pfdset->fd_mutex);
>  	}
>  }
> diff --git a/lib/librte_vhost/fd_man.h b/lib/librte_vhost/fd_man.h
> index bd66ed1..03e7881 100644
> --- a/lib/librte_vhost/fd_man.h
> +++ b/lib/librte_vhost/fd_man.h
> @@ -35,6 +35,7 @@
>  #define _FD_MAN_H_
>  #include <stdint.h>
>  #include <pthread.h>
> +#include <poll.h>
>  
>  #define MAX_FDS 1024
>  
> @@ -49,9 +50,10 @@ struct fdentry {
>  };
>  
>  struct fdset {
> +	struct pollfd rwfds[MAX_FDS];
>  	struct fdentry fd[MAX_FDS];
>  	pthread_mutex_t fd_mutex;
> -	int num;	/* current fd number of this fdset */
> +	int num;	/* highest index occupied in fd array + 1 */

I don't see the comment change makes it more readable.

	--yliu

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

[Jan Wickbom]

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

Signed-off-by: Jan Wickbom <jan.wickbom@ericsson.com>
Reported-by: Patrik Andersson <patrik.r.andersson@ericsson.com>
---

v5:
* pack of arrays moved after poll loop

v4:
* fdset_del can not shrink the array. Took care of that in sveral places
* Moved rwfds[] into struct fdset

v3:
* removed unnecessary include
* removed fdset_fill, made it functionally part of poll loop

v2:
* removed unnecessary casts
* static array replacing allocated memory

 lib/librte_vhost/fd_man.c | 234 ++++++++++++++++++++++++++--------------------
 lib/librte_vhost/fd_man.h |   2 +
 2 files changed, 135 insertions(+), 101 deletions(-)

diff --git a/lib/librte_vhost/fd_man.c b/lib/librte_vhost/fd_man.c
index 2d3eeb7..c7c2b35 100644
--- a/lib/librte_vhost/fd_man.c
+++ b/lib/librte_vhost/fd_man.c
@@ -35,93 +35,111 @@
 #include <stdio.h>
 #include <stdlib.h>
 #include <sys/socket.h>
-#include <sys/select.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)
+
 /**
- * Returns the index in the fdset for a given fd.
- * If fd is -1, it means to search for a free entry.
+ * Adjusts the highest index populated in the array of fds
  * @return
- *   index for the fd, or -1 if fd isn't in the fdset.
+ *   The new size of fdset.
  */
 static int
-fdset_find_fd(struct fdset *pfdset, int fd)
+fdset_shrink(struct fdset *pfdset)
 {
-	int i;
+	int idx;
 
-	if (pfdset == NULL)
-		return -1;
+	/* Remove all empty spots in the end */
 
-	for (i = 0; i < MAX_FDS && pfdset->fd[i].fd != fd; i++)
+	for (idx = pfdset->num - 1;
+	     idx >= 0 && pfdset->fd[idx].fd == -1;
+	     idx--)
 		;
 
-	return i ==  MAX_FDS ? -1 : i;
+	pfdset->num = idx + 1;
+
+	return pfdset->num;
 }
 
-static int
-fdset_find_free_slot(struct fdset *pfdset)
+/**
+ *  Moves the fd from last slot to specified slot, including
+ *  corresponding pollfd
+ */
+static void
+fdset_move_last(struct fdset *pfdset, int idx)
 {
-	return fdset_find_fd(pfdset, -1);
+	int last_idx = pfdset->num - 1;
+
+	if (idx < last_idx) {
+		pfdset->fd[idx] = pfdset->fd[last_idx];
+		pfdset->fd[last_idx].fd = -1;
+
+		pfdset->rwfds[idx] = pfdset->rwfds[last_idx];
+		pfdset->rwfds[last_idx].revents = 0;
+	}
 }
 
-static int
-fdset_add_fd(struct fdset  *pfdset, int idx, int fd,
-	fd_cb rcb, fd_cb wcb, void *dat)
-{
-	struct fdentry *pfdentry;
 
-	if (pfdset == NULL || idx >= MAX_FDS || fd >= FD_SETSIZE)
-		return -1;
+/**
+ *  Packs the two arrays in the fdset structure
+ */
+static void
+fdset_pack(struct fdset *pfdset)
+{
+	int idx;
+	int num = pfdset->num;
 
-	pfdentry = &pfdset->fd[idx];
-	pfdentry->fd = fd;
-	pfdentry->rcb = rcb;
-	pfdentry->wcb = wcb;
-	pfdentry->dat = dat;
+	for (idx = 0; idx < num; idx++) {
+		if (pfdset->fd[idx].fd == -1) {
+			num = fdset_shrink(pfdset);
+			fdset_move_last(pfdset, idx);
+		}
+	}
 
-	return 0;
+	fdset_shrink(pfdset);
 }
 
 /**
- * Fill the read/write fd_set with the fds in the fdset.
+ * Returns the index in the fdset for a given fd.
+ * If fd is -1, it means to search for a free entry.
  * @return
- *  the maximum fds filled in the read/write fd_set.
+ *   index for the fd, or -1 if fd isn't in the fdset.
  */
 static int
-fdset_fill(fd_set *rfset, fd_set *wfset, struct fdset *pfdset)
+fdset_find_fd(struct fdset *pfdset, int fd)
 {
-	struct fdentry *pfdentry;
-	int i, maxfds = -1;
-	int num = MAX_FDS;
+	int i;
 
-	if (pfdset == NULL)
-		return -1;
+	for (i = 0; i < pfdset->num && pfdset->fd[i].fd != fd; i++)
+		;
 
-	for (i = 0; i < num; i++) {
-		pfdentry = &pfdset->fd[i];
-		if (pfdentry->fd != -1) {
-			int added = 0;
-			if (pfdentry->rcb && rfset) {
-				FD_SET(pfdentry->fd, rfset);
-				added = 1;
-			}
-			if (pfdentry->wcb && wfset) {
-				FD_SET(pfdentry->fd, wfset);
-				added = 1;
-			}
-			if (added)
-				maxfds = pfdentry->fd < maxfds ?
-					maxfds : pfdentry->fd;
-		}
-	}
-	return maxfds;
+	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
@@ -132,11 +150,15 @@
 	if (pfdset == NULL)
 		return;
 
+	pthread_mutex_init(&pfdset->fd_mutex, NULL);
+
 	for (i = 0; i < MAX_FDS; i++) {
 		pfdset->fd[i].fd = -1;
 		pfdset->fd[i].dat = NULL;
 	}
 	pfdset->num = 0;
+
+	memset(pfdset->rwfds, 0, sizeof(pfdset->rwfds));
 }
 
 /**
@@ -152,14 +174,14 @@
 
 	pthread_mutex_lock(&pfdset->fd_mutex);
 
-	/* Find a free slot in the list. */
-	i = fdset_find_free_slot(pfdset);
-	if (i == -1 || fdset_add_fd(pfdset, i, fd, rcb, wcb, dat) < 0) {
+	i = pfdset->num < MAX_FDS ? pfdset->num++ : -1;
+
+	if (i == -1) {
 		pthread_mutex_unlock(&pfdset->fd_mutex);
 		return -2;
 	}
 
-	pfdset->num++;
+	fdset_add_fd(pfdset, i, fd, rcb, wcb, dat);
 
 	pthread_mutex_unlock(&pfdset->fd_mutex);
 
@@ -189,7 +211,6 @@
 			pfdset->fd[i].fd = -1;
 			pfdset->fd[i].rcb = pfdset->fd[i].wcb = NULL;
 			pfdset->fd[i].dat = NULL;
-			pfdset->num--;
 			i = -1;
 		}
 		pthread_mutex_unlock(&pfdset->fd_mutex);
@@ -198,24 +219,6 @@
 	return dat;
 }
 
-/**
- *  Unregister the fd at the specified slot from the fdset.
- */
-static void
-fdset_del_slot(struct fdset *pfdset, int index)
-{
-	if (pfdset == NULL || index < 0 || index >= MAX_FDS)
-		return;
-
-	pthread_mutex_lock(&pfdset->fd_mutex);
-
-	pfdset->fd[index].fd = -1;
-	pfdset->fd[index].rcb = pfdset->fd[index].wcb = NULL;
-	pfdset->fd[index].dat = NULL;
-	pfdset->num--;
-
-	pthread_mutex_unlock(&pfdset->fd_mutex);
-}
 
 /**
  * This functions runs in infinite blocking loop until there is no fd in
@@ -229,55 +232,74 @@
 void
 fdset_event_dispatch(struct fdset *pfdset)
 {
-	fd_set rfds, wfds;
-	int i, maxfds;
+	int i;
+	struct pollfd *pfd;
 	struct fdentry *pfdentry;
-	int num = MAX_FDS;
 	fd_cb rcb, wcb;
 	void *dat;
-	int fd;
+	int fd, numfds;
 	int remove1, remove2;
-	int ret;
 
 	if (pfdset == NULL)
 		return;
 
 	while (1) {
-		struct timeval tv;
-		tv.tv_sec = 1;
-		tv.tv_usec = 0;
-		FD_ZERO(&rfds);
-		FD_ZERO(&wfds);
-		pthread_mutex_lock(&pfdset->fd_mutex);
-
-		maxfds = fdset_fill(&rfds, &wfds, pfdset);
-
-		pthread_mutex_unlock(&pfdset->fd_mutex);
+		int has_holes;
 
 		/*
-		 * When select is blocked, other threads might unregister
+		 * When poll is blocked, other threads might unregister
 		 * listenfds from and register new listenfds into fdset.
-		 * When select returns, the entries for listenfds in the 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.
 		 */
-		ret = select(maxfds + 1, &rfds, &wfds, NULL, &tv);
-		if (ret <= 0)
-			continue;
+		pthread_mutex_lock(&pfdset->fd_mutex);
 
-		for (i = 0; i < num; i++) {
-			remove1 = remove2 = 0;
+		numfds = pfdset->num;
+
+		pthread_mutex_unlock(&pfdset->fd_mutex);
+
+		poll(pfdset->rwfds, numfds, 1000 /* millisecs */);
+
+		has_holes = 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) {
+				/* Removed during poll, care later */
+
+				has_holes = 1;
+
+				pthread_mutex_unlock(&pfdset->fd_mutex);
+
+				continue;
+			}
+
+			if (!pfd->revents) {
+
+				pthread_mutex_unlock(&pfdset->fd_mutex);
+
+				continue;
+			}
+
+			/* Valid fd, and at least one revent ... */
+
+			remove1 = remove2 = 0;
+
 			rcb = pfdentry->rcb;
 			wcb = pfdentry->wcb;
 			dat = pfdentry->dat;
 			pfdentry->busy = 1;
+
 			pthread_mutex_unlock(&pfdset->fd_mutex);
-			if (fd >= 0 && FD_ISSET(fd, &rfds) && rcb)
+
+			if (rcb && pfd->revents & (POLLIN | FDPOLLERR))
 				rcb(fd, dat, &remove1);
-			if (fd >= 0 && FD_ISSET(fd, &wfds) && wcb)
+			if (wcb && pfd->revents & (POLLOUT | FDPOLLERR))
 				wcb(fd, dat, &remove2);
 			pfdentry->busy = 0;
 			/*
@@ -292,8 +314,18 @@
 			 * listen fd in another thread, we couldn't call
 			 * fd_set_del.
 			 */
-			if (remove1 || remove2)
-				fdset_del_slot(pfdset, i);
+			if (remove1 || remove2) {
+				pfdentry->fd = -1;
+				has_holes = 1;
+			}
+		}
+
+		if (has_holes) {
+			pthread_mutex_lock(&pfdset->fd_mutex);
+
+			fdset_pack(pfdset);
+
+			pthread_mutex_unlock(&pfdset->fd_mutex);
 		}
 	}
 }
diff --git a/lib/librte_vhost/fd_man.h b/lib/librte_vhost/fd_man.h
index bd66ed1..d319cac 100644
--- a/lib/librte_vhost/fd_man.h
+++ b/lib/librte_vhost/fd_man.h
@@ -35,6 +35,7 @@
 #define _FD_MAN_H_
 #include <stdint.h>
 #include <pthread.h>
+#include <poll.h>
 
 #define MAX_FDS 1024
 
@@ -49,6 +50,7 @@ struct fdentry {
 };
 
 struct fdset {
+	struct pollfd rwfds[MAX_FDS];
 	struct fdentry fd[MAX_FDS];
 	pthread_mutex_t fd_mutex;
 	int num;	/* current fd number of this fdset */
-- 
1.9.1

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

[Yuanhan Liu]

From: Jan Wickbom <jan.wickbom@ericsson.com>

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

Reported-by: Patrik Andersson <patrik.r.andersson@ericsson.com>
Signed-off-by: Jan Wickbom <jan.wickbom@ericsson.com>
Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
---

    For some reason, Jan won't be able to make v6, so I made it for
    him.

v6:
* simplify fdset_shrink a bit
* minor cleanups
* don't touch fdset_init, which has nothing to do with this patchset,
  yet this function will be removed in coming patch.

v5:
* pack of arrays moved after poll loop

v4:
* fdset_del can not shrink the array. Took care of that in sveral places
* Moved rwfds[] into struct fdset

v3:
* removed unnecessary include
* removed fdset_fill, made it functionally part of poll loop

v2:
* removed unnecessary casts
* static array replacing allocated memory
---
 lib/librte_vhost/fd_man.c | 200 ++++++++++++++++++++++------------------------
 lib/librte_vhost/fd_man.h |   2 +
 2 files changed, 97 insertions(+), 105 deletions(-)

diff --git a/lib/librte_vhost/fd_man.c b/lib/librte_vhost/fd_man.c
index 2d3eeb7..8a075da 100644
--- a/lib/librte_vhost/fd_man.c
+++ b/lib/librte_vhost/fd_man.c
@@ -35,93 +35,91 @@
 #include <stdio.h>
 #include <stdlib.h>
 #include <sys/socket.h>
-#include <sys/select.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"
 
-/**
- * Returns the index in the fdset for a given fd.
- * If fd is -1, it means to search for a free entry.
- * @return
- *   index for the fd, or -1 if fd isn't in the fdset.
- */
+#define FDPOLLERR (POLLERR | POLLHUP | POLLNVAL)
+
 static int
-fdset_find_fd(struct fdset *pfdset, int fd)
+get_last_valid_idx(struct fdset *pfdset, int last_valid_idx)
 {
 	int i;
 
-	if (pfdset == NULL)
-		return -1;
-
-	for (i = 0; i < MAX_FDS && pfdset->fd[i].fd != fd; i++)
+	for (i = last_valid_idx; i >= 0 && pfdset->fd[i].fd == -1; i--)
 		;
 
-	return i ==  MAX_FDS ? -1 : i;
+	return i;
 }
 
-static int
-fdset_find_free_slot(struct fdset *pfdset)
+static void
+fdset_move(struct fdset *pfdset, int dst, int src)
 {
-	return fdset_find_fd(pfdset, -1);
+	pfdset->fd[dst]    = pfdset->fd[src];
+	pfdset->rwfds[dst] = pfdset->rwfds[src];
 }
 
-static int
-fdset_add_fd(struct fdset  *pfdset, int idx, int fd,
-	fd_cb rcb, fd_cb wcb, void *dat)
+/*
+ * Find deleted fd entries and remove them
+ */
+static void
+fdset_shrink(struct fdset *pfdset)
 {
-	struct fdentry *pfdentry;
+	int i;
+	int last_valid_idx = get_last_valid_idx(pfdset, pfdset->num - 1);
 
-	if (pfdset == NULL || idx >= MAX_FDS || fd >= FD_SETSIZE)
-		return -1;
+	pthread_mutex_lock(&pfdset->fd_mutex);
 
-	pfdentry = &pfdset->fd[idx];
-	pfdentry->fd = fd;
-	pfdentry->rcb = rcb;
-	pfdentry->wcb = wcb;
-	pfdentry->dat = dat;
+	for (i = 0; i < last_valid_idx; i++) {
+		if (pfdset->fd[i].fd != -1)
+			continue;
 
-	return 0;
+		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;
+
+	pthread_mutex_unlock(&pfdset->fd_mutex);
 }
 
 /**
- * Fill the read/write fd_set with the fds in the fdset.
+ * Returns the index in the fdset for a given fd.
  * @return
- *  the maximum fds filled in the read/write fd_set.
+ *   index for the fd, or -1 if fd isn't in the fdset.
  */
 static int
-fdset_fill(fd_set *rfset, fd_set *wfset, struct fdset *pfdset)
+fdset_find_fd(struct fdset *pfdset, int fd)
 {
-	struct fdentry *pfdentry;
-	int i, maxfds = -1;
-	int num = MAX_FDS;
+	int i;
 
-	if (pfdset == NULL)
-		return -1;
+	for (i = 0; i < pfdset->num && pfdset->fd[i].fd != fd; i++)
+		;
 
-	for (i = 0; i < num; i++) {
-		pfdentry = &pfdset->fd[i];
-		if (pfdentry->fd != -1) {
-			int added = 0;
-			if (pfdentry->rcb && rfset) {
-				FD_SET(pfdentry->fd, rfset);
-				added = 1;
-			}
-			if (pfdentry->wcb && wfset) {
-				FD_SET(pfdentry->fd, wfset);
-				added = 1;
-			}
-			if (added)
-				maxfds = pfdentry->fd < maxfds ?
-					maxfds : pfdentry->fd;
-		}
-	}
-	return maxfds;
+	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
@@ -151,16 +149,13 @@
 		return -1;
 
 	pthread_mutex_lock(&pfdset->fd_mutex);
-
-	/* Find a free slot in the list. */
-	i = fdset_find_free_slot(pfdset);
-	if (i == -1 || fdset_add_fd(pfdset, i, fd, rcb, wcb, dat) < 0) {
+	i = pfdset->num < MAX_FDS ? pfdset->num++ : -1;
+	if (i == -1) {
 		pthread_mutex_unlock(&pfdset->fd_mutex);
 		return -2;
 	}
 
-	pfdset->num++;
-
+	fdset_add_fd(pfdset, i, fd, rcb, wcb, dat);
 	pthread_mutex_unlock(&pfdset->fd_mutex);
 
 	return 0;
@@ -189,7 +184,6 @@
 			pfdset->fd[i].fd = -1;
 			pfdset->fd[i].rcb = pfdset->fd[i].wcb = NULL;
 			pfdset->fd[i].dat = NULL;
-			pfdset->num--;
 			i = -1;
 		}
 		pthread_mutex_unlock(&pfdset->fd_mutex);
@@ -198,24 +192,6 @@
 	return dat;
 }
 
-/**
- *  Unregister the fd at the specified slot from the fdset.
- */
-static void
-fdset_del_slot(struct fdset *pfdset, int index)
-{
-	if (pfdset == NULL || index < 0 || index >= MAX_FDS)
-		return;
-
-	pthread_mutex_lock(&pfdset->fd_mutex);
-
-	pfdset->fd[index].fd = -1;
-	pfdset->fd[index].rcb = pfdset->fd[index].wcb = NULL;
-	pfdset->fd[index].dat = NULL;
-	pfdset->num--;
-
-	pthread_mutex_unlock(&pfdset->fd_mutex);
-}
 
 /**
  * This functions runs in infinite blocking loop until there is no fd in
@@ -229,55 +205,64 @@
 void
 fdset_event_dispatch(struct fdset *pfdset)
 {
-	fd_set rfds, wfds;
-	int i, maxfds;
+	int i;
+	struct pollfd *pfd;
 	struct fdentry *pfdentry;
-	int num = MAX_FDS;
 	fd_cb rcb, wcb;
 	void *dat;
-	int fd;
+	int fd, numfds;
 	int remove1, remove2;
-	int ret;
+	int need_shrink;
 
 	if (pfdset == NULL)
 		return;
 
 	while (1) {
-		struct timeval tv;
-		tv.tv_sec = 1;
-		tv.tv_usec = 0;
-		FD_ZERO(&rfds);
-		FD_ZERO(&wfds);
-		pthread_mutex_lock(&pfdset->fd_mutex);
-
-		maxfds = fdset_fill(&rfds, &wfds, pfdset);
-
-		pthread_mutex_unlock(&pfdset->fd_mutex);
 
 		/*
-		 * When select is blocked, other threads might unregister
+		 * When poll is blocked, other threads might unregister
 		 * listenfds from and register new listenfds into fdset.
-		 * When select returns, the entries for listenfds in the 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.
 		 */
-		ret = select(maxfds + 1, &rfds, &wfds, NULL, &tv);
-		if (ret <= 0)
-			continue;
+		pthread_mutex_lock(&pfdset->fd_mutex);
+		numfds = pfdset->num;
+		pthread_mutex_unlock(&pfdset->fd_mutex);
 
-		for (i = 0; i < num; i++) {
-			remove1 = remove2 = 0;
+		poll(pfdset->rwfds, numfds, 1000 /* millisecs */);
+
+		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 (fd >= 0 && FD_ISSET(fd, &rfds) && rcb)
+
+			if (rcb && pfd->revents & (POLLIN | FDPOLLERR))
 				rcb(fd, dat, &remove1);
-			if (fd >= 0 && FD_ISSET(fd, &wfds) && wcb)
+			if (wcb && pfd->revents & (POLLOUT | FDPOLLERR))
 				wcb(fd, dat, &remove2);
 			pfdentry->busy = 0;
 			/*
@@ -292,8 +277,13 @@
 			 * listen fd in another thread, we couldn't call
 			 * fd_set_del.
 			 */
-			if (remove1 || remove2)
-				fdset_del_slot(pfdset, i);
+			if (remove1 || remove2) {
+				pfdentry->fd = -1;
+				need_shrink = 1;
+			}
 		}
+
+		if (need_shrink)
+			fdset_shrink(pfdset);
 	}
 }
diff --git a/lib/librte_vhost/fd_man.h b/lib/librte_vhost/fd_man.h
index bd66ed1..d319cac 100644
--- a/lib/librte_vhost/fd_man.h
+++ b/lib/librte_vhost/fd_man.h
@@ -35,6 +35,7 @@
 #define _FD_MAN_H_
 #include <stdint.h>
 #include <pthread.h>
+#include <poll.h>
 
 #define MAX_FDS 1024
 
@@ -49,6 +50,7 @@ struct fdentry {
 };
 
 struct fdset {
+	struct pollfd rwfds[MAX_FDS];
 	struct fdentry fd[MAX_FDS];
 	pthread_mutex_t fd_mutex;
 	int num;	/* current fd number of this fdset */
-- 
1.9.0

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

[Stephen Hemminger]

On Wed, 21 Dec 2016 17:45:13 +0800
Yuanhan Liu <yuanhan.liu@linux.intel.com> wrote:

> From: Jan Wickbom <jan.wickbom@ericsson.com>
> 
> 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.
> 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
> 
> Reported-by: Patrik Andersson <patrik.r.andersson@ericsson.com>
> Signed-off-by: Jan Wickbom <jan.wickbom@ericsson.com>
> Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>

Why not epoll()? It scales much better. The old System V poll
is just as bad with 1023 fd's.

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

[Yuanhan Liu]

On Wed, Dec 21, 2016 at 10:06:36AM -0800, Stephen Hemminger wrote:
> On Wed, 21 Dec 2016 17:45:13 +0800
> Yuanhan Liu <yuanhan.liu@linux.intel.com> wrote:
> 
> > From: Jan Wickbom <jan.wickbom@ericsson.com>
> > 
> > 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.
> > 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
> > 
> > Reported-by: Patrik Andersson <patrik.r.andersson@ericsson.com>
> > Signed-off-by: Jan Wickbom <jan.wickbom@ericsson.com>
> > Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
> 
> Why not epoll()? It scales much better. The old System V poll
> is just as bad with 1023 fd's.

Indeed, there was a plan to use epoll() to fix this issue. It's been
delayed long enough that Jan came up with a fix with poll recently.

I don't have strong preference to epoll() over poll, for following
reasons:

- epoll() is Linux only, though I don't know other platforms (mainly
  *BSD) actually care vhost-user or not.

- epoll indeed scales much better, but I don't think the scale actually
  matters a lot here: vhost-user negotiation normally happens once on
  QEMU startup, and it's a short lived process. Both facts make it be
  non-performance critical.

- 1023 is also not a big problem so far: vhost lib hardcodes the max
  vhost devices we could support to be 1024 after all.

For above reasons and Jan already came up with a solution (that would
work for most platforms), I think I'm fine with this poll() so far.

	--yliu

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

[Yuanhan Liu]

On Wed, Dec 21, 2016 at 05:45:13PM +0800, Yuanhan Liu wrote:
> From: Jan Wickbom <jan.wickbom@ericsson.com>
> 
> 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.
> 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
> 
> Reported-by: Patrik Andersson <patrik.r.andersson@ericsson.com>
> Signed-off-by: Jan Wickbom <jan.wickbom@ericsson.com>
> Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>

Applied to dpdk-next-virtio.

	--yliu