| Message ID | 1359979973-31338-4-git-send-email-stefanha@redhat.com |
|---|---|
| State | New |
| Headers | show |
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>
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
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;
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(-)