Hi
On Fri, Feb 22, 2019 at 4:14 AM Peter Xu <pet...@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 <pet...@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.lur...@redhat.com>
