qemu-thread: allow cur_mon be per thread

We will conditionally have a wrapper layer depending on whether the host has the PTHREAD_SETNAME capability. It complicates stuff. Let's just keep the wrapper there, meanwhile we opt out the pthread_setname_np() call only. The layer can be helpful in future patches to pass data from the parent thread to the child thread.

Signed-off-by Peter Xu <[email protected]>

---
 util/qemu-thread-posix.c | 33 +++++++++++++--------------------
 1 file changed, 13 insertions(+), 20 deletions(-)

util/qemu-thread-posix.c +0 -0

index b789cf32e9..3ae96210d6 100644

Quoted 16 lines · Peter Xu

We will conditionally have a wrapper layer depending on whether the host has the PTHREAD_SETNAME capability. It complicates stuff. Let's just keep the wrapper there, meanwhile we opt out the pthread_setname_np() call only. The layer can be helpful in future patches to pass data from the parent thread to the child thread.

Signed-off-by Peter Xu <[email protected]>

---
 util/qemu-thread-posix.c | 33 +++++++++++++--------------------
 1 file changed, 13 insertions(+), 20 deletions(-)

util/qemu-thread-posix.c +0 -0

index b789cf32e9..3ae96210d6 100644

util/qemu-thread-posix.c static void __attribute__((constructor)) qemu_thread_atexit_init(void) · line 483

Quoted 2 lines

util/qemu-thread-posix.c +0 -0

@@ -482,7 +482,6 @@ static void __attribute__((constructor)) qemu_thread_atexit_init(void)
 }
 

More context:

static bool name_threads;

void qemu_thread_naming(bool enable)

{
    name_threads = enable;

#ifndef CONFIG_THREAD_SETNAME_BYTHREAD
    /* This is a debugging option, not fatal */
    if (enable) {
        fprintf(stderr, "qemu: thread naming not supported on this host\n");
    }

#endif
}

util/qemu-thread-posix.c static void __attribute__((constructor)) qemu_thread_atexit_init(void) · line 484

Quoted 2 lines

util/qemu-thread-posix.c +0 -1

-#ifdef CONFIG_PTHREAD_SETNAME_NP

Why are we using CONFIG_THREAD_SETNAME_BYTHREAD in one place, and CONFIG_PTHREAD_SETNAME_NP in another?

/me checks configure - oh:

# Hold two types of flag:
# CONFIG_THREAD_SETNAME_BYTHREAD - we've got a way of setting the name on
# a thread we have a handle to
# CONFIG_PTHREAD_SETNAME_NP - A way of doing it on a particular
# platform

even though, right now, we only either set both flags at once or leave both clear, since we don't (yet?) have any other platform-specific ways to do it.

util/qemu-thread-posix.c static void *qemu_thread_start(void *args) · line 502

Quoted 12 lines

