[Qemu-devel] [PATCH 1/2] Add glib support to main loop

[Qemu-devel] [PATCH 1/2] Add glib support to main loop

[Anthony Liguori]

This allows GSources to be used to register callback events in QEMU.  This is
useful as it allows us to take greater advantage of glib and also because it
allows us to write code that is more easily testable outside of QEMU since we
can make use of glib's main loop in unit tests.

All new code should use glib's callback mechanisms for registering fd events
which are very well documented at:

http://developer.gnome.org/glib/stable/glib-The-Main-Event-Loop.html

And:

http://developer.gnome.org/gio/stable/

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 vl.c |   74 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 74 insertions(+), 0 deletions(-)

diff --git a/vl.c b/vl.c
index 06a6f80..d661e8e 100644
--- a/vl.c
+++ b/vl.c
@@ -111,6 +111,8 @@ int main(int argc, char **argv)
 #define main qemu_main
 #endif /* CONFIG_COCOA */
 
+#include <glib.h>
+
 #include "hw/hw.h"
 #include "hw/boards.h"
 #include "hw/usb.h"
@@ -1321,6 +1323,75 @@ void qemu_system_vmstop_request(int reason)
     qemu_notify_event();
 }
 
+static GPollFD poll_fds[1024 * 2]; /* this is probably overkill */
+static int n_poll_fds;
+static int max_priority;
+
+static void glib_select_fill(int *max_fd, fd_set *rfds, fd_set *wfds,
+                             fd_set *xfds, struct timeval *tv)
+{
+    GMainContext *context = g_main_context_default();
+    int i;
+    int timeout = 0, cur_timeout;
+
+    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);
+        }
+    }
+
+    cur_timeout = (tv->tv_sec * 1000) + ((tv->tv_usec + 500) / 1000);
+    if (timeout >= 0 && timeout < cur_timeout) {
+        tv->tv_sec = timeout / 1000;
+        tv->tv_usec = (timeout % 1000) * 1000;
+    }
+}
+
+static void glib_select_poll(fd_set *rfds, fd_set *wfds, fd_set *xfds,
+                             bool err)
+{
+    GMainContext *context = g_main_context_default();
+
+    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)) {
+        g_main_context_dispatch(context);
+    }
+}
+
 void main_loop_wait(int nonblocking)
 {
     fd_set rfds, wfds, xfds;
@@ -1346,8 +1417,10 @@ void main_loop_wait(int nonblocking)
     FD_ZERO(&rfds);
     FD_ZERO(&wfds);
     FD_ZERO(&xfds);
+
     qemu_iohandler_fill(&nfds, &rfds, &wfds, &xfds);
     slirp_select_fill(&nfds, &rfds, &wfds, &xfds);
+    glib_select_fill(&nfds, &rfds, &wfds, &xfds, &tv);
 
     qemu_mutex_unlock_iothread();
     ret = select(nfds + 1, &rfds, &wfds, &xfds, &tv);
@@ -1355,6 +1428,7 @@ void main_loop_wait(int nonblocking)
 
     qemu_iohandler_poll(&rfds, &wfds, &xfds, ret);
     slirp_select_poll(&rfds, &wfds, &xfds, (ret < 0));
+    glib_select_poll(&rfds, &wfds, &xfds, (ret < 0));
 
     qemu_run_all_timers();
 
-- 
1.7.4.1

[Qemu-devel] [PATCH 2/2] main: switch qemu_set_fd_handler to g_io_add_watch

[Anthony Liguori]

This patch changes qemu_set_fd_handler to be implemented in terms of
g_io_add_watch().  The semantics are a bit different so some glue is required.

qemu_set_fd_handler2 is much harder to convert because of its use of polling.

The glib main loop has the major of advantage of having a proven thread safe
architecture.  By using the glib main loop instead of our own, it will allow us
to eventually introduce multiple I/O threads.

I'm pretty sure that this will work on Win32, but I would appreciate some help
testing.  I think the semantics of g_io_channel_unix_new() are really just tied
to the notion of a "unix fd" and not necessarily unix itself.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 iohandler.c |   57 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 56 insertions(+), 1 deletions(-)

diff --git a/iohandler.c b/iohandler.c
index 4deae1e..5ef66fb 100644
--- a/iohandler.c
+++ b/iohandler.c
@@ -80,12 +80,67 @@ int qemu_set_fd_handler2(int fd,
     return 0;
 }
 
+typedef struct IOTrampoline
+{
+    GIOChannel *chan;
+    IOHandler *fd_read;
+    IOHandler *fd_write;
+    void *opaque;
+    guint tag;
+} IOTrampoline;
+
+static gboolean fd_trampoline(GIOChannel *chan, GIOCondition cond, gpointer opaque)
+{
+    IOTrampoline *tramp = opaque;
+
+    if (tramp->opaque == NULL) {
+        return FALSE;
+    }
+
+    if ((cond & G_IO_IN) && tramp->fd_read) {
+        tramp->fd_read(tramp->opaque);
+    }
+
+    if ((cond & G_IO_OUT) && tramp->fd_write) {
+        tramp->fd_write(tramp->opaque);
+    }
+
+    return TRUE;
+}
+
 int qemu_set_fd_handler(int fd,
                         IOHandler *fd_read,
                         IOHandler *fd_write,
                         void *opaque)
 {
-    return qemu_set_fd_handler2(fd, NULL, fd_read, fd_write, opaque);
+    static IOTrampoline fd_trampolines[FD_SETSIZE];
+    IOTrampoline *tramp = &fd_trampolines[fd];
+
+    if (tramp->tag != 0) {
+        g_io_channel_unref(tramp->chan);
+        g_source_remove(tramp->tag);
+    }
+
+    if (opaque) {
+        GIOCondition cond = 0;
+
+        tramp->fd_read = fd_read;
+        tramp->fd_write = fd_write;
+        tramp->opaque = opaque;
+
+        if (fd_read) {
+            cond |= G_IO_IN | G_IO_ERR;
+        }
+
+        if (fd_write) {
+            cond |= G_IO_OUT | G_IO_ERR;
+        }
+
+        tramp->chan = g_io_channel_unix_new(fd);
+        tramp->tag = g_io_add_watch(tramp->chan, cond, fd_trampoline, tramp);
+    }
+
+    return 0;
 }
 
 void qemu_iohandler_fill(int *pnfds, fd_set *readfds, fd_set *writefds, fd_set *xfds)
-- 
1.7.4.1

Re: [Qemu-devel] [PATCH 2/2] main: switch qemu_set_fd_handler to g_io_add_watch

[Paolo Bonzini]

On 08/22/2011 03:12 PM, Anthony Liguori wrote:
> I'm pretty sure that this will work on Win32, but I would appreciate some help
> testing.  I think the semantics of g_io_channel_unix_new() are really just tied
> to the notion of a "unix fd" and not necessarily unix itself.

Almost: in Win32 you need to use g_io_channel_win32_new_socket.  But 
indeed on Windows you can only use qemu_set_fd_handler for sockets too.

Paolo

Re: [Qemu-devel] [PATCH 2/2] main: switch qemu_set_fd_handler to g_io_add_watch

[Anthony Liguori]

On 08/22/2011 08:40 AM, Paolo Bonzini wrote:
> On 08/22/2011 03:12 PM, Anthony Liguori wrote:
>> I'm pretty sure that this will work on Win32, but I would appreciate
>> some help
>> testing. I think the semantics of g_io_channel_unix_new() are really
>> just tied
>> to the notion of a "unix fd" and not necessarily unix itself.
>
> Almost: in Win32 you need to use g_io_channel_win32_new_socket. But
> indeed on Windows you can only use qemu_set_fd_handler for sockets too.

I think that's really only for read/write though.  If you're just 
polling on I/O, it shouldn't matter IIUC.

If someone has a Windows box, they can confirm/deny by using qemu 
-monitor tcp:localhost:1024,socket,nowait with this patch.

Regards,

Anthony Liguori

>
> Paolo
>

Re: [Qemu-devel] [PATCH 2/2] main: switch qemu_set_fd_handler to g_io_add_watch

[Paolo Bonzini]

On 08/22/2011 03:45 PM, Anthony Liguori wrote:
>>
>> Almost: in Win32 you need to use g_io_channel_win32_new_socket. But
>> indeed on Windows you can only use qemu_set_fd_handler for sockets too.
>
> I think that's really only for read/write though.  If you're just
> polling on I/O, it shouldn't matter IIUC.
>
> If someone has a Windows box, they can confirm/deny by using qemu
> -monitor tcp:localhost:1024,socket,nowait with this patch.

Actually you're right, it works automagically:

  * On Win32, this can be used either for files opened with the MSVCRT
  * (the Microsoft run-time C library) _open() or _pipe, including file
  * descriptors 0, 1 and 2 (corresponding to stdin, stdout and stderr),
  * or for Winsock SOCKETs. If the parameter is a legal file
  * descriptor, it is assumed to be such, otherwise it should be a
  * SOCKET. This relies on SOCKETs and file descriptors not
  * overlapping. If you want to be certain, call either
  * g_io_channel_win32_new_fd() or g_io_channel_win32_new_socket()
  * instead as appropriate.

So this patch would even let interested people enable exec migration on 
Windows.

Paolo

Re: [Qemu-devel] [PATCH 1/2] Add glib support to main loop

[Anthony Liguori]

On 08/22/2011 08:12 AM, Anthony Liguori wrote:
> This allows GSources to be used to register callback events in QEMU.  This is
> useful as it allows us to take greater advantage of glib and also because it
> allows us to write code that is more easily testable outside of QEMU since we
> can make use of glib's main loop in unit tests.
>
> All new code should use glib's callback mechanisms for registering fd events
> which are very well documented at:
>
> http://developer.gnome.org/glib/stable/glib-The-Main-Event-Loop.html
>
> And:
>
> http://developer.gnome.org/gio/stable/
>
> Signed-off-by: Anthony Liguori<aliguori@us.ibm.com>

Applied both of these.

Regards,

Anthony Liguori

> ---
>   vl.c |   74 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>   1 files changed, 74 insertions(+), 0 deletions(-)
>
> diff --git a/vl.c b/vl.c
> index 06a6f80..d661e8e 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -111,6 +111,8 @@ int main(int argc, char **argv)
>   #define main qemu_main
>   #endif /* CONFIG_COCOA */
>
> +#include<glib.h>
> +
>   #include "hw/hw.h"
>   #include "hw/boards.h"
>   #include "hw/usb.h"
> @@ -1321,6 +1323,75 @@ void qemu_system_vmstop_request(int reason)
>       qemu_notify_event();
>   }
>
> +static GPollFD poll_fds[1024 * 2]; /* this is probably overkill */
> +static int n_poll_fds;
> +static int max_priority;
> +
> +static void glib_select_fill(int *max_fd, fd_set *rfds, fd_set *wfds,
> +                             fd_set *xfds, struct timeval *tv)
> +{
> +    GMainContext *context = g_main_context_default();
> +    int i;
> +    int timeout = 0, cur_timeout;
> +
> +    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);
> +        }
> +    }
> +
> +    cur_timeout = (tv->tv_sec * 1000) + ((tv->tv_usec + 500) / 1000);
> +    if (timeout>= 0&&  timeout<  cur_timeout) {
> +        tv->tv_sec = timeout / 1000;
> +        tv->tv_usec = (timeout % 1000) * 1000;
> +    }
> +}
> +
> +static void glib_select_poll(fd_set *rfds, fd_set *wfds, fd_set *xfds,
> +                             bool err)
> +{
> +    GMainContext *context = g_main_context_default();
> +
> +    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)) {
> +        g_main_context_dispatch(context);
> +    }
> +}
> +
>   void main_loop_wait(int nonblocking)
>   {
>       fd_set rfds, wfds, xfds;
> @@ -1346,8 +1417,10 @@ void main_loop_wait(int nonblocking)
>       FD_ZERO(&rfds);
>       FD_ZERO(&wfds);
>       FD_ZERO(&xfds);
> +
>       qemu_iohandler_fill(&nfds,&rfds,&wfds,&xfds);
>       slirp_select_fill(&nfds,&rfds,&wfds,&xfds);
> +    glib_select_fill(&nfds,&rfds,&wfds,&xfds,&tv);
>
>       qemu_mutex_unlock_iothread();
>       ret = select(nfds + 1,&rfds,&wfds,&xfds,&tv);
> @@ -1355,6 +1428,7 @@ void main_loop_wait(int nonblocking)
>
>       qemu_iohandler_poll(&rfds,&wfds,&xfds, ret);
>       slirp_select_poll(&rfds,&wfds,&xfds, (ret<  0));
> +    glib_select_poll(&rfds,&wfds,&xfds, (ret<  0));
>
>       qemu_run_all_timers();
>

Re: [Qemu-devel] [PATCH 2/2] main: switch qemu_set_fd_handler to g_io_add_watch

[Avi Kivity]

On 08/22/2011 04:12 PM, Anthony Liguori wrote:
> This patch changes qemu_set_fd_handler to be implemented in terms of
> g_io_add_watch().  The semantics are a bit different so some glue is required.
>
> qemu_set_fd_handler2 is much harder to convert because of its use of polling.
>
> The glib main loop has the major of advantage of having a proven thread safe
> architecture.  By using the glib main loop instead of our own, it will allow us
> to eventually introduce multiple I/O threads.
>
> I'm pretty sure that this will work on Win32, but I would appreciate some help
> testing.  I think the semantics of g_io_channel_unix_new() are really just tied
> to the notion of a "unix fd" and not necessarily unix itself.

'git bisect' fingered this as responsible for breaking 
qcow2+cache=unsafe.  I think there's an off-by-one here and the guilty 
patch is the one that switches the main loop, but that's just a guess.

The symptoms are that a guest that is restarted (new qemu process) after 
install doesn't make it through grub - some image data didn't make it do 
disk.  With qcow2 and cache=unsafe that can easily happen through exit 
notifiers not being run and the entire qcow2 metadata being thrown out 
the window.  Running with raw+cache=unsafe works.


>
> diff --git a/iohandler.c b/iohandler.c
> index 4deae1e..5ef66fb 100644
> --- a/iohandler.c
> +++ b/iohandler.c
> @@ -80,12 +80,67 @@ int qemu_set_fd_handler2(int fd,
>       return 0;
>   }
>
> +typedef struct IOTrampoline
> +{
> +    GIOChannel *chan;
> +    IOHandler *fd_read;
> +    IOHandler *fd_write;
> +    void *opaque;
> +    guint tag;
> +} IOTrampoline;
> +
> +static gboolean fd_trampoline(GIOChannel *chan, GIOCondition cond, gpointer opaque)
> +{
> +    IOTrampoline *tramp = opaque;
> +
> +    if (tramp->opaque == NULL) {
> +        return FALSE;
> +    }
> +
> +    if ((cond&  G_IO_IN)&&  tramp->fd_read) {
> +        tramp->fd_read(tramp->opaque);
> +    }
> +
> +    if ((cond&  G_IO_OUT)&&  tramp->fd_write) {
> +        tramp->fd_write(tramp->opaque);
> +    }
> +
> +    return TRUE;
> +}
> +
>   int qemu_set_fd_handler(int fd,
>                           IOHandler *fd_read,
>                           IOHandler *fd_write,
>                           void *opaque)
>   {
> -    return qemu_set_fd_handler2(fd, NULL, fd_read, fd_write, opaque);
> +    static IOTrampoline fd_trampolines[FD_SETSIZE];
> +    IOTrampoline *tramp =&fd_trampolines[fd];
> +
> +    if (tramp->tag != 0) {
> +        g_io_channel_unref(tramp->chan);
> +        g_source_remove(tramp->tag);
> +    }
> +
> +    if (opaque) {
> +        GIOCondition cond = 0;
> +
> +        tramp->fd_read = fd_read;
> +        tramp->fd_write = fd_write;
> +        tramp->opaque = opaque;
> +
> +        if (fd_read) {
> +            cond |= G_IO_IN | G_IO_ERR;
> +        }
> +
> +        if (fd_write) {
> +            cond |= G_IO_OUT | G_IO_ERR;
> +        }
> +
> +        tramp->chan = g_io_channel_unix_new(fd);
> +        tramp->tag = g_io_add_watch(tramp->chan, cond, fd_trampoline, tramp);
> +    }
> +
> +    return 0;
>   }
>
>   void qemu_iohandler_fill(int *pnfds, fd_set *readfds, fd_set *writefds, fd_set *xfds)


-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH 2/2] main: switch qemu_set_fd_handler to g_io_add_watch

