Re: [PATCH] Switch to poll in xenconsoled's io loop.

[Mats Petersson <mats.petersson@citrix.com>]

On 03/01/13 17:14, Wei Liu wrote:
> The original implementation utilies select(). In Linux select() typically
> supports up to 1024 file descriptors. This can be a problem when user tries to
> boot up many guests. Switching to poll() has minimum impact on existing code
> and has better scalibility.
>
> Up to 8192 file descriptors are supported in the current implementation.
>
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> ---
>   tools/console/daemon/io.c |   90 +++++++++++++++++++++++++--------------------
>   1 file changed, 50 insertions(+), 40 deletions(-)
>
> diff --git a/tools/console/daemon/io.c b/tools/console/daemon/io.c
> index 48fe151..4e3c55c 100644
> --- a/tools/console/daemon/io.c
> +++ b/tools/console/daemon/io.c
> @@ -28,7 +28,7 @@
>   #include <stdlib.h>
>   #include <errno.h>
>   #include <string.h>
> -#include <sys/select.h>
> +#include <poll.h>
>   #include <fcntl.h>
>   #include <unistd.h>
>   #include <termios.h>
> @@ -930,7 +930,6 @@ static void handle_log_reload(void)
>   
>   void handle_io(void)
>   {
> -	fd_set readfds, writefds;
>   	int ret;
>   
>   	if (log_hv) {
> @@ -959,21 +958,33 @@ void handle_io(void)
>   
>   	for (;;) {
>   		struct domain *d, *n;
> -		int max_fd = -1;
> -		struct timeval timeout;
> +		int poll_timeout; /* timeout in milliseconds */
>   		struct timespec ts;
>   		long long now, next_timeout = 0;
>   
> -		FD_ZERO(&readfds);
> -		FD_ZERO(&writefds);
> -
> -		FD_SET(xs_fileno(xs), &readfds);
> -		max_fd = MAX(xs_fileno(xs), max_fd);
> -
> -		if (log_hv) {
> -			FD_SET(xc_evtchn_fd(xce_handle), &readfds);
> -			max_fd = MAX(xc_evtchn_fd(xce_handle), max_fd);
> -		}
> +#define MAX_POLL_FDS 8192
> +		static struct pollfd fds[MAX_POLL_FDS];
> +		static struct pollfd *fd_to_pollfd[MAX_POLL_FDS];
> +		int nr_fds;
> +#define SET_FDS(_fd, _events) do {				\
> +			if (_fd >= MAX_POLL_FDS)		\
> +				break;				\
> +			fds[nr_fds].fd = (_fd);			\
> +			fds[nr_fds].events = (_events);		\
> +			fd_to_pollfd[(_fd)] = &fds[nr_fds];	\
> +			nr_fds++;				\
> +		} while (0)
> +#define FD_REVENTS(_fd) (((_fd) < MAX_POLL_FDS && fd_to_pollfd[(_fd)]) ? \
> +			 fd_to_pollfd[(_fd)]->revents : 0)
> +
> +		nr_fds = 0;
> +		memset(fds, 0, sizeof(fds));
> +		memset(fd_to_pollfd, 0, sizeof(fd_to_pollfd));
> +
> +		SET_FDS(xs_fileno(xs), POLLIN);
> +
> +		if (log_hv)
> +			SET_FDS(xc_evtchn_fd(xce_handle), POLLIN);
>   
Would it not make sense to use dynamically allocated memory instead - 
that way, when we run out of 8192, there is nothing to change.

At the very least, can we have some sort of error output when it goes 
above the limit, so that we don't just silently "miss" a few consoles 
out from the poll.

--
Mats
>   		if (clock_gettime(CLOCK_MONOTONIC, &ts) < 0)
>   			return;
> @@ -982,11 +993,7 @@ void handle_io(void)
>   		/* Re-calculate any event counter allowances & unblock
>   		   domains with new allowance */
>   		for (d = dom_head; d; d = d->next) {
> -			/* Add 5ms of fuzz since select() often returns
> -			   a couple of ms sooner than requested. Without
> -			   the fuzz we typically do an extra spin in select()
> -			   with a 1/2 ms timeout every other iteration */
> -			if ((now+5) > d->next_period) {
> +			if (now > d->next_period) {
>   				d->next_period = now + RATE_LIMIT_PERIOD;
>   				if (d->event_count >= RATE_LIMIT_ALLOWANCE) {
>   					(void)xc_evtchn_unmask(d->xce_handle, d->local_port);
> @@ -1006,74 +1013,73 @@ void handle_io(void)
>   				    !d->buffer.max_capacity ||
>   				    d->buffer.size < d->buffer.max_capacity) {
>   					int evtchn_fd = xc_evtchn_fd(d->xce_handle);
> -					FD_SET(evtchn_fd, &readfds);
> -					max_fd = MAX(evtchn_fd, max_fd);
> +					SET_FDS(evtchn_fd, POLLIN);
>   				}
>   			}
>   
>   			if (d->master_fd != -1) {
> +				short events = 0;
>   				if (!d->is_dead && ring_free_bytes(d))
> -					FD_SET(d->master_fd, &readfds);
> +					events |= POLLIN;
>   
>   				if (!buffer_empty(&d->buffer))
> -					FD_SET(d->master_fd, &writefds);
> -				max_fd = MAX(d->master_fd, max_fd);
> +					events |= POLLOUT;
> +
> +				if (events)
> +					SET_FDS(d->master_fd, events);
>   			}
>   		}
>   
>   		/* If any domain has been rate limited, we need to work
> -		   out what timeout to supply to select */
> +		   out what timeout to supply to poll */
>   		if (next_timeout) {
>   			long long duration = (next_timeout - now);
>   			if (duration <= 0) /* sanity check */
>   				duration = 1;
> -			timeout.tv_sec = duration / 1000;
> -			timeout.tv_usec = ((duration - (timeout.tv_sec * 1000))
> -					   * 1000);
> +			poll_timeout = (int)duration;
>   		}
>   
> -		ret = select(max_fd + 1, &readfds, &writefds, 0,
> -			     next_timeout ? &timeout : NULL);
> +		ret = poll(fds, nr_fds, next_timeout ? poll_timeout : -1);
>   
>   		if (log_reload) {
>   			handle_log_reload();
>   			log_reload = 0;
>   		}
>   
> -		/* Abort if select failed, except for EINTR cases
> +		/* Abort if poll failed, except for EINTR cases
>   		   which indicate a possible log reload */
>   		if (ret == -1) {
>   			if (errno == EINTR)
>   				continue;
> -			dolog(LOG_ERR, "Failure in select: %d (%s)",
> +			dolog(LOG_ERR, "Failure in poll: %d (%s)",
>   			      errno, strerror(errno));
>   			break;
>   		}
>   
> -		if (log_hv && FD_ISSET(xc_evtchn_fd(xce_handle), &readfds))
> +		if (log_hv && FD_REVENTS(xc_evtchn_fd(xce_handle)) & POLLIN)
>   			handle_hv_logs();
>   
>   		if (ret <= 0)
>   			continue;
>   
> -		if (FD_ISSET(xs_fileno(xs), &readfds))
> +		if (FD_REVENTS(xs_fileno(xs)) & POLLIN)
>   			handle_xs();
>   
>   		for (d = dom_head; d; d = n) {
>   			n = d->next;
>   			if (d->event_count < RATE_LIMIT_ALLOWANCE) {
>   				if (d->xce_handle != NULL &&
> -				    FD_ISSET(xc_evtchn_fd(d->xce_handle),
> -					     &readfds))
> +				    FD_REVENTS(xc_evtchn_fd(d->xce_handle)) &
> +				    POLLIN)
>   					handle_ring_read(d);
>   			}
>   
> -			if (d->master_fd != -1 && FD_ISSET(d->master_fd,
> -							   &readfds))
> +			if (d->master_fd != -1 &&
> +			    FD_REVENTS(d->master_fd) & POLLIN)
>   				handle_tty_read(d);
>   
> -			if (d->master_fd != -1 && FD_ISSET(d->master_fd,
> -							   &writefds))
> +			if (d->master_fd != -1 &&
> +			    FD_REVENTS(d->master_fd) & POLLOUT)
>   				handle_tty_write(d);
>   
>   			if (d->last_seen != enum_pass)
> @@ -1082,6 +1088,10 @@ void handle_io(void)
>   			if (d->is_dead)
>   				cleanup_domain(d);
>   		}
> +
> +#undef MAX_POLL_FDS
> +#undef SET_FDS
> +#undef FD_REVENTS
>   	}
>   
>    out: