| Message ID | 1314018774-27482-2-git-send-email-aliguori@us.ibm.com |
|---|---|
| State | New |
| Headers | show |
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
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 >
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
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)
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) > >
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.
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
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
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
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
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 >
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.
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
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 >
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
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,
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
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,
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
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.
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
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 ;)
> > > > 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
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?
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
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,
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)
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(-)