[Anthony Liguori]

On 09/04/2011 09:03 AM, Avi Kivity wrote:
> On 08/22/2011 04:12 PM, Anthony Liguori wrote:
>> This patch changes qemu_set_fd_handler to be implemented in terms of
>> g_io_add_watch(). The semantics are a bit different so some glue is
>> required.
>>
>> qemu_set_fd_handler2 is much harder to convert because of its use of
>> polling.
>>
>> The glib main loop has the major of advantage of having a proven
>> thread safe
>> architecture. By using the glib main loop instead of our own, it will
>> allow us
>> to eventually introduce multiple I/O threads.
>>
>> I'm pretty sure that this will work on Win32, but I would appreciate
>> some help
>> testing. I think the semantics of g_io_channel_unix_new() are really
>> just tied
>> to the notion of a "unix fd" and not necessarily unix itself.
>
> 'git bisect' fingered this as responsible for breaking
> qcow2+cache=unsafe. I think there's an off-by-one here and the guilty
> patch is the one that switches the main loop, but that's just a guess.

Semantically, this patch changes when a file descriptor callback is issued.

Does the following make a difference:

diff --git a/vl.c b/vl.c
index 5ba9b35..02f694d 100644
--- a/vl.c
+++ b/vl.c
@@ -1432,9 +1432,9 @@ int main_loop_wait(int nonblocking)
          qemu_mutex_lock_iothread();
      }

