Message ID | 20190222031413.20250-3-peterx@redhat.com |
---|---|
State | New |
Headers | show |
Series | iothread: create gcontext unconditionally | expand |
Hi On Fri, Feb 22, 2019 at 4:14 AM Peter Xu <peterx@redhat.com> wrote: > > In existing code we create the gcontext dynamically at the first > access of the gcontext from caller. That can bring some complexity > and potential races during using iothread. Since the context itself > is not that big a resource, and we won't have millions of iothread, > let's simply create the gcontext unconditionally. > > This will also be a preparation work further to move the thread > context push operation earlier than before (now it's only pushed right > before we want to start running the gmainloop). > > Removing the g_once since it's not necessary, while introducing a new > run_gcontext boolean to show whether we want to run the gcontext. > > Signed-off-by: Peter Xu <peterx@redhat.com> > --- > include/sysemu/iothread.h | 2 +- > iothread.c | 43 +++++++++++++++++++-------------------- > 2 files changed, 22 insertions(+), 23 deletions(-) > > diff --git a/include/sysemu/iothread.h b/include/sysemu/iothread.h > index 50411ba54a..5f6240d5cb 100644 > --- a/include/sysemu/iothread.h > +++ b/include/sysemu/iothread.h > @@ -24,9 +24,9 @@ typedef struct { > > QemuThread thread; > AioContext *ctx; > + bool run_gcontext; /* whether we should run gcontext */ > GMainContext *worker_context; > GMainLoop *main_loop; > - GOnce once; > QemuSemaphore init_done_sem; /* is thread init done? */ > bool stopping; /* has iothread_stop() been called? */ > bool running; /* should iothread_run() continue? */ > diff --git a/iothread.c b/iothread.c > index 6e297e9ef1..6fa87876e0 100644 > --- a/iothread.c > +++ b/iothread.c > @@ -65,7 +65,7 @@ static void *iothread_run(void *opaque) > * We must check the running state again in case it was > * changed in previous aio_poll() > */ > - if (iothread->running && atomic_read(&iothread->worker_context)) { > + if (iothread->running && atomic_read(&iothread->run_gcontext)) { > GMainLoop *loop; > > g_main_context_push_thread_default(iothread->worker_context); > @@ -114,6 +114,8 @@ static void iothread_instance_init(Object *obj) > iothread->poll_max_ns = IOTHREAD_POLL_MAX_NS_DEFAULT; > iothread->thread_id = -1; > qemu_sem_init(&iothread->init_done_sem, 0); > + /* By default, we don't run gcontext */ > + atomic_set(&iothread->run_gcontext, 0); I think that initialization isn't really necessary, your call. > } > > static void iothread_instance_finalize(Object *obj) > @@ -143,6 +145,16 @@ static void iothread_instance_finalize(Object *obj) > qemu_sem_destroy(&iothread->init_done_sem); > } > > +static void iothread_init_gcontext(IOThread *iothread) > +{ > + GSource *source; > + > + iothread->worker_context = g_main_context_new(); > + source = aio_get_g_source(iothread_get_aio_context(iothread)); > + g_source_attach(source, iothread->worker_context); > + g_source_unref(source); > +} > + > static void iothread_complete(UserCreatable *obj, Error **errp) > { > Error *local_error = NULL; > @@ -157,6 +169,12 @@ static void iothread_complete(UserCreatable *obj, Error **errp) > return; > } > > + /* > + * Init one GMainContext for the iothread unconditionally, even if > + * it's not used > + */ > + iothread_init_gcontext(iothread); > + > aio_context_set_poll_params(iothread->ctx, > iothread->poll_max_ns, > iothread->poll_grow, > @@ -169,8 +187,6 @@ static void iothread_complete(UserCreatable *obj, Error **errp) > return; > } > > - iothread->once = (GOnce) G_ONCE_INIT; > - > /* This assumes we are called from a thread with useful CPU affinity for us > * to inherit. > */ > @@ -333,27 +349,10 @@ IOThreadInfoList *qmp_query_iothreads(Error **errp) > return head; > } > > -static gpointer iothread_g_main_context_init(gpointer opaque) > -{ > - AioContext *ctx; > - IOThread *iothread = opaque; > - GSource *source; > - > - iothread->worker_context = g_main_context_new(); > - > - ctx = iothread_get_aio_context(iothread); > - source = aio_get_g_source(ctx); > - g_source_attach(source, iothread->worker_context); > - g_source_unref(source); > - > - aio_notify(iothread->ctx); > - return NULL; > -} > - > GMainContext *iothread_get_g_main_context(IOThread *iothread) > { > - g_once(&iothread->once, iothread_g_main_context_init, iothread); > - > + atomic_set(&iothread->run_gcontext, 1); > + aio_notify(iothread->ctx); > return iothread->worker_context; > } > > -- > 2.17.1 > looks good otherwise, Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
On Fri, Feb 22, 2019 at 07:29:09AM +0100, Marc-André Lureau wrote: [...] > > diff --git a/iothread.c b/iothread.c > > index 6e297e9ef1..6fa87876e0 100644 > > --- a/iothread.c > > +++ b/iothread.c > > @@ -65,7 +65,7 @@ static void *iothread_run(void *opaque) > > * We must check the running state again in case it was > > * changed in previous aio_poll() > > */ > > - if (iothread->running && atomic_read(&iothread->worker_context)) { > > + if (iothread->running && atomic_read(&iothread->run_gcontext)) { > > GMainLoop *loop; > > > > g_main_context_push_thread_default(iothread->worker_context); > > @@ -114,6 +114,8 @@ static void iothread_instance_init(Object *obj) > > iothread->poll_max_ns = IOTHREAD_POLL_MAX_NS_DEFAULT; > > iothread->thread_id = -1; > > qemu_sem_init(&iothread->init_done_sem, 0); > > + /* By default, we don't run gcontext */ > > + atomic_set(&iothread->run_gcontext, 0); > > I think that initialization isn't really necessary, your call. True; it's more a hint for readers who suddenly jumped into iothread_run. Thanks for being kind, since I would prefer to have it for now. ;) [...] > looks good otherwise, > Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> Thanks for the quick review,
diff --git a/include/sysemu/iothread.h b/include/sysemu/iothread.h index 50411ba54a..5f6240d5cb 100644 --- a/include/sysemu/iothread.h +++ b/include/sysemu/iothread.h @@ -24,9 +24,9 @@ typedef struct { QemuThread thread; AioContext *ctx; + bool run_gcontext; /* whether we should run gcontext */ GMainContext *worker_context; GMainLoop *main_loop; - GOnce once; QemuSemaphore init_done_sem; /* is thread init done? */ bool stopping; /* has iothread_stop() been called? */ bool running; /* should iothread_run() continue? */ diff --git a/iothread.c b/iothread.c index 6e297e9ef1..6fa87876e0 100644 --- a/iothread.c +++ b/iothread.c @@ -65,7 +65,7 @@ static void *iothread_run(void *opaque) * We must check the running state again in case it was * changed in previous aio_poll() */ - if (iothread->running && atomic_read(&iothread->worker_context)) { + if (iothread->running && atomic_read(&iothread->run_gcontext)) { GMainLoop *loop; g_main_context_push_thread_default(iothread->worker_context); @@ -114,6 +114,8 @@ static void iothread_instance_init(Object *obj) iothread->poll_max_ns = IOTHREAD_POLL_MAX_NS_DEFAULT; iothread->thread_id = -1; qemu_sem_init(&iothread->init_done_sem, 0); + /* By default, we don't run gcontext */ + atomic_set(&iothread->run_gcontext, 0); } static void iothread_instance_finalize(Object *obj) @@ -143,6 +145,16 @@ static void iothread_instance_finalize(Object *obj) qemu_sem_destroy(&iothread->init_done_sem); } +static void iothread_init_gcontext(IOThread *iothread) +{ + GSource *source; + + iothread->worker_context = g_main_context_new(); + source = aio_get_g_source(iothread_get_aio_context(iothread)); + g_source_attach(source, iothread->worker_context); + g_source_unref(source); +} + static void iothread_complete(UserCreatable *obj, Error **errp) { Error *local_error = NULL; @@ -157,6 +169,12 @@ static void iothread_complete(UserCreatable *obj, Error **errp) return; } + /* + * Init one GMainContext for the iothread unconditionally, even if + * it's not used + */ + iothread_init_gcontext(iothread); + aio_context_set_poll_params(iothread->ctx, iothread->poll_max_ns, iothread->poll_grow, @@ -169,8 +187,6 @@ static void iothread_complete(UserCreatable *obj, Error **errp) return; } - iothread->once = (GOnce) G_ONCE_INIT; - /* This assumes we are called from a thread with useful CPU affinity for us * to inherit. */ @@ -333,27 +349,10 @@ IOThreadInfoList *qmp_query_iothreads(Error **errp) return head; } -static gpointer iothread_g_main_context_init(gpointer opaque) -{ - AioContext *ctx; - IOThread *iothread = opaque; - GSource *source; - - iothread->worker_context = g_main_context_new(); - - ctx = iothread_get_aio_context(iothread); - source = aio_get_g_source(ctx); - g_source_attach(source, iothread->worker_context); - g_source_unref(source); - - aio_notify(iothread->ctx); - return NULL; -} - GMainContext *iothread_get_g_main_context(IOThread *iothread) { - g_once(&iothread->once, iothread_g_main_context_init, iothread); - + atomic_set(&iothread->run_gcontext, 1); + aio_notify(iothread->ctx); return iothread->worker_context; }
In existing code we create the gcontext dynamically at the first access of the gcontext from caller. That can bring some complexity and potential races during using iothread. Since the context itself is not that big a resource, and we won't have millions of iothread, let's simply create the gcontext unconditionally. This will also be a preparation work further to move the thread context push operation earlier than before (now it's only pushed right before we want to start running the gmainloop). Removing the g_once since it's not necessary, while introducing a new run_gcontext boolean to show whether we want to run the gcontext. Signed-off-by: Peter Xu <peterx@redhat.com> --- include/sysemu/iothread.h | 2 +- iothread.c | 43 +++++++++++++++++++-------------------- 2 files changed, 22 insertions(+), 23 deletions(-)