Patchwork [v3,03/10] main-loop: switch POSIX glib integration to GPollFD

login
register
mail settings
Submitter Stefan Hajnoczi
Date Feb. 4, 2013, 12:12 p.m.
Message ID <1359979973-31338-4-git-send-email-stefanha@redhat.com>
Download mbox | patch
Permalink /patch/217895/
State New
Headers show

Comments

Stefan Hajnoczi - Feb. 4, 2013, 12:12 p.m.
Convert glib file descriptor polling from rfds/wfds/xfds to GPollFD.

The Windows code still needs poll_fds[] and n_poll_fds but they can now
become local variables.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 main-loop.c | 71 +++++++++++++++++++------------------------------------------
 1 file changed, 22 insertions(+), 49 deletions(-)
Laszlo Ersek - Feb. 4, 2013, 4:56 p.m.
one remark below:

On 02/04/13 13:12, Stefan Hajnoczi wrote:
> Convert glib file descriptor polling from rfds/wfds/xfds to GPollFD.
> 
> The Windows code still needs poll_fds[] and n_poll_fds but they can now
> become local variables.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  main-loop.c | 71 +++++++++++++++++++------------------------------------------
>  1 file changed, 22 insertions(+), 49 deletions(-)
> 
> diff --git a/main-loop.c b/main-loop.c
> index f1dcd14..12b0213 100644
> --- a/main-loop.c
> +++ b/main-loop.c
> @@ -145,8 +145,6 @@ int qemu_init_main_loop(void)
>  
>  static fd_set rfds, wfds, xfds;
>  static int nfds;
> -static GPollFD poll_fds[1024 * 2]; /* this is probably overkill */
> -static int n_poll_fds;
>  static int max_priority;
>  
>  /* Load rfds/wfds/xfds into gpollfds.  Will be removed a few commits later. */
> @@ -206,65 +204,39 @@ static void gpollfds_to_select(int ret)
>  }
>  
>  #ifndef _WIN32
> -static void glib_select_fill(int *max_fd, fd_set *rfds, fd_set *wfds,
> -                             fd_set *xfds, uint32_t *cur_timeout)
> +static int glib_pollfds_idx;
> +static int glib_n_poll_fds;
> +
> +static void glib_pollfds_fill(uint32_t *cur_timeout)
>  {
>      GMainContext *context = g_main_context_default();
> -    int i;
>      int timeout = 0;
> +    int n;
>  
>      g_main_context_prepare(context, &max_priority);
>  
> -    n_poll_fds = g_main_context_query(context, max_priority, &timeout,
> -                                      poll_fds, ARRAY_SIZE(poll_fds));
> -    g_assert(n_poll_fds <= ARRAY_SIZE(poll_fds));
> -
> -    for (i = 0; i < n_poll_fds; i++) {
> -        GPollFD *p = &poll_fds[i];
> -
> -        if ((p->events & G_IO_IN)) {
> -            FD_SET(p->fd, rfds);
> -            *max_fd = MAX(*max_fd, p->fd);
> -        }
> -        if ((p->events & G_IO_OUT)) {
> -            FD_SET(p->fd, wfds);
> -            *max_fd = MAX(*max_fd, p->fd);
> -        }
> -        if ((p->events & G_IO_ERR)) {
> -            FD_SET(p->fd, xfds);
> -            *max_fd = MAX(*max_fd, p->fd);
> -        }
> -    }
> +    glib_pollfds_idx = gpollfds->len;
> +    n = glib_n_poll_fds;
> +    do {
> +        GPollFD *pfds;
> +        glib_n_poll_fds = n;
> +        g_array_set_size(gpollfds, glib_pollfds_idx + glib_n_poll_fds);
> +        pfds = &g_array_index(gpollfds, GPollFD, glib_pollfds_idx);
> +        n = g_main_context_query(context, max_priority, &timeout, pfds,
> +                                 glib_n_poll_fds);
> +    } while (n != glib_n_poll_fds);

This seems clever, but I'm not sure about all the use cases.

When it runs for the very first time, it goes in with
glib_n_poll_fds==0, ie. it queries the number of records needed. Then it
resizes the array so that there are that many records from
glib_pollfds_idx onwards. The second query is expected to succeed.

I wonder if you could just issue a query call up-front with n_fds==0 (to
retrieve the number of records needed) and eliminate the loop. Also,
although the "gpollfds" array is emptied for each iteration in
main_loop_wait() -- and consequently "glib_pollfds_idx" is re-set --,
"glib_n_poll_fds" seems to survive the iterations. Do you intend that as
some kind of caching?

If this "glib_n_poll_fds" from the prior main-loop iteration is exactly
correct, we save one query / iteration here. If the previous value is
too small, then we (maybe) store a leading subsequence of the records,
and retry with the correct size (same case as with glib_n_poll_fds==0
described above).

However if the prior value is too big, our first query succeeds, but
then we truncate the array (which I guess leaves the remaining elements,
ie. all what we need, intact), and then we redo the query unnecessarily.

Again, I wonder if you could just set glib_n_poll_fds=0 right beside
where you re-set "glib_pollfds_idx". If so, then you could eliminate the
loop I believe, just pre-query the number of records needed without
actually storing any.

Anyhow, the patch seems good to me.

Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Stefan Hajnoczi - Feb. 5, 2013, 12:19 p.m.
On Mon, Feb 04, 2013 at 05:56:43PM +0100, Laszlo Ersek wrote:
> one remark below:
> 
> On 02/04/13 13:12, Stefan Hajnoczi wrote:
> > Convert glib file descriptor polling from rfds/wfds/xfds to GPollFD.
> > 
> > The Windows code still needs poll_fds[] and n_poll_fds but they can now
> > become local variables.
> > 
> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > ---
> >  main-loop.c | 71 +++++++++++++++++++------------------------------------------
> >  1 file changed, 22 insertions(+), 49 deletions(-)
> > 
> > diff --git a/main-loop.c b/main-loop.c
> > index f1dcd14..12b0213 100644
> > --- a/main-loop.c
> > +++ b/main-loop.c
> > @@ -145,8 +145,6 @@ int qemu_init_main_loop(void)
> >  
> >  static fd_set rfds, wfds, xfds;
> >  static int nfds;
> > -static GPollFD poll_fds[1024 * 2]; /* this is probably overkill */
> > -static int n_poll_fds;
> >  static int max_priority;
> >  
> >  /* Load rfds/wfds/xfds into gpollfds.  Will be removed a few commits later. */
> > @@ -206,65 +204,39 @@ static void gpollfds_to_select(int ret)
> >  }
> >  
> >  #ifndef _WIN32
> > -static void glib_select_fill(int *max_fd, fd_set *rfds, fd_set *wfds,
> > -                             fd_set *xfds, uint32_t *cur_timeout)
> > +static int glib_pollfds_idx;
> > +static int glib_n_poll_fds;
> > +
> > +static void glib_pollfds_fill(uint32_t *cur_timeout)
> >  {
> >      GMainContext *context = g_main_context_default();
> > -    int i;
> >      int timeout = 0;
> > +    int n;
> >  
> >      g_main_context_prepare(context, &max_priority);
> >  
> > -    n_poll_fds = g_main_context_query(context, max_priority, &timeout,
> > -                                      poll_fds, ARRAY_SIZE(poll_fds));
> > -    g_assert(n_poll_fds <= ARRAY_SIZE(poll_fds));
> > -
> > -    for (i = 0; i < n_poll_fds; i++) {
> > -        GPollFD *p = &poll_fds[i];
> > -
> > -        if ((p->events & G_IO_IN)) {
> > -            FD_SET(p->fd, rfds);
> > -            *max_fd = MAX(*max_fd, p->fd);
> > -        }
> > -        if ((p->events & G_IO_OUT)) {
> > -            FD_SET(p->fd, wfds);
> > -            *max_fd = MAX(*max_fd, p->fd);
> > -        }
> > -        if ((p->events & G_IO_ERR)) {
> > -            FD_SET(p->fd, xfds);
> > -            *max_fd = MAX(*max_fd, p->fd);
> > -        }
> > -    }
> > +    glib_pollfds_idx = gpollfds->len;
> > +    n = glib_n_poll_fds;
> > +    do {
> > +        GPollFD *pfds;
> > +        glib_n_poll_fds = n;
> > +        g_array_set_size(gpollfds, glib_pollfds_idx + glib_n_poll_fds);
> > +        pfds = &g_array_index(gpollfds, GPollFD, glib_pollfds_idx);
> > +        n = g_main_context_query(context, max_priority, &timeout, pfds,
> > +                                 glib_n_poll_fds);
> > +    } while (n != glib_n_poll_fds);
> 
> This seems clever, but I'm not sure about all the use cases.
> 
> When it runs for the very first time, it goes in with
> glib_n_poll_fds==0, ie. it queries the number of records needed. Then it
> resizes the array so that there are that many records from
> glib_pollfds_idx onwards. The second query is expected to succeed.
> 
> I wonder if you could just issue a query call up-front with n_fds==0 (to
> retrieve the number of records needed) and eliminate the loop. Also,
> although the "gpollfds" array is emptied for each iteration in
> main_loop_wait() -- and consequently "glib_pollfds_idx" is re-set --,
> "glib_n_poll_fds" seems to survive the iterations. Do you intend that as
> some kind of caching?

Yes and this is the approach that glib itself uses when it calls
g_main_context_query().

g_main_context_query() takes O(n) time in the number of GPollFDs.  I had
hoped n_fds==0 would be a O(1) operation but that is not the case.
Because of this I figured it is better to keep the last n_fds stored
between iterations - we'll guess right most of the time.

> Again, I wonder if you could just set glib_n_poll_fds=0 right beside
> where you re-set "glib_pollfds_idx". If so, then you could eliminate the
> loop I believe, just pre-query the number of records needed without
> actually storing any.

The most common scenario is where glib_n_poll_fds stays the same between
calls.  Querying with n_fds==0 would iterate over all GPollFDs
needlessly.  We need glib_n_poll_fds again in the glib_pollfds_poll()
function (it cannot be dropped) so I feel the complexity trade-off is
worth it here.

Stefan

Patch

diff --git a/main-loop.c b/main-loop.c
index f1dcd14..12b0213 100644
--- a/main-loop.c
+++ b/main-loop.c
@@ -145,8 +145,6 @@  int qemu_init_main_loop(void)
 
 static fd_set rfds, wfds, xfds;
 static int nfds;
-static GPollFD poll_fds[1024 * 2]; /* this is probably overkill */
-static int n_poll_fds;
 static int max_priority;
 
 /* Load rfds/wfds/xfds into gpollfds.  Will be removed a few commits later. */
@@ -206,65 +204,39 @@  static void gpollfds_to_select(int ret)
 }
 
 #ifndef _WIN32
