[Qemu-devel] [PATCH v4 0/5] iothread: allow to create internal iothreads

[Qemu-devel] [PATCH v4 0/5] iothread: allow to create internal iothreads

[Peter Xu]

v4:
- fix comment in patch 1 [Fam, Stefan]
- add one more patch from Stefan to fix the aio glib warning

v3:
- pick up r-bs (one missing for Fam's on last patch)
- fix patch 1 to create isolated internal container

v2:
- add one patch to provide object_get_internal_root() [Daniel]
- patch 2: use the new object_get_internal_root()
- patch 3: fix commit message, "reentrant" is wrongly used by me. it
  should be "called multiple times"; move iothread->ctx check into
  iothread_stop() [Fam]
- patch 4: add one paragraph in commit message, mention about the glib
  issue. [Fam]

When trying to support monitor OOB (out-of-band) commands, I found
that the monitor IO thread I did looks just like iothread.  It would
be best if I can use iothread directly.  However it seems that it was
mostly used by "-object iothread" before but not friendly to internal
usages.  This series tries to export essential functions to do it.

Also, I think patch 2 also fixes a bug in iothread_stop().

Please review. Thanks.

Peter Xu (4):
  qom: provide root container for internal objs
  iothread: provide helpers for internal use
  iothread: export iothread_stop()
  iothread: delay the context release to finalize

Stefan Hajnoczi (1):
  aio: fix assert when remove poll during destroy

 include/qom/object.h      | 11 +++++++++++
 include/sysemu/iothread.h |  9 +++++++++
 iothread.c                | 46 ++++++++++++++++++++++++++++++++++++----------
 qom/object.c              | 11 +++++++++++
 util/aio-posix.c          |  9 ++++++++-
 5 files changed, 75 insertions(+), 11 deletions(-)

-- 
2.13.5

[Qemu-devel] [PATCH v4 1/5] qom: provide root container for internal objs

[Peter Xu]

We have object_get_objects_root() to keep user created objects, however
no place for objects that will be used internally.  Create such a
container for internal objects.

CC: Andreas Färber <afaerber@suse.de>
CC: Markus Armbruster <armbru@redhat.com>
CC: Paolo Bonzini <pbonzini@redhat.com>
Suggested-by: Daniel P. Berrange <berrange@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 include/qom/object.h | 11 +++++++++++
 qom/object.c         | 11 +++++++++++
 2 files changed, 22 insertions(+)

diff --git a/include/qom/object.h b/include/qom/object.h
index f3e5cff37a..e0d9824415 100644
--- a/include/qom/object.h
+++ b/include/qom/object.h
@@ -1214,6 +1214,17 @@ Object *object_get_root(void);
 Object *object_get_objects_root(void);
 
 /**
+ * object_get_internal_root:
+ *
+ * Get the container object that holds internally used object
+ * instances.  Any object which is put into this container must not be
+ * user visible, and it will not be exposed in the QOM tree.
+ *
+ * Returns: the internal object container
+ */
+Object *object_get_internal_root(void);
+
+/**
  * object_get_canonical_path_component:
  *
  * Returns: The final component in the object's canonical path.  The canonical
diff --git a/qom/object.c b/qom/object.c
index 3e18537e9b..6a7bd9257b 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -1370,6 +1370,17 @@ Object *object_get_objects_root(void)
     return container_get(object_get_root(), "/objects");
 }
 
+Object *object_get_internal_root(void)
+{
+    static Object *internal_root;
+
+    if (!internal_root) {
+        internal_root = object_new("container");
+    }
+
+    return internal_root;
+}
+
 static void object_get_child_property(Object *obj, Visitor *v,
                                       const char *name, void *opaque,
                                       Error **errp)
-- 
2.13.5

Re: [Qemu-devel] [PATCH v4 1/5] qom: provide root container for internal objs

[Fam Zheng]

On Thu, 09/28 10:59, Peter Xu wrote:
> We have object_get_objects_root() to keep user created objects, however
> no place for objects that will be used internally.  Create such a
> container for internal objects.
> 
> CC: Andreas Färber <afaerber@suse.de>
> CC: Markus Armbruster <armbru@redhat.com>
> CC: Paolo Bonzini <pbonzini@redhat.com>
> Suggested-by: Daniel P. Berrange <berrange@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  include/qom/object.h | 11 +++++++++++
>  qom/object.c         | 11 +++++++++++
>  2 files changed, 22 insertions(+)
> 
> diff --git a/include/qom/object.h b/include/qom/object.h
> index f3e5cff37a..e0d9824415 100644
> --- a/include/qom/object.h
> +++ b/include/qom/object.h
> @@ -1214,6 +1214,17 @@ Object *object_get_root(void);
>  Object *object_get_objects_root(void);
>  
>  /**
> + * object_get_internal_root:
> + *
> + * Get the container object that holds internally used object
> + * instances.  Any object which is put into this container must not be
> + * user visible, and it will not be exposed in the QOM tree.
> + *
> + * Returns: the internal object container
> + */
> +Object *object_get_internal_root(void);
> +
> +/**
>   * object_get_canonical_path_component:
>   *
>   * Returns: The final component in the object's canonical path.  The canonical
> diff --git a/qom/object.c b/qom/object.c
> index 3e18537e9b..6a7bd9257b 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -1370,6 +1370,17 @@ Object *object_get_objects_root(void)
>      return container_get(object_get_root(), "/objects");
>  }
>  
> +Object *object_get_internal_root(void)
> +{
> +    static Object *internal_root;
> +
> +    if (!internal_root) {
> +        internal_root = object_new("container");
> +    }
> +
> +    return internal_root;
> +}
> +
>  static void object_get_child_property(Object *obj, Visitor *v,
>                                        const char *name, void *opaque,
>                                        Error **errp)
> -- 
> 2.13.5
> 

Reviewed-by: Fam Zheng <famz@redhat.com>

[Qemu-devel] [PATCH v4 2/5] iothread: provide helpers for internal use

[Peter Xu]

IOThread is a general framework that contains IO loop environment and a
real thread behind.  It's also good to be used internally inside qemu.
Provide some helpers for it to create iothreads to be used internally.

Put all the internal used iothreads into the internal object container.

Reviewed-by: Fam Zheng <famz@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 include/sysemu/iothread.h |  8 ++++++++
 iothread.c                | 16 ++++++++++++++++
 2 files changed, 24 insertions(+)

diff --git a/include/sysemu/iothread.h b/include/sysemu/iothread.h
index d2985b30ba..b07663f0a1 100644
--- a/include/sysemu/iothread.h
+++ b/include/sysemu/iothread.h
@@ -46,4 +46,12 @@ AioContext *iothread_get_aio_context(IOThread *iothread);
 void iothread_stop_all(void);
 GMainContext *iothread_get_g_main_context(IOThread *iothread);
 
+/*
+ * Helpers used to allocate iothreads for internal use.  These
+ * iothreads will not be seen by monitor clients when query using
+ * "query-iothreads".
+ */
+IOThread *iothread_create(const char *id, Error **errp);
+void iothread_destroy(IOThread *iothread);
+
 #endif /* IOTHREAD_H */
diff --git a/iothread.c b/iothread.c
index 44c8944dc4..33f996e561 100644
--- a/iothread.c
+++ b/iothread.c
@@ -354,3 +354,19 @@ GMainContext *iothread_get_g_main_context(IOThread *iothread)
 
     return iothread->worker_context;
 }