+    glib_select_poll(&rfds, &wfds, &xfds, (ret < 0));
      qemu_iohandler_poll(&rfds, &wfds, &xfds, ret);
      slirp_select_poll(&rfds, &wfds, &xfds, (ret < 0));
-    glib_select_poll(&rfds, &wfds, &xfds, (ret < 0));

      qemu_run_all_timers();

What type of guest install is it that fails for you and is this 
qemu-kvm.git or qemu.git?

Regards,

Anthony Liguori

>
> The symptoms are that a guest that is restarted (new qemu process) after
> install doesn't make it through grub - some image data didn't make it do
> disk. With qcow2 and cache=unsafe that can easily happen through exit
> notifiers not being run and the entire qcow2 metadata being thrown out
> the window. Running with raw+cache=unsafe works.
>
>
>>
>> diff --git a/iohandler.c b/iohandler.c
>> index 4deae1e..5ef66fb 100644
>> --- a/iohandler.c
>> +++ b/iohandler.c
>> @@ -80,12 +80,67 @@ int qemu_set_fd_handler2(int fd,
>> return 0;
>> }
>>
>> +typedef struct IOTrampoline
>> +{
>> + GIOChannel *chan;
>> + IOHandler *fd_read;
>> + IOHandler *fd_write;
>> + void *opaque;
>> + guint tag;
>> +} IOTrampoline;
>> +
>> +static gboolean fd_trampoline(GIOChannel *chan, GIOCondition cond,
>> gpointer opaque)
>> +{
>> + IOTrampoline *tramp = opaque;
>> +
>> + if (tramp->opaque == NULL) {
>> + return FALSE;
>> + }
>> +
>> + if ((cond& G_IO_IN)&& tramp->fd_read) {
>> + tramp->fd_read(tramp->opaque);
>> + }
>> +
>> + if ((cond& G_IO_OUT)&& tramp->fd_write) {
>> + tramp->fd_write(tramp->opaque);
>> + }
>> +
>> + return TRUE;
>> +}
>> +
>> int qemu_set_fd_handler(int fd,
>> IOHandler *fd_read,
>> IOHandler *fd_write,
>> void *opaque)
>> {
>> - return qemu_set_fd_handler2(fd, NULL, fd_read, fd_write, opaque);
>> + static IOTrampoline fd_trampolines[FD_SETSIZE];
>> + IOTrampoline *tramp =&fd_trampolines[fd];
>> +
>> + if (tramp->tag != 0) {
>> + g_io_channel_unref(tramp->chan);
>> + g_source_remove(tramp->tag);
>> + }
>> +
>> + if (opaque) {
>> + GIOCondition cond = 0;
>> +
>> + tramp->fd_read = fd_read;
>> + tramp->fd_write = fd_write;
>> + tramp->opaque = opaque;
>> +
>> + if (fd_read) {
>> + cond |= G_IO_IN | G_IO_ERR;
>> + }
>> +
>> + if (fd_write) {
>> + cond |= G_IO_OUT | G_IO_ERR;
>> + }
>> +
>> + tramp->chan = g_io_channel_unix_new(fd);
>> + tramp->tag = g_io_add_watch(tramp->chan, cond, fd_trampoline, tramp);
>> + }
>> +
>> + return 0;
>> }
>>
>> void qemu_iohandler_fill(int *pnfds, fd_set *readfds, fd_set
>> *writefds, fd_set *xfds)
>
>

Re: [Qemu-devel] [PATCH 2/2] main: switch qemu_set_fd_handler to g_io_add_watch

[Anthony Liguori]