-static void glib_select_fill(int *max_fd, fd_set *rfds, fd_set *wfds,
-                             fd_set *xfds, uint32_t *cur_timeout)
+static int glib_pollfds_idx;
+static int glib_n_poll_fds;
+
+static void glib_pollfds_fill(uint32_t *cur_timeout)
 {
     GMainContext *context = g_main_context_default();
-    int i;
     int timeout = 0;
+    int n;
 
     g_main_context_prepare(context, &max_priority);
 
-    n_poll_fds = g_main_context_query(context, max_priority, &timeout,
-                                      poll_fds, ARRAY_SIZE(poll_fds));
-    g_assert(n_poll_fds <= ARRAY_SIZE(poll_fds));
-
-    for (i = 0; i < n_poll_fds; i++) {
-        GPollFD *p = &poll_fds[i];
-
-        if ((p->events & G_IO_IN)) {
-            FD_SET(p->fd, rfds);
-            *max_fd = MAX(*max_fd, p->fd);
-        }
-        if ((p->events & G_IO_OUT)) {
-            FD_SET(p->fd, wfds);
-            *max_fd = MAX(*max_fd, p->fd);
-        }
-        if ((p->events & G_IO_ERR)) {
-            FD_SET(p->fd, xfds);
-            *max_fd = MAX(*max_fd, p->fd);
-        }
-    }
+    glib_pollfds_idx = gpollfds->len;
+    n = glib_n_poll_fds;
+    do {
+        GPollFD *pfds;
+        glib_n_poll_fds = n;
+        g_array_set_size(gpollfds, glib_pollfds_idx + glib_n_poll_fds);
+        pfds = &g_array_index(gpollfds, GPollFD, glib_pollfds_idx);
+        n = g_main_context_query(context, max_priority, &timeout, pfds,
+                                 glib_n_poll_fds);
+    } while (n != glib_n_poll_fds);
 
     if (timeout >= 0 && timeout < *cur_timeout) {
         *cur_timeout = timeout;
     }
 }
 
