* [Qemu-devel] [RFC PATCH V2] qemu-char: Fix context for g_source_attach() @ 2016-06-22 10:49 Zhang Chen 2016-06-29 6:46 ` Zhang Chen 2016-07-08 1:48 ` Fam Zheng 0 siblings, 2 replies; 11+ messages in thread From: Zhang Chen @ 2016-06-22 10:49 UTC (permalink / raw) To: qemu devel, Daniel P . Berrange, Paolo Bonzini Cc: Jason Wang, Zhang Chen, Li Zhijian, Wen Congyang, eddie . dong We want to poll and handle chardev in another thread other than main loop. But qemu_chr_add_handlers() can only work for global default context other than thread default context. So we use g_source_attach(xx, g_main_context_get_thread_default()) replace g_source_attach(xx, NULL) to attach g_source. Comments from jason. Signed-off-by: Zhang Chen <zhangchen.fnst@cn.fujitsu.com> Signed-off-by: Jason Wang <jasowang@redhat.com> --- io/channel.c | 2 +- qemu-char.c | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/io/channel.c b/io/channel.c index 692eb17..cd25677 100644 --- a/io/channel.c +++ b/io/channel.c @@ -146,7 +146,7 @@ guint qio_channel_add_watch(QIOChannel *ioc, g_source_set_callback(source, (GSourceFunc)func, user_data, notify); - id = g_source_attach(source, NULL); + id = g_source_attach(source, g_main_context_get_thread_default()); g_source_unref(source); return id; diff --git a/qemu-char.c b/qemu-char.c index 84f49ac..4340457 100644 --- a/qemu-char.c +++ b/qemu-char.c @@ -859,7 +859,7 @@ static gboolean io_watch_poll_prepare(GSource *source, gint *timeout_) 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, NULL); + g_source_attach(iwp->src, g_main_context_get_thread_default()); } else { g_source_destroy(iwp->src); g_source_unref(iwp->src); @@ -918,7 +918,7 @@ static guint io_add_watch_poll(QIOChannel *ioc, iwp->fd_read = (GSourceFunc) fd_read; iwp->src = NULL; - tag = g_source_attach(&iwp->parent, NULL); + tag = g_source_attach(&iwp->parent, g_main_context_get_thread_default()); g_source_unref(&iwp->parent); return tag; } @@ -3982,7 +3982,7 @@ int qemu_chr_fe_add_watch(CharDriverState *s, GIOCondition cond, } g_source_set_callback(src, (GSourceFunc)func, user_data, NULL); - tag = g_source_attach(src, NULL); + tag = g_source_attach(src, g_main_context_get_thread_default()); g_source_unref(src); return tag; -- 2.7.4 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [RFC PATCH V2] qemu-char: Fix context for g_source_attach() 2016-06-22 10:49 [Qemu-devel] [RFC PATCH V2] qemu-char: Fix context for g_source_attach() Zhang Chen @ 2016-06-29 6:46 ` Zhang Chen 2016-07-08 1:29 ` Zhang Chen 2016-07-08 1:48 ` Fam Zheng 1 sibling, 1 reply; 11+ messages in thread From: Zhang Chen @ 2016-06-29 6:46 UTC (permalink / raw) To: qemu devel, Daniel P . Berrange, Paolo Bonzini Cc: Jason Wang, Li Zhijian, Wen Congyang, eddie . dong On 06/22/2016 06:49 PM, Zhang Chen wrote: Hi~ Paolo~ Just a ping...no news for a week~ > We want to poll and handle chardev in another thread > other than main loop. But qemu_chr_add_handlers() can only > work for global default context other than thread default context. > So we use g_source_attach(xx, g_main_context_get_thread_default()) > replace g_source_attach(xx, NULL) to attach g_source. > Comments from jason. > > Signed-off-by: Zhang Chen <zhangchen.fnst@cn.fujitsu.com> > Signed-off-by: Jason Wang <jasowang@redhat.com> > --- > io/channel.c | 2 +- > qemu-char.c | 6 +++--- > 2 files changed, 4 insertions(+), 4 deletions(-) > > diff --git a/io/channel.c b/io/channel.c > index 692eb17..cd25677 100644 > --- a/io/channel.c > +++ b/io/channel.c > @@ -146,7 +146,7 @@ guint qio_channel_add_watch(QIOChannel *ioc, > > g_source_set_callback(source, (GSourceFunc)func, user_data, notify); > > - id = g_source_attach(source, NULL); > + id = g_source_attach(source, g_main_context_get_thread_default()); > g_source_unref(source); > > return id; > diff --git a/qemu-char.c b/qemu-char.c > index 84f49ac..4340457 100644 > --- a/qemu-char.c > +++ b/qemu-char.c > @@ -859,7 +859,7 @@ static gboolean io_watch_poll_prepare(GSource *source, gint *timeout_) > 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, NULL); > + g_source_attach(iwp->src, g_main_context_get_thread_default()); > } else { > g_source_destroy(iwp->src); > g_source_unref(iwp->src); > @@ -918,7 +918,7 @@ static guint io_add_watch_poll(QIOChannel *ioc, > iwp->fd_read = (GSourceFunc) fd_read; > iwp->src = NULL; > > - tag = g_source_attach(&iwp->parent, NULL); > + tag = g_source_attach(&iwp->parent, g_main_context_get_thread_default()); > g_source_unref(&iwp->parent); > return tag; > } > @@ -3982,7 +3982,7 @@ int qemu_chr_fe_add_watch(CharDriverState *s, GIOCondition cond, > } > > g_source_set_callback(src, (GSourceFunc)func, user_data, NULL); > - tag = g_source_attach(src, NULL); > + tag = g_source_attach(src, g_main_context_get_thread_default()); > g_source_unref(src); > > return tag; -- Thanks zhangchen ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [RFC PATCH V2] qemu-char: Fix context for g_source_attach() 2016-06-29 6:46 ` Zhang Chen @ 2016-07-08 1:29 ` Zhang Chen 0 siblings, 0 replies; 11+ messages in thread From: Zhang Chen @ 2016-07-08 1:29 UTC (permalink / raw) To: qemu devel, Daniel P . Berrange, Paolo Bonzini Cc: Jason Wang, eddie . dong, Li Zhijian, Fam Zheng On 06/29/2016 02:46 PM, Zhang Chen wrote: > > > On 06/22/2016 06:49 PM, Zhang Chen wrote: > > Hi~ Paolo~ > > Just a ping...no news for a week~ > > Ping...again... CC Fam This patch about main loop. Thanks Zhang Chen > >> We want to poll and handle chardev in another thread >> other than main loop. But qemu_chr_add_handlers() can only >> work for global default context other than thread default context. >> So we use g_source_attach(xx, g_main_context_get_thread_default()) >> replace g_source_attach(xx, NULL) to attach g_source. >> Comments from jason. >> >> Signed-off-by: Zhang Chen <zhangchen.fnst@cn.fujitsu.com> >> Signed-off-by: Jason Wang <jasowang@redhat.com> >> --- >> io/channel.c | 2 +- >> qemu-char.c | 6 +++--- >> 2 files changed, 4 insertions(+), 4 deletions(-) >> >> diff --git a/io/channel.c b/io/channel.c >> index 692eb17..cd25677 100644 >> --- a/io/channel.c >> +++ b/io/channel.c >> @@ -146,7 +146,7 @@ guint qio_channel_add_watch(QIOChannel *ioc, >> g_source_set_callback(source, (GSourceFunc)func, user_data, >> notify); >> - id = g_source_attach(source, NULL); >> + id = g_source_attach(source, g_main_context_get_thread_default()); >> g_source_unref(source); >> return id; >> diff --git a/qemu-char.c b/qemu-char.c >> index 84f49ac..4340457 100644 >> --- a/qemu-char.c >> +++ b/qemu-char.c >> @@ -859,7 +859,7 @@ static gboolean io_watch_poll_prepare(GSource >> *source, gint *timeout_) >> 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, NULL); >> + g_source_attach(iwp->src, g_main_context_get_thread_default()); >> } else { >> g_source_destroy(iwp->src); >> g_source_unref(iwp->src); >> @@ -918,7 +918,7 @@ static guint io_add_watch_poll(QIOChannel *ioc, >> iwp->fd_read = (GSourceFunc) fd_read; >> iwp->src = NULL; >> - tag = g_source_attach(&iwp->parent, NULL); >> + tag = g_source_attach(&iwp->parent, >> g_main_context_get_thread_default()); >> g_source_unref(&iwp->parent); >> return tag; >> } >> @@ -3982,7 +3982,7 @@ int qemu_chr_fe_add_watch(CharDriverState *s, >> GIOCondition cond, >> } >> g_source_set_callback(src, (GSourceFunc)func, user_data, NULL); >> - tag = g_source_attach(src, NULL); >> + tag = g_source_attach(src, g_main_context_get_thread_default()); >> g_source_unref(src); >> return tag; > -- Thanks zhangchen ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [RFC PATCH V2] qemu-char: Fix context for g_source_attach() 2016-06-22 10:49 [Qemu-devel] [RFC PATCH V2] qemu-char: Fix context for g_source_attach() Zhang Chen 2016-06-29 6:46 ` Zhang Chen @ 2016-07-08 1:48 ` Fam Zheng 2016-07-08 2:12 ` Zhang Chen 2016-07-08 8:54 ` Daniel P. Berrange 1 sibling, 2 replies; 11+ messages in thread From: Fam Zheng @ 2016-07-08 1:48 UTC (permalink / raw) To: Zhang Chen Cc: qemu devel, Daniel P . Berrange, Paolo Bonzini, eddie . dong, Jason Wang, Li Zhijian On Wed, 06/22 18:49, Zhang Chen wrote: > We want to poll and handle chardev in another thread > other than main loop. But qemu_chr_add_handlers() can only > work for global default context other than thread default context. > So we use g_source_attach(xx, g_main_context_get_thread_default()) > replace g_source_attach(xx, NULL) to attach g_source. > Comments from jason. > > Signed-off-by: Zhang Chen <zhangchen.fnst@cn.fujitsu.com> > Signed-off-by: Jason Wang <jasowang@redhat.com> > --- > io/channel.c | 2 +- > qemu-char.c | 6 +++--- > 2 files changed, 4 insertions(+), 4 deletions(-) > > diff --git a/io/channel.c b/io/channel.c > index 692eb17..cd25677 100644 > --- a/io/channel.c > +++ b/io/channel.c > @@ -146,7 +146,7 @@ guint qio_channel_add_watch(QIOChannel *ioc, > > g_source_set_callback(source, (GSourceFunc)func, user_data, notify); > > - id = g_source_attach(source, NULL); > + id = g_source_attach(source, g_main_context_get_thread_default()); > g_source_unref(source); > > return id; > diff --git a/qemu-char.c b/qemu-char.c > index 84f49ac..4340457 100644 > --- a/qemu-char.c > +++ b/qemu-char.c > @@ -859,7 +859,7 @@ static gboolean io_watch_poll_prepare(GSource *source, gint *timeout_) > 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, NULL); > + g_source_attach(iwp->src, g_main_context_get_thread_default()); > } else { > g_source_destroy(iwp->src); > g_source_unref(iwp->src); > @@ -918,7 +918,7 @@ static guint io_add_watch_poll(QIOChannel *ioc, > iwp->fd_read = (GSourceFunc) fd_read; > iwp->src = NULL; > > - tag = g_source_attach(&iwp->parent, NULL); > + tag = g_source_attach(&iwp->parent, g_main_context_get_thread_default()); > g_source_unref(&iwp->parent); > return tag; > } > @@ -3982,7 +3982,7 @@ int qemu_chr_fe_add_watch(CharDriverState *s, GIOCondition cond, > } > > g_source_set_callback(src, (GSourceFunc)func, user_data, NULL); > - tag = g_source_attach(src, NULL); > + tag = g_source_attach(src, g_main_context_get_thread_default()); > g_source_unref(src); > > return tag; > -- IIRC this opens a gate for your special thread (COLO compare thread?) to use QIOChannel. I think in the long run it is better to think about allowing integrating QIO to AioContext, to support its usage outside main loop. Given how opaque GSource is, I'm not sure how feasible that is, or how useful it will be. Anyway we should definitely hear more opinions from Daniel and Paolo. Fam ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [RFC PATCH V2] qemu-char: Fix context for g_source_attach() 2016-07-08 1:48 ` Fam Zheng @ 2016-07-08 2:12 ` Zhang Chen 2016-07-08 8:54 ` Daniel P. Berrange 1 sibling, 0 replies; 11+ messages in thread From: Zhang Chen @ 2016-07-08 2:12 UTC (permalink / raw) To: Fam Zheng Cc: qemu devel, Daniel P . Berrange, Paolo Bonzini, eddie . dong, Jason Wang, Li Zhijian On 07/08/2016 09:48 AM, Fam Zheng wrote: > On Wed, 06/22 18:49, Zhang Chen wrote: >> We want to poll and handle chardev in another thread >> other than main loop. But qemu_chr_add_handlers() can only >> work for global default context other than thread default context. >> So we use g_source_attach(xx, g_main_context_get_thread_default()) >> replace g_source_attach(xx, NULL) to attach g_source. >> Comments from jason. >> >> Signed-off-by: Zhang Chen <zhangchen.fnst@cn.fujitsu.com> >> Signed-off-by: Jason Wang <jasowang@redhat.com> >> --- >> io/channel.c | 2 +- >> qemu-char.c | 6 +++--- >> 2 files changed, 4 insertions(+), 4 deletions(-) >> >> diff --git a/io/channel.c b/io/channel.c >> index 692eb17..cd25677 100644 >> --- a/io/channel.c >> +++ b/io/channel.c >> @@ -146,7 +146,7 @@ guint qio_channel_add_watch(QIOChannel *ioc, >> >> g_source_set_callback(source, (GSourceFunc)func, user_data, notify); >> >> - id = g_source_attach(source, NULL); >> + id = g_source_attach(source, g_main_context_get_thread_default()); >> g_source_unref(source); >> >> return id; >> diff --git a/qemu-char.c b/qemu-char.c >> index 84f49ac..4340457 100644 >> --- a/qemu-char.c >> +++ b/qemu-char.c >> @@ -859,7 +859,7 @@ static gboolean io_watch_poll_prepare(GSource *source, gint *timeout_) >> 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, NULL); >> + g_source_attach(iwp->src, g_main_context_get_thread_default()); >> } else { >> g_source_destroy(iwp->src); >> g_source_unref(iwp->src); >> @@ -918,7 +918,7 @@ static guint io_add_watch_poll(QIOChannel *ioc, >> iwp->fd_read = (GSourceFunc) fd_read; >> iwp->src = NULL; >> >> - tag = g_source_attach(&iwp->parent, NULL); >> + tag = g_source_attach(&iwp->parent, g_main_context_get_thread_default()); >> g_source_unref(&iwp->parent); >> return tag; >> } >> @@ -3982,7 +3982,7 @@ int qemu_chr_fe_add_watch(CharDriverState *s, GIOCondition cond, >> } >> >> g_source_set_callback(src, (GSourceFunc)func, user_data, NULL); >> - tag = g_source_attach(src, NULL); >> + tag = g_source_attach(src, g_main_context_get_thread_default()); >> g_source_unref(src); >> >> return tag; >> -- > IIRC this opens a gate for your special thread (COLO compare thread?) to use > QIOChannel. Yes,you are right. > > I think in the long run it is better to think about allowing integrating QIO to > AioContext, to support its usage outside main loop. Given how opaque GSource > is, I'm not sure how feasible that is, or how useful it will be. Anyway we > should definitely hear more opinions from Daniel and Paolo. I have test it. COLO compare thread(include qemu_chr_add_handlers()) works good after apply this patch. We do this job in this way: In net/colo-compare.c static void *colo_compare_thread(void *opaque) { GMainContext *worker_context; GMainLoop *compare_loop; CompareState *s = opaque; worker_context = g_main_context_new(); g_assert(g_main_context_get_thread_default() == NULL); g_main_context_push_thread_default(worker_context); g_assert(g_main_context_get_thread_default() == worker_context); qemu_chr_add_handlers(s->chr_pri_in, compare_chr_can_read, compare_pri_chr_in, NULL, s); qemu_chr_add_handlers(s->chr_sec_in, compare_chr_can_read, compare_sec_chr_in, NULL, s); compare_loop = g_main_loop_new(worker_context, FALSE); g_main_loop_run(compare_loop); ...... } Thanks Zhang Chen > Fam > > > . > -- Thanks zhangchen ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [RFC PATCH V2] qemu-char: Fix context for g_source_attach() 2016-07-08 1:48 ` Fam Zheng 2016-07-08 2:12 ` Zhang Chen @ 2016-07-08 8:54 ` Daniel P. Berrange 2016-07-08 14:27 ` Paolo Bonzini 1 sibling, 1 reply; 11+ messages in thread From: Daniel P. Berrange @ 2016-07-08 8:54 UTC (permalink / raw) To: Fam Zheng Cc: Zhang Chen, qemu devel, Paolo Bonzini, eddie . dong, Jason Wang, Li Zhijian On Fri, Jul 08, 2016 at 09:48:23AM +0800, Fam Zheng wrote: > On Wed, 06/22 18:49, Zhang Chen wrote: > > We want to poll and handle chardev in another thread > > other than main loop. But qemu_chr_add_handlers() can only > > work for global default context other than thread default context. > > So we use g_source_attach(xx, g_main_context_get_thread_default()) > > replace g_source_attach(xx, NULL) to attach g_source. > > Comments from jason. > > > > Signed-off-by: Zhang Chen <zhangchen.fnst@cn.fujitsu.com> > > Signed-off-by: Jason Wang <jasowang@redhat.com> > > --- > > io/channel.c | 2 +- > > qemu-char.c | 6 +++--- > > 2 files changed, 4 insertions(+), 4 deletions(-) > > > > diff --git a/io/channel.c b/io/channel.c > > index 692eb17..cd25677 100644 > > --- a/io/channel.c > > +++ b/io/channel.c > > @@ -146,7 +146,7 @@ guint qio_channel_add_watch(QIOChannel *ioc, > > > > g_source_set_callback(source, (GSourceFunc)func, user_data, notify); > > > > - id = g_source_attach(source, NULL); > > + id = g_source_attach(source, g_main_context_get_thread_default()); > > g_source_unref(source); > > > > return id; > > diff --git a/qemu-char.c b/qemu-char.c > > index 84f49ac..4340457 100644 > > --- a/qemu-char.c > > +++ b/qemu-char.c > > @@ -859,7 +859,7 @@ static gboolean io_watch_poll_prepare(GSource *source, gint *timeout_) > > 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, NULL); > > + g_source_attach(iwp->src, g_main_context_get_thread_default()); > > } else { > > g_source_destroy(iwp->src); > > g_source_unref(iwp->src); > > @@ -918,7 +918,7 @@ static guint io_add_watch_poll(QIOChannel *ioc, > > iwp->fd_read = (GSourceFunc) fd_read; > > iwp->src = NULL; > > > > - tag = g_source_attach(&iwp->parent, NULL); > > + tag = g_source_attach(&iwp->parent, g_main_context_get_thread_default()); > > g_source_unref(&iwp->parent); > > return tag; > > } > > @@ -3982,7 +3982,7 @@ int qemu_chr_fe_add_watch(CharDriverState *s, GIOCondition cond, > > } > > > > g_source_set_callback(src, (GSourceFunc)func, user_data, NULL); > > - tag = g_source_attach(src, NULL); > > + tag = g_source_attach(src, g_main_context_get_thread_default()); > > g_source_unref(src); > > > > return tag; > > -- > > IIRC this opens a gate for your special thread (COLO compare thread?) to use > QIOChannel. I've no real objection to this proposed patch, though it is fairly pointless to take it now without seeing any following patch that actually makes use of this added feature. > I think in the long run it is better to think about allowing integrating QIO to > AioContext, to support its usage outside main loop. Given how opaque GSource > is, I'm not sure how feasible that is, or how useful it will be. Anyway we > should definitely hear more opinions from Daniel and Paolo. Personally I think it is preferable to stick as close to the standard GSource model as possible, as that's widely used & well understood API, compared to the QEMU specific AioContext. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :| ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [RFC PATCH V2] qemu-char: Fix context for g_source_attach() 2016-07-08 8:54 ` Daniel P. Berrange @ 2016-07-08 14:27 ` Paolo Bonzini 2016-07-11 2:12 ` Zhang Chen 0 siblings, 1 reply; 11+ messages in thread From: Paolo Bonzini @ 2016-07-08 14:27 UTC (permalink / raw) To: Daniel P. Berrange, Fam Zheng Cc: Zhang Chen, qemu devel, eddie . dong, Jason Wang, Li Zhijian On 08/07/2016 10:54, Daniel P. Berrange wrote: > On Fri, Jul 08, 2016 at 09:48:23AM +0800, Fam Zheng wrote: >> On Wed, 06/22 18:49, Zhang Chen wrote: >>> We want to poll and handle chardev in another thread >>> other than main loop. But qemu_chr_add_handlers() can only >>> work for global default context other than thread default context. >>> So we use g_source_attach(xx, g_main_context_get_thread_default()) >>> replace g_source_attach(xx, NULL) to attach g_source. >>> Comments from jason. >>> >>> Signed-off-by: Zhang Chen <zhangchen.fnst@cn.fujitsu.com> >>> Signed-off-by: Jason Wang <jasowang@redhat.com> >>> --- >>> io/channel.c | 2 +- >>> qemu-char.c | 6 +++--- >>> 2 files changed, 4 insertions(+), 4 deletions(-) >>> >>> diff --git a/io/channel.c b/io/channel.c >>> index 692eb17..cd25677 100644 >>> --- a/io/channel.c >>> +++ b/io/channel.c >>> @@ -146,7 +146,7 @@ guint qio_channel_add_watch(QIOChannel *ioc, >>> >>> g_source_set_callback(source, (GSourceFunc)func, user_data, notify); >>> >>> - id = g_source_attach(source, NULL); >>> + id = g_source_attach(source, g_main_context_get_thread_default()); >>> g_source_unref(source); >>> >>> return id; >>> diff --git a/qemu-char.c b/qemu-char.c >>> index 84f49ac..4340457 100644 >>> --- a/qemu-char.c >>> +++ b/qemu-char.c >>> @@ -859,7 +859,7 @@ static gboolean io_watch_poll_prepare(GSource *source, gint *timeout_) >>> 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, NULL); >>> + g_source_attach(iwp->src, g_main_context_get_thread_default()); >>> } else { >>> g_source_destroy(iwp->src); >>> g_source_unref(iwp->src); >>> @@ -918,7 +918,7 @@ static guint io_add_watch_poll(QIOChannel *ioc, >>> iwp->fd_read = (GSourceFunc) fd_read; >>> iwp->src = NULL; >>> >>> - tag = g_source_attach(&iwp->parent, NULL); >>> + tag = g_source_attach(&iwp->parent, g_main_context_get_thread_default()); >>> g_source_unref(&iwp->parent); >>> return tag; >>> } >>> @@ -3982,7 +3982,7 @@ int qemu_chr_fe_add_watch(CharDriverState *s, GIOCondition cond, >>> } >>> >>> g_source_set_callback(src, (GSourceFunc)func, user_data, NULL); >>> - tag = g_source_attach(src, NULL); >>> + tag = g_source_attach(src, g_main_context_get_thread_default()); >>> g_source_unref(src); >>> >>> return tag; >>> -- >> >> IIRC this opens a gate for your special thread (COLO compare thread?) to use >> QIOChannel. > > I've no real objection to this proposed patch, though it is fairly pointless > to take it now without seeing any following patch that actually makes use > of this added feature. I agree. >> I think in the long run it is better to think about allowing integrating QIO to >> AioContext, to support its usage outside main loop. Given how opaque GSource >> is, I'm not sure how feasible that is, or how useful it will be. Anyway we >> should definitely hear more opinions from Daniel and Paolo. > > Personally I think it is preferable to stick as close to the standard GSource > model as possible, as that's widely used & well understood API, compared to the > QEMU specific AioContext. AioContext is more optimized for the case where the callbacks are static. In general this is not the case for qemu-char.c. Paolo ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [RFC PATCH V2] qemu-char: Fix context for g_source_attach() 2016-07-08 14:27 ` Paolo Bonzini @ 2016-07-11 2:12 ` Zhang Chen 2016-07-18 1:55 ` Zhang Chen 0 siblings, 1 reply; 11+ messages in thread From: Zhang Chen @ 2016-07-11 2:12 UTC (permalink / raw) To: Paolo Bonzini, Daniel P. Berrange, Fam Zheng Cc: qemu devel, eddie . dong, Jason Wang, Li Zhijian On 07/08/2016 10:27 PM, Paolo Bonzini wrote: > > On 08/07/2016 10:54, Daniel P. Berrange wrote: >> On Fri, Jul 08, 2016 at 09:48:23AM +0800, Fam Zheng wrote: >>> On Wed, 06/22 18:49, Zhang Chen wrote: >>>> We want to poll and handle chardev in another thread >>>> other than main loop. But qemu_chr_add_handlers() can only >>>> work for global default context other than thread default context. >>>> So we use g_source_attach(xx, g_main_context_get_thread_default()) >>>> replace g_source_attach(xx, NULL) to attach g_source. >>>> Comments from jason. >>>> >>>> Signed-off-by: Zhang Chen <zhangchen.fnst@cn.fujitsu.com> >>>> Signed-off-by: Jason Wang <jasowang@redhat.com> >>>> --- >>>> io/channel.c | 2 +- >>>> qemu-char.c | 6 +++--- >>>> 2 files changed, 4 insertions(+), 4 deletions(-) >>>> >>>> diff --git a/io/channel.c b/io/channel.c >>>> index 692eb17..cd25677 100644 >>>> --- a/io/channel.c >>>> +++ b/io/channel.c >>>> @@ -146,7 +146,7 @@ guint qio_channel_add_watch(QIOChannel *ioc, >>>> >>>> g_source_set_callback(source, (GSourceFunc)func, user_data, notify); >>>> >>>> - id = g_source_attach(source, NULL); >>>> + id = g_source_attach(source, g_main_context_get_thread_default()); >>>> g_source_unref(source); >>>> >>>> return id; >>>> diff --git a/qemu-char.c b/qemu-char.c >>>> index 84f49ac..4340457 100644 >>>> --- a/qemu-char.c >>>> +++ b/qemu-char.c >>>> @@ -859,7 +859,7 @@ static gboolean io_watch_poll_prepare(GSource *source, gint *timeout_) >>>> 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, NULL); >>>> + g_source_attach(iwp->src, g_main_context_get_thread_default()); >>>> } else { >>>> g_source_destroy(iwp->src); >>>> g_source_unref(iwp->src); >>>> @@ -918,7 +918,7 @@ static guint io_add_watch_poll(QIOChannel *ioc, >>>> iwp->fd_read = (GSourceFunc) fd_read; >>>> iwp->src = NULL; >>>> >>>> - tag = g_source_attach(&iwp->parent, NULL); >>>> + tag = g_source_attach(&iwp->parent, g_main_context_get_thread_default()); >>>> g_source_unref(&iwp->parent); >>>> return tag; >>>> } >>>> @@ -3982,7 +3982,7 @@ int qemu_chr_fe_add_watch(CharDriverState *s, GIOCondition cond, >>>> } >>>> >>>> g_source_set_callback(src, (GSourceFunc)func, user_data, NULL); >>>> - tag = g_source_attach(src, NULL); >>>> + tag = g_source_attach(src, g_main_context_get_thread_default()); >>>> g_source_unref(src); >>>> >>>> return tag; >>>> -- >>> IIRC this opens a gate for your special thread (COLO compare thread?) to use >>> QIOChannel. >> I've no real objection to this proposed patch, though it is fairly pointless >> to take it now without seeing any following patch that actually makes use >> of this added feature. > I agree. Should I move this patch to the "[RFC PATCH V5 0/4] Introduce COLO-compare" patch set? that can show how it works. you can see this patch for how to use: http://lists.nongnu.org/archive/html/qemu-devel/2016-06/msg06754.html > >>> I think in the long run it is better to think about allowing integrating QIO to >>> AioContext, to support its usage outside main loop. Given how opaque GSource >>> is, I'm not sure how feasible that is, or how useful it will be. Anyway we >>> should definitely hear more opinions from Daniel and Paolo. >> Personally I think it is preferable to stick as close to the standard GSource >> model as possible, as that's widely used & well understood API, compared to the >> QEMU specific AioContext. > AioContext is more optimized for the case where the callbacks are > static. In general this is not the case for qemu-char.c. I don't sure AioContext can do this job good, but I think we can make qemu more flexible to do same one job. All roads lead to Rome. Thanks Zhang Chen > Paolo > > > . > -- Thanks zhangchen ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [RFC PATCH V2] qemu-char: Fix context for g_source_attach() 2016-07-11 2:12 ` Zhang Chen @ 2016-07-18 1:55 ` Zhang Chen 2016-07-18 5:31 ` Jason Wang 0 siblings, 1 reply; 11+ messages in thread From: Zhang Chen @ 2016-07-18 1:55 UTC (permalink / raw) To: Paolo Bonzini, Daniel P. Berrange, Fam Zheng Cc: Jason Wang, eddie . dong, qemu devel, Li Zhijian Hi~ All~~ Can you give me some feedback for this patch? We need more comments~~ COLO project depend on this patch to work. Because this patch colo-compare can make handler of qemu_chr_add_handlers() run in compare thread, reduce workload of main_loop in network busy situation. This idea from Jason. Thanks Zhang Chen On 07/11/2016 10:12 AM, Zhang Chen wrote: > > > On 07/08/2016 10:27 PM, Paolo Bonzini wrote: >> >> On 08/07/2016 10:54, Daniel P. Berrange wrote: >>> On Fri, Jul 08, 2016 at 09:48:23AM +0800, Fam Zheng wrote: >>>> On Wed, 06/22 18:49, Zhang Chen wrote: >>>>> We want to poll and handle chardev in another thread >>>>> other than main loop. But qemu_chr_add_handlers() can only >>>>> work for global default context other than thread default context. >>>>> So we use g_source_attach(xx, g_main_context_get_thread_default()) >>>>> replace g_source_attach(xx, NULL) to attach g_source. >>>>> Comments from jason. >>>>> >>>>> Signed-off-by: Zhang Chen <zhangchen.fnst@cn.fujitsu.com> >>>>> Signed-off-by: Jason Wang <jasowang@redhat.com> >>>>> --- >>>>> io/channel.c | 2 +- >>>>> qemu-char.c | 6 +++--- >>>>> 2 files changed, 4 insertions(+), 4 deletions(-) >>>>> >>>>> diff --git a/io/channel.c b/io/channel.c >>>>> index 692eb17..cd25677 100644 >>>>> --- a/io/channel.c >>>>> +++ b/io/channel.c >>>>> @@ -146,7 +146,7 @@ guint qio_channel_add_watch(QIOChannel *ioc, >>>>> g_source_set_callback(source, (GSourceFunc)func, >>>>> user_data, notify); >>>>> - id = g_source_attach(source, NULL); >>>>> + id = g_source_attach(source, >>>>> g_main_context_get_thread_default()); >>>>> g_source_unref(source); >>>>> return id; >>>>> diff --git a/qemu-char.c b/qemu-char.c >>>>> index 84f49ac..4340457 100644 >>>>> --- a/qemu-char.c >>>>> +++ b/qemu-char.c >>>>> @@ -859,7 +859,7 @@ static gboolean io_watch_poll_prepare(GSource >>>>> *source, gint *timeout_) >>>>> 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, NULL); >>>>> + g_source_attach(iwp->src, >>>>> g_main_context_get_thread_default()); >>>>> } else { >>>>> g_source_destroy(iwp->src); >>>>> g_source_unref(iwp->src); >>>>> @@ -918,7 +918,7 @@ static guint io_add_watch_poll(QIOChannel *ioc, >>>>> iwp->fd_read = (GSourceFunc) fd_read; >>>>> iwp->src = NULL; >>>>> - tag = g_source_attach(&iwp->parent, NULL); >>>>> + tag = g_source_attach(&iwp->parent, >>>>> g_main_context_get_thread_default()); >>>>> g_source_unref(&iwp->parent); >>>>> return tag; >>>>> } >>>>> @@ -3982,7 +3982,7 @@ int qemu_chr_fe_add_watch(CharDriverState >>>>> *s, GIOCondition cond, >>>>> } >>>>> g_source_set_callback(src, (GSourceFunc)func, user_data, >>>>> NULL); >>>>> - tag = g_source_attach(src, NULL); >>>>> + tag = g_source_attach(src, g_main_context_get_thread_default()); >>>>> g_source_unref(src); >>>>> return tag; >>>>> -- >>>> IIRC this opens a gate for your special thread (COLO compare >>>> thread?) to use >>>> QIOChannel. >>> I've no real objection to this proposed patch, though it is fairly >>> pointless >>> to take it now without seeing any following patch that actually >>> makes use >>> of this added feature. >> I agree. > > Should I move this patch to the "[RFC PATCH V5 0/4] Introduce > COLO-compare" > patch set? that can show how it works. > > you can see this patch for how to use: > http://lists.nongnu.org/archive/html/qemu-devel/2016-06/msg06754.html > > In colo_compare_thread() >> >>>> I think in the long run it is better to think about allowing >>>> integrating QIO to >>>> AioContext, to support its usage outside main loop. Given how >>>> opaque GSource >>>> is, I'm not sure how feasible that is, or how useful it will be. >>>> Anyway we >>>> should definitely hear more opinions from Daniel and Paolo. >>> Personally I think it is preferable to stick as close to the >>> standard GSource >>> model as possible, as that's widely used & well understood API, >>> compared to the >>> QEMU specific AioContext. >> AioContext is more optimized for the case where the callbacks are >> static. In general this is not the case for qemu-char.c. > > I don't sure AioContext can do this job good, but I think > we can make qemu more flexible to do same one job. > All roads lead to Rome. > > Thanks > Zhang Chen > > >> Paolo >> >> >> . >> > -- Thanks zhangchen ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [RFC PATCH V2] qemu-char: Fix context for g_source_attach() 2016-07-18 1:55 ` Zhang Chen @ 2016-07-18 5:31 ` Jason Wang 2016-07-18 5:43 ` Zhang Chen 0 siblings, 1 reply; 11+ messages in thread From: Jason Wang @ 2016-07-18 5:31 UTC (permalink / raw) To: Zhang Chen, Paolo Bonzini, Daniel P. Berrange, Fam Zheng Cc: eddie . dong, qemu devel, Li Zhijian On 2016年07月18日 09:55, Zhang Chen wrote: > Hi~ All~~ > > Can you give me some feedback for this patch? > > We need more comments~~ > > COLO project depend on this patch to work. > > Because this patch colo-compare can make handler of > qemu_chr_add_handlers() > > run in compare thread, reduce workload of main_loop in network busy > situation. > > This idea from Jason. > > > Thanks > Zhang Chen I think you can put this patch in the series of COLO comparing thread which shows its using. And then you can ask acked-by or reviewed-by from other maintainers. Thanks ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [RFC PATCH V2] qemu-char: Fix context for g_source_attach() 2016-07-18 5:31 ` Jason Wang @ 2016-07-18 5:43 ` Zhang Chen 0 siblings, 0 replies; 11+ messages in thread From: Zhang Chen @ 2016-07-18 5:43 UTC (permalink / raw) To: Jason Wang, Paolo Bonzini, Daniel P. Berrange, Fam Zheng Cc: eddie . dong, qemu devel, Li Zhijian On 07/18/2016 01:31 PM, Jason Wang wrote: > > > On 2016年07月18日 09:55, Zhang Chen wrote: >> Hi~ All~~ >> >> Can you give me some feedback for this patch? >> >> We need more comments~~ >> >> COLO project depend on this patch to work. >> >> Because this patch colo-compare can make handler of >> qemu_chr_add_handlers() >> >> run in compare thread, reduce workload of main_loop in network busy >> situation. >> >> This idea from Jason. >> >> >> Thanks >> Zhang Chen > > I think you can put this patch in the series of COLO comparing thread > which shows its using. And then you can ask acked-by or reviewed-by > from other maintainers. > > Thanks > > Make sense. I will add this patch in next colo-compare series. Thanks Zhang Chen > > -- Thanks zhangchen ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2016-07-18 5:42 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-06-22 10:49 [Qemu-devel] [RFC PATCH V2] qemu-char: Fix context for g_source_attach() Zhang Chen 2016-06-29 6:46 ` Zhang Chen 2016-07-08 1:29 ` Zhang Chen 2016-07-08 1:48 ` Fam Zheng 2016-07-08 2:12 ` Zhang Chen 2016-07-08 8:54 ` Daniel P. Berrange 2016-07-08 14:27 ` Paolo Bonzini 2016-07-11 2:12 ` Zhang Chen 2016-07-18 1:55 ` Zhang Chen 2016-07-18 5:31 ` Jason Wang 2016-07-18 5:43 ` Zhang Chen
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.