Re: [Qemu-devel] [PATCH 1/2] qemu-thread: always keep the posix wrapper layer
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 > > --- > > 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
Re: [Qemu-devel] [PATCH 1/2] qemu-thread: always keep the posix wrapper layer
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 > --- > 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 signature.asc Description: OpenPGP digital signature
[Qemu-devel] [PATCH 1/2] qemu-thread: always keep the posix wrapper layer
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 --- 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