On 09/04/2011 09:03 AM, Avi Kivity wrote:
> On 08/22/2011 04:12 PM, Anthony Liguori wrote:
>> This patch changes qemu_set_fd_handler to be implemented in terms of
>> g_io_add_watch(). The semantics are a bit different so some glue is
>> required.
>>
>> qemu_set_fd_handler2 is much harder to convert because of its use of
>> polling.
>>
>> The glib main loop has the major of advantage of having a proven
>> thread safe
>> architecture. By using the glib main loop instead of our own, it will
>> allow us
>> to eventually introduce multiple I/O threads.
>>
>> I'm pretty sure that this will work on Win32, but I would appreciate
>> some help
>> testing. I think the semantics of g_io_channel_unix_new() are really
>> just tied
>> to the notion of a "unix fd" and not necessarily unix itself.
>
> 'git bisect' fingered this as responsible for breaking
> qcow2+cache=unsafe. I think there's an off-by-one here and the guilty
> patch is the one that switches the main loop, but that's just a guess.
>
> The symptoms are that a guest that is restarted (new qemu process) after
> install doesn't make it through grub - some image data didn't make it do
> disk. With qcow2 and cache=unsafe that can easily happen through exit
> notifiers not being run and the entire qcow2 metadata being thrown out
> the window. Running with raw+cache=unsafe works.

Can you share your full command line?

Nothing that would be in the obvious path actually uses 
qemu_set_fd_handler...

Regards,

Anthony Liguori

>
>
>>
>> diff --git a/iohandler.c b/iohandler.c
>> index 4deae1e..5ef66fb 100644
>> --- a/iohandler.c
>> +++ b/iohandler.c
>> @@ -80,12 +80,67 @@ int qemu_set_fd_handler2(int fd,
>> return 0;
>> }
>>
>> +typedef struct IOTrampoline
>> +{
>> + GIOChannel *chan;
>> + IOHandler *fd_read;
>> + IOHandler *fd_write;
>> + void *opaque;
>> + guint tag;
>> +} IOTrampoline;
>> +
>> +static gboolean fd_trampoline(GIOChannel *chan, GIOCondition cond,
>> gpointer opaque)
>> +{
>> + IOTrampoline *tramp = opaque;
>> +
>> + if (tramp->opaque == NULL) {
>> + return FALSE;
>> + }
>> +
>> + if ((cond& G_IO_IN)&& tramp->fd_read) {
>> + tramp->fd_read(tramp->opaque);
>> + }
>> +
>> + if ((cond& G_IO_OUT)&& tramp->fd_write) {
>> + tramp->fd_write(tramp->opaque);
>> + }
>> +
>> + return TRUE;
>> +}
>> +
>> int qemu_set_fd_handler(int fd,
>> IOHandler *fd_read,
>> IOHandler *fd_write,
>> void *opaque)
>> {
>> - return qemu_set_fd_handler2(fd, NULL, fd_read, fd_write, opaque);
>> + static IOTrampoline fd_trampolines[FD_SETSIZE];
>> + IOTrampoline *tramp =&fd_trampolines[fd];
>> +
>> + if (tramp->tag != 0) {
>> + g_io_channel_unref(tramp->chan);
>> + g_source_remove(tramp->tag);
>> + }
>> +
>> + if (opaque) {
>> + GIOCondition cond = 0;
>> +
>> + tramp->fd_read = fd_read;
>> + tramp->fd_write = fd_write;
>> + tramp->opaque = opaque;
>> +
>> + if (fd_read) {
>> + cond |= G_IO_IN | G_IO_ERR;
>> + }
>> +
>> + if (fd_write) {
>> + cond |= G_IO_OUT | G_IO_ERR;
>> + }
>> +
>> + tramp->chan = g_io_channel_unix_new(fd);
>> + tramp->tag = g_io_add_watch(tramp->chan, cond, fd_trampoline, tramp);
>> + }
>> +
>> + return 0;
>> }
>>
>> void qemu_iohandler_fill(int *pnfds, fd_set *readfds, fd_set
>> *writefds, fd_set *xfds)
>
>

Re: [Qemu-devel] [PATCH 2/2] main: switch qemu_set_fd_handler to g_io_add_watch

[Avi Kivity]

On 09/04/2011 05:03 PM, Avi Kivity wrote:
> On 08/22/2011 04:12 PM, Anthony Liguori wrote:
>> This patch changes qemu_set_fd_handler to be implemented in terms of
>> g_io_add_watch().  The semantics are a bit different so some glue is 
>> required.
>>
>> qemu_set_fd_handler2 is much harder to convert because of its use of 
>> polling.
>>
>> The glib main loop has the major of advantage of having a proven 
>> thread safe
>> architecture.  By using the glib main loop instead of our own, it 
>> will allow us
>> to eventually introduce multiple I/O threads.
>>
>> I'm pretty sure that this will work on Win32, but I would appreciate 
>> some help
>> testing.  I think the semantics of g_io_channel_unix_new() are really 
>> just tied
>> to the notion of a "unix fd" and not necessarily unix itself.
>
> 'git bisect' fingered this as responsible for breaking 
> qcow2+cache=unsafe.  I think there's an off-by-one here and the guilty 
> patch is the one that switches the main loop, but that's just a guess.
>
> The symptoms are that a guest that is restarted (new qemu process) 
> after install doesn't make it through grub - some image data didn't 
> make it do disk.  With qcow2 and cache=unsafe that can easily happen 
> through exit notifiers not being run and the entire qcow2 metadata 
> being thrown out the window.  Running with raw+cache=unsafe works.
>

Upstream appears to work for me... strange.

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH 2/2] main: switch qemu_set_fd_handler to g_io_add_watch

[Paolo Bonzini]

On 08/22/2011 03:47 PM, Paolo Bonzini wrote:
> On 08/22/2011 03:45 PM, Anthony Liguori wrote:
>>>
>>> Almost: in Win32 you need to use g_io_channel_win32_new_socket. But
>>> indeed on Windows you can only use qemu_set_fd_handler for sockets too.
>>
>> I think that's really only for read/write though. If you're just
>> polling on I/O, it shouldn't matter IIUC.
>>
>> If someone has a Windows box, they can confirm/deny by using qemu
>> -monitor tcp:localhost:1024,socket,nowait with this patch.
>
> Actually you're right, it works automagically:
>
> * On Win32, this can be used either for files opened with the MSVCRT
> * (the Microsoft run-time C library) _open() or _pipe, including file
> * descriptors 0, 1 and 2 (corresponding to stdin, stdout and stderr),
> * or for Winsock SOCKETs. If the parameter is a legal file
> * descriptor, it is assumed to be such, otherwise it should be a
> * SOCKET. This relies on SOCKETs and file descriptors not
> * overlapping. If you want to be certain, call either
> * g_io_channel_win32_new_fd() or g_io_channel_win32_new_socket()
> * instead as appropriate.
>
> So this patch would even let interested people enable exec migration on
> Windows.

Hmmm, after reading documentation better, this unfortunately is 
completely broken under Windows, for two reasons:

1) in patch 1/2 you're using the glib pollfds and passing them to 
select().  Unfortunately under Windows they are special and can only be 
passed to g_poll().  Unfortunately, this can be fixed by changing the 
QEMU main loop to use poll() instead of select()...