util/qemu-thread-posix.c +3 -1

 typedef struct {
     void *(*start_routine)(void *);
     void *arg;

@@ -498,13 +497,15 @@ static void *qemu_thread_start(void *args)
     /* Attempt to set the threads name; note that this is for debug, so
      * we're not going to fail if we can't set it.
      */
-    pthread_setname_np(pthread_self(), qemu_thread_args->name);
+#ifdef CONFIG_PTHREAD_SETNAME_NP
+    if (qemu_thread_args->name) {
+        pthread_setname_np(pthread_self(), qemu_thread_args->name);

Post-patch, this (attempts to) set the thread name if a non-NULL name is present...

util/qemu-thread-posix.c void qemu_thread_create(QemuThread *thread, const char *name, · line 534

Quoted 6 lines

util/qemu-thread-posix.c +0 -5

-#ifdef CONFIG_PTHREAD_SETNAME_NP
-    if (name_threads) {
-        QemuThreadArgs *qemu_thread_args;
-        qemu_thread_args = g_new0(QemuThreadArgs, 1);
-        qemu_thread_args->name = g_strdup(name);

...but pre-patch, qemu_thread_args->name was left NULL unless name_threads was true, because someone had called qemu_thread_naming(true)...

util/qemu-thread-posix.c void qemu_thread_create(QemuThread *thread, const char *name, · line 533

Quoted 13 lines

util/qemu-thread-posix.c +2 -11

-        qemu_thread_args->start_routine = start_routine;
-        qemu_thread_args->arg = arg;
-
-        err = pthread_create(&thread->thread, &attr,
-                             qemu_thread_start, qemu_thread_args);
-    } else
-#endif
-    {
-        err = pthread_create(&thread->thread, &attr,
-                             start_routine, arg);
-    }
+    qemu_thread_args = g_new0(QemuThreadArgs, 1);
+    qemu_thread_args->name = g_strdup(name);

...so you have changed semantics - you are now unconditionally trying to set the thread name, instead of honoring qemu_thread_naming(). Do we still need qemu_thread_naming() (tied to opt debug-threads)?

You need to either fix your code to remain conditional on whether name_threads is set, or document the semantic change as intentional in the commit message.

However, the idea for refactoring to always use the shim makes sense.

cur_mon was only used in main loop so we don't really need that to be per-thread variable. Now it's possible that we have more than one thread to operate on it. Let's start to let it be per-thread variable.

In case we'll create threads within a valid cur_mon setup, we'd better let the child threads to inherit the cur_mon from parent thread too. Do that for both posix and win32 threads.

Signed-off-by Peter Xu <[email protected]>

---
 include/monitor/monitor.h   | 2 +-
 include/qemu/thread-win32.h | 1 +
 monitor.c                   | 2 +-
 stubs/monitor.c             | 2 +-
 tests/test-util-sockets.c   | 2 +-
 util/qemu-thread-posix.c    | 6 ++++++
 util/qemu-thread-win32.c    | 6 ++++++
 7 files changed, 17 insertions(+), 4 deletions(-)

Quoted 19 lines · Peter Xu

cur_mon was only used in main loop so we don't really need that to be per-thread variable. Now it's possible that we have more than one thread to operate on it. Let's start to let it be per-thread variable.

In case we'll create threads within a valid cur_mon setup, we'd better let the child threads to inherit the cur_mon from parent thread too. Do that for both posix and win32 threads.

Signed-off-by Peter Xu <[email protected]>

---
 include/monitor/monitor.h   | 2 +-
 include/qemu/thread-win32.h | 1 +
 monitor.c                   | 2 +-
 stubs/monitor.c             | 2 +-
 tests/test-util-sockets.c   | 2 +-
 util/qemu-thread-posix.c    | 6 ++++++
 util/qemu-thread-win32.c    | 6 ++++++
 7 files changed, 17 insertions(+), 4 deletions(-)

util/qemu-thread-posix.c static void *qemu_thread_start(void *args) · line 499

Quoted 5 lines

util/qemu-thread-posix.c +1 -0

@@ -494,6 +496,9 @@ static void *qemu_thread_start(void *args)
     void *(*start_routine)(void *) = qemu_thread_args->start_routine;
     void *arg = qemu_thread_args->arg;
 
+    /* Inherit the cur_mon pointer from father thread */

More typical as s/father/parent/

util/qemu-thread-win32.c static unsigned __stdcall win32_start_routine(void *arg) · line 345

Quoted 9 lines

util/qemu-thread-win32.c +3 -0

+++ b/util/qemu-thread-win32.c
 

@@ -339,6 +341,9 @@ static unsigned __stdcall win32_start_routine(void *arg)
     void *(*start_routine)(void *) = data->start_routine;
     void *thread_arg = data->arg;
 
+    /* Inherit the cur_mon pointer from father thread */
+    cur_mon = data->current_monitor;

Otherwise makes sense to me.

I agree with your analysis that the set of existing OOB commands (just 'x-oob-test') has no direct use of cur_mon. I'm a little fuzzier on whether the OOB changes can cause cur_mon to be modified by two threads in parallel (monitor_qmp_dispatch_one() is futzing around with 'cur_mon' around the call to qmp_dispatch(), and at least qmp_human_monitor_command() is also futzing around with it; is there a case where handling qmp_human_monitor_command() in the dispatch thread in parallel with more input on the main thread could break?) Thus I'm not sure whether this is needed for 2.12 to avoid a regression.

cur_mon was only used in main loop so we don't really need that to be per-thread variable. Now it's possible that we have more than one thread to operate on it. Let's start to let it be per-thread variable.

In case we'll create threads within a valid cur_mon setup, we'd better let the child threads to inherit the cur_mon from parent thread too. Do that for both posix and win32 threads.

Quoted 3 lines · Peter Xu

cur_mon was only used in main loop so we don't really need that to be per-thread variable. Now it's possible that we have more than one thread to operate on it. Let's start to let it be per-thread variable.

Trying to understand the reason for this patch:

Are there any users of per-thread cur_mon?

or

Does this patch fix a bug?

Quoted 4 lines

In case we'll create threads within a valid cur_mon setup, we'd better let the child threads to inherit the cur_mon from parent thread too. Do that for both posix and win32 threads.

Without actual users I don't like this. It sounds like "let's make it global just in case something needs it some day".

It's ugly for QEMU's thread API to know about the monitor - that's a layering violation.

If there's a legitimate need I think this patch might be necessary, but I don't see enough justification to do this yet.

Quoted 7 lines · Stefan Hajnoczi

Quoted 3 lines · Peter Xu

cur_mon was only used in main loop so we don't really need that to be per-thread variable. Now it's possible that we have more than one thread to operate on it. Let's start to let it be per-thread variable.

Trying to understand the reason for this patch:

Are there any users of per-thread cur_mon?

Currently no. But if considering future OOB-capable commands, they will modify cur_mon in monitor IOThread at least.

Quoted 7 lines · Peter Xu

Quoted 6 lines · Stefan Hajnoczi

Quoted 3 lines · Peter Xu

cur_mon was only used in main loop so we don't really need that to be per-thread variable. Now it's possible that we have more than one thread to operate on it. Let's start to let it be per-thread variable.

Trying to understand the reason for this patch:

Quoted 4 lines

Quoted 1 line

Are there any users of per-thread cur_mon?

Currently no. But if considering future OOB-capable commands, they will modify cur_mon in monitor IOThread at least.

That's fine, but it shouldn't need the inheritance part. The monitor IOThread can set cur_mon when it starts.

Yeah, the inheritance will only make sure cur_mon be initialized always with correct value just like when we are without Out-Of-Band. For example, it's still possible a thread is created within a QMP handler. If without current change, the cur_mon in the new thread would be NULL.

AFAIU even if cur_mon==NULL we should mostly be fine (e.g., error_vprintf will handle that case well). If any of you can help me confirm this, then I agree that this patch is not really needed.

If so, maybe even we don't need to setup cur_mon at entry of monitor iothread, since cur_mon is always used in the way like:

Quoted 6 lines · Peter Xu

Yeah, the inheritance will only make sure cur_mon be initialized always with correct value just like when we are without Out-Of-Band. For example, it's still possible a thread is created within a QMP handler. If without current change, the cur_mon in the new thread would be NULL.

Quoted 3 lines

AFAIU even if cur_mon==NULL we should mostly be fine (e.g., error_vprintf will handle that case well). If any of you can help me confirm this, then I agree that this patch is not really needed.

Yes, though the solution is to not use error_vprintf from auxiliary threads. :) Just make sure all the communication happens through a struct that's passed in the thread entry point, and possibly bottom halves from the auxiliary thread to the monitor iothread.

Okay. :) Thanks for confirming. Then let me repost this patch without touching the qemu-threads.

Btw, do you want me to repost the first patch separately too, or keep the code as is? I believe it depends on whether you treat that one as a cleanup or not. Thanks,

Quoted 3 lines · Peter Xu

Okay. :) Thanks for confirming. Then let me repost this patch without touching the qemu-threads.

Quoted 3 lines

Btw, do you want me to repost the first patch separately too, or keep the code as is? I believe it depends on whether you treat that one as a cleanup or not. Thanks,

The first patch is no longer necessary for your new approach, but as it is a cleanup and you've already written it, it does not hurt to still send it as a separate cleanup patch useful in its own right.