[Qemu-devel] [PATCH 0/4] iothread: create gcontext unconditionally

[Qemu-devel] [PATCH 0/4] iothread: create gcontext unconditionally

[Peter Xu]

When I first read the iothread code, the gcontext confused me for
quite a while.  Meanwhile, I've been tackling with some races due to
this complexity as well.  How much we'll pay for creating the gcontext
unconditionally?  Do we really need this flexibitily (or is it really
a flexibility after all)?  I don't see much gain of existing code, but
I might be wrong.  Anyway, I wrote this patchset to see how the list
would think about it.

This series directly originates from previous discussion with
Marc-Andre where there's a slightly hacky way to try to acquire the
gcontext:

https://lists.gnu.org/archive/html/qemu-devel/2019-02/msg05460.html

Now with this series logically above patch is not needed any more.
Please read patch 4 for more information.

And if this patchset can survive... how about running gcontext
directly in iothread_run()?  I believe there could be a bit more
things to clean but I'll see.

Make check passes for me.

Comments welcomed.  Thanks,

Peter Xu (4):
  iothread: replace init_done_cond with a semaphore
  iothread: create the gcontext onconditionally
  iothread: create main loop unconditionally
  iothread: push gcontext earlier in the thread_fn

 include/sysemu/iothread.h |  5 +--
 iothread.c                | 77 +++++++++++++++------------------------
 2 files changed, 32 insertions(+), 50 deletions(-)

-- 
2.17.1

[Qemu-devel] [PATCH 1/4] iothread: replace init_done_cond with a semaphore

[Peter Xu]

Only sending an init-done message using lock+cond seems an overkill to
me.  Replacing it with a simpler semaphore.

Meanwhile, init the semaphore unconditionally, then we can destroy it
unconditionally too in finalize which seems cleaner.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 include/sysemu/iothread.h |  3 +--
 iothread.c                | 17 ++++-------------
 2 files changed, 5 insertions(+), 15 deletions(-)

diff --git a/include/sysemu/iothread.h b/include/sysemu/iothread.h
index 8a7ac2c528..50411ba54a 100644
--- a/include/sysemu/iothread.h
+++ b/include/sysemu/iothread.h
@@ -27,8 +27,7 @@ typedef struct {
     GMainContext *worker_context;
     GMainLoop *main_loop;
     GOnce once;
-    QemuMutex init_done_lock;
-    QemuCond init_done_cond;    /* is thread initialization done? */
+    QemuSemaphore init_done_sem; /* is thread init done? */
     bool stopping;              /* has iothread_stop() been called? */
     bool running;               /* should iothread_run() continue? */
     int thread_id;
diff --git a/iothread.c b/iothread.c
index e615b7ae52..6e297e9ef1 100644
--- a/iothread.c
+++ b/iothread.c
@@ -55,10 +55,8 @@ static void *iothread_run(void *opaque)
     rcu_register_thread();
 
     my_iothread = iothread;
-    qemu_mutex_lock(&iothread->init_done_lock);
     iothread->thread_id = qemu_get_thread_id();
-    qemu_cond_signal(&iothread->init_done_cond);
-    qemu_mutex_unlock(&iothread->init_done_lock);
+    qemu_sem_post(&iothread->init_done_sem);
 
     while (iothread->running) {
         aio_poll(iothread->ctx, true);
@@ -115,6 +113,7 @@ 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);
 }
 
 static void iothread_instance_finalize(Object *obj)
@@ -123,10 +122,6 @@ static void iothread_instance_finalize(Object *obj)
 
     iothread_stop(iothread);
 
-    if (iothread->thread_id != -1) {
-        qemu_cond_destroy(&iothread->init_done_cond);
-        qemu_mutex_destroy(&iothread->init_done_lock);
-    }
     /*
      * Before glib2 2.33.10, there is a glib2 bug that GSource context
      * pointer may not be cleared even if the context has already been
@@ -145,6 +140,7 @@ static void iothread_instance_finalize(Object *obj)
         g_main_context_unref(iothread->worker_context);
         iothread->worker_context = NULL;
     }
+    qemu_sem_destroy(&iothread->init_done_sem);
 }
 
 static void iothread_complete(UserCreatable *obj, Error **errp)
@@ -173,8 +169,6 @@ static void iothread_complete(UserCreatable *obj, Error **errp)
         return;
     }
 
-    qemu_mutex_init(&iothread->init_done_lock);
-    qemu_cond_init(&iothread->init_done_cond);
     iothread->once = (GOnce) G_ONCE_INIT;
 
     /* This assumes we are called from a thread with useful CPU affinity for us
@@ -188,12 +182,9 @@ static void iothread_complete(UserCreatable *obj, Error **errp)
     g_free(name);
 
     /* Wait for initialization to complete */
-    qemu_mutex_lock(&iothread->init_done_lock);
     while (iothread->thread_id == -1) {
-        qemu_cond_wait(&iothread->init_done_cond,
-                       &iothread->init_done_lock);
+        qemu_sem_wait(&iothread->init_done_sem);
     }
-    qemu_mutex_unlock(&iothread->init_done_lock);
 }
 
 typedef struct {
-- 
2.17.1

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

[Peter Xu]

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);
 }
 
 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

[Qemu-devel] [PATCH 3/4] iothread: create main loop unconditionally

[Peter Xu]

Since we've have the gcontext always there, create the main loop
altogether.  The iothread_run() is even cleaner.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 iothread.c | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/iothread.c b/iothread.c
index 6fa87876e0..9abdbace66 100644
--- a/iothread.c
+++ b/iothread.c
@@ -66,17 +66,8 @@ static void *iothread_run(void *opaque)
          * changed in previous aio_poll()
          */
         if (iothread->running && atomic_read(&iothread->run_gcontext)) {
-            GMainLoop *loop;
-
             g_main_context_push_thread_default(iothread->worker_context);
-            iothread->main_loop =
-                g_main_loop_new(iothread->worker_context, TRUE);
-            loop = iothread->main_loop;
-
             g_main_loop_run(iothread->main_loop);
-            iothread->main_loop = NULL;
-            g_main_loop_unref(loop);
-
             g_main_context_pop_thread_default(iothread->worker_context);
         }
     }
@@ -141,6 +132,8 @@ static void iothread_instance_finalize(Object *obj)
     if (iothread->worker_context) {
         g_main_context_unref(iothread->worker_context);
         iothread->worker_context = NULL;
+        g_main_loop_unref(iothread->main_loop);
+        iothread->main_loop = NULL;
     }
     qemu_sem_destroy(&iothread->init_done_sem);
 }
@@ -153,6 +146,7 @@ static void iothread_init_gcontext(IOThread *iothread)
     source = aio_get_g_source(iothread_get_aio_context(iothread));
     g_source_attach(source, iothread->worker_context);
     g_source_unref(source);
+    iothread->main_loop = g_main_loop_new(iothread->worker_context, TRUE);
 }
 
 static void iothread_complete(UserCreatable *obj, Error **errp)
-- 
2.17.1

[Qemu-devel] [PATCH 4/4] iothread: push gcontext earlier in the thread_fn

[Peter Xu]

We were pushing the context until right before running the gmainloop.
Now since we have everything unconditionally, we can move this
earlier.

One benefit is that now it's done even before init_done_sem, so as
long as the iothread user calls iothread_create() and completes, we
know that the thread stack is ready.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 iothread.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/iothread.c b/iothread.c
index 9abdbace66..7b7cba5d04 100644
--- a/iothread.c
+++ b/iothread.c
@@ -53,7 +53,7 @@ static void *iothread_run(void *opaque)
     IOThread *iothread = opaque;
 
     rcu_register_thread();
-
+    g_main_context_push_thread_default(iothread->worker_context);
     my_iothread = iothread;
     iothread->thread_id = qemu_get_thread_id();
     qemu_sem_post(&iothread->init_done_sem);
@@ -66,12 +66,11 @@ static void *iothread_run(void *opaque)
          * changed in previous aio_poll()
          */
         if (iothread->running && atomic_read(&iothread->run_gcontext)) {
-            g_main_context_push_thread_default(iothread->worker_context);
             g_main_loop_run(iothread->main_loop);
-            g_main_context_pop_thread_default(iothread->worker_context);
         }
     }
 
+    g_main_context_pop_thread_default(iothread->worker_context);
     rcu_unregister_thread();
     return NULL;
 }
-- 
2.17.1

Re: [Qemu-devel] [PATCH 1/4] iothread: replace init_done_cond with a semaphore

[Marc-André Lureau]

Hi

On Fri, Feb 22, 2019 at 4:14 AM Peter Xu <peterx@redhat.com> wrote:
>
> Only sending an init-done message using lock+cond seems an overkill to
> me.  Replacing it with a simpler semaphore.
>
> Meanwhile, init the semaphore unconditionally, then we can destroy it
> unconditionally too in finalize which seems cleaner.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>

The lock is also protecting thread_id.

> ---
>  include/sysemu/iothread.h |  3 +--
>  iothread.c                | 17 ++++-------------
>  2 files changed, 5 insertions(+), 15 deletions(-)
>
> diff --git a/include/sysemu/iothread.h b/include/sysemu/iothread.h
> index 8a7ac2c528..50411ba54a 100644
> --- a/include/sysemu/iothread.h
> +++ b/include/sysemu/iothread.h
> @@ -27,8 +27,7 @@ typedef struct {
>      GMainContext *worker_context;
>      GMainLoop *main_loop;
>      GOnce once;
> -    QemuMutex init_done_lock;
> -    QemuCond init_done_cond;    /* is thread initialization done? */
> +    QemuSemaphore init_done_sem; /* is thread init done? */
>      bool stopping;              /* has iothread_stop() been called? */
>      bool running;               /* should iothread_run() continue? */
>      int thread_id;
> diff --git a/iothread.c b/iothread.c
> index e615b7ae52..6e297e9ef1 100644
> --- a/iothread.c
> +++ b/iothread.c
> @@ -55,10 +55,8 @@ static void *iothread_run(void *opaque)
>      rcu_register_thread();
>
>      my_iothread = iothread;
> -    qemu_mutex_lock(&iothread->init_done_lock);
>      iothread->thread_id = qemu_get_thread_id();
> -    qemu_cond_signal(&iothread->init_done_cond);
> -    qemu_mutex_unlock(&iothread->init_done_lock);
> +    qemu_sem_post(&iothread->init_done_sem);
>
>      while (iothread->running) {
>          aio_poll(iothread->ctx, true);
> @@ -115,6 +113,7 @@ 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);
>  }
>
>  static void iothread_instance_finalize(Object *obj)
> @@ -123,10 +122,6 @@ static void iothread_instance_finalize(Object *obj)
>
>      iothread_stop(iothread);
>
> -    if (iothread->thread_id != -1) {
> -        qemu_cond_destroy(&iothread->init_done_cond);
> -        qemu_mutex_destroy(&iothread->init_done_lock);
> -    }
>      /*
>       * Before glib2 2.33.10, there is a glib2 bug that GSource context
>       * pointer may not be cleared even if the context has already been
> @@ -145,6 +140,7 @@ static void iothread_instance_finalize(Object *obj)
>          g_main_context_unref(iothread->worker_context);
>          iothread->worker_context = NULL;
>      }
> +    qemu_sem_destroy(&iothread->init_done_sem);
>  }
>
>  static void iothread_complete(UserCreatable *obj, Error **errp)
> @@ -173,8 +169,6 @@ static void iothread_complete(UserCreatable *obj, Error **errp)
>          return;
>      }
>
> -    qemu_mutex_init(&iothread->init_done_lock);
> -    qemu_cond_init(&iothread->init_done_cond);
>      iothread->once = (GOnce) G_ONCE_INIT;
>
>      /* This assumes we are called from a thread with useful CPU affinity for us
> @@ -188,12 +182,9 @@ static void iothread_complete(UserCreatable *obj, Error **errp)
>      g_free(name);
>
>      /* Wait for initialization to complete */
> -    qemu_mutex_lock(&iothread->init_done_lock);
>      while (iothread->thread_id == -1) {
> -        qemu_cond_wait(&iothread->init_done_cond,
> -                       &iothread->init_done_lock);
> +        qemu_sem_wait(&iothread->init_done_sem);
>      }
> -    qemu_mutex_unlock(&iothread->init_done_lock);
>  }
>
>  typedef struct {
> --
> 2.17.1
>

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 <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>

Re: [Qemu-devel] [PATCH 3/4] iothread: create main loop unconditionally

[Marc-André Lureau]

On Fri, Feb 22, 2019 at 4:14 AM Peter Xu <peterx@redhat.com> wrote:
>
> Since we've have the gcontext always there, create the main loop
> altogether.  The iothread_run() is even cleaner.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

> ---
>  iothread.c | 12 +++---------
>  1 file changed, 3 insertions(+), 9 deletions(-)
>
> diff --git a/iothread.c b/iothread.c
> index 6fa87876e0..9abdbace66 100644
> --- a/iothread.c
> +++ b/iothread.c
> @@ -66,17 +66,8 @@ static void *iothread_run(void *opaque)
>           * changed in previous aio_poll()
>           */
>          if (iothread->running && atomic_read(&iothread->run_gcontext)) {
> -            GMainLoop *loop;
> -
>              g_main_context_push_thread_default(iothread->worker_context);
> -            iothread->main_loop =
> -                g_main_loop_new(iothread->worker_context, TRUE);
> -            loop = iothread->main_loop;
> -
>              g_main_loop_run(iothread->main_loop);
> -            iothread->main_loop = NULL;
> -            g_main_loop_unref(loop);
> -
>              g_main_context_pop_thread_default(iothread->worker_context);
>          }
>      }
> @@ -141,6 +132,8 @@ static void iothread_instance_finalize(Object *obj)
>      if (iothread->worker_context) {
>          g_main_context_unref(iothread->worker_context);
>          iothread->worker_context = NULL;
> +        g_main_loop_unref(iothread->main_loop);
> +        iothread->main_loop = NULL;
>      }
>      qemu_sem_destroy(&iothread->init_done_sem);
>  }
> @@ -153,6 +146,7 @@ static void iothread_init_gcontext(IOThread *iothread)
>      source = aio_get_g_source(iothread_get_aio_context(iothread));
>      g_source_attach(source, iothread->worker_context);
>      g_source_unref(source);
> +    iothread->main_loop = g_main_loop_new(iothread->worker_context, TRUE);
>  }
>
>  static void iothread_complete(UserCreatable *obj, Error **errp)
> --
> 2.17.1
>

