| Message ID | 20180410124913.10832-2-peterx@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | qemu-thread: allow cur_mon be per thread | expand |
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.
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,
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__);
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(-)