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 <stefa...@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 <ler...@redhat.com>
