Re: [Qemu-devel] [PATCH 1/2] qemu-thread: always keep the posix wrapper layer

[Eric Blake <eblake@redhat.com>]

[-- 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 --]