On Tue, Aug 15, 2017 at 03:28:15PM +0800, Wang yong wrote: > From: Wang Yong <wang.yong...@zte.com.cn> > > IOThread uses AioContext event loop and does not run a GMainContext. > Therefore,chardev cannot work in IOThread,such as the chardev is > used for colo-compare packets reception. > > This patch makes the IOThread run the GMainContext event loop, > chardev and IOThread can work together. > > Signed-off-by: Wang Yong<wang.yong...@zte.com.cn> > Signed-off-by: Wang Guang<wang.guan...@zte.com.cn>
There is usually a space between the name and email address. Tools and humans searching git log might not find you if you format the S-o-b line without a space. > --- > include/sysemu/iothread.h | 10 +++++++++ > iothread.c | 54 > +++++++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 64 insertions(+) > > diff --git a/include/sysemu/iothread.h b/include/sysemu/iothread.h > index e6da1a4..89e913c 100644 > --- a/include/sysemu/iothread.h > +++ b/include/sysemu/iothread.h > @@ -20,10 +20,19 @@ > #define TYPE_IOTHREAD "iothread" > > typedef struct { > + GMainContext *worker_context; > + GMainLoop *main_loop; > + GOnce once; > + > + QEMUBH *bh; > +} GMainOnce; Please do not use the glib namespace (G*, g_*) because it's easy to confuse GMainOnce with glib APIs like GOnce. I suggest inlining these fields in IOThread instead of defining a new struct. > + > +typedef struct { > Object parent_obj; > > QemuThread thread; > AioContext *ctx; > + GMainOnce thread_gonce; > QemuMutex init_done_lock; > QemuCond init_done_cond; /* is thread initialization done? */ > bool stopping; > @@ -41,5 +50,6 @@ typedef struct { > char *iothread_get_id(IOThread *iothread); > AioContext *iothread_get_aio_context(IOThread *iothread); > void iothread_stop_all(void); > +GMainContext *iothread_get_g_main_context(IOThread *iothread); > > #endif /* IOTHREAD_H */ > diff --git a/iothread.c b/iothread.c > index beeb870..9995eb0 100644 > --- a/iothread.c > +++ b/iothread.c > @@ -72,6 +72,9 @@ static int iothread_stop(Object *object, void *opaque) > return 0; > } > iothread->stopping = true; > + if (iothread->thread_gonce.main_loop) { > + g_main_loop_quit(iothread->thread_gonce.main_loop); > + } > aio_notify(iothread->ctx); > qemu_thread_join(&iothread->thread); > return 0; > @@ -125,6 +128,7 @@ static void iothread_complete(UserCreatable *obj, Error > **errp) > > qemu_mutex_init(&iothread->init_done_lock); > qemu_cond_init(&iothread->init_done_cond); > + iothread->thread_gonce.once = (GOnce) G_ONCE_INIT; > > /* This assumes we are called from a thread with useful CPU affinity for > us > * to inherit. > @@ -309,3 +313,53 @@ void iothread_stop_all(void) > > object_child_foreach(container, iothread_stop, NULL); > } > + > +static void iothread_g_main_context_bh(void *opaque) > +{ > + GMainOnce *g = opaque; > + > + qemu_bh_delete(g->bh); > + g->bh = NULL; > + > + g_main_context_push_thread_default(g->worker_context); > + > + g->main_loop = g_main_loop_new(g->worker_context, TRUE); > + g_main_loop_run(g->main_loop); Running the glib main loop from a BH function means deleted BHs can never be freed in aio-posix.c:aio_bh_poll(): if (qemu_lockcnt_dec_if_lock(&ctx->list_lock)) { bhp = &ctx->first_bh; while (*bhp) { bh = *bhp; if (bh->deleted && !bh->scheduled) { *bhp = bh->next; g_free(bh); } else { bhp = &bh->next; } } qemu_lockcnt_inc_and_unlock(&ctx->list_lock); } ctx->list_lock is held by iothread_run() -> aio_poll() so qemu_lockcnt_dec_if_lock() always returns false. Instead of running the glib event loop inside aio_poll() I would modify iothread_run(): while (!atomic_read(&iothread->stopping)) { if (atomic_read(&iothread->use_glib_event_loop)) { iothread_glib_event_loop(); break; } aio_poll(iothread->ctx, true); } > + > + g_main_loop_unref(g->main_loop); > + g->main_loop = NULL; > + > + g_main_context_pop_thread_default(g->worker_context); > + g_main_context_unref(g->worker_context); > + g->worker_context = NULL; > +} > + > +static gpointer iothread_g_main_context_init(gpointer g_data) > +{ > + AioContext *ctx; > + IOThread *iothread = (IOThread *)g_data; No cast is necessary from void * to another pointer type in C. Please remove the cast. > + GMainOnce *g = &iothread->thread_gonce; > + GSource *source; > + > + g->worker_context = g_main_context_new(); > + > + ctx = iothread_get_aio_context(iothread); > + source = aio_get_g_source(ctx); > + g_source_attach(source, g->worker_context); > + g_source_unref(source); > + > + g->bh = aio_bh_new(ctx, > + iothread_g_main_context_bh, g); > + qemu_bh_schedule(g->bh); > + > + return (gpointer) g->worker_context; No cast is necessary from a pointer type to void * in C. Please remove the cast. > +} > + > +GMainContext *iothread_get_g_main_context(IOThread *iothread) > +{ > + GMainOnce *g = &iothread->thread_gonce; > + > + g_once(&g->once, iothread_g_main_context_init, iothread); > + > + return (GMainContext *) g->once.retval; No cast is necessary from a pointer type to void * in C. Please remove the cast. A dangling pointer is returned if this this function is called after iothread_g_main_context_bh() has terminated. It would be safer to return g->worker_context instead of g->once.retval.
signature.asc
Description: PGP signature