2) ... because glib IO channels cannot be used just for watches under 
Windows:

/* Create an IO channel for C runtime (emulated Unix-like) file
  * descriptors. After calling g_io_add_watch() on a IO channel
  * returned by this function, you shouldn't call read() on the file
  * descriptor. This is because adding polling for a file descriptor is
  * implemented on Win32 by starting a thread that sits blocked in a
  * read() from the file descriptor most of the time. All reads from
  * the file descriptor should be done by this internal GLib
  * thread. Your code should call only g_io_channel_read().
  */

So, I believe the right solution would be to drop this patch for now and 
make 1/2 conditional on !_WIN32.

Paolo

Re: [Qemu-devel] [PATCH 2/2] main: switch qemu_set_fd_handler to g_io_add_watch

[Anthony Liguori]

On 09/06/2011 09:31 AM, Paolo Bonzini wrote:
> On 08/22/2011 03:47 PM, Paolo Bonzini wrote:
>> On 08/22/2011 03:45 PM, Anthony Liguori wrote:
>>>>
>>>> Almost: in Win32 you need to use g_io_channel_win32_new_socket. But
>>>> indeed on Windows you can only use qemu_set_fd_handler for sockets too.
>>>
>>> I think that's really only for read/write though. If you're just
>>> polling on I/O, it shouldn't matter IIUC.
>>>
>>> If someone has a Windows box, they can confirm/deny by using qemu
>>> -monitor tcp:localhost:1024,socket,nowait with this patch.
>>
>> Actually you're right, it works automagically:
>>
>> * On Win32, this can be used either for files opened with the MSVCRT
>> * (the Microsoft run-time C library) _open() or _pipe, including file
>> * descriptors 0, 1 and 2 (corresponding to stdin, stdout and stderr),
>> * or for Winsock SOCKETs. If the parameter is a legal file
>> * descriptor, it is assumed to be such, otherwise it should be a
>> * SOCKET. This relies on SOCKETs and file descriptors not
>> * overlapping. If you want to be certain, call either
>> * g_io_channel_win32_new_fd() or g_io_channel_win32_new_socket()
>> * instead as appropriate.
>>
>> So this patch would even let interested people enable exec migration on
>> Windows.
>
> Hmmm, after reading documentation better, this unfortunately is
> completely broken under Windows, for two reasons:
>
> 1) in patch 1/2 you're using the glib pollfds and passing them to
> select(). Unfortunately under Windows they are special and can only be
> passed to g_poll(). Unfortunately, this can be fixed by changing the
> QEMU main loop to use poll() instead of select()...

Hrm, okay.

> 2) ... because glib IO channels cannot be used just for watches under
> Windows:
>
> /* Create an IO channel for C runtime (emulated Unix-like) file
> * descriptors. After calling g_io_add_watch() on a IO channel
> * returned by this function, you shouldn't call read() on the file
> * descriptor. This is because adding polling for a file descriptor is
> * implemented on Win32 by starting a thread that sits blocked in a
> * read() from the file descriptor most of the time. All reads from
> * the file descriptor should be done by this internal GLib
> * thread. Your code should call only g_io_channel_read().
> */
>
> So, I believe the right solution would be to drop this patch for now and
> make 1/2 conditional on !_WIN32.

So it should be possible to add a new Source type that just selects on a 
file descriptor and avoid GIOChannels?

Regards,

Anthony Liguori

>
> Paolo

Re: [Qemu-devel] [PATCH 2/2] main: switch qemu_set_fd_handler to g_io_add_watch

[Paolo Bonzini]

On 09/06/2011 05:59 PM, Anthony Liguori wrote:
> So it should be possible to add a new Source type that just selects on a
> file descriptor and avoid GIOChannels?

I think you still have the problem that glib on Windows waits for 
HANDLEs, not file descriptors.  Also, I'm not sure it's worth it though 
as long as slirp still does its own fill/poll.

Paolo

Re: [Qemu-devel] [PATCH 2/2] main: switch qemu_set_fd_handler to g_io_add_watch

[Jan Kiszka]

On 2011-09-07 09:03, Paolo Bonzini wrote:
> On 09/06/2011 05:59 PM, Anthony Liguori wrote:
>> So it should be possible to add a new Source type that just selects on a
>> file descriptor and avoid GIOChannels?
> 
> I think you still have the problem that glib on Windows waits for
> HANDLEs, not file descriptors.  Also, I'm not sure it's worth it though
> as long as slirp still does its own fill/poll.

The latter can surely be improved, just takes someone to put on some
gloves and dig in the dirt...

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [PATCH 2/2] main: switch qemu_set_fd_handler to g_io_add_watch

[Anthony Liguori]

On 09/07/2011 02:03 AM, Paolo Bonzini wrote:
> On 09/06/2011 05:59 PM, Anthony Liguori wrote:
>> So it should be possible to add a new Source type that just selects on a
>> file descriptor and avoid GIOChannels?
>
> I think you still have the problem that glib on Windows waits for
> HANDLEs, not file descriptors. Also, I'm not sure it's worth it though
> as long as slirp still does its own fill/poll.

So how do we fix this long term?  We seem to get away with using fds 
today and not HANDLEs, do fds on Windows not work the same with poll as 
they do with select?

Regards,

Anthony Liguori

>
> Paolo
>

Re: [Qemu-devel] [PATCH 2/2] main: switch qemu_set_fd_handler to g_io_add_watch

[Avi Kivity]

On 09/04/2011 06:01 PM, Anthony Liguori wrote:
> On 09/04/2011 09:03 AM, Avi Kivity wrote:
>> On 08/22/2011 04:12 PM, Anthony Liguori wrote:
>>> This patch changes qemu_set_fd_handler to be implemented in terms of
>>> g_io_add_watch(). The semantics are a bit different so some glue is
>>> required.
>>>
>>> qemu_set_fd_handler2 is much harder to convert because of its use of
>>> polling.
>>>
>>> The glib main loop has the major of advantage of having a proven
>>> thread safe
>>> architecture. By using the glib main loop instead of our own, it will
>>> allow us
>>> to eventually introduce multiple I/O threads.
>>>
>>> I'm pretty sure that this will work on Win32, but I would appreciate
>>> some help
>>> testing. I think the semantics of g_io_channel_unix_new() are really
>>> just tied
>>> to the notion of a "unix fd" and not necessarily unix itself.
>>
>> 'git bisect' fingered this as responsible for breaking
>> qcow2+cache=unsafe. I think there's an off-by-one here and the guilty
>> patch is the one that switches the main loop, but that's just a guess.
>>
>> The symptoms are that a guest that is restarted (new qemu process) after
>> install doesn't make it through grub - some image data didn't make it do
>> disk. With qcow2 and cache=unsafe that can easily happen through exit
>> notifiers not being run and the entire qcow2 metadata being thrown out
>> the window. Running with raw+cache=unsafe works.
>
> Can you share your full command line?
>
> Nothing that would be in the obvious path actually uses 
> qemu_set_fd_handler...
>

I upgraded my autotest setup due to other issues, and now the symptoms 
are much worse... even before the merge that introduced this patch.

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH 2/2] main: switch qemu_set_fd_handler to g_io_add_watch

