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

[Mats Petersson <mats.petersson@xxxxxxxxxx>]

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

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel