[Qemu-devel] [PATCH v2 0/2] dataplane: add query-iothreads QMP command

[Qemu-devel] [PATCH v2 0/2] dataplane: add query-iothreads QMP command

[Stefan Hajnoczi]

v2:
 * Use "thread-id" instead of "thread_id" in QAPI [eblake]
 * Avoid mutex unlock/destroy race between threads [pbonzini]

This series applies on top of "[PATCH v4 0/6] dataplane: switch to N:M
devices-per-thread model".

The new "query-iothreads" command allows QMP clients to fetch the thread IDs
for dataplane threads.  This will allow libvirt and friends to bind dataplane
threads.

The approach is similar to "query-cpus" which is also used for vcpu thread
binding (among other things).

Stefan Hajnoczi (2):
  iothread: stash thread ID away
  qmp: add query-iothreads command

 iothread.c       | 57 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 qapi-schema.json | 29 ++++++++++++++++++++++++++++
 qmp-commands.hx  | 39 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 125 insertions(+)

-- 
1.8.5.3

[Qemu-devel] [PATCH v2 1/2] iothread: stash thread ID away

[Stefan Hajnoczi]

Keep the thread ID around so we can report it via QMP.

There's only one problem: qemu_get_thread_id() (gettid() wrapper on
Linux) must be called from the thread itself.  There is no way to get
the thread ID outside the thread.

This patch uses a condvar to wait for iothread_run() to populate the
thread_id inside the thread.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 iothread.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/iothread.c b/iothread.c
index 033de7f..36cb4cf 100644
--- a/iothread.c
+++ b/iothread.c
@@ -25,7 +25,10 @@ struct IOThread {
     Object parent;
     QemuThread thread;
     AioContext *ctx;
+    QemuMutex init_done_lock;
+    QemuCond init_done_cond;    /* is thread initialization done? */
     bool stopping;
+    int thread_id;
 };
 
 #define IOTHREAD_GET_CLASS(obj) \