[Paolo Bonzini]

On 09/07/2011 02:42 PM, Anthony Liguori wrote:
> On 09/07/2011 02:03 AM, Paolo Bonzini wrote:
>> On 09/06/2011 05:59 PM, Anthony Liguori wrote:
>>> So it should be possible to add a new Source type that just selects on a
>>> file descriptor and avoid GIOChannels?
>>
>> I think you still have the problem that glib on Windows waits for
>> HANDLEs, not file descriptors. Also, I'm not sure it's worth it though
>> as long as slirp still does its own fill/poll.
>
> So how do we fix this long term?

Long term, we use GIOChannels for everything, assuming that's possible 
at all.  More realistically, we could rewrite socket handling on Windows 
so that we can use g_poll instead of select (don't wait for me doing that).

Another possibility, the ugliest but also the most realistic, is to 
separate the Windows and POSIX implementations of the main loop more 
sharply.  This way glib's main loop can be integrated (differently) into 
both implementations.

In the meanwhile: just do not rely on glib sources on Windows.  There 
isn't any large benefit in this patch, and it actually complicates the 
straightforward code in iohandler.  Just revert it and #ifdef the glib 
integration in patch 1/2.  Since I don't see a 100%-glib main loop 
anytime soon, we are unlikely to lose much.  If anybody introduces a 
feature that requires Avahi or GTK+, it won't be supported on Windows.

> We seem to get away with using fds
> today and not HANDLEs, do fds on Windows not work the same with poll as
> they do with select?

Here is a summary table:

select                    socket HANDLEs only
poll                      does not exist
WaitForMultipleObjects     all other HANDLEs
g_poll                    all other HANDLEs

We only use select for Windows socket handles today.  Everything else is 
handled separately (with WaitForMultipleObjects) by 
osdep-win32.c/oslib-win32.c.

Paolo

Re: [Qemu-devel] [PATCH 2/2] main: switch qemu_set_fd_handler to g_io_add_watch

[Anthony Liguori]

On 09/07/2011 09:40 AM, Paolo Bonzini wrote:
> On 09/07/2011 02:42 PM, Anthony Liguori wrote:
>> On 09/07/2011 02:03 AM, Paolo Bonzini wrote:
>>> On 09/06/2011 05:59 PM, Anthony Liguori wrote:
>>>> So it should be possible to add a new Source type that just selects
>>>> on a
>>>> file descriptor and avoid GIOChannels?
>>>
>>> I think you still have the problem that glib on Windows waits for
>>> HANDLEs, not file descriptors. Also, I'm not sure it's worth it though
>>> as long as slirp still does its own fill/poll.
>>
>> So how do we fix this long term?
>
> Long term, we use GIOChannels for everything, assuming that's possible
> at all. More realistically, we could rewrite socket handling on Windows
> so that we can use g_poll instead of select (don't wait for me doing that).

I assume switching to GIO would resolve all of these issues?

>
> Another possibility, the ugliest but also the most realistic, is to
> separate the Windows and POSIX implementations of the main loop more
> sharply. This way glib's main loop can be integrated (differently) into
> both implementations.
>
> In the meanwhile: just do not rely on glib sources on Windows. There
> isn't any large benefit in this patch, and it actually complicates the
> straightforward code in iohandler. Just revert it and #ifdef the glib
> integration in patch 1/2. Since I don't see a 100%-glib main loop
> anytime soon, we are unlikely to lose much. If anybody introduces a
> feature that requires Avahi or GTK+, it won't be supported on Windows.

My main motivation is unit testing.  I want to be able to have device 
models only rely on glib main loop primitives such that we can easily 
use devices in a simple glib main loop.

The split main loop approach won't work for that.

Regards,

Anthony Liguori

>
>> We seem to get away with using fds
>> today and not HANDLEs, do fds on Windows not work the same with poll as
>> they do with select?
>
> Here is a summary table:
>
> select socket HANDLEs only
> poll does not exist
> WaitForMultipleObjects all other HANDLEs
> g_poll all other HANDLEs
>
> We only use select for Windows socket handles today. Everything else is
> handled separately (with WaitForMultipleObjects) by
> osdep-win32.c/oslib-win32.c.
>
> Paolo
>

Re: [Qemu-devel] [PATCH 2/2] main: switch qemu_set_fd_handler to g_io_add_watch

[Paolo Bonzini]

On 09/07/2011 04:53 PM, Anthony Liguori wrote:
>>
>> Long term, we use GIOChannels for everything, assuming that's possible
>> at all. More realistically, we could rewrite socket handling on Windows
>> so that we can use g_poll instead of select (don't wait for me doing
>> that).
>
> I assume switching to GIO would resolve all of these issues?

Yes.

>> Another possibility, the ugliest but also the most realistic, is to
>> separate the Windows and POSIX implementations of the main loop more
>> sharply. This way glib's main loop can be integrated (differently) into
>> both implementations.
>>
>> In the meanwhile: just do not rely on glib sources on Windows. There
>> isn't any large benefit in this patch, and it actually complicates the
>> straightforward code in iohandler. Just revert it and #ifdef the glib
>> integration in patch 1/2. Since I don't see a 100%-glib main loop
>> anytime soon, we are unlikely to lose much. If anybody introduces a
>> feature that requires Avahi or GTK+, it won't be supported on Windows.
>
> My main motivation is unit testing.  I want to be able to have device
> models only rely on glib main loop primitives such that we can easily
> use devices in a simple glib main loop.
>
> The split main loop approach won't work for that.

What if you extract the QEMU main loop to common code, and use it in the 
tests?  Sounds not hard at all with iothread.

Paolo

Re: [Qemu-devel] [PATCH 2/2] main: switch qemu_set_fd_handler to g_io_add_watch

[Fabien Chouteau]

On 22/08/2011 15:47, Paolo Bonzini wrote:
> On 08/22/2011 03:45 PM, Anthony Liguori wrote:
>>>
>>> Almost: in Win32 you need to use g_io_channel_win32_new_socket. But
>>> indeed on Windows you can only use qemu_set_fd_handler for sockets too.
>>
>> I think that's really only for read/write though.  If you're just
>> polling on I/O, it shouldn't matter IIUC.
>>
>> If someone has a Windows box, they can confirm/deny by using qemu
>> -monitor tcp:localhost:1024,socket,nowait with this patch.
> 
> Actually you're right, it works automagically:
> 
>  * On Win32, this can be used either for files opened with the MSVCRT
>  * (the Microsoft run-time C library) _open() or _pipe, including file
>  * descriptors 0, 1 and 2 (corresponding to stdin, stdout and stderr),
>  * or for Winsock SOCKETs. If the parameter is a legal file
>  * descriptor, it is assumed to be such, otherwise it should be a
>  * SOCKET. This relies on SOCKETs and file descriptors not
>  * overlapping. If you want to be certain, call either
>  * g_io_channel_win32_new_fd() or g_io_channel_win32_new_socket()
>  * instead as appropriate.
> 
> So this patch would even let interested people enable exec migration on Windows.
> 

Hello,

I've run into some problems with this patch on Windows. The thing is
that select() should be used only with socket file descriptors.

If glib_select_fill() put a non-socket file descriptor in rfds or wfds,
select() will fail with this error (btw the return value of select is
not checked):

WSAENOTSOCK - Error 10038 - An operation was attempted on something that
is not a socket. The specified socket parameter refers to a file, not a
socket.

I've look at the patch and I don't see why do you pick file descriptors
from g_main_context_query's "poll_fds" to put them in the fd_sets (rfds,
wfds...) and then re-build a "poll_fds" to call g_main_context_check and
g_main_context_dispatch. From my understanding we can just do:

g_main_context_prepare(context, &max_priority);

n_poll_fds = g_main_context_query(context, max_priority, &timeout,
                                      poll_fds, ARRAY_SIZE(poll_fds));

if (g_main_context_check(context, max_priority, poll_fds, n_poll_fds)) {
    g_main_context_dispatch(context);
}

Or even just call g_main_context_iteration(). What do you think?

Regards,

-- 
Fabien Chouteau

Re: [Qemu-devel] [PATCH 2/2] main: switch qemu_set_fd_handler to g_io_add_watch

[Paolo Bonzini]

On 11/24/2011 06:11 PM, Fabien Chouteau wrote:
> Hello,
>
> I've run into some problems with this patch on Windows. The thing is
> that select() should be used only with socket file descriptors.
>
> If glib_select_fill() put a non-socket file descriptor in rfds or wfds,
> select() will fail with this error (btw the return value of select is
> not checked):

This patch actually has been reverted in commit be08e65.  Is it the 
revert that is causing problems?  If so, what is it that 
glib_select_fill puts in rfds and wfds?

> I've look at the patch and I don't see why do you pick file descriptors
> from g_main_context_query's "poll_fds" to put them in the fd_sets (rfds,
> wfds...) and then re-build a "poll_fds" to call g_main_context_check and
> g_main_context_dispatch. From my understanding we can just do:
>
> g_main_context_prepare(context,&max_priority);
>
> n_poll_fds = g_main_context_query(context, max_priority,&timeout,
>                                        poll_fds, ARRAY_SIZE(poll_fds));
>
> if (g_main_context_check(context, max_priority, poll_fds, n_poll_fds)) {
>      g_main_context_dispatch(context);
> }
>
> Or even just call g_main_context_iteration(). What do you think?

