All of lore.kernel.org
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH v2 0/5] iothread: create gcontext unconditionally
@ 2019-03-06 11:55 Peter Xu
  2019-03-06 11:55 ` [Qemu-devel] [PATCH v2 1/5] iothread: replace init_done_cond with a semaphore Peter Xu
                   ` (5 more replies)
  0 siblings, 6 replies; 11+ messages in thread
From: Peter Xu @ 2019-03-06 11:55 UTC (permalink / raw)
  To: qemu-devel
  Cc: Marc-André Lureau, Eric Blake, Stefan Hajnoczi, peterx,
	Paolo Bonzini

v2:
- add comment in patch 4
- add another patch to comment why we need explicit aio_poll() in
  iothread_run loop

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 (5):
  iothread: replace init_done_cond with a semaphore
  iothread: create the gcontext unconditionally
  iothread: create main loop unconditionally
  iothread: push gcontext earlier in the thread_fn
  iothread: document about why we need explicit aio_poll()

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

-- 
2.17.1

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [Qemu-devel] [PATCH v2 1/5] iothread: replace init_done_cond with a semaphore
  2019-03-06 11:55 [Qemu-devel] [PATCH v2 0/5] iothread: create gcontext unconditionally Peter Xu
@ 2019-03-06 11:55 ` Peter Xu
  2019-03-07  3:09   ` Peter Xu
  2019-03-06 11:55 ` [Qemu-devel] [PATCH v2 2/5] iothread: create the gcontext unconditionally Peter Xu
                   ` (4 subsequent siblings)
  5 siblings, 1 reply; 11+ messages in thread
From: Peter Xu @ 2019-03-06 11:55 UTC (permalink / raw)
  To: qemu-devel
  Cc: Marc-André Lureau, Eric Blake, Stefan Hajnoczi, peterx,
	Paolo Bonzini

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

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [Qemu-devel] [PATCH v2 2/5] iothread: create the gcontext unconditionally
  2019-03-06 11:55 [Qemu-devel] [PATCH v2 0/5] iothread: create gcontext unconditionally Peter Xu
  2019-03-06 11:55 ` [Qemu-devel] [PATCH v2 1/5] iothread: replace init_done_cond with a semaphore Peter Xu
@ 2019-03-06 11:55 ` Peter Xu
  2019-03-06 11:55 ` [Qemu-devel] [PATCH v2 3/5] iothread: create main loop unconditionally Peter Xu
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 11+ messages in thread
From: Peter Xu @ 2019-03-06 11:55 UTC (permalink / raw)
  To: qemu-devel
  Cc: Marc-André Lureau, Eric Blake, Stefan Hajnoczi, peterx,
	Paolo Bonzini

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.

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
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

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [Qemu-devel] [PATCH v2 3/5] iothread: create main loop unconditionally
  2019-03-06 11:55 [Qemu-devel] [PATCH v2 0/5] iothread: create gcontext unconditionally Peter Xu
  2019-03-06 11:55 ` [Qemu-devel] [PATCH v2 1/5] iothread: replace init_done_cond with a semaphore Peter Xu
  2019-03-06 11:55 ` [Qemu-devel] [PATCH v2 2/5] iothread: create the gcontext unconditionally Peter Xu
@ 2019-03-06 11:55 ` Peter Xu
  2019-03-06 11:55 ` [Qemu-devel] [PATCH v2 4/5] iothread: push gcontext earlier in the thread_fn Peter Xu
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 11+ messages in thread
From: Peter Xu @ 2019-03-06 11:55 UTC (permalink / raw)
  To: qemu-devel
  Cc: Marc-André Lureau, Eric Blake, Stefan Hajnoczi, peterx,
	Paolo Bonzini

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

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
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

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [Qemu-devel] [PATCH v2 4/5] iothread: push gcontext earlier in the thread_fn
  2019-03-06 11:55 [Qemu-devel] [PATCH v2 0/5] iothread: create gcontext unconditionally Peter Xu
                   ` (2 preceding siblings ...)
  2019-03-06 11:55 ` [Qemu-devel] [PATCH v2 3/5] iothread: create main loop unconditionally Peter Xu
@ 2019-03-06 11:55 ` Peter Xu
  2019-03-07 14:40   ` Stefan Hajnoczi
  2019-03-06 11:55 ` [Qemu-devel] [PATCH v2 5/5] iothread: document about why we need explicit aio_poll() Peter Xu
  2019-03-08 10:21 ` [Qemu-devel] [PATCH v2 0/5] iothread: create gcontext unconditionally Stefan Hajnoczi
  5 siblings, 1 reply; 11+ messages in thread
From: Peter Xu @ 2019-03-06 11:55 UTC (permalink / raw)
  To: qemu-devel
  Cc: Marc-André Lureau, Eric Blake, Stefan Hajnoczi, peterx,
	Paolo Bonzini

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 | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/iothread.c b/iothread.c
index 9abdbace66..045825a348 100644
--- a/iothread.c
+++ b/iothread.c
@@ -53,7 +53,12 @@ static void *iothread_run(void *opaque)
     IOThread *iothread = opaque;
 
     rcu_register_thread();
-
+    /*
+     * We should do this as soon as we enter the thread, because the
+     * function will silently fail if it fails to acquire the
+     * gcontext.
+     */
+    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 +71,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

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [Qemu-devel] [PATCH v2 5/5] iothread: document about why we need explicit aio_poll()
  2019-03-06 11:55 [Qemu-devel] [PATCH v2 0/5] iothread: create gcontext unconditionally Peter Xu
                   ` (3 preceding siblings ...)
  2019-03-06 11:55 ` [Qemu-devel] [PATCH v2 4/5] iothread: push gcontext earlier in the thread_fn Peter Xu
@ 2019-03-06 11:55 ` Peter Xu
  2019-03-06 12:59   ` Marc-André Lureau
  2019-03-08 10:21 ` [Qemu-devel] [PATCH v2 0/5] iothread: create gcontext unconditionally Stefan Hajnoczi
  5 siblings, 1 reply; 11+ messages in thread