+
+IOThread *iothread_create(const char *id, Error **errp)
+{
+    Object *obj;
+
+    obj = object_new_with_props(TYPE_IOTHREAD,
+                                object_get_internal_root(),
+                                id, errp, NULL);
+
+    return IOTHREAD(obj);
+}
+
+void iothread_destroy(IOThread *iothread)
+{
+    object_unparent(OBJECT(iothread));
+}
-- 
2.13.5

[Qemu-devel] [PATCH v4 3/5] iothread: export iothread_stop()

[Peter Xu]

So that internal iothread users can explicitly stop one iothread without
destroying it.

Since at it, fix iothread_stop() to allow it to be called multiple
times.  Before this patch we may call iothread_stop() more than once on
single iothread, while that may not be correct since qemu_thread_join()
is not allowed to run twice.  From manual of pthread_join():

  Joining with a thread that has previously been joined results in
  undefined behavior.

Reviewed-by: Fam Zheng <famz@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 include/sysemu/iothread.h |  1 +
 iothread.c                | 24 ++++++++++++++++--------
 2 files changed, 17 insertions(+), 8 deletions(-)

diff --git a/include/sysemu/iothread.h b/include/sysemu/iothread.h
index b07663f0a1..110329b2b4 100644
--- a/include/sysemu/iothread.h
+++ b/include/sysemu/iothread.h
@@ -52,6 +52,7 @@ GMainContext *iothread_get_g_main_context(IOThread *iothread);
  * "query-iothreads".
  */
 IOThread *iothread_create(const char *id, Error **errp);