You would have to call it in nonblocking mode from a polling handler 
(qemu_add_polling_cb).

A better solution is to move the whole main loop polling into 
os_host_main_loop_wait.

For POSIX, it would be just a call to 
glib_select_fill+select+glib_select_poll.  (Everything around these 
three would stay in the caller, and the fd_sets would be passed to 
os_host_main_loop_wait).

For Windows, it would work like this (and would not use glib_select_* at 
all):

1) call the polling handlers;

2) call select with timeout zero.  If no socket is ready, call 
WSAEventSelect on the sockets listed in the fd_sets;

3) call g_main_context_prepare+query.

4) add the event from (2) and the registered wait objects to the 
poll_fds.  Call g_poll on it.  If sockets were ready, force 0 timeout.

5) If no sockets were ready, call again select with timeout zero.

6) Check the output of g_poll and dispatch the wait objects that are now 
ready.

7) Call g_main_context_check+dispatch.

Paolo

Re: [Qemu-devel] [PATCH 2/2] main: switch qemu_set_fd_handler to g_io_add_watch

[Fabien Chouteau]

On 24/11/2011 18:30, Paolo Bonzini wrote:
> On 11/24/2011 06:11 PM, Fabien Chouteau wrote:
>> Hello,
>>
>> I've run into some problems with this patch on Windows. The thing is
>> that select() should be used only with socket file descriptors.
>>
>> If glib_select_fill() put a non-socket file descriptor in rfds or wfds,
>> select() will fail with this error (btw the return value of select is
>> not checked):
> 
> This patch actually has been reverted in commit be08e65.  Is it the revert that is causing problems?  If so, what is it that glib_select_fill puts in rfds and wfds?
> 

OK my bad, I should have checked on the upstream repository.
But I can see that glib_select_fill/poll() are still there.

>> I've look at the patch and I don't see why do you pick file descriptors
>> from g_main_context_query's "poll_fds" to put them in the fd_sets (rfds,
>> wfds...) and then re-build a "poll_fds" to call g_main_context_check and
>> g_main_context_dispatch. From my understanding we can just do:
>>
>> g_main_context_prepare(context,&max_priority);
>>
>> n_poll_fds = g_main_context_query(context, max_priority,&timeout,
>>                                        poll_fds, ARRAY_SIZE(poll_fds));
>>
>> if (g_main_context_check(context, max_priority, poll_fds, n_poll_fds)) {
>>      g_main_context_dispatch(context);
>> }
>>
>> Or even just call g_main_context_iteration(). What do you think?
> 
> You would have to call it in nonblocking mode from a polling handler (qemu_add_polling_cb).
> 
> A better solution is to move the whole main loop polling into os_host_main_loop_wait.
> 
> For POSIX, it would be just a call to glib_select_fill+select+glib_select_poll.  (Everything around these three would stay in the caller, and the fd_sets would be passed to os_host_main_loop_wait).

Are you sure we have to use select()? I would expect Glib to help us
avoid this kind of os-dependent syscalls.

> 
> For Windows, it would work like this (and would not use glib_select_* at all):
> 
> 1) call the polling handlers;
> 
> 2) call select with timeout zero.  If no socket is ready, call WSAEventSelect on the sockets listed in the fd_sets;
> 
> 3) call g_main_context_prepare+query.
> 
> 4) add the event from (2) and the registered wait objects to the poll_fds.  Call g_poll on it.  If sockets were ready, force 0 timeout.
> 
> 5) If no sockets were ready, call again select with timeout zero.
> 
> 6) Check the output of g_poll and dispatch the wait objects that are now ready.
> 
> 7) Call g_main_context_check+dispatch.


Again, Glib should help us skip all these complicated os-dependent stuff.

Maybe it's already the plan and I don't want to beat a dead horse, but I
think the best way is to get rid of file descriptors and sockets to use
GIOChannel all the way. Not only for event loop, but also for reads and
writes.

Regards,

-- 
Fabien Chouteau

Re: [Qemu-devel] [PATCH 2/2] main: switch qemu_set_fd_handler to g_io_add_watch

[Paolo Bonzini]

On 11/25/2011 11:24 AM, Fabien Chouteau wrote:
>> For POSIX, it would be just a call to
>> glib_select_fill+select+glib_select_poll.  (Everything around
>> these three would stay in the caller, and the fd_sets would be
>> passed to os_host_main_loop_wait).
>
> Are you sure we have to use select()?

slirp is fd_set---thus select()---based.  iohandler too, though it would 
likely be simpler to switch it to poll().

> I would expect Glib to help us
> avoid this kind of os-dependent syscalls.

Long term, yes.  However, even with the iothread and other recent 
refactorings, the QEMU event loop is still in control of everything 
including glib sources.  This is not a problem; the glib event loop is 
designed to be integrated into other event loops.

> Again, Glib should help us skip all these complicated os-dependent
> stuff.

To do this, you need to reimplement the various components of the QEMU 
main loop as GSources.  I did it for eventfd (including bottom halves), 
timerfd and signalfd, but really it was only to learn GSources rather 
than as something planned for inclusion.

