Re: [Qemu-devel] [PATCH 2/4] iothread: create the gcontext onconditionally

[Marc-André Lureau]

Hi

On Fri, Feb 22, 2019 at 4:14 AM Peter Xu <address@hidden> 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 <address@hidden>
> ---
>  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 <address@hidden>