Re: [Qemu-devel] [PATCH 1/4] iothread: replace init_done_cond with a semaphore

[Peter Xu]

On Fri, Feb 22, 2019 at 07:25:16AM +0100, Marc-André Lureau wrote:
> Hi
> 
> On Fri, Feb 22, 2019 at 4:14 AM Peter Xu <peterx@redhat.com> wrote:
> >
> > Only sending an init-done message using lock+cond seems an overkill to
> > me.  Replacing it with a simpler semaphore.
> >
> > Meanwhile, init the semaphore unconditionally, then we can destroy it
> > unconditionally too in finalize which seems cleaner.
> >
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> 
> The lock is also protecting thread_id.

IMHO it's fine because thread_id is only changed at the beginning of
iothread_run where the caller will definitely wait for the thread_id
to be generated.  Here qemu_sem_post() should at least contain one
write memory barrier there to make sure the waker will read the
correct value after sem_wait() and then later on thread_id is never
changed.

Regards,

-- 
Peter Xu

Re: [Qemu-devel] [PATCH 4/4] iothread: push gcontext earlier in the thread_fn

[Marc-André Lureau]

Hi

On Fri, Feb 22, 2019 at 4:14 AM Peter Xu <peterx@redhat.com> wrote:
>
> We were pushing the context until right before running the gmainloop.
> Now since we have everything unconditionally, we can move this
> earlier.
>
> One benefit is that now it's done even before init_done_sem, so as
> long as the iothread user calls iothread_create() and completes, we
> know that the thread stack is ready.
>

This will change the default context in the iothread, for code running
there. This may not be a good idea. Until now, only sources dispatched
from iothread_get_g_main_context() would have default context
associated to it.

I don't know if the current behaviour is intentional, but it has some
logic. With this change, you may create hidden races, by changing the
default context of sources to the iothread.

> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  iothread.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/iothread.c b/iothread.c
> index 9abdbace66..7b7cba5d04 100644
> --- a/iothread.c
> +++ b/iothread.c
> @@ -53,7 +53,7 @@ static void *iothread_run(void *opaque)
>      IOThread *iothread = opaque;
>
>      rcu_register_thread();
> -
> +    g_main_context_push_thread_default(iothread->worker_context);
>      my_iothread = iothread;
>      iothread->thread_id = qemu_get_thread_id();
>      qemu_sem_post(&iothread->init_done_sem);
> @@ -66,12 +66,11 @@ static void *iothread_run(void *opaque)
>           * changed in previous aio_poll()
>           */
>          if (iothread->running && atomic_read(&iothread->run_gcontext)) {
> -            g_main_context_push_thread_default(iothread->worker_context);
>              g_main_loop_run(iothread->main_loop);
> -            g_main_context_pop_thread_default(iothread->worker_context);
>          }
>      }
>
> +    g_main_context_pop_thread_default(iothread->worker_context);
>      rcu_unregister_thread();
>      return NULL;
>  }
> --
> 2.17.1
>

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

[Peter Xu]

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,

-- 
Peter Xu

Re: [Qemu-devel] [PATCH 4/4] iothread: push gcontext earlier in the thread_fn

[Peter Xu]

On Fri, Feb 22, 2019 at 07:37:02AM +0100, Marc-André Lureau wrote:
> Hi
> 
> On Fri, Feb 22, 2019 at 4:14 AM Peter Xu <peterx@redhat.com> wrote:
> >
> > We were pushing the context until right before running the gmainloop.
> > Now since we have everything unconditionally, we can move this
> > earlier.
> >
> > One benefit is that now it's done even before init_done_sem, so as
> > long as the iothread user calls iothread_create() and completes, we
> > know that the thread stack is ready.
> >
> 
> This will change the default context in the iothread, for code running
> there. This may not be a good idea. Until now, only sources dispatched
> from iothread_get_g_main_context() would have default context
> associated to it.
> 
> I don't know if the current behaviour is intentional, but it has some
> logic. With this change, you may create hidden races, by changing the
> default context of sources to the iothread.

