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

Message ID 1480606010-6132-1-git-send-email-jan.wickbom@ericsson.com (mailing list archive)
State Superseded, archived
Delegated to: Yuanhan Liu
Headers

Checks

Context Check Description
checkpatch/checkpatch success coding style OK

Commit Message

Jan Wickbom Dec. 1, 2016, 3:26 p.m. UTC
  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(-)
  

Comments

Yuanhan Liu Dec. 6, 2016, 6:56 a.m. UTC | #1
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
  
Jan Wickbom Dec. 6, 2016, 11:42 a.m. UTC | #2
> -----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
  
Yuanhan Liu Dec. 7, 2016, 7:43 a.m. UTC | #3
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
  
Yuanhan Liu Dec. 7, 2016, 10:12 a.m. UTC | #4
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
  
Jan Wickbom Dec. 7, 2016, 1:23 p.m. UTC | #5
> -----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
  
Yuanhan Liu Dec. 8, 2016, 5:50 a.m. UTC | #6
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
  
Jan Wickbom Dec. 12, 2016, 4:55 p.m. UTC | #7
> -----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
  

Patch

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 */
 };