* [Qemu-devel] [PATCH 0/2] qemu-thread: allow cur_mon be per thread @ 2018-04-10 12:49 Peter Xu 2018-04-10 12:49 ` [Qemu-devel] [PATCH 1/2] qemu-thread: always keep the posix wrapper layer Peter Xu 2018-04-10 12:49 ` [Qemu-devel] [PATCH 2/2] qemu-thread: let cur_mon be per-thread Peter Xu 0 siblings, 2 replies; 15+ messages in thread From: Peter Xu @ 2018-04-10 12:49 UTC (permalink / raw) To: qemu-devel Cc: Paolo Bonzini, Alex Bennée, Fam Zheng, peterx, Eric Blake, Marc-André Lureau, Markus Armbruster, Stefan Hajnoczi, Dr . David Alan Gilbert This should be for 2.13. But I'd like to get early review comments too if there is any. Now cur_mon is still only be accessed by the main thread. So we don't even need per-thread cur_mon. However after more commands become OOB compatible, cur_mon can be accessed by more than main thread now. The major user should be the monitor IOThread. This series tries to let cur_mon be per-thread, so that we can be well-prepared. The first patch is a cleanup. The second patch does the idea. Any early review comments would be welcomed. Thanks, Peter Xu (2): qemu-thread: always keep the posix wrapper layer qemu-thread: let cur_mon be per-thread 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 | 39 +++++++++++++++++++-------------------- util/qemu-thread-win32.c | 6 ++++++ 7 files changed, 30 insertions(+), 24 deletions(-) -- 2.14.3 ^ permalink raw reply [flat|nested] 15+ messages in thread
* [Qemu-devel] [PATCH 1/2] qemu-thread: always keep the posix wrapper layer 2018-04-10 12:49 [Qemu-devel] [PATCH 0/2] qemu-thread: allow cur_mon be per thread Peter Xu @ 2018-04-10 12:49 ` Peter Xu 2018-04-10 13:35 ` Eric Blake 2018-04-10 12:49 ` [Qemu-devel] [PATCH 2/2] qemu-thread: let cur_mon be per-thread Peter Xu 1 sibling, 1 reply; 15+ messages in thread From: Peter Xu @ 2018-04-10 12:49 UTC (permalink / raw) To: qemu-devel Cc: Paolo Bonzini, Alex Bennée, Fam Zheng, peterx, Eric Blake, Marc-André Lureau, Markus Armbruster, Stefan Hajnoczi, Dr . David Alan Gilbert 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 <peterx@redhat.com> --- util/qemu-thread-posix.c | 33 +++++++++++++-------------------- 1 file changed, 13 insertions(+), 20 deletions(-) diff --git a/util/qemu-thread-posix.c b/util/qemu-thread-posix.c index b789cf32e9..3ae96210d6 100644 --- a/util/qemu-thread-posix.c +++ b/util/qemu-thread-posix.c @@ -482,7 +482,6 @@ static void __attribute__((constructor)) qemu_thread_atexit_init(void) } -#ifdef CONFIG_PTHREAD_SETNAME_NP 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); + } +#endif g_free(qemu_thread_args->name); g_free(qemu_thread_args); return start_routine(arg); } -#endif - void qemu_thread_create(QemuThread *thread, const char *name, void *(*start_routine)(void*), @@ -513,6 +514,7 @@ void qemu_thread_create(QemuThread *thread, const char *name, sigset_t set, oldset; int err; pthread_attr_t attr; + QemuThreadArgs *qemu_thread_args; err = pthread_attr_init(&attr); if (err) { @@ -527,22 +529,13 @@ void qemu_thread_create(QemuThread *thread, const char *name, sigfillset(&set); pthread_sigmask(SIG_SETMASK, &set, &oldset); -#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); - 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); + qemu_thread_args->start_routine = start_routine; + qemu_thread_args->arg = arg; + + err = pthread_create(&thread->thread, &attr, + qemu_thread_start, qemu_thread_args); if (err) error_exit(err, __func__); -- 2.14.3 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] qemu-thread: always keep the posix wrapper layer 2018-04-10 12:49 ` [Qemu-devel] [PATCH 1/2] qemu-thread: always keep the posix wrapper layer Peter Xu @ 2018-04-10 13:35 ` Eric Blake 2018-04-11 3:18 ` Peter Xu 0 siblings, 1 reply; 15+ messages in thread From: Eric Blake @ 2018-04-10 13:35 UTC (permalink / raw) To: Peter Xu, qemu-devel Cc: Paolo Bonzini, Alex Bennée, Fam Zheng, Marc-André Lureau, Markus Armbruster, Stefan Hajnoczi, Dr . David Alan Gilbert [-- Attachment #1: Type: text/plain, Size: 3852 bytes --] On 04/10/2018 07:49 AM, Peter Xu wrote: > 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 <peterx@redhat.com> > --- > util/qemu-thread-posix.c | 33 +++++++++++++-------------------- > 1 file changed, 13 insertions(+), 20 deletions(-) > > diff --git a/util/qemu-thread-posix.c b/util/qemu-thread-posix.c > index b789cf32e9..3ae96210d6 100644 > --- a/util/qemu-thread-posix.c > +++ b/util/qemu-thread-posix.c > @@ -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 } > > -#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. > 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... > > -#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)... > - 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. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 619 bytes --] ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] qemu-thread: always keep the posix wrapper layer 2018-04-10 13:35 ` Eric Blake @ 2018-04-11 3:18 ` Peter Xu 0 siblings, 0 replies; 15+ messages in thread From: Peter Xu @ 2018-04-11 3:18 UTC (permalink / raw) To: Eric Blake Cc: qemu-devel, Paolo Bonzini, Alex Bennée, Fam Zheng, Marc-André Lureau, Markus Armbruster, Stefan Hajnoczi, Dr . David Alan Gilbert On Tue, Apr 10, 2018 at 08:35:40AM -0500, Eric Blake wrote: > On 04/10/2018 07:49 AM, Peter Xu wrote: > > 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 <peterx@redhat.com> > > --- > > util/qemu-thread-posix.c | 33 +++++++++++++-------------------- > > 1 file changed, 13 insertions(+), 20 deletions(-) > > > > diff --git a/util/qemu-thread-posix.c b/util/qemu-thread-posix.c > > index b789cf32e9..3ae96210d6 100644 > > --- a/util/qemu-thread-posix.c > > +++ b/util/qemu-thread-posix.c > > @@ -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 > } > > > > > > -#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. It seems so. I'm not sure whether they could be useful in the future and I'm fine with them, hence I keep it as is. > > > 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... > > > > > > -#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)... > > > - 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. Indeed, thanks for catching that. What I really wanted is probably this: static void *qemu_thread_start(void *args) { ... #ifdef CONFIG_PTHREAD_SETNAME_NP if (name_threads && qemu_thread_args->name) { pthread_setname_np(pthread_self(), qemu_thread_args->name); } #endif ... } I'll fix that up in next version. > > However, the idea for refactoring to always use the shim makes sense. Thanks, -- Peter Xu ^ permalink raw reply [flat|nested] 15+ messages in thread
* [Qemu-devel] [PATCH 2/2] qemu-thread: let cur_mon be per-thread 2018-04-10 12:49 [Qemu-devel] [PATCH 0/2] qemu-thread: allow cur_mon be per thread Peter Xu 2018-04-10 12:49 ` [Qemu-devel] [PATCH 1/2] qemu-thread: always keep the posix wrapper layer Peter Xu @ 2018-04-10 12:49 ` Peter Xu 2018-04-10 13:54 ` Eric Blake 2018-04-11 1:45 ` Stefan Hajnoczi 1 sibling, 2 replies; 15+ messages in thread From: Peter Xu @ 2018-04-10 12:49 UTC (permalink / raw) To: qemu-devel Cc: Paolo Bonzini, Alex Bennée, Fam Zheng, peterx, Eric Blake, Marc-André Lureau, Markus Armbruster, Stefan Hajnoczi, Dr . David Alan Gilbert 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 <peterx@redhat.com> --- 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(-) diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h index d6ab70cae2..2ef5e04b37 100644 --- a/include/monitor/monitor.h +++ b/include/monitor/monitor.h @@ -6,7 +6,7 @@ #include "qapi/qapi-types-misc.h" #include "qemu/readline.h" -extern Monitor *cur_mon; +extern __thread Monitor *cur_mon; /* flags for monitor_init */ /* 0x01 unused */ diff --git a/include/qemu/thread-win32.h b/include/qemu/thread-win32.h index 3a05e3b3aa..f4d4cd96a1 100644 --- a/include/qemu/thread-win32.h +++ b/include/qemu/thread-win32.h @@ -39,6 +39,7 @@ typedef struct QemuThreadData QemuThreadData; struct QemuThread { QemuThreadData *data; unsigned tid; + Monitor *current_monitor; }; /* Only valid for joinable threads. */ diff --git a/monitor.c b/monitor.c index 51f4cf480f..5035e42364 100644 --- a/monitor.c +++ b/monitor.c @@ -266,7 +266,7 @@ static mon_cmd_t info_cmds[]; QmpCommandList qmp_commands, qmp_cap_negotiation_commands; -Monitor *cur_mon; +__thread Monitor *cur_mon; static QEMUClockType event_clock_type = QEMU_CLOCK_REALTIME; diff --git a/stubs/monitor.c b/stubs/monitor.c index e018c8f594..3890771bb5 100644 --- a/stubs/monitor.c +++ b/stubs/monitor.c @@ -3,7 +3,7 @@ #include "qemu-common.h" #include "monitor/monitor.h" -Monitor *cur_mon = NULL; +__thread Monitor *cur_mon; int monitor_get_fd(Monitor *mon, const char *name, Error **errp) { diff --git a/tests/test-util-sockets.c b/tests/test-util-sockets.c index acadd85e8f..6195a3ac36 100644 --- a/tests/test-util-sockets.c +++ b/tests/test-util-sockets.c @@ -69,7 +69,7 @@ int monitor_get_fd(Monitor *mon, const char *fdname, Error **errp) * stubs/monitor.c is defined, to make sure monitor.o is discarded * otherwise we get duplicate syms at link time. */ -Monitor *cur_mon; +__thread Monitor *cur_mon; void monitor_init(Chardev *chr, int flags) {} diff --git a/util/qemu-thread-posix.c b/util/qemu-thread-posix.c index 3ae96210d6..8d13da1b09 100644 --- a/util/qemu-thread-posix.c +++ b/util/qemu-thread-posix.c @@ -14,6 +14,7 @@ #include "qemu/thread.h" #include "qemu/atomic.h" #include "qemu/notify.h" +#include "monitor/monitor.h" #include "trace.h" static bool name_threads; @@ -486,6 +487,7 @@ typedef struct { void *(*start_routine)(void *); void *arg; char *name; + Monitor *current_monitor; } QemuThreadArgs; static void *qemu_thread_start(void *args) @@ -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 */ + cur_mon = qemu_thread_args->current_monitor; + /* 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. */ @@ -533,6 +538,7 @@ void qemu_thread_create(QemuThread *thread, const char *name, qemu_thread_args->name = g_strdup(name); qemu_thread_args->start_routine = start_routine; qemu_thread_args->arg = arg; + qemu_thread_args->current_monitor = cur_mon; err = pthread_create(&thread->thread, &attr, qemu_thread_start, qemu_thread_args); diff --git a/util/qemu-thread-win32.c b/util/qemu-thread-win32.c index ab60c0d557..b5197dbc78 100644 --- a/util/qemu-thread-win32.c +++ b/util/qemu-thread-win32.c @@ -19,6 +19,7 @@ #include "qemu-common.h" #include "qemu/thread.h" #include "qemu/notify.h" +#include "monitor/monitor.h" #include "trace.h" #include <process.h> @@ -298,6 +299,7 @@ struct QemuThreadData { void *arg; short mode; NotifierList exit; + Monitor *current_monitor; /* Only used for joinable threads. */ bool exited; @@ -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; + qemu_thread_data = data; qemu_thread_exit(start_routine(thread_arg)); abort(); @@ -401,6 +406,7 @@ void qemu_thread_create(QemuThread *thread, const char *name, data->arg = arg; data->mode = mode; data->exited = false; + data->current_monitor = cur_mon; notifier_list_init(&data->exit); if (data->mode != QEMU_THREAD_DETACHED) { -- 2.14.3 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] qemu-thread: let cur_mon be per-thread 2018-04-10 12:49 ` [Qemu-devel] [PATCH 2/2] qemu-thread: let cur_mon be per-thread Peter Xu @ 2018-04-10 13:54 ` Eric Blake 2018-04-11 3:31 ` Peter Xu 2018-04-11 1:45 ` Stefan Hajnoczi 1 sibling, 1 reply; 15+ messages in thread From: Eric Blake @ 2018-04-10 13:54 UTC (permalink / raw) To: Peter Xu, qemu-devel Cc: Paolo Bonzini, Alex Bennée, Fam Zheng, Marc-André Lureau, Markus Armbruster, Stefan Hajnoczi, Dr . David Alan Gilbert [-- Attachment #1: Type: text/plain, Size: 2248 bytes --] On 04/10/2018 07:49 AM, Peter Xu wrote: > 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 <peterx@redhat.com> > --- > 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(-) > > @@ -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/ > +++ 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. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 619 bytes --] ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] qemu-thread: let cur_mon be per-thread 2018-04-10 13:54 ` Eric Blake @ 2018-04-11 3:31 ` Peter Xu 0 siblings, 0 replies; 15+ messages in thread From: Peter Xu @ 2018-04-11 3:31 UTC (permalink / raw) To: Eric Blake Cc: qemu-devel, Paolo Bonzini, Alex Bennée, Fam Zheng, Marc-André Lureau, Markus Armbruster, Stefan Hajnoczi, Dr . David Alan Gilbert On Tue, Apr 10, 2018 at 08:54:31AM -0500, Eric Blake wrote: > On 04/10/2018 07:49 AM, Peter Xu wrote: > > 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 <peterx@redhat.com> > > --- > > 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(-) > > > > > @@ -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/ Fixed. > > > +++ 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. Could I ask what's the "more input on the main thread"? AFAIU, if we don't take x-oob-test into account, then still only the main thread will touch (not only modify, even read) the cur_mon variable. And as long as that's true, we are keeping the old behavior as when we are without Out-Of-Band, then IMHO we'll be fine. qmp_human_monitor_command() is only one QMP command handler, now it can only be run within main thread. So even we can have the stack like monitor_qmp_dispatch_one (which modifies cur_mon) calls qmp_human_monitor_command (which modifies cur_mon again), still we'll be fine too as long as they are in the same thread, just like before. That's why I think it's not a 2.12 regression. We just need to be prepared for what might come. Thanks, -- Peter Xu ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] qemu-thread: let cur_mon be per-thread 2018-04-10 12:49 ` [Qemu-devel] [PATCH 2/2] qemu-thread: let cur_mon be per-thread Peter Xu 2018-04-10 13:54 ` Eric Blake @ 2018-04-11 1:45 ` Stefan Hajnoczi 2018-04-11 3:49 ` Peter Xu 1 sibling, 1 reply; 15+ messages in thread From: Stefan Hajnoczi @ 2018-04-11 1:45 UTC (permalink / raw) To: Peter Xu Cc: qemu-devel, Paolo Bonzini, Alex Bennée, Fam Zheng, Eric Blake, Marc-André Lureau, Markus Armbruster, Dr . David Alan Gilbert [-- Attachment #1: Type: text/plain, Size: 930 bytes --] On Tue, Apr 10, 2018 at 08:49:13PM +0800, Peter Xu wrote: > 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? > 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. Stefan [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 455 bytes --] ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] qemu-thread: let cur_mon be per-thread 2018-04-11 1:45 ` Stefan Hajnoczi @ 2018-04-11 3:49 ` Peter Xu 2018-04-11 9:23 ` Paolo Bonzini 0 siblings, 1 reply; 15+ messages in thread From: Peter Xu @ 2018-04-11 3:49 UTC (permalink / raw) To: Stefan Hajnoczi Cc: qemu-devel, Paolo Bonzini, Alex Bennée, Fam Zheng, Eric Blake, Marc-André Lureau, Markus Armbruster, Dr . David Alan Gilbert On Wed, Apr 11, 2018 at 09:45:32AM +0800, Stefan Hajnoczi wrote: > On Tue, Apr 10, 2018 at 08:49:13PM +0800, Peter Xu wrote: > > 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. > > or > > Does this patch fix a bug? No; currently we have no bug. But we have encounter the bug when we start to add more OOB commands. Here is the problem (Marc-Andre reported this, and I'll try to summarize): after we have valid OOB commands, monitor_qmp_dispatch_one() can be run not only in main thread, but also in monitor iothread. When that happens, both of them (main thread, and monitor iothread) can be modifying the cur_mon variable at the same time. [1] Considering that cur_mon is only used "just like" a stack variable, it should be perfectly fine we just make it as a per thread variable, hence this patch. > > > 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. Yes, I'm sorry about it. Actually I don't like it too. But that seems to be an efficient and simple solution to me now. The ideal solution should be totally removing cur_mon, which is non-trivial. And for sure we can try to avoid layer violation. For example, we can have something like qemu_thread_register_variable(pthread_key_t), then monitor code can register the cur_mon pthread_key to the qemu thread module. That'll somehow achieve isolation between modules but I'm not sure whether that would be necessary for now, hence I chose the simple. > > If there's a legitimate need I think this patch might be necessary, but > I don't see enough justification to do this yet. The problem was described at [1]. Thanks, -- Peter Xu ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] qemu-thread: let cur_mon be per-thread 2018-04-11 3:49 ` Peter Xu @ 2018-04-11 9:23 ` Paolo Bonzini 2018-04-11 9:35 ` Peter Xu 0 siblings, 1 reply; 15+ messages in thread From: Paolo Bonzini @ 2018-04-11 9:23 UTC (permalink / raw) To: Peter Xu, Stefan Hajnoczi Cc: qemu-devel, Alex Bennée, Fam Zheng, Eric Blake, Marc-André Lureau, Markus Armbruster, Dr . David Alan Gilbert On 11/04/2018 05:49, Peter Xu wrote: > On Wed, Apr 11, 2018 at 09:45:32AM +0800, Stefan Hajnoczi wrote: >> On Tue, Apr 10, 2018 at 08:49:13PM +0800, Peter Xu wrote: >>> 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. That's fine, but it shouldn't need the inheritance part. The monitor IOThread can set cur_mon when it starts. In general, relying on cur_mon should be avoided. Paolo ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] qemu-thread: let cur_mon be per-thread 2018-04-11 9:23 ` Paolo Bonzini @ 2018-04-11 9:35 ` Peter Xu 2018-04-11 9:38 ` Paolo Bonzini 0 siblings, 1 reply; 15+ messages in thread From: Peter Xu @ 2018-04-11 9:35 UTC (permalink / raw) To: Paolo Bonzini Cc: Stefan Hajnoczi, qemu-devel, Alex Bennée, Fam Zheng, Eric Blake, Marc-André Lureau, Markus Armbruster, Dr . David Alan Gilbert On Wed, Apr 11, 2018 at 11:23:57AM +0200, Paolo Bonzini wrote: > On 11/04/2018 05:49, Peter Xu wrote: > > On Wed, Apr 11, 2018 at 09:45:32AM +0800, Stefan Hajnoczi wrote: > >> On Tue, Apr 10, 2018 at 08:49:13PM +0800, Peter Xu wrote: > >>> 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. > > 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: old_mon = cur_mon; cur_mon = xxx; ... (do something) cur_mon = old_mon; And it'll be fine old_mon==NULL here. Then IMHO the only thing we need to do is to mark cur_mon as per-thread and we're done. Thanks, -- Peter Xu ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] qemu-thread: let cur_mon be per-thread 2018-04-11 9:35 ` Peter Xu @ 2018-04-11 9:38 ` Paolo Bonzini 2018-04-11 9:48 ` Peter Xu 0 siblings, 1 reply; 15+ messages in thread From: Paolo Bonzini @ 2018-04-11 9:38 UTC (permalink / raw) To: Peter Xu Cc: Stefan Hajnoczi, qemu-devel, Alex Bennée, Fam Zheng, Eric Blake, Marc-André Lureau, Markus Armbruster, Dr . David Alan Gilbert On 11/04/2018 11:35, Peter Xu wrote: > 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. 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. Paolo > 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: ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] qemu-thread: let cur_mon be per-thread 2018-04-11 9:38 ` Paolo Bonzini @ 2018-04-11 9:48 ` Peter Xu 2018-04-11 13:06 ` Eric Blake 0 siblings, 1 reply; 15+ messages in thread From: Peter Xu @ 2018-04-11 9:48 UTC (permalink / raw) To: Paolo Bonzini Cc: Stefan Hajnoczi, qemu-devel, Alex Bennée, Fam Zheng, Eric Blake, Marc-André Lureau, Markus Armbruster, Dr . David Alan Gilbert On Wed, Apr 11, 2018 at 11:38:58AM +0200, Paolo Bonzini wrote: > On 11/04/2018 11:35, Peter Xu wrote: > > 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. > > 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, -- Peter Xu ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] qemu-thread: let cur_mon be per-thread 2018-04-11 9:48 ` Peter Xu @ 2018-04-11 13:06 ` Eric Blake 2018-04-12 5:24 ` Peter Xu 0 siblings, 1 reply; 15+ messages in thread From: Eric Blake @ 2018-04-11 13:06 UTC (permalink / raw) To: Peter Xu, Paolo Bonzini Cc: Stefan Hajnoczi, qemu-devel, Alex Bennée, Fam Zheng, Marc-André Lureau, Markus Armbruster, Dr . David Alan Gilbert [-- Attachment #1: Type: text/plain, Size: 668 bytes --] On 04/11/2018 04:48 AM, Peter Xu wrote: > 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, 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. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 619 bytes --] ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] qemu-thread: let cur_mon be per-thread 2018-04-11 13:06 ` Eric Blake @ 2018-04-12 5:24 ` Peter Xu 0 siblings, 0 replies; 15+ messages in thread From: Peter Xu @ 2018-04-12 5:24 UTC (permalink / raw) To: Eric Blake Cc: Paolo Bonzini, Stefan Hajnoczi, qemu-devel, Alex Bennée, Fam Zheng, Marc-André Lureau, Markus Armbruster, Dr . David Alan Gilbert On Wed, Apr 11, 2018 at 08:06:04AM -0500, Eric Blake wrote: > On 04/11/2018 04:48 AM, Peter Xu wrote: > > > 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, > > 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. Thank you, Eric. I will repost. -- Peter Xu ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2018-04-12 5:24 UTC | newest] Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-04-10 12:49 [Qemu-devel] [PATCH 0/2] qemu-thread: allow cur_mon be per thread Peter Xu 2018-04-10 12:49 ` [Qemu-devel] [PATCH 1/2] qemu-thread: always keep the posix wrapper layer Peter Xu 2018-04-10 13:35 ` Eric Blake 2018-04-11 3:18 ` Peter Xu 2018-04-10 12:49 ` [Qemu-devel] [PATCH 2/2] qemu-thread: let cur_mon be per-thread Peter Xu 2018-04-10 13:54 ` Eric Blake 2018-04-11 3:31 ` Peter Xu 2018-04-11 1:45 ` Stefan Hajnoczi 2018-04-11 3:49 ` Peter Xu 2018-04-11 9:23 ` Paolo Bonzini 2018-04-11 9:35 ` Peter Xu 2018-04-11 9:38 ` Paolo Bonzini 2018-04-11 9:48 ` Peter Xu 2018-04-11 13:06 ` Eric Blake 2018-04-12 5:24 ` Peter Xu
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.