@@ -37,6 +40,13 @@ static void *iothread_run(void *opaque)
 {
     IOThread *iothread = opaque;
 
+    iothread->thread_id = qemu_get_thread_id();
+
+    /* Signal that initialization is done */
+    qemu_mutex_lock(&iothread->init_done_lock);
+    qemu_cond_signal(&iothread->init_done_cond);
+    qemu_mutex_unlock(&iothread->init_done_lock);
+
     while (!iothread->stopping) {
         aio_context_acquire(iothread->ctx);
         while (!iothread->stopping && aio_poll(iothread->ctx, true)) {
@@ -54,6 +64,8 @@ static void iothread_instance_finalize(Object *obj)
     iothread->stopping = true;
     aio_notify(iothread->ctx);
     qemu_thread_join(&iothread->thread);
+    qemu_cond_destroy(&iothread->init_done_cond);
+    qemu_mutex_destroy(&iothread->init_done_lock);
     aio_context_unref(iothread->ctx);
 }
 
@@ -64,11 +76,20 @@ static void iothread_complete(UserCreatable *obj, Error **errp)
     iothread->stopping = false;
     iothread->ctx = aio_context_new();
 
+    qemu_mutex_init(&iothread->init_done_lock);
+    qemu_cond_init(&iothread->init_done_cond);
+
     /* This assumes we are called from a thread with useful CPU affinity for us
      * to inherit.
      */
     qemu_thread_create(&iothread->thread, iothread_run,
                        iothread, QEMU_THREAD_JOINABLE);
+
+    /* Wait for initialization to complete */
+    qemu_mutex_lock(&iothread->init_done_lock);
+    qemu_cond_wait(&iothread->init_done_cond,
+                   &iothread->init_done_lock);
+    qemu_mutex_unlock(&iothread->init_done_lock);
 }
 
 static void iothread_class_init(ObjectClass *klass, void *class_data)
-- 
1.8.5.3

[Qemu-devel] [PATCH v2 2/2] qmp: add query-iothreads command

[Stefan Hajnoczi]

The "query-iothreads" command returns a list of information about
iothreads.  See the patch for API documentation.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 iothread.c       | 36 ++++++++++++++++++++++++++++++++++++
 qapi-schema.json | 29 +++++++++++++++++++++++++++++
 qmp-commands.hx  | 39 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 104 insertions(+)

diff --git a/iothread.c b/iothread.c
index 36cb4cf..fa87f71 100644
--- a/iothread.c
+++ b/iothread.c
@@ -17,6 +17,7 @@
 #include "qemu/thread.h"
 #include "block/aio.h"
 #include "sysemu/iothread.h"
+#include "qmp-commands.h"
 
 #define IOTHREADS_PATH "/objects"
 
@@ -142,3 +143,38 @@ AioContext *iothread_get_aio_context(IOThread *iothread)
 {
     return iothread->ctx;
 }
+
+static int query_one_iothread(Object *object, void *opaque)
+{
+    IOThreadInfoList ***prev = opaque;
+    IOThreadInfoList *elem;
+    IOThreadInfo *info;
+    IOThread *iothread;
+
+    iothread = (IOThread *)object_dynamic_cast(object, TYPE_IOTHREAD);
+    if (!iothread) {
+        return 0;
+    }
+
+    info = g_new0(IOThreadInfo, 1);
+    info->id = iothread_get_id(iothread);
+    info->thread_id = iothread->thread_id;
+
+    elem = g_new0(IOThreadInfoList, 1);
+    elem->value = info;
+    elem->next = NULL;
+
+    **prev = elem;
+    *prev = &elem->next;
+    return 0;
+}
+
+IOThreadInfoList *qmp_query_iothreads(Error **errp)
+{
+    IOThreadInfoList *head = NULL;
+    IOThreadInfoList **prev = &head;
+    Object *container = container_get(object_get_root(), IOTHREADS_PATH);
+
+    object_child_foreach(container, query_one_iothread, &prev);
+    return head;
+}
diff --git a/qapi-schema.json b/qapi-schema.json
index 473c096..8bef8ed 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -884,6 +884,35 @@
 { 'command': 'query-cpus', 'returns': ['CpuInfo'] }
 
 ##
+# @IOThreadInfo:
+#
+# Information about an iothread
+#
+# @id: the identifier of the iothread
+#
+# @thread-id: ID of the underlying host thread
+#
+# Since: 2.0
+##
+{ 'type': 'IOThreadInfo',
+  'data': {'id': 'str', 'thread-id': 'int'} }
+
+##
+# @query-iothreads:
+#
+# Returns a list of information about each iothread.
+#
+# Note this list excludes the QEMU main loop thread, which is not declared
+# using the -object iothread command-line option.  It is always the main thread
+# of the process.
+#
+# Returns: a list of @IOThreadInfo for each iothread
+#
+# Since: 2.0
+##
+{ 'command': 'query-iothreads', 'returns': ['IOThreadInfo'] }
+
+##
 # @BlockDeviceInfo:
 #
 # Information about the backing device for a block device.
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 8a0e832..ba16b61 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -2304,6 +2304,45 @@ EQMP
     },
 
 SQMP
+query-iothreads
+---------------
+
+Returns a list of information about each iothread.
+
+Note this list excludes the QEMU main loop thread, which is not declared
+using the -object iothread command-line option.  It is always the main thread
+of the process.
+
+Return a json-array. Each iothread is represented by a json-object, which contains:
+
+- "id": name of iothread (json-str)
+- "thread_id": ID of the underlying host thread (json-int)
+
+Example:
+
+-> { "execute": "query-iothreads" }
+<- {
+      "return":[
+         {
+            "id":"iothread0",
+            "thread_id":3134
+         },
+         {
+            "id":"iothread1",
+            "thread_id":3135
+         }
+      ]
+   }
+
+EQMP
+
+    {
+        .name       = "query-iothreads",
+        .args_type  = "",
+        .mhandler.cmd_new = qmp_marshal_input_query_iothreads,
+    },
+
+SQMP
 query-pci
 ---------
 