Yes I agree that the behavior will be changed in this patch that even
if the iothread user does not use the gcontext they'll also have the
context set.  I would think it should be ok because IMHO events hooked
onto the aio context should not depend on the gcontext, but indeed I'd
like to get some confirmation from others, especially the block layer.

Stefan?

Thanks,

-- 
Peter Xu

Re: [Qemu-devel] [PATCH 4/4] iothread: push gcontext earlier in the thread_fn

[Paolo Bonzini]

On 22/02/19 07:57, Peter Xu wrote:
> On Fri, Feb 22, 2019 at 07:37:02AM +0100, Marc-André Lureau wrote:
>> Hi
>>
>> On Fri, Feb 22, 2019 at 4:14 AM Peter Xu <peterx@redhat.com> wrote:
>>>
>>> We were pushing the context until right before running the gmainloop.
>>> Now since we have everything unconditionally, we can move this
>>> earlier.
>>>
>>> One benefit is that now it's done even before init_done_sem, so as
>>> long as the iothread user calls iothread_create() and completes, we
>>> know that the thread stack is ready.
>>>
>>
>> This will change the default context in the iothread, for code running
>> there. This may not be a good idea. Until now, only sources dispatched
>> from iothread_get_g_main_context() would have default context
>> associated to it.
>>
>> I don't know if the current behaviour is intentional, but it has some
>> logic. With this change, you may create hidden races, by changing the
>> default context of sources to the iothread.
> 
> Yes I agree that the behavior will be changed in this patch that even
> if the iothread user does not use the gcontext they'll also have the
> context set.  I would think it should be ok because IMHO events hooked
> onto the aio context should not depend on the gcontext, but indeed I'd
> like to get some confirmation from others, especially the block layer.

The block layer does not use GSource at all.

Paolo

Re: [Qemu-devel] [PATCH 1/4] iothread: replace init_done_cond with a semaphore

[Paolo Bonzini]

On 22/02/19 07:36, Peter Xu wrote:
> On Fri, Feb 22, 2019 at 07:25:16AM +0100, Marc-André Lureau wrote:
>> Hi
>>
>> On Fri, Feb 22, 2019 at 4:14 AM Peter Xu <peterx@redhat.com> wrote:
>>>
>>> Only sending an init-done message using lock+cond seems an overkill to
>>> me.  Replacing it with a simpler semaphore.
>>>
>>> Meanwhile, init the semaphore unconditionally, then we can destroy it
>>> unconditionally too in finalize which seems cleaner.
>>>
>>> Signed-off-by: Peter Xu <peterx@redhat.com>
>>
>> The lock is also protecting thread_id.
> 
> IMHO it's fine because thread_id is only changed at the beginning of
> iothread_run where the caller will definitely wait for the thread_id
> to be generated.  Here qemu_sem_post() should at least contain one
> write memory barrier there to make sure the waker will read the
> correct value after sem_wait() and then later on thread_id is never
> changed.

Yes, qemu_sem_post() is a "release" operation.  Anything that happens
before it is visible to the thread that does qemu_sem_wait().

In fact, thread_id is accessed outside the lock in
iothread_instance_finalize, so Peter's change is overall an improvement.

Paolo

Re: [Qemu-devel] [PATCH 0/4] iothread: create gcontext unconditionally

[Paolo Bonzini]

On 22/02/19 04:14, Peter Xu wrote:
> And if this patchset can survive... how about running gcontext
> directly in iothread_run()?  I believe there could be a bit more
> things to clean but I'll see.

Do you mean instead of aio_poll?  The problem is that GMainContext is
quite a bit slower than aio_poll.

Frediano and I tried to bring some of the optimizations of aio_poll to
GMainContext
(https://github.com/GNOME/glib/commit/e91c11841808ccca408da96136f433a82b2e2145),
but they broke webkit. :(

Paolo

Re: [Qemu-devel] [PATCH 1/4] iothread: replace init_done_cond with a semaphore

[Marc-André Lureau]

Hi

On Fri, Feb 22, 2019 at 10:33 AM Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On 22/02/19 07:36, Peter Xu wrote:
> > On Fri, Feb 22, 2019 at 07:25:16AM +0100, Marc-André Lureau wrote:
> >> Hi
> >>
> >> On Fri, Feb 22, 2019 at 4:14 AM Peter Xu <peterx@redhat.com> wrote:
> >>>
> >>> Only sending an init-done message using lock+cond seems an overkill to
> >>> me.  Replacing it with a simpler semaphore.
> >>>
> >>> Meanwhile, init the semaphore unconditionally, then we can destroy it
> >>> unconditionally too in finalize which seems cleaner.
> >>>
> >>> Signed-off-by: Peter Xu <peterx@redhat.com>
> >>
> >> The lock is also protecting thread_id.
> >
> > IMHO it's fine because thread_id is only changed at the beginning of
> > iothread_run where the caller will definitely wait for the thread_id
> > to be generated.  Here qemu_sem_post() should at least contain one
> > write memory barrier there to make sure the waker will read the
> > correct value after sem_wait() and then later on thread_id is never
> > changed.
>
> Yes, qemu_sem_post() is a "release" operation.  Anything that happens
> before it is visible to the thread that does qemu_sem_wait().

ok

> In fact, thread_id is accessed outside the lock in
> iothread_instance_finalize, so Peter's change is overall an improvement.
>

finalize() stops the threadm though


-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH 0/4] iothread: create gcontext unconditionally

[Peter Xu]

On Fri, Feb 22, 2019 at 10:28:57AM +0100, Paolo Bonzini wrote:
> On 22/02/19 04:14, Peter Xu wrote:
> > And if this patchset can survive... how about running gcontext
> > directly in iothread_run()?  I believe there could be a bit more
> > things to clean but I'll see.
> 
> Do you mean instead of aio_poll?

Yes.

> The problem is that GMainContext is
> quite a bit slower than aio_poll.

That's really what I wanted to know; so it's about performance.

We should mention it somewhere in iothread_run.  I can do that after I
know how this series will go.

> 
> Frediano and I tried to bring some of the optimizations of aio_poll to
> GMainContext
> (https://github.com/GNOME/glib/commit/e91c11841808ccca408da96136f433a82b2e2145),
> but they broke webkit. :(

What a pity!

-- 
Peter Xu

Re: [Qemu-devel] [PATCH 1/4] iothread: replace init_done_cond with a semaphore

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 571 bytes --]

On Fri, Feb 22, 2019 at 11:14:10AM +0800, Peter Xu wrote:
> Only sending an init-done message using lock+cond seems an overkill to
> me.  Replacing it with a simpler semaphore.
> 
> Meanwhile, init the semaphore unconditionally, then we can destroy it
> unconditionally too in finalize which seems cleaner.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  include/sysemu/iothread.h |  3 +--
>  iothread.c                | 17 ++++-------------
>  2 files changed, 5 insertions(+), 15 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH 4/4] iothread: push gcontext earlier in the thread_fn

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1629 bytes --]

