From mboxrd@z Thu Jan 1 00:00:00 1970 From: Mats Petersson Subject: Re: [PATCH] Switch from select() to poll() in xenconsoled's IO loop Date: Tue, 8 Jan 2013 14:01:10 +0000 Message-ID: <50EC26A6.90405@citrix.com> References: <1357645812-25072-1-git-send-email-wei.liu2@citrix.com> <1357649563.13581.19.camel@iceland> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1357649563.13581.19.camel@iceland> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Wei Liu Cc: Ian Jackson , Ian Campbell , "xen-devel@lists.xen.org" List-Id: xen-devel@lists.xenproject.org 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 >> --- >> 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 >> #include >> #include >> -#include >> +#include >> #include >> #include >> #include >> @@ -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