+void iothread_stop(IOThread *iothread);
 void iothread_destroy(IOThread *iothread);
 
 #endif /* IOTHREAD_H */
diff --git a/iothread.c b/iothread.c
index 33f996e561..b3c092b2d7 100644
--- a/iothread.c
+++ b/iothread.c
@@ -80,13 +80,10 @@ static void *iothread_run(void *opaque)
     return NULL;
 }
 
-static int iothread_stop(Object *object, void *opaque)
+void iothread_stop(IOThread *iothread)
 {
-    IOThread *iothread;
-
-    iothread = (IOThread *)object_dynamic_cast(object, TYPE_IOTHREAD);
-    if (!iothread || !iothread->ctx) {
-        return 0;
+    if (!iothread->ctx || iothread->stopping) {
+        return;
     }
     iothread->stopping = true;
     aio_notify(iothread->ctx);
@@ -94,6 +91,17 @@ static int iothread_stop(Object *object, void *opaque)
         g_main_loop_quit(iothread->main_loop);
     }
     qemu_thread_join(&iothread->thread);
+}
+
+static int iothread_stop_iter(Object *object, void *opaque)
+{
+    IOThread *iothread;
+
+    iothread = (IOThread *)object_dynamic_cast(object, TYPE_IOTHREAD);
+    if (!iothread) {
+        return 0;
+    }
+    iothread_stop(iothread);
     return 0;
 }
 
@@ -108,7 +116,7 @@ static void iothread_instance_finalize(Object *obj)
 {
     IOThread *iothread = IOTHREAD(obj);
 
-    iothread_stop(obj, NULL);
+    iothread_stop(iothread);
     qemu_cond_destroy(&iothread->init_done_cond);
     qemu_mutex_destroy(&iothread->init_done_lock);
     if (!iothread->ctx) {
@@ -328,7 +336,7 @@ void iothread_stop_all(void)
         aio_context_release(ctx);
     }
 
-    object_child_foreach(container, iothread_stop, NULL);
+    object_child_foreach(container, iothread_stop_iter, NULL);
 }
 
 static gpointer iothread_g_main_context_init(gpointer opaque)
-- 
2.13.5

[Qemu-devel] [PATCH v4 4/5] iothread: delay the context release to finalize

[Peter Xu]

When gcontext is used with iothread, the context will be destroyed
during iothread_stop().  That's not good since sometimes we would like
to keep the resources until iothread is destroyed, but we may want to
stop the thread before that point.

Delay the destruction of gcontext to iothread finalize.  Then we can do:

  iothread_stop(thread);
  some_cleanup_on_resources();
  iothread_destroy(thread);