On Fri, Feb 22, 2019 at 02:57:24PM +0800, Peter Xu wrote:
> On Fri, Feb 22, 2019 at 07:37:02AM +0100, Marc-André Lureau wrote:
> > Hi
> > 
> > On Fri, Feb 22, 2019 at 4:14 AM Peter Xu <peterx@redhat.com> wrote:
> > >
> > > We were pushing the context until right before running the gmainloop.
> > > Now since we have everything unconditionally, we can move this
> > > earlier.
> > >
> > > One benefit is that now it's done even before init_done_sem, so as
> > > long as the iothread user calls iothread_create() and completes, we
> > > know that the thread stack is ready.
> > >
> > 
> > This will change the default context in the iothread, for code running
> > there. This may not be a good idea. Until now, only sources dispatched
> > from iothread_get_g_main_context() would have default context
> > associated to it.
> > 
> > I don't know if the current behaviour is intentional, but it has some
> > logic. With this change, you may create hidden races, by changing the
> > default context of sources to the iothread.
> 
> Yes I agree that the behavior will be changed in this patch that even
> if the iothread user does not use the gcontext they'll also have the
> context set.  I would think it should be ok because IMHO events hooked
> onto the aio context should not depend on the gcontext, but indeed I'd
> like to get some confirmation from others, especially the block layer.

I don't understand why Patch 4 is desirable.  The comment about
init_done_sem isn't clear to me but I also wondered the same thing as
Marc-André.

Can you explain why we should apply this patch?

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH 4/4] iothread: push gcontext earlier in the thread_fn

[Peter Xu]

On Wed, Feb 27, 2019 at 01:38:38PM +0000, Stefan Hajnoczi wrote:
> On Fri, Feb 22, 2019 at 02:57:24PM +0800, Peter Xu wrote:
> > On Fri, Feb 22, 2019 at 07:37:02AM +0100, Marc-André Lureau wrote:
> > > Hi
> > > 
> > > On Fri, Feb 22, 2019 at 4:14 AM Peter Xu <peterx@redhat.com> wrote:
> > > >
> > > > We were pushing the context until right before running the gmainloop.
> > > > Now since we have everything unconditionally, we can move this
> > > > earlier.
> > > >
> > > > One benefit is that now it's done even before init_done_sem, so as
> > > > long as the iothread user calls iothread_create() and completes, we
> > > > know that the thread stack is ready.
> > > >
> > > 
> > > This will change the default context in the iothread, for code running
> > > there. This may not be a good idea. Until now, only sources dispatched
> > > from iothread_get_g_main_context() would have default context
> > > associated to it.
> > > 
> > > I don't know if the current behaviour is intentional, but it has some
> > > logic. With this change, you may create hidden races, by changing the
> > > default context of sources to the iothread.
> > 
> > Yes I agree that the behavior will be changed in this patch that even
> > if the iothread user does not use the gcontext they'll also have the
> > context set.  I would think it should be ok because IMHO events hooked
> > onto the aio context should not depend on the gcontext, but indeed I'd
> > like to get some confirmation from others, especially the block layer.
> 
> I don't understand why Patch 4 is desirable.  The comment about
> init_done_sem isn't clear to me but I also wondered the same thing as
> Marc-André.
> 
> Can you explain why we should apply this patch?

Hi, Stefan,

The patch 4 itself does not help much for current QEMU, but it should
be required to replace the patch that Marc-Andre proposed below:

https://lists.gnu.org/archive/html/qemu-devel/2019-02/msg05460.html [1]

And IMHO patch 4 along with this whole series should be a cleaner
approach comparing to the one proposed in [1].  Here if my
understanding is correct the problem is that
g_main_context_push_thread_default() is really designed to be called
at the very beginning of a thread creation but not dynamically called
during the execution of a thread (prove is that it even does not have
any error to return when failed to acquire the context so the caller
will never know if it failed! see [2] below), in that sense this patch
4 can be seen as a tiny cleanup too.

g_main_context_push_thread_default (GMainContext *context)
{
  GQueue *stack;
  gboolean acquired_context;

  acquired_context = g_main_context_acquire (context);
  g_return_if_fail (acquired_context);  <------------- [2]

  ...
}

Regards,

-- 
Peter Xu

Re: [Qemu-devel] [PATCH 4/4] iothread: push gcontext earlier in the thread_fn

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 3429 bytes --]