-- 
1.8.5.3

Re: [Qemu-devel] [PATCH v2 1/2] iothread: stash thread ID away

[Eric Blake]

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

On 02/25/2014 10:19 AM, Stefan Hajnoczi wrote:
> Keep the thread ID around so we can report it via QMP.
> 
> There's only one problem: qemu_get_thread_id() (gettid() wrapper on
> Linux) must be called from the thread itself.  There is no way to get
> the thread ID outside the thread.
> 
> This patch uses a condvar to wait for iothread_run() to populate the
> thread_id inside the thread.
> 

> +
> +    /* Wait for initialization to complete */
> +    qemu_mutex_lock(&iothread->init_done_lock);
> +    qemu_cond_wait(&iothread->init_done_cond,
> +                   &iothread->init_done_lock);
> +    qemu_mutex_unlock(&iothread->init_done_lock);

Generally, cond_wait needs to be done in a loop, where you also check
the condition (in this case, that thread_id was actually set) - a
pthread implementation is allowed to do spurious wakeups even if no
other thread has managed to change the condition that you were waiting for.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v2 2/2] qmp: add query-iothreads command

[Eric Blake]

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

On 02/25/2014 10:19 AM, Stefan Hajnoczi wrote:
> The "query-iothreads" command returns a list of information about
> iothreads.  See the patch for API documentation.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  iothread.c       | 36 ++++++++++++++++++++++++++++++++++++
>  qapi-schema.json | 29 +++++++++++++++++++++++++++++
>  qmp-commands.hx  | 39 +++++++++++++++++++++++++++++++++++++++
>  3 files changed, 104 insertions(+)

> +
> +Return a json-array. Each iothread is represented by a json-object, which contains:
> +
> +- "id": name of iothread (json-str)
> +- "thread_id": ID of the underlying host thread (json-int)

s/thread_id/thread-id/

> +
> +Example:
> +
> +-> { "execute": "query-iothreads" }
> +<- {
> +      "return":[
> +         {
> +            "id":"iothread0",
> +            "thread_id":3134
> +         },
> +         {
> +            "id":"iothread1",
> +            "thread_id":3135

twice more.

With that fixed,
Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v2 1/2] iothread: stash thread ID away

[Stefan Hajnoczi]

On Tue, Feb 25, 2014 at 10:33:49AM -0700, Eric Blake wrote:
> On 02/25/2014 10:19 AM, Stefan Hajnoczi wrote:
> > Keep the thread ID around so we can report it via QMP.
> > 
> > There's only one problem: qemu_get_thread_id() (gettid() wrapper on
> > Linux) must be called from the thread itself.  There is no way to get
> > the thread ID outside the thread.
> > 
> > This patch uses a condvar to wait for iothread_run() to populate the
> > thread_id inside the thread.
> > 
> 
> > +
> > +    /* Wait for initialization to complete */
> > +    qemu_mutex_lock(&iothread->init_done_lock);
> > +    qemu_cond_wait(&iothread->init_done_cond,
> > +                   &iothread->init_done_lock);
> > +    qemu_mutex_unlock(&iothread->init_done_lock);
> 
> Generally, cond_wait needs to be done in a loop, where you also check
> the condition (in this case, that thread_id was actually set) - a
> pthread implementation is allowed to do spurious wakeups even if no
> other thread has managed to change the condition that you were waiting for.

You are right, POSIX is explicit about this:

"When using condition variables there is always a Boolean predicate
involving shared variables associated with each condition wait that is
true if the thread should proceed. Spurious wakeups from the
pthread_cond_timedwait() or pthread_cond_wait() functions may occur.
Since the return from pthread_cond_timedwait() or pthread_cond_wait()
does not imply anything about the value of this predicate, the predicate
should be re-evaluated upon such return."

I figured no other thread issues pthread_cond_signal/broadcast so
spurious wakeups cannot occur but I was wrong.

Stefan