We may need this patch if we want to run chardev IOs in iothreads and
hopefully clean them up correctly.  For more specific information,
please see 2b316774f6 ("qemu-char: do not operate on sources from
finalize callbacks").

Reviewed-by: Fam Zheng <famz@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 iothread.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/iothread.c b/iothread.c
index b3c092b2d7..27a4288578 100644
--- a/iothread.c
+++ b/iothread.c
@@ -71,8 +71,6 @@ static void *iothread_run(void *opaque)
             g_main_loop_unref(loop);
 
             g_main_context_pop_thread_default(iothread->worker_context);
-            g_main_context_unref(iothread->worker_context);
-            iothread->worker_context = NULL;
         }
     }
 
@@ -117,6 +115,10 @@ static void iothread_instance_finalize(Object *obj)
     IOThread *iothread = IOTHREAD(obj);
 
     iothread_stop(iothread);
+    if (iothread->worker_context) {
+        g_main_context_unref(iothread->worker_context);
+        iothread->worker_context = NULL;
+    }
     qemu_cond_destroy(&iothread->init_done_cond);
     qemu_mutex_destroy(&iothread->init_done_lock);
     if (!iothread->ctx) {
-- 
2.13.5

[Qemu-devel] [PATCH v4 5/5] aio: fix assert when remove poll during destroy

[Peter Xu]

From: Stefan Hajnoczi <stefanha@redhat.com>

After iothread is enabled internally inside QEMU with GMainContext, we
may encounter this warning when destroying the iothread:

(qemu-system-x86_64:19925): GLib-CRITICAL **: g_source_remove_poll:
 assertion '!SOURCE_DESTROYED (source)' failed

The problem is that g_source_remove_poll() does not allow to remove one
source from array if the source is detached from its owner
context. (peterx: which IMHO does not make much sense)

Fix it on QEMU side by avoid calling g_source_remove_poll() if we know
the object is during destruction, and we won't leak anything after all
since the array will be gone soon cleanly even with that fd.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
[peterx: write the commit message]
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 util/aio-posix.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/util/aio-posix.c b/util/aio-posix.c
index 2d51239ec6..5946ac09f0 100644
--- a/util/aio-posix.c
+++ b/util/aio-posix.c
@@ -223,7 +223,14 @@ void aio_set_fd_handler(AioContext *ctx,
             return;
         }
 
-        g_source_remove_poll(&ctx->source, &node->pfd);
+        /* If the GSource is in the process of being destroyed then
+         * g_source_remove_poll() causes an assertion failure.  Skip
+         * removal in that case, because glib cleans up its state during
+         * destruction anyway.
+         */
+        if (!g_source_is_destroyed(&ctx->source)) {
+            g_source_remove_poll(&ctx->source, &node->pfd);
+        }
 
         /* If the lock is held, just mark the node as deleted */
         if (qemu_lockcnt_count(&ctx->list_lock)) {
-- 
2.13.5

Re: [Qemu-devel] [PATCH v4 5/5] aio: fix assert when remove poll during destroy

[Fam Zheng]

On Thu, 09/28 10:59, Peter Xu wrote:
> From: Stefan Hajnoczi <stefanha@redhat.com>
> 
> After iothread is enabled internally inside QEMU with GMainContext, we
> may encounter this warning when destroying the iothread:
> 
> (qemu-system-x86_64:19925): GLib-CRITICAL **: g_source_remove_poll:
>  assertion '!SOURCE_DESTROYED (source)' failed
> 
> The problem is that g_source_remove_poll() does not allow to remove one
> source from array if the source is detached from its owner
> context. (peterx: which IMHO does not make much sense)
> 
> Fix it on QEMU side by avoid calling g_source_remove_poll() if we know
> the object is during destruction, and we won't leak anything after all
> since the array will be gone soon cleanly even with that fd.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> [peterx: write the commit message]
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  util/aio-posix.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/util/aio-posix.c b/util/aio-posix.c
> index 2d51239ec6..5946ac09f0 100644
> --- a/util/aio-posix.c
> +++ b/util/aio-posix.c
> @@ -223,7 +223,14 @@ void aio_set_fd_handler(AioContext *ctx,
>              return;
>          }
>  
> -        g_source_remove_poll(&ctx->source, &node->pfd);
> +        /* If the GSource is in the process of being destroyed then
> +         * g_source_remove_poll() causes an assertion failure.  Skip
> +         * removal in that case, because glib cleans up its state during
> +         * destruction anyway.
> +         */
> +        if (!g_source_is_destroyed(&ctx->source)) {
> +            g_source_remove_poll(&ctx->source, &node->pfd);
> +        }
>  
>          /* If the lock is held, just mark the node as deleted */
>          if (qemu_lockcnt_count(&ctx->list_lock)) {
> -- 
> 2.13.5
> 

Reviewed-by: Fam Zheng <famz@redhat.com>

Re: [Qemu-devel] [PATCH v4 5/5] aio: fix assert when remove poll during destroy

[Stefan Hajnoczi]

On Thu, Sep 28, 2017 at 10:59:58AM +0800, Peter Xu wrote:
> From: Stefan Hajnoczi <stefanha@redhat.com>
> 
> After iothread is enabled internally inside QEMU with GMainContext, we
> may encounter this warning when destroying the iothread:
> 
> (qemu-system-x86_64:19925): GLib-CRITICAL **: g_source_remove_poll:
>  assertion '!SOURCE_DESTROYED (source)' failed
> 
> The problem is that g_source_remove_poll() does not allow to remove one
> source from array if the source is detached from its owner
> context. (peterx: which IMHO does not make much sense)
> 
> Fix it on QEMU side by avoid calling g_source_remove_poll() if we know
> the object is during destruction, and we won't leak anything after all
> since the array will be gone soon cleanly even with that fd.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> [peterx: write the commit message]
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  util/aio-posix.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)

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

Re: [Qemu-devel] [PATCH v4 0/5] iothread: allow to create internal iothreads

[Stefan Hajnoczi]

On Thu, Sep 28, 2017 at 10:59:53AM +0800, Peter Xu wrote:
> v4:
> - fix comment in patch 1 [Fam, Stefan]
> - add one more patch from Stefan to fix the aio glib warning
> 
> v3:
> - pick up r-bs (one missing for Fam's on last patch)
> - fix patch 1 to create isolated internal container
> 
> v2:
> - add one patch to provide object_get_internal_root() [Daniel]
> - patch 2: use the new object_get_internal_root()
> - patch 3: fix commit message, "reentrant" is wrongly used by me. it
>   should be "called multiple times"; move iothread->ctx check into
>   iothread_stop() [Fam]
> - patch 4: add one paragraph in commit message, mention about the glib
>   issue. [Fam]
> 
> When trying to support monitor OOB (out-of-band) commands, I found
> that the monitor IO thread I did looks just like iothread.  It would
> be best if I can use iothread directly.  However it seems that it was
> mostly used by "-object iothread" before but not friendly to internal
> usages.  This series tries to export essential functions to do it.
> 
> Also, I think patch 2 also fixes a bug in iothread_stop().
> 
> Please review. Thanks.
> 
> Peter Xu (4):
>   qom: provide root container for internal objs
>   iothread: provide helpers for internal use
>   iothread: export iothread_stop()
>   iothread: delay the context release to finalize
> 
> Stefan Hajnoczi (1):
>   aio: fix assert when remove poll during destroy
> 
>  include/qom/object.h      | 11 +++++++++++
>  include/sysemu/iothread.h |  9 +++++++++
>  iothread.c                | 46 ++++++++++++++++++++++++++++++++++++----------
>  qom/object.c              | 11 +++++++++++
>  util/aio-posix.c          |  9 ++++++++-
>  5 files changed, 75 insertions(+), 11 deletions(-)
> 
> -- 
> 2.13.5
> 

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

Stefan