Re: [PATCH] Switch from select() to poll() in xenconsoled's IO loop

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

On 08/01/13 12:52, Wei Liu wrote:
> Just to be clear, this is version 5 of the patch.
>
> Git send-email mysteriously dropped my subject prefix.
>
>
> Wei.
>
> On Tue, 2013-01-08 at 11:50 +0000, Wei Liu wrote:
>> 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.
>>
>> pollfd array is dynamically allocated / reallocated. If the array fails to
>> expand, we just ignore the incoming fd.
>>
>> Change from V2:
>>    * remove unnecessary malloc in initialize_pollfd_arrays
>>    * use ROUND_UP to get new size of arrays
>>
>> Change from V3:
>>    * remove initialize and destroy function for array
>>    * embedded tracking structure in struct domain, eliminate fd_to_pollfd
>>
>> Change from V4:
>>    * make xs_pollfd local to io.c
>>    * add back the 5 ms fuzz
>>    * handle POLLERR and POLLHUP
>>    * treat POLLPRI as error if set in revents
>>    * abort if xenconsoled's own fds get screwed
>>    * handle broken tty if tty's fds get screwed
>>
>> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
>> ---
>>   tools/console/daemon/io.c |  189 +++++++++++++++++++++++++++++++--------------
>>   1 file changed, 131 insertions(+), 58 deletions(-)
>>
>> diff --git a/tools/console/daemon/io.c b/tools/console/daemon/io.c
>> index 48fe151..8d16cac 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>
>> @@ -69,6 +69,7 @@ static int log_hv_fd = -1;
>>   static evtchn_port_or_error_t log_hv_evtchn = -1;
>>   static xc_interface *xch; /* why does xenconsoled have two xc handles ? */
>>   static xc_evtchn *xce_handle = NULL;
>> +static struct pollfd *xce_pollfd = NULL;
>>   
>>   struct buffer {
>>   	char *data;
>> @@ -81,7 +82,9 @@ struct buffer {
>>   struct domain {
>>   	int domid;
>>   	int master_fd;
>> +	struct pollfd *master_pollfd;
>>   	int slave_fd;
>> +	struct pollfd *slave_pollfd;
>>   	int log_fd;
>>   	bool is_dead;
>>   	unsigned last_seen;
>> @@ -92,6 +95,7 @@ struct domain {
>>   	evtchn_port_or_error_t local_port;
>>   	evtchn_port_or_error_t remote_port;
>>   	xc_evtchn *xce_handle;
>> +	struct pollfd *xce_pollfd;
>>   	struct xencons_interface *interface;
>>   	int event_count;
>>   	long long next_period;
>> @@ -769,6 +773,17 @@ static int ring_free_bytes(struct domain *dom)
>>   	return (sizeof(intf->in) - space);
>>   }
>>   
>> +static void domain_handle_broken_tty(struct domain *dom, int recreate)
>> +{
>> +	domain_close_tty(dom);
>> +
>> +	if (recreate) {
>> +		domain_create_tty(dom);
>> +	} else {
>> +		shutdown_domain(dom);
>> +	}
>> +}
>> +
>>   static void handle_tty_read(struct domain *dom)
>>   {
>>   	ssize_t len = 0;
>> @@ -794,13 +809,7 @@ static void handle_tty_read(struct domain *dom)
>>   	 * keep the slave open for the duration.
>>   	 */
>>   	if (len < 0) {
>> -		domain_close_tty(dom);
>> -
>> -		if (domain_is_valid(dom->domid)) {
>> -			domain_create_tty(dom);
>> -		} else {
>> -			shutdown_domain(dom);
>> -		}
>> +		domain_handle_broken_tty(dom, domain_is_valid(dom->domid));
>>   	} else if (domain_is_valid(dom->domid)) {
>>   		prod = intf->in_prod;
>>   		for (i = 0; i < len; i++) {
>> @@ -828,14 +837,7 @@ static void handle_tty_write(struct domain *dom)
>>    	if (len < 1) {
>>   		dolog(LOG_DEBUG, "Write failed on domain %d: %zd, %d\n",
>>   		      dom->domid, len, errno);
>> -
>> -		domain_close_tty(dom);
>> -
>> -		if (domain_is_valid(dom->domid)) {
>> -			domain_create_tty(dom);
>> -		} else {
>> -			shutdown_domain(dom);
>> -		}
>> +		domain_handle_broken_tty(dom, domain_is_valid(dom->domid));
>>   	} else {
>>   		buffer_advance(&dom->buffer, len);
>>   	}
>> @@ -928,9 +930,53 @@ static void handle_log_reload(void)
>>   	}
>>   }
>>   
>> +struct pollfd *xs_pollfd;
Why is this not "static"?
>> +static struct pollfd  *fds;
>> +static unsigned int current_array_size;
>> +static unsigned int nr_fds;
There seems to be no particular reason why these variables are here, and 
the other ones up the top of the file. For example xce_handle is not 
used "above" here, but it's declared at the top of the file. I think 
keeping all variables together makes more sense than scattering them around.

Otherwise, looks good to me.

--
Mats