* [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
* [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 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 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 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 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
* 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-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.