On Thu, Feb 28, 2019 at 01:58:56PM +0800, Peter Xu wrote:
> On Wed, Feb 27, 2019 at 01:38:38PM +0000, Stefan Hajnoczi wrote:
> > On Fri, Feb 22, 2019 at 02:57:24PM +0800, Peter Xu wrote:
> > > On Fri, Feb 22, 2019 at 07:37:02AM +0100, Marc-André Lureau wrote:
> > > > Hi
> > > > 
> > > > On Fri, Feb 22, 2019 at 4:14 AM Peter Xu <peterx@redhat.com> wrote:
> > > > >
> > > > > We were pushing the context until right before running the gmainloop.
> > > > > Now since we have everything unconditionally, we can move this
> > > > > earlier.
> > > > >
> > > > > One benefit is that now it's done even before init_done_sem, so as
> > > > > long as the iothread user calls iothread_create() and completes, we
> > > > > know that the thread stack is ready.
> > > > >
> > > > 
> > > > This will change the default context in the iothread, for code running
> > > > there. This may not be a good idea. Until now, only sources dispatched
> > > > from iothread_get_g_main_context() would have default context
> > > > associated to it.
> > > > 
> > > > I don't know if the current behaviour is intentional, but it has some
> > > > logic. With this change, you may create hidden races, by changing the
> > > > default context of sources to the iothread.
> > > 
> > > Yes I agree that the behavior will be changed in this patch that even
> > > if the iothread user does not use the gcontext they'll also have the
> > > context set.  I would think it should be ok because IMHO events hooked
> > > onto the aio context should not depend on the gcontext, but indeed I'd
> > > like to get some confirmation from others, especially the block layer.
> > 
> > I don't understand why Patch 4 is desirable.  The comment about
> > init_done_sem isn't clear to me but I also wondered the same thing as
> > Marc-André.
> > 
> > Can you explain why we should apply this patch?
> 
> Hi, Stefan,
> 
> The patch 4 itself does not help much for current QEMU, but it should
> be required to replace the patch that Marc-Andre proposed below:
> 
> https://lists.gnu.org/archive/html/qemu-devel/2019-02/msg05460.html [1]
> 
> And IMHO patch 4 along with this whole series should be a cleaner
> approach comparing to the one proposed in [1].  Here if my
> understanding is correct the problem is that
> g_main_context_push_thread_default() is really designed to be called
> at the very beginning of a thread creation but not dynamically called
> during the execution of a thread (prove is that it even does not have
> any error to return when failed to acquire the context so the caller
> will never know if it failed! see [2] below), in that sense this patch
> 4 can be seen as a tiny cleanup too.
> 
> g_main_context_push_thread_default (GMainContext *context)
> {
>   GQueue *stack;
>   gboolean acquired_context;
> 
>   acquired_context = g_main_context_acquire (context);
>   g_return_if_fail (acquired_context);  <------------- [2]
> 
>   ...
> }

I see.  This explains why you want to call it early.  If you're worried
about that then there should also be a comment warning people that this
must happen first before anything implicitly uses the thread's
GMainContext.

What about Marc-André's concern about the change in behavior?  Now this
thread is associated with the GMainContext that isn't processed at in
aio_poll().  Previously the default main context would be used.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH 4/4] iothread: push gcontext earlier in the thread_fn

[Peter Xu]

On Fri, Mar 01, 2019 at 04:25:17PM +0000, Stefan Hajnoczi wrote:
> On Thu, Feb 28, 2019 at 01:58:56PM +0800, Peter Xu wrote:
> > On Wed, Feb 27, 2019 at 01:38:38PM +0000, Stefan Hajnoczi wrote:
> > > On Fri, Feb 22, 2019 at 02:57:24PM +0800, Peter Xu wrote:
> > > > On Fri, Feb 22, 2019 at 07:37:02AM +0100, Marc-André Lureau wrote:
> > > > > Hi
> > > > > 
> > > > > On Fri, Feb 22, 2019 at 4:14 AM Peter Xu <peterx@redhat.com> wrote:
> > > > > >
> > > > > > We were pushing the context until right before running the gmainloop.
> > > > > > Now since we have everything unconditionally, we can move this
> > > > > > earlier.
> > > > > >
> > > > > > One benefit is that now it's done even before init_done_sem, so as
> > > > > > long as the iothread user calls iothread_create() and completes, we
> > > > > > know that the thread stack is ready.
> > > > > >
> > > > > 
> > > > > This will change the default context in the iothread, for code running
> > > > > there. This may not be a good idea. Until now, only sources dispatched
> > > > > from iothread_get_g_main_context() would have default context
> > > > > associated to it.
> > > > > 
> > > > > I don't know if the current behaviour is intentional, but it has some
> > > > > logic. With this change, you may create hidden races, by changing the
> > > > > default context of sources to the iothread.
> > > > 
> > > > Yes I agree that the behavior will be changed in this patch that even
> > > > if the iothread user does not use the gcontext they'll also have the
> > > > context set.  I would think it should be ok because IMHO events hooked
> > > > onto the aio context should not depend on the gcontext, but indeed I'd
> > > > like to get some confirmation from others, especially the block layer.
> > > 
> > > I don't understand why Patch 4 is desirable.  The comment about
> > > init_done_sem isn't clear to me but I also wondered the same thing as
> > > Marc-André.
> > > 
> > > Can you explain why we should apply this patch?
> > 
> > Hi, Stefan,
> > 
> > The patch 4 itself does not help much for current QEMU, but it should
> > be required to replace the patch that Marc-Andre proposed below:
> > 
> > https://lists.gnu.org/archive/html/qemu-devel/2019-02/msg05460.html [1]
> > 
> > And IMHO patch 4 along with this whole series should be a cleaner
> > approach comparing to the one proposed in [1].  Here if my
> > understanding is correct the problem is that
> > g_main_context_push_thread_default() is really designed to be called
> > at the very beginning of a thread creation but not dynamically called
> > during the execution of a thread (prove is that it even does not have
> > any error to return when failed to acquire the context so the caller
> > will never know if it failed! see [2] below), in that sense this patch
> > 4 can be seen as a tiny cleanup too.
> > 
> > g_main_context_push_thread_default (GMainContext *context)
> > {
> >   GQueue *stack;
> >   gboolean acquired_context;
> > 
> >   acquired_context = g_main_context_acquire (context);
> >   g_return_if_fail (acquired_context);  <------------- [2]
> > 
> >   ...
> > }
> 
> I see.  This explains why you want to call it early.  If you're worried
> about that then there should also be a comment warning people that this
> must happen first before anything implicitly uses the thread's
> GMainContext.

Sure, I can add a comment above g_main_context_push_thread_default()
to emphasize why it's preferred at the entry.

> 
> What about Marc-André's concern about the change in behavior?  Now this
> thread is associated with the GMainContext that isn't processed at in
> aio_poll().  Previously the default main context would be used.