From: Peter Xu @ 2019-03-06 11:55 UTC (permalink / raw)
  To: qemu-devel
  Cc: Marc-André Lureau, Eric Blake, Stefan Hajnoczi, peterx,
	Paolo Bonzini

After consulting Paolo I know why we'd better keep the explicit
aio_poll() in iothread_run().  Document it directly into the code so
that future readers will know the answer from day one.

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

diff --git a/iothread.c b/iothread.c
index 045825a348..14e9f3779e 100644
--- a/iothread.c
+++ b/iothread.c
@@ -64,6 +64,15 @@ static void *iothread_run(void *opaque)
     qemu_sem_post(&iothread->init_done_sem);
 
     while (iothread->running) {
+        /*
+         * Note: from functional-wise the g_main_loop_run() below can
+         * already cover the aio_poll() events, but we can't run the
+         * main loop unconditionally because explicit aio_poll() here
+         * is faster than g_main_loop_run() when we do not need the
+         * gcontext at all (e.g., pure block layer iothreads).  In
+         * other words, when we want to run the gcontext with the
+         * iothread we need to pay some performance for functionality.
+         */
         aio_poll(iothread->ctx, true);
 
         /*
-- 
2.17.1

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [Qemu-devel] [PATCH v2 5/5] iothread: document about why we need explicit aio_poll()
  2019-03-06 11:55 ` [Qemu-devel] [PATCH v2 5/5] iothread: document about why we need explicit aio_poll() Peter Xu
@ 2019-03-06 12:59   ` Marc-André Lureau
  0 siblings, 0 replies; 11+ messages in thread
From: Marc-André Lureau @ 2019-03-06 12:59 UTC (permalink / raw)
  To: Peter Xu; +Cc: QEMU, Stefan Hajnoczi, Paolo Bonzini

On Wed, Mar 6, 2019 at 1:05 PM Peter Xu <peterx@redhat.com> wrote:
>
> After consulting Paolo I know why we'd better keep the explicit
> aio_poll() in iothread_run().  Document it directly into the code so
> that future readers will know the answer from day one.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>

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

> ---
>  iothread.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/iothread.c b/iothread.c
> index 045825a348..14e9f3779e 100644
> --- a/iothread.c
> +++ b/iothread.c
> @@ -64,6 +64,15 @@ static void *iothread_run(void *opaque)
>      qemu_sem_post(&iothread->init_done_sem);
>
>      while (iothread->running) {
> +        /*
> +         * Note: from functional-wise the g_main_loop_run() below can
> +         * already cover the aio_poll() events, but we can't run the
> +         * main loop unconditionally because explicit aio_poll() here
> +         * is faster than g_main_loop_run() when we do not need the
> +         * gcontext at all (e.g., pure block layer iothreads).  In
> +         * other words, when we want to run the gcontext with the
> +         * iothread we need to pay some performance for functionality.
> +         */
>          aio_poll(iothread->ctx, true);
>
>          /*
> --
> 2.17.1
>
>


-- 
Marc-André Lureau

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Qemu-devel] [PATCH v2 1/5] iothread: replace init_done_cond with a semaphore
  2019-03-06 11:55 ` [Qemu-devel] [PATCH v2 1/5] iothread: replace init_done_cond with a semaphore Peter Xu
@ 2019-03-07  3:09   ` Peter Xu
  0 siblings, 0 replies; 11+ messages in thread
From: Peter Xu @ 2019-03-07  3:09 UTC (permalink / raw)
  To: qemu-devel
  Cc: Marc-André Lureau, Eric Blake, Stefan Hajnoczi, Paolo Bonzini

On Wed, Mar 06, 2019 at 07:55:28PM +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>

Sorry I've definitely lost Stefan's r-b for this patch.  It's the same
patch as the one I posted in v1.

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

Regards,

-- 
Peter Xu

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Qemu-devel] [PATCH v2 4/5] iothread: push gcontext earlier in the thread_fn
  2019-03-06 11:55 ` [Qemu-devel] [PATCH v2 4/5] iothread: push gcontext earlier in the thread_fn Peter Xu
@ 2019-03-07 14:40   ` Stefan Hajnoczi
  2019-03-08  3:05     ` Peter Xu
  0 siblings, 1 reply; 11+ messages in thread
From: Stefan Hajnoczi @ 2019-03-07 14:40 UTC (permalink / raw)
  To: Peter Xu
  Cc: qemu-devel, Marc-André Lureau, Stefan Hajnoczi, Paolo Bonzini

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

On Wed, Mar 06, 2019 at 07:55:31PM +0800, Peter Xu wrote:
> +    /*
> +     * We should do this as soon as we enter the thread, because the
> +     * function will silently fail if it fails to acquire the
> +     * gcontext.
> +     */
> +    g_main_context_push_thread_default(iothread->worker_context);

I have a hard time understanding this comment.  The mention of how it
fails makes me think "we'll never find out about failures anyway, so how
does it help to call this early?".

I suggest sticking to the point that this function must always be called
first:

  /*
   * g_main_context_push_thread_default() must be called before anything
   * in this new thread uses glib.
   */

Now people will think before moving this function call.

Stefan

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Qemu-devel] [PATCH v2 4/5] iothread: push gcontext earlier in the thread_fn
  2019-03-07 14:40   ` Stefan Hajnoczi
@ 2019-03-08  3:05     ` Peter Xu
  0 siblings, 0 replies; 11+ messages in thread