What's missing is iohandlers and qemu_aio_wait/flush are needed too. 
Either this, or you would need to touch all uses of iohandlers: 
character devices, the non-raw block protocols (nbd, curl, iscsi, etc.), 
slirp, and migration.

If you do not want to do this all at once, the first step is to fix the 
glib main loop for Windows and move things over slowly.

Paolo

Re: [Qemu-devel] [PATCH 2/2] main: switch qemu_set_fd_handler to g_io_add_watch

[Fabien Chouteau]

On 25/11/2011 11:46, Paolo Bonzini wrote:
> On 11/25/2011 11:24 AM, Fabien Chouteau wrote:
>>> For POSIX, it would be just a call to
>>> glib_select_fill+select+glib_select_poll.  (Everything around
>>> these three would stay in the caller, and the fd_sets would be
>>> passed to os_host_main_loop_wait).
>>
>> Are you sure we have to use select()?
> 
> slirp is fd_set---thus select()---based.  iohandler too, though it would likely be simpler to switch it to poll().

Right, for slirp and iohandler, but it seems wrong to take file
descriptors from g_main_context_query() and put them in the fd_sets for
select(). This part is still in the code today.

-- 
Fabien Chouteau

Re: [Qemu-devel] [PATCH 2/2] main: switch qemu_set_fd_handler to g_io_add_watch

[Paolo Bonzini]

On 11/25/2011 03:46 PM, Fabien Chouteau wrote:
>> >  slirp is fd_set---thus select()---based.  iohandler too, though it would likely be simpler to switch it to poll().
> Right, for slirp and iohandler, but it seems wrong to take file
> descriptors from g_main_context_query() and put them in the fd_sets for
> select(). This part is still in the code today.

It's ugly, but it works.  There's a fundamental impedence mismatch 
between glib and slirp/iohandler.  Either you convert glib's pollfds to 
fd_sets, or you take slirp and iohandler's fd_sets and put them in 
pollfds.  Converting slirp and iohandler to produce pollfds is not easy 
because Windows does not have poll---so you'd still have a 
pollfd-to-fd_set conversion somewhere.  Believe me, I thought this 
through. :)

Paolo

Re: [Qemu-devel] [PATCH 2/2] main: switch qemu_set_fd_handler to g_io_add_watch

[Fabien Chouteau]

On 25/11/2011 15:49, Paolo Bonzini wrote:
> On 11/25/2011 03:46 PM, Fabien Chouteau wrote:
>>> >  slirp is fd_set---thus select()---based.  iohandler too, though it would likely be simpler to switch it to poll().
>> Right, for slirp and iohandler, but it seems wrong to take file
>> descriptors from g_main_context_query() and put them in the fd_sets for
>> select(). This part is still in the code today.
> 
> It's ugly, but it works.

For Windows I'm not sure it will work.

> There's a fundamental impedence mismatch between glib and
> slirp/iohandler.  Either you convert glib's pollfds to fd_sets, or you
> take slirp and iohandler's fd_sets and put them in pollfds.
> Converting slirp and iohandler to produce pollfds is not easy because
> Windows does not have poll---so you'd still have a pollfd-to-fd_set
> conversion somewhere.

Is it possible to use both? Keep the select scheme for iohandlers and
slirp, but use g_main_context_iteration() for Glib stuff.

> Believe me, I thought this through. :)
>

I know, I just try to understand ;)

-- 
Fabien Chouteau

Re: [Qemu-devel] [PATCH 2/2] main: switch qemu_set_fd_handler to g_io_add_watch

[Paolo Bonzini]

> > > >  slirp is fd_set---thus select()---based.  iohandler too,
> > > >  though it would likely be simpler to switch it to poll().
> > > Right, for slirp and iohandler, but it seems wrong to take file
> > > descriptors from g_main_context_query() and put them in the
> > > fd_sets for
> >> select(). This part is still in the code today.
> > 
> > It's ugly, but it works.
> 
> For Windows I'm not sure it will work.

No, it doesn't (see my other message).

> > There's a fundamental impedence mismatch between glib and
> > slirp/iohandler.  Either you convert glib's pollfds to fd_sets, or
> > you
> > take slirp and iohandler's fd_sets and put them in pollfds.
> > Converting slirp and iohandler to produce pollfds is not easy
> > because
> > Windows does not have poll---so you'd still have a pollfd-to-fd_set
> > conversion somewhere.
> 
> Is it possible to use both? Keep the select scheme for iohandlers and
> slirp, but use g_main_context_iteration() for Glib stuff.

Perhaps with two threads, but I think it's more complicated than
merging the handle/fd sets and doing a single poll.

For Windows you already have three/four separate polls and
unifying some of them would improve responsiveness.

Paolo

Re: [Qemu-devel] [PATCH 2/2] main: switch qemu_set_fd_handler to g_io_add_watch

[Fabien Chouteau]

On 25/11/2011 16:48, Paolo Bonzini wrote:
>>> There's a fundamental impedence mismatch between glib and
>>> slirp/iohandler.  Either you convert glib's pollfds to fd_sets, or
>>> you
>>> take slirp and iohandler's fd_sets and put them in pollfds.
>>> Converting slirp and iohandler to produce pollfds is not easy
>>> because
>>> Windows does not have poll---so you'd still have a pollfd-to-fd_set
>>> conversion somewhere.
>>
>> Is it possible to use both? Keep the select scheme for iohandlers and
>> slirp, but use g_main_context_iteration() for Glib stuff.
> 
> Perhaps with two threads, but I think it's more complicated than
> merging the handle/fd sets and doing a single poll.

Why two threads?

-- 
Fabien Chouteau

Re: [Qemu-devel] [PATCH 2/2] main: switch qemu_set_fd_handler to g_io_add_watch

[Paolo Bonzini]

On 11/25/2011 05:56 PM, Fabien Chouteau wrote:
>>> >>  Is it possible to use both? Keep the select scheme for iohandlers and
>>> >>  slirp, but use g_main_context_iteration() for Glib stuff.
>> >
>> >  Perhaps with two threads, but I think it's more complicated than
>> >  merging the handle/fd sets and doing a single poll.
> Why two threads?

Because you have two disjoint sets of file descriptors (iohandler+slirp 
and glib), both of which have to be waited on for a possibly infinite 
file.  You cannot do that at the same time without two threads (unless 
you alternatively poll one and the other).

Paolo

Re: [Qemu-devel] [PATCH 2/2] main: switch qemu_set_fd_handler to g_io_add_watch

[Fabien Chouteau]

On 25/11/2011 20:36, Paolo Bonzini wrote:
> On 11/25/2011 05:56 PM, Fabien Chouteau wrote:
>>>> >>  Is it possible to use both? Keep the select scheme for iohandlers and
>>>> >>  slirp, but use g_main_context_iteration() for Glib stuff.
>>> >
>>> >  Perhaps with two threads, but I think it's more complicated than
>>> >  merging the handle/fd sets and doing a single poll.
>> Why two threads?
> 
> Because you have two disjoint sets of file descriptors (iohandler+slirp and glib), both of which have to be waited on for a possibly infinite file.  You cannot do that at the same time without two threads (unless you alternatively poll one and the other).
> 

Right, thank you for all these explanations.

Regards,

-- 
Fabien Chouteau