| Message ID | 1365177573-11817-1-git-send-email-pbonzini@redhat.com |
|---|---|
| State | New |
| Headers | show |
On (Fri) 05 Apr 2013 [17:59:33], Paolo Bonzini wrote: > The character backend refactoring introduced an undesirable busy wait. > The busy wait happens if can_read returns zero and there is data available > on the character device's file descriptor. Then, the I/O watch will > fire continuously and, with TCG, the CPU thread will never run. > > 1) Char backend asks front end if it can write > 2) Front end says no > 3) poll() finds the char backend's descriptor is available > 4) Goto (1) > > What we really want is this (note that step 3 avoids the busy wait): > > 1) Char backend asks front end if it can write > 2) Front end says no > 3) poll() goes on without char backend's descriptor > 4) Goto (1) until qemu_chr_accept_input() called > > 5) Char backend asks front end if it can write > 6) Front end says yes > 7) poll() finds the char backend's descriptor is available > 8) Backend handler called > > After this patch, the IOWatchPoll source and the watch source are > separated. The IOWatchPoll is simply a hook that runs during the prepare > phase on each main loop iteration. The hook adds/removes the actual > source depending on the return value from can_read. > > A simple reproducer is > > qemu-system-i386 -serial mon:stdio > > ... followed by banging on the terminal as much as you can. :) Without > this patch, emulation will hang. > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > v1->v2: use g_source_get_context to find if the watch was active > static gboolean io_watch_poll_prepare(GSource *source, gint *timeout_) > { > IOWatchPoll *iwp = io_watch_poll_from_source(source); > - > - iwp->max_size = iwp->fd_can_read(iwp->opaque); > - if (iwp->max_size == 0) { > + bool now_active = iwp->fd_can_read(iwp->opaque) > 0; > + bool was_active = g_source_get_context(iwp->src) != NULL; This gives me a bunch of (process:30075): GLib-CRITICAL **: g_source_get_context: assertion `!SOURCE_DESTROYED (source)' failed messages Amit
Hi Paolo, On Sat, Apr 6, 2013 at 1:59 AM, Paolo Bonzini <pbonzini@redhat.com> wrote: > The character backend refactoring introduced an undesirable busy wait. > The busy wait happens if can_read returns zero and there is data available > on the character device's file descriptor. Then, the I/O watch will > fire continuously and, with TCG, the CPU thread will never run. > > 1) Char backend asks front end if it can write > 2) Front end says no > 3) poll() finds the char backend's descriptor is available > 4) Goto (1) > > What we really want is this (note that step 3 avoids the busy wait): > > 1) Char backend asks front end if it can write > 2) Front end says no > 3) poll() goes on without char backend's descriptor > 4) Goto (1) until qemu_chr_accept_input() called > > 5) Char backend asks front end if it can write > 6) Front end says yes > 7) poll() finds the char backend's descriptor is available > 8) Backend handler called > > After this patch, the IOWatchPoll source and the watch source are > separated. The IOWatchPoll is simply a hook that runs during the prepare > phase on each main loop iteration. The hook adds/removes the actual > source depending on the return value from can_read. > > A simple reproducer is > > qemu-system-i386 -serial mon:stdio > > ... followed by banging on the terminal as much as you can. :) Mouse wheel paste does the trick nicely for me :) > Without this patch, emulation will hang. > Emulation still hangs for me with this patch. It also plays foul with Anthonys rework of my patch. The only way I can get it to work is either mine or Anthonys patch and a revert of this one. I think your patch adversely affects the timeout logic that Anthonys patch relies on. Ping me if you want my replication instructions. I will have to send images as Microblaze doesn't like a null guest. Regards, Peter
Applied. Thanks. Regards, Anthony Liguori
diff --git a/qemu-char.c b/qemu-char.c index e5eb8dd..85ebcf9 100644 --- a/qemu-char.c +++ b/qemu-char.c @@ -594,65 +594,51 @@ int recv_all(int fd, void *_buf, int len1, bool single_read) typedef struct IOWatchPoll { + GSource parent; + GSource *src; - int max_size; IOCanReadHandler *fd_can_read; void *opaque; - - QTAILQ_ENTRY(IOWatchPoll) node; } IOWatchPoll; -static QTAILQ_HEAD(, IOWatchPoll) io_watch_poll_list = - QTAILQ_HEAD_INITIALIZER(io_watch_poll_list); - static IOWatchPoll *io_watch_poll_from_source(GSource *source) { - IOWatchPoll *i; - - QTAILQ_FOREACH(i, &io_watch_poll_list, node) { - if (i->src == source) { - return i; - } - } - - return NULL; + return container_of(source, IOWatchPoll, parent); } static gboolean io_watch_poll_prepare(GSource *source, gint *timeout_) { IOWatchPoll *iwp = io_watch_poll_from_source(source); - - iwp->max_size = iwp->fd_can_read(iwp->opaque); - if (iwp->max_size == 0) { + bool now_active = iwp->fd_can_read(iwp->opaque) > 0; + bool was_active = g_source_get_context(iwp->src) != NULL; + if (was_active == now_active) { return FALSE; } - return g_io_watch_funcs.prepare(source, timeout_); + if (now_active) { + g_source_attach(iwp->src, NULL); + } else { + g_source_remove(g_source_get_id(iwp->src)); + } + return FALSE; } static gboolean io_watch_poll_check(GSource *source) { - IOWatchPoll *iwp = io_watch_poll_from_source(source); - - if (iwp->max_size == 0) { - return FALSE; - } - - return g_io_watch_funcs.check(source); + return FALSE; } static gboolean io_watch_poll_dispatch(GSource *source, GSourceFunc callback, gpointer user_data) { - return g_io_watch_funcs.dispatch(source, callback, user_data); + abort(); } static void io_watch_poll_finalize(GSource *source) { IOWatchPoll *iwp = io_watch_poll_from_source(source); - QTAILQ_REMOVE(&io_watch_poll_list, iwp, node); - g_io_watch_funcs.finalize(source); + g_source_unref(iwp->src); } static GSourceFuncs io_watch_poll_funcs = { @@ -669,24 +655,14 @@ static guint io_add_watch_poll(GIOChannel *channel, gpointer user_data) { IOWatchPoll *iwp; - GSource *src; - guint tag; - - src = g_io_create_watch(channel, G_IO_IN | G_IO_ERR | G_IO_HUP); - g_source_set_funcs(src, &io_watch_poll_funcs); - g_source_set_callback(src, (GSourceFunc)fd_read, user_data, NULL); - tag = g_source_attach(src, NULL); - g_source_unref(src); - iwp = g_malloc0(sizeof(*iwp)); - iwp->src = src; - iwp->max_size = 0; + iwp = (IOWatchPoll *) g_source_new(&io_watch_poll_funcs, sizeof(IOWatchPoll)); iwp->fd_can_read = fd_can_read; iwp->opaque = user_data; + iwp->src = g_io_create_watch(channel, G_IO_IN | G_IO_ERR | G_IO_HUP); + g_source_set_callback(iwp->src, (GSourceFunc)fd_read, user_data, NULL); - QTAILQ_INSERT_HEAD(&io_watch_poll_list, iwp, node); - - return tag; + return g_source_attach(&iwp->parent, NULL); } #ifndef _WIN32
The character backend refactoring introduced an undesirable busy wait. The busy wait happens if can_read returns zero and there is data available on the character device's file descriptor. Then, the I/O watch will fire continuously and, with TCG, the CPU thread will never run. 1) Char backend asks front end if it can write 2) Front end says no 3) poll() finds the char backend's descriptor is available 4) Goto (1) What we really want is this (note that step 3 avoids the busy wait): 1) Char backend asks front end if it can write 2) Front end says no 3) poll() goes on without char backend's descriptor 4) Goto (1) until qemu_chr_accept_input() called 5) Char backend asks front end if it can write 6) Front end says yes 7) poll() finds the char backend's descriptor is available 8) Backend handler called After this patch, the IOWatchPoll source and the watch source are separated. The IOWatchPoll is simply a hook that runs during the prepare phase on each main loop iteration. The hook adds/removes the actual source depending on the return value from can_read. A simple reproducer is qemu-system-i386 -serial mon:stdio ... followed by banging on the terminal as much as you can. :) Without this patch, emulation will hang. Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- v1->v2: use g_source_get_context to find if the watch was active qemu-char.c | 62 +++++++++++++++++++------------------------------------------ 1 file changed, 19 insertions(+), 43 deletions(-)