From: Peter Xu @ 2019-03-08  3:05 UTC (permalink / raw)
  To: Stefan Hajnoczi
  Cc: qemu-devel, Marc-André Lureau, Stefan Hajnoczi, Paolo Bonzini

On Thu, Mar 07, 2019 at 02:40:39PM +0000, Stefan Hajnoczi wrote:
> On Wed, Mar 06, 2019 at 07:55:31PM +0800, Peter Xu wrote:
> > +    /*
> > +     * We should do this as soon as we enter the thread, because the
> > +     * function will silently fail if it fails to acquire the
> > +     * gcontext.
> > +     */
> > +    g_main_context_push_thread_default(iothread->worker_context);
> 
> I have a hard time understanding this comment.  The mention of how it
> fails makes me think "we'll never find out about failures anyway, so how
> does it help to call this early?".
> 
> I suggest sticking to the point that this function must always be called
> first:
> 
>   /*
>    * g_main_context_push_thread_default() must be called before anything
>    * in this new thread uses glib.
>    */
> 
> Now people will think before moving this function call.

Sorry to be confusing; this looks good to me.  Please let me know if
you want me to repost this patch alone or the patchset with the change.

Thanks,

-- 
Peter Xu

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Qemu-devel] [PATCH v2 0/5] iothread: create gcontext unconditionally
  2019-03-06 11:55 [Qemu-devel] [PATCH v2 0/5] iothread: create gcontext unconditionally Peter Xu
                   ` (4 preceding siblings ...)
  2019-03-06 11:55 ` [Qemu-devel] [PATCH v2 5/5] iothread: document about why we need explicit aio_poll() Peter Xu