IIUC Paolo has answered this question (Message-ID:
<0faeceb2-68fa-59b0-48c3-b8e907b2a75f@redhat.com>) - if the block
layer (or say, the explicit aio_poll in the iothread_run) does not use
the GMainContext at all then it should affect nothing, and with that
there should have no real functional change.

Regards,

-- 
Peter Xu

Re: [Qemu-devel] [PATCH 4/4] iothread: push gcontext earlier in the thread_fn

[Marc-André Lureau]

Hi

On Mon, Mar 4, 2019 at 3:27 AM Peter Xu <peterx@redhat.com> wrote:
>
> On Fri, Mar 01, 2019 at 04:25:17PM +0000, Stefan Hajnoczi wrote:
> > On Thu, Feb 28, 2019 at 01:58:56PM +0800, Peter Xu wrote:
> > > On Wed, Feb 27, 2019 at 01:38:38PM +0000, Stefan Hajnoczi wrote:
> > > > On Fri, Feb 22, 2019 at 02:57:24PM +0800, Peter Xu wrote:
> > > > > On Fri, Feb 22, 2019 at 07:37:02AM +0100, Marc-André Lureau wrote:
> > > > > > Hi
> > > > > >
> > > > > > On Fri, Feb 22, 2019 at 4:14 AM Peter Xu <peterx@redhat.com> wrote:
> > > > > > >
> > > > > > > We were pushing the context until right before running the gmainloop.
> > > > > > > Now since we have everything unconditionally, we can move this
> > > > > > > earlier.
> > > > > > >
> > > > > > > One benefit is that now it's done even before init_done_sem, so as
> > > > > > > long as the iothread user calls iothread_create() and completes, we
> > > > > > > know that the thread stack is ready.
> > > > > > >
> > > > > >
> > > > > > This will change the default context in the iothread, for code running
> > > > > > there. This may not be a good idea. Until now, only sources dispatched
> > > > > > from iothread_get_g_main_context() would have default context
> > > > > > associated to it.
> > > > > >
> > > > > > I don't know if the current behaviour is intentional, but it has some
> > > > > > logic. With this change, you may create hidden races, by changing the
> > > > > > default context of sources to the iothread.
> > > > >
> > > > > Yes I agree that the behavior will be changed in this patch that even
> > > > > if the iothread user does not use the gcontext they'll also have the
> > > > > context set.  I would think it should be ok because IMHO events hooked
> > > > > onto the aio context should not depend on the gcontext, but indeed I'd
> > > > > like to get some confirmation from others, especially the block layer.
> > > >
> > > > I don't understand why Patch 4 is desirable.  The comment about
> > > > init_done_sem isn't clear to me but I also wondered the same thing as
> > > > Marc-André.
> > > >
> > > > Can you explain why we should apply this patch?
> > >
> > > Hi, Stefan,
> > >
> > > The patch 4 itself does not help much for current QEMU, but it should
> > > be required to replace the patch that Marc-Andre proposed below:
> > >
> > > https://lists.gnu.org/archive/html/qemu-devel/2019-02/msg05460.html [1]
> > >
> > > And IMHO patch 4 along with this whole series should be a cleaner
> > > approach comparing to the one proposed in [1].  Here if my
> > > understanding is correct the problem is that
> > > g_main_context_push_thread_default() is really designed to be called
> > > at the very beginning of a thread creation but not dynamically called
> > > during the execution of a thread (prove is that it even does not have
> > > any error to return when failed to acquire the context so the caller
> > > will never know if it failed! see [2] below), in that sense this patch
> > > 4 can be seen as a tiny cleanup too.
> > >
> > > g_main_context_push_thread_default (GMainContext *context)
> > > {
> > >   GQueue *stack;
> > >   gboolean acquired_context;
> > >
> > >   acquired_context = g_main_context_acquire (context);
> > >   g_return_if_fail (acquired_context);  <------------- [2]
> > >
> > >   ...
> > > }
> >
> > I see.  This explains why you want to call it early.  If you're worried
> > about that then there should also be a comment warning people that this
> > must happen first before anything implicitly uses the thread's
> > GMainContext.
>
> Sure, I can add a comment above g_main_context_push_thread_default()
> to emphasize why it's preferred at the entry.
>
> >
> > What about Marc-André's concern about the change in behavior?  Now this
> > thread is associated with the GMainContext that isn't processed at in
> > aio_poll().  Previously the default main context would be used.
>
> IIUC Paolo has answered this question (Message-ID:
> <0faeceb2-68fa-59b0-48c3-b8e907b2a75f@redhat.com>) - if the block
> layer (or say, the explicit aio_poll in the iothread_run) does not use
> the GMainContext at all then it should affect nothing, and with that
> there should have no real functional change.
>

It's a bold claim though, iothread isn't used only by the block layer.
It's used also by colo in qemu tree, and I used to have a branch for
virgl rendering for example. There might be other experimental work or
usage I missed. Furthermore, even the block layer, and its various
dependencies, is hard to review thoroughly. But it looks like it
doesn't rely on glib main context, I agree. But I would get prepared
for some weird (difficult to debug) regressions eventually.

Re: [Qemu-devel] [PATCH 4/4] iothread: push gcontext earlier in the thread_fn

[Peter Xu]

