| Message ID | 20180827222322.26009-4-marcandre.lureau@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | chardev: various cleanups and improvements | expand |
On Tue, Aug 28, 2018 at 12:23:16AM +0200, Marc-André Lureau wrote: > GLib child source were added with version 2.28. We can use them now > that we bumped our requirement to 2.40. > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > --- > chardev/char-io.c | 48 +++++------------------------------------------ > 1 file changed, 5 insertions(+), 43 deletions(-) Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Regards, Daniel
Hi guys, We have some random delays with gdbstub since this commit under Windows. Basically while debugging the next command can sometimes take more than 5 seconds while it usually tooks 30ms before. It is not target dependent I see that happening with sparc, ppc, riscv, etc.. but only on Windows host. Does that ring a bell? Cheers, Fred Le 8/28/18 à 12:23 AM, Marc-André Lureau a écrit : > GLib child source were added with version 2.28. We can use them now > that we bumped our requirement to 2.40. > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > --- > chardev/char-io.c | 48 +++++------------------------------------------ > 1 file changed, 5 insertions(+), 43 deletions(-) > > diff --git a/chardev/char-io.c b/chardev/char-io.c > index f81052481a..8ced184160 100644 > --- a/chardev/char-io.c > +++ b/chardev/char-io.c > @@ -33,7 +33,6 @@ typedef struct IOWatchPoll { > IOCanReadHandler *fd_can_read; > GSourceFunc fd_read; > void *opaque; > - GMainContext *context; > } IOWatchPoll; > > static IOWatchPoll *io_watch_poll_from_source(GSource *source) > @@ -55,47 +54,24 @@ static gboolean io_watch_poll_prepare(GSource *source, > iwp->src = qio_channel_create_watch( > iwp->ioc, G_IO_IN | G_IO_ERR | G_IO_HUP | G_IO_NVAL); > g_source_set_callback(iwp->src, iwp->fd_read, iwp->opaque, NULL); > - g_source_attach(iwp->src, iwp->context); > - } else { > - g_source_destroy(iwp->src); > + g_source_add_child_source(source, iwp->src); > g_source_unref(iwp->src); > + } else { > + g_source_remove_child_source(source, iwp->src); > iwp->src = NULL; > } > return FALSE; > } > > -static gboolean io_watch_poll_check(GSource *source) > -{ > - return FALSE; > -} > - > static gboolean io_watch_poll_dispatch(GSource *source, GSourceFunc callback, > gpointer user_data) > { > - abort(); > -} > - > -static void io_watch_poll_finalize(GSource *source) > -{ > - /* Due to a glib bug, removing the last reference to a source > - * inside a finalize callback causes recursive locking (and a > - * deadlock). This is not a problem inside other callbacks, > - * including dispatch callbacks, so we call io_remove_watch_poll > - * to remove this source. At this point, iwp->src must > - * be NULL, or we would leak it. > - * > - * This would be solved much more elegantly by child sources, > - * but we support older glib versions that do not have them. > - */ > - IOWatchPoll *iwp = io_watch_poll_from_source(source); > - assert(iwp->src == NULL); > + return G_SOURCE_CONTINUE; > } > > static GSourceFuncs io_watch_poll_funcs = { > .prepare = io_watch_poll_prepare, > - .check = io_watch_poll_check, > .dispatch = io_watch_poll_dispatch, > - .finalize = io_watch_poll_finalize, > }; > > GSource *io_add_watch_poll(Chardev *chr, > @@ -115,7 +91,6 @@ GSource *io_add_watch_poll(Chardev *chr, > iwp->ioc = ioc; > iwp->fd_read = (GSourceFunc) fd_read; > iwp->src = NULL; > - iwp->context = context; > > name = g_strdup_printf("chardev-iowatch-%s", chr->label); > g_source_set_name((GSource *)iwp, name); > @@ -126,23 +101,10 @@ GSource *io_add_watch_poll(Chardev *chr, > return (GSource *)iwp; > } > > -static void io_remove_watch_poll(GSource *source) > -{ > - IOWatchPoll *iwp; > - > - iwp = io_watch_poll_from_source(source); > - if (iwp->src) { > - g_source_destroy(iwp->src); > - g_source_unref(iwp->src); > - iwp->src = NULL; > - } > - g_source_destroy(&iwp->parent); > -} > - > void remove_fd_in_watch(Chardev *chr) > { > if (chr->gsource) { > - io_remove_watch_poll(chr->gsource); > + g_source_destroy(chr->gsource); > chr->gsource = NULL; > } > } >
Hi On Thu, Apr 4, 2019 at 5:18 PM KONRAD Frederic <frederic.konrad@adacore.com> wrote: > > Hi guys, > > We have some random delays with gdbstub since this commit under Windows. > Basically while debugging the next command can sometimes take more than 5 > seconds while it usually tooks 30ms before. It is not target dependent I see > that happening with sparc, ppc, riscv, etc.. but only on Windows host. > > Does that ring a bell? No, is this with a socket chardev or other kind? Could you check the chardev behaviour with the HMP monitor for ex: qemu -chardev socket,id=foo,server,host=localhost,port=9999 -monitor chardev:foo Is this also taking a few seconds to reply? Have you checked running tests/test-char on windows? Is this also taking long (>2sec)? > > Cheers, > Fred > > Le 8/28/18 à 12:23 AM, Marc-André Lureau a écrit : > > GLib child source were added with version 2.28. We can use them now > > that we bumped our requirement to 2.40. > > > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > > --- > > chardev/char-io.c | 48 +++++------------------------------------------ > > 1 file changed, 5 insertions(+), 43 deletions(-) > > > > diff --git a/chardev/char-io.c b/chardev/char-io.c > > index f81052481a..8ced184160 100644 > > --- a/chardev/char-io.c > > +++ b/chardev/char-io.c > > @@ -33,7 +33,6 @@ typedef struct IOWatchPoll { > > IOCanReadHandler *fd_can_read; > > GSourceFunc fd_read; > > void *opaque; > > - GMainContext *context; > > } IOWatchPoll; > > > > static IOWatchPoll *io_watch_poll_from_source(GSource *source) > > @@ -55,47 +54,24 @@ static gboolean io_watch_poll_prepare(GSource *source, > > iwp->src = qio_channel_create_watch( > > iwp->ioc, G_IO_IN | G_IO_ERR | G_IO_HUP | G_IO_NVAL); > > g_source_set_callback(iwp->src, iwp->fd_read, iwp->opaque, NULL); > > - g_source_attach(iwp->src, iwp->context); > > - } else { > > - g_source_destroy(iwp->src); > > + g_source_add_child_source(source, iwp->src); > > g_source_unref(iwp->src); > > + } else { > > + g_source_remove_child_source(source, iwp->src); > > iwp->src = NULL; > > } > > return FALSE; > > } > > > > -static gboolean io_watch_poll_check(GSource *source) > > -{ > > - return FALSE; > > -} > > - > > static gboolean io_watch_poll_dispatch(GSource *source, GSourceFunc callback, > > gpointer user_data) > > { > > - abort(); > > -} > > - > > -static void io_watch_poll_finalize(GSource *source) > > -{ > > - /* Due to a glib bug, removing the last reference to a source > > - * inside a finalize callback causes recursive locking (and a > > - * deadlock). This is not a problem inside other callbacks, > > - * including dispatch callbacks, so we call io_remove_watch_poll > > - * to remove this source. At this point, iwp->src must > > - * be NULL, or we would leak it. > > - * > > - * This would be solved much more elegantly by child sources, > > - * but we support older glib versions that do not have them. > > - */ > > - IOWatchPoll *iwp = io_watch_poll_from_source(source); > > - assert(iwp->src == NULL); > > + return G_SOURCE_CONTINUE; > > } > > > > static GSourceFuncs io_watch_poll_funcs = { > > .prepare = io_watch_poll_prepare, > > - .check = io_watch_poll_check, > > .dispatch = io_watch_poll_dispatch, > > - .finalize = io_watch_poll_finalize, > > }; > > > > GSource *io_add_watch_poll(Chardev *chr, > > @@ -115,7 +91,6 @@ GSource *io_add_watch_poll(Chardev *chr, > > iwp->ioc = ioc; > > iwp->fd_read = (GSourceFunc) fd_read; > > iwp->src = NULL; > > - iwp->context = context; > > > > name = g_strdup_printf("chardev-iowatch-%s", chr->label); > > g_source_set_name((GSource *)iwp, name); > > @@ -126,23 +101,10 @@ GSource *io_add_watch_poll(Chardev *chr, > > return (GSource *)iwp; > > } > > > > -static void io_remove_watch_poll(GSource *source) > > -{ > > - IOWatchPoll *iwp; > > - > > - iwp = io_watch_poll_from_source(source); > > - if (iwp->src) { > > - g_source_destroy(iwp->src); > > - g_source_unref(iwp->src); > > - iwp->src = NULL; > > - } > > - g_source_destroy(&iwp->parent); > > -} > > - > > void remove_fd_in_watch(Chardev *chr) > > { > > if (chr->gsource) { > > - io_remove_watch_poll(chr->gsource); > > + g_source_destroy(chr->gsource); > > chr->gsource = NULL; > > } > > } > >
Le 4/4/19 à 5:44 PM, Marc-André Lureau a écrit : > Hi > > On Thu, Apr 4, 2019 at 5:18 PM KONRAD Frederic > <frederic.konrad@adacore.com> wrote: >> >> Hi guys, >> >> We have some random delays with gdbstub since this commit under Windows. >> Basically while debugging the next command can sometimes take more than 5 >> seconds while it usually tooks 30ms before. It is not target dependent I see >> that happening with sparc, ppc, riscv, etc.. but only on Windows host. >> >> Does that ring a bell? > > No, is this with a socket chardev or other kind? It's connecting through the TCP socket. '-s -S' option. > > Could you check the chardev behaviour with the HMP monitor for ex: > qemu -chardev socket,id=foo,server,host=localhost,port=9999 -monitor chardev:foo > > Is this also taking a few seconds to reply? > > Have you checked running tests/test-char on windows? Is this also > taking long (>2sec)? I'll try to test that. Thanks! Fred > >> >> Cheers, >> Fred >> >> Le 8/28/18 à 12:23 AM, Marc-André Lureau a écrit : >>> GLib child source were added with version 2.28. We can use them now >>> that we bumped our requirement to 2.40. >>> >>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> >>> --- >>> chardev/char-io.c | 48 +++++------------------------------------------ >>> 1 file changed, 5 insertions(+), 43 deletions(-) >>> >>> diff --git a/chardev/char-io.c b/chardev/char-io.c >>> index f81052481a..8ced184160 100644 >>> --- a/chardev/char-io.c >>> +++ b/chardev/char-io.c >>> @@ -33,7 +33,6 @@ typedef struct IOWatchPoll { >>> IOCanReadHandler *fd_can_read; >>> GSourceFunc fd_read; >>> void *opaque; >>> - GMainContext *context; >>> } IOWatchPoll; >>> >>> static IOWatchPoll *io_watch_poll_from_source(GSource *source) >>> @@ -55,47 +54,24 @@ static gboolean io_watch_poll_prepare(GSource *source, >>> iwp->src = qio_channel_create_watch( >>> iwp->ioc, G_IO_IN | G_IO_ERR | G_IO_HUP | G_IO_NVAL); >>> g_source_set_callback(iwp->src, iwp->fd_read, iwp->opaque, NULL); >>> - g_source_attach(iwp->src, iwp->context); >>> - } else { >>> - g_source_destroy(iwp->src); >>> + g_source_add_child_source(source, iwp->src); >>> g_source_unref(iwp->src); >>> + } else { >>> + g_source_remove_child_source(source, iwp->src); >>> iwp->src = NULL; >>> } >>> return FALSE; >>> } >>> >>> -static gboolean io_watch_poll_check(GSource *source) >>> -{ >>> - return FALSE; >>> -} >>> - >>> static gboolean io_watch_poll_dispatch(GSource *source, GSourceFunc callback, >>> gpointer user_data) >>> { >>> - abort(); >>> -} >>> - >>> -static void io_watch_poll_finalize(GSource *source) >>> -{ >>> - /* Due to a glib bug, removing the last reference to a source >>> - * inside a finalize callback causes recursive locking (and a >>> - * deadlock). This is not a problem inside other callbacks, >>> - * including dispatch callbacks, so we call io_remove_watch_poll >>> - * to remove this source. At this point, iwp->src must >>> - * be NULL, or we would leak it. >>> - * >>> - * This would be solved much more elegantly by child sources, >>> - * but we support older glib versions that do not have them. >>> - */ >>> - IOWatchPoll *iwp = io_watch_poll_from_source(source); >>> - assert(iwp->src == NULL); >>> + return G_SOURCE_CONTINUE; >>> } >>> >>> static GSourceFuncs io_watch_poll_funcs = { >>> .prepare = io_watch_poll_prepare, >>> - .check = io_watch_poll_check, >>> .dispatch = io_watch_poll_dispatch, >>> - .finalize = io_watch_poll_finalize, >>> }; >>> >>> GSource *io_add_watch_poll(Chardev *chr, >>> @@ -115,7 +91,6 @@ GSource *io_add_watch_poll(Chardev *chr, >>> iwp->ioc = ioc; >>> iwp->fd_read = (GSourceFunc) fd_read; >>> iwp->src = NULL; >>> - iwp->context = context; >>> >>> name = g_strdup_printf("chardev-iowatch-%s", chr->label); >>> g_source_set_name((GSource *)iwp, name); >>> @@ -126,23 +101,10 @@ GSource *io_add_watch_poll(Chardev *chr, >>> return (GSource *)iwp; >>> } >>> >>> -static void io_remove_watch_poll(GSource *source) >>> -{ >>> - IOWatchPoll *iwp; >>> - >>> - iwp = io_watch_poll_from_source(source); >>> - if (iwp->src) { >>> - g_source_destroy(iwp->src); >>> - g_source_unref(iwp->src); >>> - iwp->src = NULL; >>> - } >>> - g_source_destroy(&iwp->parent); >>> -} >>> - >>> void remove_fd_in_watch(Chardev *chr) >>> { >>> if (chr->gsource) { >>> - io_remove_watch_poll(chr->gsource); >>> + g_source_destroy(chr->gsource); >>> chr->gsource = NULL; >>> } >>> } >>>
Hi Marc, I did more experiments.. It seems to be an issue in glib-2.44.1 which is quite old. I updated and it seems I don't see the issue anymore. Thanks for your input on this! Cheers, Fred Le 4/4/19 à 5:49 PM, KONRAD Frederic a écrit : > > > Le 4/4/19 à 5:44 PM, Marc-André Lureau a écrit : >> Hi >> >> On Thu, Apr 4, 2019 at 5:18 PM KONRAD Frederic >> <frederic.konrad@adacore.com> wrote: >>> >>> Hi guys, >>> >>> We have some random delays with gdbstub since this commit under Windows. >>> Basically while debugging the next command can sometimes take more than 5 >>> seconds while it usually tooks 30ms before. It is not target dependent I see >>> that happening with sparc, ppc, riscv, etc.. but only on Windows host. >>> >>> Does that ring a bell? >> >> No, is this with a socket chardev or other kind? > > It's connecting through the TCP socket. '-s -S' option. > >> >> Could you check the chardev behaviour with the HMP monitor for ex: >> qemu -chardev socket,id=foo,server,host=localhost,port=9999 -monitor chardev:foo >> >> Is this also taking a few seconds to reply? >> >> Have you checked running tests/test-char on windows? Is this also >> taking long (>2sec)? > > I'll try to test that. > > Thanks! > Fred >
diff --git a/chardev/char-io.c b/chardev/char-io.c index f81052481a..8ced184160 100644 --- a/chardev/char-io.c +++ b/chardev/char-io.c @@ -33,7 +33,6 @@ typedef struct IOWatchPoll { IOCanReadHandler *fd_can_read; GSourceFunc fd_read; void *opaque; - GMainContext *context; } IOWatchPoll; static IOWatchPoll *io_watch_poll_from_source(GSource *source) @@ -55,47 +54,24 @@ static gboolean io_watch_poll_prepare(GSource *source, iwp->src = qio_channel_create_watch( iwp->ioc, G_IO_IN | G_IO_ERR | G_IO_HUP | G_IO_NVAL); g_source_set_callback(iwp->src, iwp->fd_read, iwp->opaque, NULL); - g_source_attach(iwp->src, iwp->context); - } else { - g_source_destroy(iwp->src); + g_source_add_child_source(source, iwp->src); g_source_unref(iwp->src); + } else { + g_source_remove_child_source(source, iwp->src); iwp->src = NULL; } return FALSE; } -static gboolean io_watch_poll_check(GSource *source) -{ - return FALSE; -} - static gboolean io_watch_poll_dispatch(GSource *source, GSourceFunc callback, gpointer user_data) { - abort(); -} - -static void io_watch_poll_finalize(GSource *source) -{ - /* Due to a glib bug, removing the last reference to a source - * inside a finalize callback causes recursive locking (and a - * deadlock). This is not a problem inside other callbacks, - * including dispatch callbacks, so we call io_remove_watch_poll - * to remove this source. At this point, iwp->src must - * be NULL, or we would leak it. - * - * This would be solved much more elegantly by child sources, - * but we support older glib versions that do not have them. - */ - IOWatchPoll *iwp = io_watch_poll_from_source(source); - assert(iwp->src == NULL); + return G_SOURCE_CONTINUE; } static GSourceFuncs io_watch_poll_funcs = { .prepare = io_watch_poll_prepare, - .check = io_watch_poll_check, .dispatch = io_watch_poll_dispatch, - .finalize = io_watch_poll_finalize, }; GSource *io_add_watch_poll(Chardev *chr, @@ -115,7 +91,6 @@ GSource *io_add_watch_poll(Chardev *chr, iwp->ioc = ioc; iwp->fd_read = (GSourceFunc) fd_read; iwp->src = NULL; - iwp->context = context; name = g_strdup_printf("chardev-iowatch-%s", chr->label); g_source_set_name((GSource *)iwp, name); @@ -126,23 +101,10 @@ GSource *io_add_watch_poll(Chardev *chr, return (GSource *)iwp; } -static void io_remove_watch_poll(GSource *source) -{ - IOWatchPoll *iwp; - - iwp = io_watch_poll_from_source(source); - if (iwp->src) { - g_source_destroy(iwp->src); - g_source_unref(iwp->src); - iwp->src = NULL; - } - g_source_destroy(&iwp->parent); -} - void remove_fd_in_watch(Chardev *chr) { if (chr->gsource) { - io_remove_watch_poll(chr->gsource); + g_source_destroy(chr->gsource); chr->gsource = NULL; } }
GLib child source were added with version 2.28. We can use them now that we bumped our requirement to 2.40. Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> --- chardev/char-io.c | 48 +++++------------------------------------------ 1 file changed, 5 insertions(+), 43 deletions(-)