@ 2019-03-08 10:21 ` Stefan Hajnoczi
  5 siblings, 0 replies; 11+ messages in thread
From: Stefan Hajnoczi @ 2019-03-08 10:21 UTC (permalink / raw)
  To: Peter Xu; +Cc: qemu-devel, Marc-André Lureau, Eric Blake, Paolo Bonzini

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

On Wed, Mar 06, 2019 at 07:55:27PM +0800, Peter Xu wrote:
> v2:
> - add comment in patch 4
> - add another patch to comment why we need explicit aio_poll() in
>   iothread_run loop
> 
> 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 (5):
>   iothread: replace init_done_cond with a semaphore
>   iothread: create the gcontext unconditionally
>   iothread: create main loop unconditionally
>   iothread: push gcontext earlier in the thread_fn
>   iothread: document about why we need explicit aio_poll()
> 
>  include/sysemu/iothread.h |  5 +--
>  iothread.c                | 91 +++++++++++++++++++--------------------
>  2 files changed, 46 insertions(+), 50 deletions(-)
> 
> -- 
> 2.17.1
> 

I made the comment tweak discussed in this thread.

Thanks, applied to my block tree:
https://github.com/stefanha/qemu/commits/block

Stefan

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2019-03-08 10:22 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-03-06 11:55 [Qemu-devel] [PATCH v2 0/5] iothread: create gcontext unconditionally Peter Xu
2019-03-06 11:55 ` [Qemu-devel] [PATCH v2 1/5] iothread: replace init_done_cond with a semaphore Peter Xu
2019-03-07  3:09   ` Peter Xu
2019-03-06 11:55 ` [Qemu-devel] [PATCH v2 2/5] iothread: create the gcontext unconditionally Peter Xu
2019-03-06 11:55 ` [Qemu-devel] [PATCH v2 3/5] iothread: create main loop unconditionally Peter Xu
2019-03-06 11:55 ` [Qemu-devel] [PATCH v2 4/5] iothread: push gcontext earlier in the thread_fn Peter Xu
2019-03-07 14:40   ` Stefan Hajnoczi
2019-03-08  3:05     ` Peter Xu
2019-03-06 11:55 ` [Qemu-devel] [PATCH v2 5/5] iothread: document about why we need explicit aio_poll() Peter Xu
2019-03-06 12:59   ` Marc-André Lureau
2019-03-08 10:21 ` [Qemu-devel] [PATCH v2 0/5] iothread: create gcontext unconditionally Stefan Hajnoczi

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.