On Mon, Mar 04, 2019 at 10:12:10AM +0100, Marc-André Lureau wrote:
> Hi
> 
> On Mon, Mar 4, 2019 at 3:27 AM Peter Xu <peterx@redhat.com> wrote:
> >
> > On Fri, Mar 01, 2019 at 04:25:17PM +0000, Stefan Hajnoczi wrote:
> > > On Thu, Feb 28, 2019 at 01:58:56PM +0800, Peter Xu wrote:
> > > > On Wed, Feb 27, 2019 at 01:38:38PM +0000, Stefan Hajnoczi wrote:
> > > > > On Fri, Feb 22, 2019 at 02:57:24PM +0800, Peter Xu wrote:
> > > > > > On Fri, Feb 22, 2019 at 07:37:02AM +0100, Marc-André Lureau wrote:
> > > > > > > Hi
> > > > > > >
> > > > > > > On Fri, Feb 22, 2019 at 4:14 AM Peter Xu <peterx@redhat.com> wrote:
> > > > > > > >
> > > > > > > > We were pushing the context until right before running the gmainloop.
> > > > > > > > Now since we have everything unconditionally, we can move this
> > > > > > > > earlier.
> > > > > > > >
> > > > > > > > One benefit is that now it's done even before init_done_sem, so as
> > > > > > > > long as the iothread user calls iothread_create() and completes, we
> > > > > > > > know that the thread stack is ready.
> > > > > > > >
> > > > > > >
> > > > > > > This will change the default context in the iothread, for code running
> > > > > > > there. This may not be a good idea. Until now, only sources dispatched
> > > > > > > from iothread_get_g_main_context() would have default context
> > > > > > > associated to it.
> > > > > > >
> > > > > > > I don't know if the current behaviour is intentional, but it has some
> > > > > > > logic. With this change, you may create hidden races, by changing the
> > > > > > > default context of sources to the iothread.
> > > > > >
> > > > > > Yes I agree that the behavior will be changed in this patch that even
> > > > > > if the iothread user does not use the gcontext they'll also have the
> > > > > > context set.  I would think it should be ok because IMHO events hooked
> > > > > > onto the aio context should not depend on the gcontext, but indeed I'd
> > > > > > like to get some confirmation from others, especially the block layer.
> > > > >
> > > > > I don't understand why Patch 4 is desirable.  The comment about
> > > > > init_done_sem isn't clear to me but I also wondered the same thing as
> > > > > Marc-André.
> > > > >
> > > > > Can you explain why we should apply this patch?
> > > >
> > > > Hi, Stefan,
> > > >
> > > > The patch 4 itself does not help much for current QEMU, but it should
> > > > be required to replace the patch that Marc-Andre proposed below:
> > > >
> > > > https://lists.gnu.org/archive/html/qemu-devel/2019-02/msg05460.html [1]

[1]

> > > >
> > > > And IMHO patch 4 along with this whole series should be a cleaner
> > > > approach comparing to the one proposed in [1].  Here if my
> > > > understanding is correct the problem is that
> > > > g_main_context_push_thread_default() is really designed to be called
> > > > at the very beginning of a thread creation but not dynamically called
> > > > during the execution of a thread (prove is that it even does not have
> > > > any error to return when failed to acquire the context so the caller
> > > > will never know if it failed! see [2] below), in that sense this patch
> > > > 4 can be seen as a tiny cleanup too.
> > > >
> > > > g_main_context_push_thread_default (GMainContext *context)
> > > > {
> > > >   GQueue *stack;
> > > >   gboolean acquired_context;
> > > >
> > > >   acquired_context = g_main_context_acquire (context);
> > > >   g_return_if_fail (acquired_context);  <------------- [2]
> > > >
> > > >   ...
> > > > }
> > >
> > > I see.  This explains why you want to call it early.  If you're worried
> > > about that then there should also be a comment warning people that this
> > > must happen first before anything implicitly uses the thread's
> > > GMainContext.
> >
> > Sure, I can add a comment above g_main_context_push_thread_default()
> > to emphasize why it's preferred at the entry.
> >
> > >
> > > What about Marc-André's concern about the change in behavior?  Now this
> > > thread is associated with the GMainContext that isn't processed at in
> > > aio_poll().  Previously the default main context would be used.
> >
> > IIUC Paolo has answered this question (Message-ID:
> > <0faeceb2-68fa-59b0-48c3-b8e907b2a75f@redhat.com>) - if the block
> > layer (or say, the explicit aio_poll in the iothread_run) does not use
> > the GMainContext at all then it should affect nothing, and with that
> > there should have no real functional change.
> >
> 
> It's a bold claim though, iothread isn't used only by the block layer.
> It's used also by colo in qemu tree, and I used to have a branch for
> virgl rendering for example. There might be other experimental work or
> usage I missed. Furthermore, even the block layer, and its various
> dependencies, is hard to review thoroughly. But it looks like it
> doesn't rely on glib main context, I agree. But I would get prepared
> for some weird (difficult to debug) regressions eventually.

For COLO: it should be using GMainContext all the time (IIRC it's the
one who introduced this "push default gcontext" stuff to iothread
world...) so it should be fine too?  After all it's the only real user
of that so far!

For virgl rendering: I'm not sure whether there's assumption on
running them on the main thread and whether there would be race
against it if running on the iothread (AFAIU that's what the "push
default gcontext will affect"), but I would assume there's no such
requirement otherwise IMHO it should not need a dedicated iothread at
all if it needs serialization on main thread tasks...

For other "experimental work": I can't tell because I never know them.

I'm fine to drop this patch if any of us worries that this patch will
break something and I cannot guarantee anything for sure if we're even
considering experimental works... but still if any of us wants to move
on with [1] above I would still prefer to consider this patch 4.

Regards,

-- 
Peter Xu

Re: [Qemu-devel] [PATCH 0/4] iothread: create gcontext unconditionally

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 400 bytes --]

On Fri, Feb 22, 2019 at 11:14:09AM +0800, Peter Xu wrote:
> Comments welcomed.  Thanks,
> 
> Peter Xu (4):
>   iothread: replace init_done_cond with a semaphore
>   iothread: create the gcontext onconditionally
>   iothread: create main loop unconditionally
>   iothread: push gcontext earlier in the thread_fn

What is the status of this series?  Will you send another revision?

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH 0/4] iothread: create gcontext unconditionally

[Peter Xu]

On Wed, Mar 06, 2019 at 10:19:59AM +0000, Stefan Hajnoczi wrote:
> On Fri, Feb 22, 2019 at 11:14:09AM +0800, Peter Xu wrote:
> > Comments welcomed.  Thanks,
> > 
> > Peter Xu (4):
> >   iothread: replace init_done_cond with a semaphore
> >   iothread: create the gcontext onconditionally
> >   iothread: create main loop unconditionally
> >   iothread: push gcontext earlier in the thread_fn
> 
> What is the status of this series?  Will you send another revision?

I'll add some comment to patch 4 and repost, probably with another
patch to document why we can't drop aio_poll in iothread_run.

Regards,

-- 
Peter Xu