-static void glib_select_poll(fd_set *rfds, fd_set *wfds, fd_set *xfds,
-                             bool err)
+static void glib_pollfds_poll(void)
 {
     GMainContext *context = g_main_context_default();
+    GPollFD *pfds = &g_array_index(gpollfds, GPollFD, glib_pollfds_idx);
 
-    if (!err) {
-        int i;
-
-        for (i = 0; i < n_poll_fds; i++) {
-            GPollFD *p = &poll_fds[i];
-
-            if ((p->events & G_IO_IN) && FD_ISSET(p->fd, rfds)) {
-                p->revents |= G_IO_IN;
-            }
-            if ((p->events & G_IO_OUT) && FD_ISSET(p->fd, wfds)) {
-                p->revents |= G_IO_OUT;
-            }
-            if ((p->events & G_IO_ERR) && FD_ISSET(p->fd, xfds)) {
-                p->revents |= G_IO_ERR;
-            }
-        }
-    }
-
-    if (g_main_context_check(context, max_priority, poll_fds, n_poll_fds)) {
+    if (g_main_context_check(context, max_priority, pfds, glib_n_poll_fds)) {
         g_main_context_dispatch(context);
     }
 }
@@ -273,7 +245,7 @@  static int os_host_main_loop_wait(uint32_t timeout)
 {
     int ret;
 
-    glib_select_fill(&nfds, &rfds, &wfds, &xfds, &timeout);
+    glib_pollfds_fill(&timeout);
 
     if (timeout > 0) {
         qemu_mutex_unlock_iothread();
@@ -292,7 +264,7 @@  static int os_host_main_loop_wait(uint32_t timeout)
         qemu_mutex_lock_iothread();
     }
 
-    glib_select_poll(&rfds, &wfds, &xfds, (ret < 0));
+    glib_pollfds_poll();
     return ret;
 }
 #else
@@ -438,8 +410,9 @@  static void pollfds_poll(GArray *pollfds, int nfds, fd_set *rfds,
 static int os_host_main_loop_wait(uint32_t timeout)
 {
     GMainContext *context = g_main_context_default();
+    GPollFD poll_fds[1024 * 2]; /* this is probably overkill */
     int select_ret = 0;
-    int g_poll_ret, ret, i;
+    int g_poll_ret, ret, i, n_poll_fds;
     PollingEntry *pe;
     WaitObjects *w = &wait_objects;
     gint poll_timeout;