diff mbox

[3/3] Add a 'name' parameter to qemu_thread_create

Message ID 1390922439-30750-4-git-send-email-dgilbert@redhat.com
State New
Headers show

Commit Message

Dr. David Alan Gilbert Jan. 28, 2014, 3:20 p.m. UTC
From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

If enabled, set the thread name at creation (on GNU systems with
  pthread_set_np)
Fix up all the callers with a thread name

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 cpus.c                          | 6 +++---
 hw/block/dataplane/virtio-blk.c | 2 +-
 hw/usb/ccid-card-emulated.c     | 8 ++++----
 include/qemu/thread.h           | 2 +-
 libcacard/vscclient.c           | 2 +-
 migration.c                     | 2 +-
 thread-pool.c                   | 2 +-
 ui/vnc-jobs.c                   | 3 ++-
 util/compatfd.c                 | 3 ++-
 util/qemu-thread-posix.c        | 9 +++++++--
 util/qemu-thread-win32.c        | 2 +-
 11 files changed, 24 insertions(+), 17 deletions(-)

Comments

Michael S. Tsirkin Jan. 28, 2014, 3:56 p.m. UTC | #1
On Tue, Jan 28, 2014 at 03:20:39PM +0000, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> 
> If enabled, set the thread name at creation (on GNU systems with
>   pthread_set_np)
> Fix up all the callers with a thread name
> 
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

Thanks for the patch.

It worries me that tool might start assuming specific
thread names - this effectively becomes part of
management interface.

We avoided this in the past except for VCPU threads -
in particular we only expose thread id for VCPU threads.
How about some generic name for non-VCPU threads
to avoid this issue?

Also - should we put VCPU # in the thread name?


> ---
>  cpus.c                          | 6 +++---
>  hw/block/dataplane/virtio-blk.c | 2 +-
>  hw/usb/ccid-card-emulated.c     | 8 ++++----
>  include/qemu/thread.h           | 2 +-
>  libcacard/vscclient.c           | 2 +-
>  migration.c                     | 2 +-
>  thread-pool.c                   | 2 +-
>  ui/vnc-jobs.c                   | 3 ++-
>  util/compatfd.c                 | 3 ++-
>  util/qemu-thread-posix.c        | 9 +++++++--
>  util/qemu-thread-win32.c        | 2 +-
>  11 files changed, 24 insertions(+), 17 deletions(-)
> 
> diff --git a/cpus.c b/cpus.c
> index ca4c59f..d519b27 100644
> --- a/cpus.c
> +++ b/cpus.c
> @@ -1125,7 +1125,7 @@ static void qemu_tcg_init_vcpu(CPUState *cpu)
>          cpu->halt_cond = g_malloc0(sizeof(QemuCond));
>          qemu_cond_init(cpu->halt_cond);
>          tcg_halt_cond = cpu->halt_cond;
> -        qemu_thread_create(cpu->thread, qemu_tcg_cpu_thread_fn, cpu,
> +        qemu_thread_create(cpu->thread, "CPU/TCG", qemu_tcg_cpu_thread_fn, cpu,
>                             QEMU_THREAD_JOINABLE);
>  #ifdef _WIN32
>          cpu->hThread = qemu_thread_get_handle(cpu->thread);
> @@ -1145,7 +1145,7 @@ static void qemu_kvm_start_vcpu(CPUState *cpu)
>      cpu->thread = g_malloc0(sizeof(QemuThread));
>      cpu->halt_cond = g_malloc0(sizeof(QemuCond));
>      qemu_cond_init(cpu->halt_cond);
> -    qemu_thread_create(cpu->thread, qemu_kvm_cpu_thread_fn, cpu,
> +    qemu_thread_create(cpu->thread, "CPU/KVM", qemu_kvm_cpu_thread_fn, cpu,
>                         QEMU_THREAD_JOINABLE);
>      while (!cpu->created) {
>          qemu_cond_wait(&qemu_cpu_cond, &qemu_global_mutex);
> @@ -1157,7 +1157,7 @@ static void qemu_dummy_start_vcpu(CPUState *cpu)
>      cpu->thread = g_malloc0(sizeof(QemuThread));
>      cpu->halt_cond = g_malloc0(sizeof(QemuCond));
>      qemu_cond_init(cpu->halt_cond);
> -    qemu_thread_create(cpu->thread, qemu_dummy_cpu_thread_fn, cpu,
> +    qemu_thread_create(cpu->thread, "CPU/DUMMY", qemu_dummy_cpu_thread_fn, cpu,
>                         QEMU_THREAD_JOINABLE);
>      while (!cpu->created) {
>          qemu_cond_wait(&qemu_cpu_cond, &qemu_global_mutex);
> diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
> index 456d437..980a684 100644
> --- a/hw/block/dataplane/virtio-blk.c
> +++ b/hw/block/dataplane/virtio-blk.c
> @@ -358,7 +358,7 @@ static void start_data_plane_bh(void *opaque)
>  
>      qemu_bh_delete(s->start_bh);
>      s->start_bh = NULL;
> -    qemu_thread_create(&s->thread, data_plane_thread,
> +    qemu_thread_create(&s->thread, "data_plane", data_plane_thread,
>                         s, QEMU_THREAD_JOINABLE);
>  }
>  
> diff --git a/hw/usb/ccid-card-emulated.c b/hw/usb/ccid-card-emulated.c
> index aa913df..7213c89 100644
> --- a/hw/usb/ccid-card-emulated.c
> +++ b/hw/usb/ccid-card-emulated.c
> @@ -546,10 +546,10 @@ static int emulated_initfn(CCIDCardState *base)
>          printf("%s: failed to initialize vcard\n", EMULATED_DEV_NAME);
>          return -1;
>      }
> -    qemu_thread_create(&card->event_thread_id, event_thread, card,
> -                       QEMU_THREAD_JOINABLE);
> -    qemu_thread_create(&card->apdu_thread_id, handle_apdu_thread, card,
> -                       QEMU_THREAD_JOINABLE);
> +    qemu_thread_create(&card->event_thread_id, "ccid/event", event_thread,
> +                       card, QEMU_THREAD_JOINABLE);
> +    qemu_thread_create(&card->apdu_thread_id, "ccid/apdu", handle_apdu_thread,
> +                       card, QEMU_THREAD_JOINABLE);
>      return 0;
>  }
>  
> diff --git a/include/qemu/thread.h b/include/qemu/thread.h
> index bf1e110..f7e3b9b 100644
> --- a/include/qemu/thread.h
> +++ b/include/qemu/thread.h
> @@ -52,7 +52,7 @@ void qemu_event_reset(QemuEvent *ev);
>  void qemu_event_wait(QemuEvent *ev);
>  void qemu_event_destroy(QemuEvent *ev);
>  
> -void qemu_thread_create(QemuThread *thread,
> +void qemu_thread_create(QemuThread *thread, const char *name,
>                          void *(*start_routine)(void *),
>                          void *arg, int mode);
>  void *qemu_thread_join(QemuThread *thread);
> diff --git a/libcacard/vscclient.c b/libcacard/vscclient.c
> index 24f7088..3477ab3 100644
> --- a/libcacard/vscclient.c
> +++ b/libcacard/vscclient.c
> @@ -269,7 +269,7 @@ on_host_init(VSCMsgHeader *mhHeader, VSCMsgInit *incoming)
>      send_msg(VSC_ReaderRemove, VSCARD_MINIMAL_READER_ID, NULL, 0);
>      /* launch the event_thread. This will trigger reader adds for all the
>       * existing readers */
> -    qemu_thread_create(&thread_id, event_thread, NULL, 0);
> +    qemu_thread_create(&thread_id, "vsc/event", event_thread, NULL, 0);
>      return 0;
>  }
>  
> diff --git a/migration.c b/migration.c
> index 7235c23..bddec7e 100644
> --- a/migration.c
> +++ b/migration.c
> @@ -679,6 +679,6 @@ void migrate_fd_connect(MigrationState *s)
>      /* Notify before starting migration thread */
>      notifier_list_notify(&migration_state_notifiers, s);
>  
> -    qemu_thread_create(&s->thread, migration_thread, s,
> +    qemu_thread_create(&s->thread, "migration", migration_thread, s,
>                         QEMU_THREAD_JOINABLE);
>  }
> diff --git a/thread-pool.c b/thread-pool.c
> index 3735fd3..fbdd3ff 100644
> --- a/thread-pool.c
> +++ b/thread-pool.c
> @@ -140,7 +140,7 @@ static void do_spawn_thread(ThreadPool *pool)
>      pool->new_threads--;
>      pool->pending_threads++;
>  
> -    qemu_thread_create(&t, worker_thread, pool, QEMU_THREAD_DETACHED);
> +    qemu_thread_create(&t, "worker", worker_thread, pool, QEMU_THREAD_DETACHED);
>  }
>  
>  static void spawn_thread_bh_fn(void *opaque)
> diff --git a/ui/vnc-jobs.c b/ui/vnc-jobs.c
> index 2d3fce8..3f3c47b 100644
> --- a/ui/vnc-jobs.c
> +++ b/ui/vnc-jobs.c
> @@ -333,7 +333,8 @@ void vnc_start_worker_thread(void)
>          return ;
>  
>      q = vnc_queue_init();
> -    qemu_thread_create(&q->thread, vnc_worker_thread, q, QEMU_THREAD_DETACHED);
> +    qemu_thread_create(&q->thread, "vnc_worker", vnc_worker_thread, q,
> +                       QEMU_THREAD_DETACHED);
>      queue = q; /* Set global queue */
>  }
>  
> diff --git a/util/compatfd.c b/util/compatfd.c
> index 430a41c..341ada6 100644
> --- a/util/compatfd.c
> +++ b/util/compatfd.c
> @@ -88,7 +88,8 @@ static int qemu_signalfd_compat(const sigset_t *mask)
>      memcpy(&info->mask, mask, sizeof(*mask));
>      info->fd = fds[1];
>  
> -    qemu_thread_create(&thread, sigwait_compat, info, QEMU_THREAD_DETACHED);
> +    qemu_thread_create(&thread, "signalfd_compat", sigwait_compat, info,
> +                       QEMU_THREAD_DETACHED);
>  
>      return fds[0];
>  }
> diff --git a/util/qemu-thread-posix.c b/util/qemu-thread-posix.c
> index 0fa6c81..45113b4 100644
> --- a/util/qemu-thread-posix.c
> +++ b/util/qemu-thread-posix.c
> @@ -394,8 +394,7 @@ void qemu_event_wait(QemuEvent *ev)
>      }
>  }
>  
> -
> -void qemu_thread_create(QemuThread *thread,
> +void qemu_thread_create(QemuThread *thread, const char *name,
>                         void *(*start_routine)(void*),
>                         void *arg, int mode)
>  {
> @@ -421,6 +420,12 @@ void qemu_thread_create(QemuThread *thread,
>      if (err)
>          error_exit(err, __func__);
>  
> +#ifdef _GNU_SOURCE
> +    if (name_threads) {
> +        pthread_setname_np(thread->thread, name);
> +    }
> +#endif
> +
>      pthread_sigmask(SIG_SETMASK, &oldset, NULL);
>  
>      pthread_attr_destroy(&attr);
> diff --git a/util/qemu-thread-win32.c b/util/qemu-thread-win32.c
> index e42cb77..b9c957b 100644
> --- a/util/qemu-thread-win32.c
> +++ b/util/qemu-thread-win32.c
> @@ -333,7 +333,7 @@ void *qemu_thread_join(QemuThread *thread)
>      return ret;
>  }
>  
> -void qemu_thread_create(QemuThread *thread,
> +void qemu_thread_create(QemuThread *thread, const char *name,
>                         void *(*start_routine)(void *),
>                         void *arg, int mode)
>  {
> -- 
> 1.8.5.3
Dr. David Alan Gilbert Jan. 28, 2014, 4:12 p.m. UTC | #2
* Michael S. Tsirkin (mst@redhat.com) wrote:
> On Tue, Jan 28, 2014 at 03:20:39PM +0000, Dr. David Alan Gilbert (git) wrote:
> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> > 
> > If enabled, set the thread name at creation (on GNU systems with
> >   pthread_set_np)
> > Fix up all the callers with a thread name
> > 
> > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> 
> Thanks for the patch.
> 
> It worries me that tool might start assuming specific
> thread names - this effectively becomes part of
> management interface.
>
> We avoided this in the past except for VCPU threads -
> in particular we only expose thread id for VCPU threads.
> How about some generic name for non-VCPU threads
> to avoid this issue?

Since I'm doing migration development, restriction to VCPU
threads doesn't help me much.

Putting big scary warnings somewhere (where?) to say that
the names aren't guaranteed is all I can think of.
(I did put that warning in the cover letter).

I guess I could change the option name to debug-threads
to make it clear it's for debug.

> Also - should we put VCPU # in the thread name?

Yeh that's something I could add.

Dave

> > ---
> >  cpus.c                          | 6 +++---
> >  hw/block/dataplane/virtio-blk.c | 2 +-
> >  hw/usb/ccid-card-emulated.c     | 8 ++++----
> >  include/qemu/thread.h           | 2 +-
> >  libcacard/vscclient.c           | 2 +-
> >  migration.c                     | 2 +-
> >  thread-pool.c                   | 2 +-
> >  ui/vnc-jobs.c                   | 3 ++-
> >  util/compatfd.c                 | 3 ++-
> >  util/qemu-thread-posix.c        | 9 +++++++--
> >  util/qemu-thread-win32.c        | 2 +-
> >  11 files changed, 24 insertions(+), 17 deletions(-)
> > 
> > diff --git a/cpus.c b/cpus.c
> > index ca4c59f..d519b27 100644
> > --- a/cpus.c
> > +++ b/cpus.c
> > @@ -1125,7 +1125,7 @@ static void qemu_tcg_init_vcpu(CPUState *cpu)
> >          cpu->halt_cond = g_malloc0(sizeof(QemuCond));
> >          qemu_cond_init(cpu->halt_cond);
> >          tcg_halt_cond = cpu->halt_cond;
> > -        qemu_thread_create(cpu->thread, qemu_tcg_cpu_thread_fn, cpu,
> > +        qemu_thread_create(cpu->thread, "CPU/TCG", qemu_tcg_cpu_thread_fn, cpu,
> >                             QEMU_THREAD_JOINABLE);
> >  #ifdef _WIN32
> >          cpu->hThread = qemu_thread_get_handle(cpu->thread);
> > @@ -1145,7 +1145,7 @@ static void qemu_kvm_start_vcpu(CPUState *cpu)
> >      cpu->thread = g_malloc0(sizeof(QemuThread));
> >      cpu->halt_cond = g_malloc0(sizeof(QemuCond));
> >      qemu_cond_init(cpu->halt_cond);
> > -    qemu_thread_create(cpu->thread, qemu_kvm_cpu_thread_fn, cpu,
> > +    qemu_thread_create(cpu->thread, "CPU/KVM", qemu_kvm_cpu_thread_fn, cpu,
> >                         QEMU_THREAD_JOINABLE);
> >      while (!cpu->created) {
> >          qemu_cond_wait(&qemu_cpu_cond, &qemu_global_mutex);
> > @@ -1157,7 +1157,7 @@ static void qemu_dummy_start_vcpu(CPUState *cpu)
> >      cpu->thread = g_malloc0(sizeof(QemuThread));
> >      cpu->halt_cond = g_malloc0(sizeof(QemuCond));
> >      qemu_cond_init(cpu->halt_cond);
> > -    qemu_thread_create(cpu->thread, qemu_dummy_cpu_thread_fn, cpu,
> > +    qemu_thread_create(cpu->thread, "CPU/DUMMY", qemu_dummy_cpu_thread_fn, cpu,
> >                         QEMU_THREAD_JOINABLE);
> >      while (!cpu->created) {
> >          qemu_cond_wait(&qemu_cpu_cond, &qemu_global_mutex);
> > diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
> > index 456d437..980a684 100644
> > --- a/hw/block/dataplane/virtio-blk.c
> > +++ b/hw/block/dataplane/virtio-blk.c
> > @@ -358,7 +358,7 @@ static void start_data_plane_bh(void *opaque)
> >  
> >      qemu_bh_delete(s->start_bh);
> >      s->start_bh = NULL;
> > -    qemu_thread_create(&s->thread, data_plane_thread,
> > +    qemu_thread_create(&s->thread, "data_plane", data_plane_thread,
> >                         s, QEMU_THREAD_JOINABLE);
> >  }
> >  
> > diff --git a/hw/usb/ccid-card-emulated.c b/hw/usb/ccid-card-emulated.c
> > index aa913df..7213c89 100644
> > --- a/hw/usb/ccid-card-emulated.c
> > +++ b/hw/usb/ccid-card-emulated.c
> > @@ -546,10 +546,10 @@ static int emulated_initfn(CCIDCardState *base)
> >          printf("%s: failed to initialize vcard\n", EMULATED_DEV_NAME);
> >          return -1;
> >      }
> > -    qemu_thread_create(&card->event_thread_id, event_thread, card,
> > -                       QEMU_THREAD_JOINABLE);
> > -    qemu_thread_create(&card->apdu_thread_id, handle_apdu_thread, card,
> > -                       QEMU_THREAD_JOINABLE);
> > +    qemu_thread_create(&card->event_thread_id, "ccid/event", event_thread,
> > +                       card, QEMU_THREAD_JOINABLE);
> > +    qemu_thread_create(&card->apdu_thread_id, "ccid/apdu", handle_apdu_thread,
> > +                       card, QEMU_THREAD_JOINABLE);
> >      return 0;
> >  }
> >  
> > diff --git a/include/qemu/thread.h b/include/qemu/thread.h
> > index bf1e110..f7e3b9b 100644
> > --- a/include/qemu/thread.h
> > +++ b/include/qemu/thread.h
> > @@ -52,7 +52,7 @@ void qemu_event_reset(QemuEvent *ev);
> >  void qemu_event_wait(QemuEvent *ev);
> >  void qemu_event_destroy(QemuEvent *ev);
> >  
> > -void qemu_thread_create(QemuThread *thread,
> > +void qemu_thread_create(QemuThread *thread, const char *name,
> >                          void *(*start_routine)(void *),
> >                          void *arg, int mode);
> >  void *qemu_thread_join(QemuThread *thread);
> > diff --git a/libcacard/vscclient.c b/libcacard/vscclient.c
> > index 24f7088..3477ab3 100644
> > --- a/libcacard/vscclient.c
> > +++ b/libcacard/vscclient.c
> > @@ -269,7 +269,7 @@ on_host_init(VSCMsgHeader *mhHeader, VSCMsgInit *incoming)
> >      send_msg(VSC_ReaderRemove, VSCARD_MINIMAL_READER_ID, NULL, 0);
> >      /* launch the event_thread. This will trigger reader adds for all the
> >       * existing readers */
> > -    qemu_thread_create(&thread_id, event_thread, NULL, 0);
> > +    qemu_thread_create(&thread_id, "vsc/event", event_thread, NULL, 0);
> >      return 0;
> >  }
> >  
> > diff --git a/migration.c b/migration.c
> > index 7235c23..bddec7e 100644
> > --- a/migration.c
> > +++ b/migration.c
> > @@ -679,6 +679,6 @@ void migrate_fd_connect(MigrationState *s)
> >      /* Notify before starting migration thread */
> >      notifier_list_notify(&migration_state_notifiers, s);
> >  
> > -    qemu_thread_create(&s->thread, migration_thread, s,
> > +    qemu_thread_create(&s->thread, "migration", migration_thread, s,
> >                         QEMU_THREAD_JOINABLE);
> >  }
> > diff --git a/thread-pool.c b/thread-pool.c
> > index 3735fd3..fbdd3ff 100644
> > --- a/thread-pool.c
> > +++ b/thread-pool.c
> > @@ -140,7 +140,7 @@ static void do_spawn_thread(ThreadPool *pool)
> >      pool->new_threads--;
> >      pool->pending_threads++;
> >  
> > -    qemu_thread_create(&t, worker_thread, pool, QEMU_THREAD_DETACHED);
> > +    qemu_thread_create(&t, "worker", worker_thread, pool, QEMU_THREAD_DETACHED);
> >  }
> >  
> >  static void spawn_thread_bh_fn(void *opaque)
> > diff --git a/ui/vnc-jobs.c b/ui/vnc-jobs.c
> > index 2d3fce8..3f3c47b 100644
> > --- a/ui/vnc-jobs.c
> > +++ b/ui/vnc-jobs.c
> > @@ -333,7 +333,8 @@ void vnc_start_worker_thread(void)
> >          return ;
> >  
> >      q = vnc_queue_init();
> > -    qemu_thread_create(&q->thread, vnc_worker_thread, q, QEMU_THREAD_DETACHED);
> > +    qemu_thread_create(&q->thread, "vnc_worker", vnc_worker_thread, q,
> > +                       QEMU_THREAD_DETACHED);
> >      queue = q; /* Set global queue */
> >  }
> >  
> > diff --git a/util/compatfd.c b/util/compatfd.c
> > index 430a41c..341ada6 100644
> > --- a/util/compatfd.c
> > +++ b/util/compatfd.c
> > @@ -88,7 +88,8 @@ static int qemu_signalfd_compat(const sigset_t *mask)
> >      memcpy(&info->mask, mask, sizeof(*mask));
> >      info->fd = fds[1];
> >  
> > -    qemu_thread_create(&thread, sigwait_compat, info, QEMU_THREAD_DETACHED);
> > +    qemu_thread_create(&thread, "signalfd_compat", sigwait_compat, info,
> > +                       QEMU_THREAD_DETACHED);
> >  
> >      return fds[0];
> >  }
> > diff --git a/util/qemu-thread-posix.c b/util/qemu-thread-posix.c
> > index 0fa6c81..45113b4 100644
> > --- a/util/qemu-thread-posix.c
> > +++ b/util/qemu-thread-posix.c
> > @@ -394,8 +394,7 @@ void qemu_event_wait(QemuEvent *ev)
> >      }
> >  }
> >  
> > -
> > -void qemu_thread_create(QemuThread *thread,
> > +void qemu_thread_create(QemuThread *thread, const char *name,
> >                         void *(*start_routine)(void*),
> >                         void *arg, int mode)
> >  {
> > @@ -421,6 +420,12 @@ void qemu_thread_create(QemuThread *thread,
> >      if (err)
> >          error_exit(err, __func__);
> >  
> > +#ifdef _GNU_SOURCE
> > +    if (name_threads) {
> > +        pthread_setname_np(thread->thread, name);
> > +    }
> > +#endif
> > +
> >      pthread_sigmask(SIG_SETMASK, &oldset, NULL);
> >  
> >      pthread_attr_destroy(&attr);
> > diff --git a/util/qemu-thread-win32.c b/util/qemu-thread-win32.c
> > index e42cb77..b9c957b 100644
> > --- a/util/qemu-thread-win32.c
> > +++ b/util/qemu-thread-win32.c
> > @@ -333,7 +333,7 @@ void *qemu_thread_join(QemuThread *thread)
> >      return ret;
> >  }
> >  
> > -void qemu_thread_create(QemuThread *thread,
> > +void qemu_thread_create(QemuThread *thread, const char *name,
> >                         void *(*start_routine)(void *),
> >                         void *arg, int mode)
> >  {
> > -- 
> > 1.8.5.3
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
Laszlo Ersek Jan. 28, 2014, 4:21 p.m. UTC | #3
On 01/28/14 17:12, Dr. David Alan Gilbert wrote:
> * Michael S. Tsirkin (mst@redhat.com) wrote:
>> On Tue, Jan 28, 2014 at 03:20:39PM +0000, Dr. David Alan Gilbert (git) wrote:
>>> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>>>
>>> If enabled, set the thread name at creation (on GNU systems with
>>>   pthread_set_np)
>>> Fix up all the callers with a thread name
>>>
>>> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
>>
>> Thanks for the patch.
>>
>> It worries me that tool might start assuming specific
>> thread names - this effectively becomes part of
>> management interface.
>>
>> We avoided this in the past except for VCPU threads -
>> in particular we only expose thread id for VCPU threads.
>> How about some generic name for non-VCPU threads
>> to avoid this issue?
> 
> Since I'm doing migration development, restriction to VCPU
> threads doesn't help me much.

I'm not doing migration development, but I agree that the feature is
only really useful if *all* threads have names. (IOW when it completely
saves the developer the work to figure out which thread is which.)

> Putting big scary warnings somewhere (where?) to say that
> the names aren't guaranteed is all I can think of.
> (I did put that warning in the cover letter).
> 
> I guess I could change the option name to debug-threads
> to make it clear it's for debug.
> 
>> Also - should we put VCPU # in the thread name?
> 
> Yeh that's something I could add.

Would be very useful.

Thanks
Laszlo
Michael S. Tsirkin Jan. 28, 2014, 4:44 p.m. UTC | #4
On Tue, Jan 28, 2014 at 04:12:44PM +0000, Dr. David Alan Gilbert wrote:
> * Michael S. Tsirkin (mst@redhat.com) wrote:
> > On Tue, Jan 28, 2014 at 03:20:39PM +0000, Dr. David Alan Gilbert (git) wrote:
> > > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> > > 
> > > If enabled, set the thread name at creation (on GNU systems with
> > >   pthread_set_np)
> > > Fix up all the callers with a thread name
> > > 
> > > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > 
> > Thanks for the patch.
> > 
> > It worries me that tool might start assuming specific
> > thread names - this effectively becomes part of
> > management interface.
> >
> > We avoided this in the past except for VCPU threads -
> > in particular we only expose thread id for VCPU threads.
> > How about some generic name for non-VCPU threads
> > to avoid this issue?
> 
> Since I'm doing migration development, restriction to VCPU
> threads doesn't help me much.
> 
> Putting big scary warnings somewhere (where?) to say that
> the names aren't guaranteed is all I can think of.
> (I did put that warning in the cover letter).

Maybe in --help output.

> I guess I could change the option name to debug-threads
> to make it clear it's for debug.

Sounds good.

> > Also - should we put VCPU # in the thread name?
> 
> Yeh that's something I could add.
> 
> Dave
> 
> > > ---
> > >  cpus.c                          | 6 +++---
> > >  hw/block/dataplane/virtio-blk.c | 2 +-
> > >  hw/usb/ccid-card-emulated.c     | 8 ++++----
> > >  include/qemu/thread.h           | 2 +-
> > >  libcacard/vscclient.c           | 2 +-
> > >  migration.c                     | 2 +-
> > >  thread-pool.c                   | 2 +-
> > >  ui/vnc-jobs.c                   | 3 ++-
> > >  util/compatfd.c                 | 3 ++-
> > >  util/qemu-thread-posix.c        | 9 +++++++--
> > >  util/qemu-thread-win32.c        | 2 +-
> > >  11 files changed, 24 insertions(+), 17 deletions(-)
> > > 
> > > diff --git a/cpus.c b/cpus.c
> > > index ca4c59f..d519b27 100644
> > > --- a/cpus.c
> > > +++ b/cpus.c
> > > @@ -1125,7 +1125,7 @@ static void qemu_tcg_init_vcpu(CPUState *cpu)
> > >          cpu->halt_cond = g_malloc0(sizeof(QemuCond));
> > >          qemu_cond_init(cpu->halt_cond);
> > >          tcg_halt_cond = cpu->halt_cond;
> > > -        qemu_thread_create(cpu->thread, qemu_tcg_cpu_thread_fn, cpu,
> > > +        qemu_thread_create(cpu->thread, "CPU/TCG", qemu_tcg_cpu_thread_fn, cpu,
> > >                             QEMU_THREAD_JOINABLE);
> > >  #ifdef _WIN32
> > >          cpu->hThread = qemu_thread_get_handle(cpu->thread);
> > > @@ -1145,7 +1145,7 @@ static void qemu_kvm_start_vcpu(CPUState *cpu)
> > >      cpu->thread = g_malloc0(sizeof(QemuThread));
> > >      cpu->halt_cond = g_malloc0(sizeof(QemuCond));
> > >      qemu_cond_init(cpu->halt_cond);
> > > -    qemu_thread_create(cpu->thread, qemu_kvm_cpu_thread_fn, cpu,
> > > +    qemu_thread_create(cpu->thread, "CPU/KVM", qemu_kvm_cpu_thread_fn, cpu,
> > >                         QEMU_THREAD_JOINABLE);
> > >      while (!cpu->created) {
> > >          qemu_cond_wait(&qemu_cpu_cond, &qemu_global_mutex);
> > > @@ -1157,7 +1157,7 @@ static void qemu_dummy_start_vcpu(CPUState *cpu)
> > >      cpu->thread = g_malloc0(sizeof(QemuThread));
> > >      cpu->halt_cond = g_malloc0(sizeof(QemuCond));
> > >      qemu_cond_init(cpu->halt_cond);
> > > -    qemu_thread_create(cpu->thread, qemu_dummy_cpu_thread_fn, cpu,
> > > +    qemu_thread_create(cpu->thread, "CPU/DUMMY", qemu_dummy_cpu_thread_fn, cpu,
> > >                         QEMU_THREAD_JOINABLE);
> > >      while (!cpu->created) {
> > >          qemu_cond_wait(&qemu_cpu_cond, &qemu_global_mutex);
> > > diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
> > > index 456d437..980a684 100644
> > > --- a/hw/block/dataplane/virtio-blk.c
> > > +++ b/hw/block/dataplane/virtio-blk.c
> > > @@ -358,7 +358,7 @@ static void start_data_plane_bh(void *opaque)
> > >  
> > >      qemu_bh_delete(s->start_bh);
> > >      s->start_bh = NULL;
> > > -    qemu_thread_create(&s->thread, data_plane_thread,
> > > +    qemu_thread_create(&s->thread, "data_plane", data_plane_thread,
> > >                         s, QEMU_THREAD_JOINABLE);
> > >  }
> > >  
> > > diff --git a/hw/usb/ccid-card-emulated.c b/hw/usb/ccid-card-emulated.c
> > > index aa913df..7213c89 100644
> > > --- a/hw/usb/ccid-card-emulated.c
> > > +++ b/hw/usb/ccid-card-emulated.c
> > > @@ -546,10 +546,10 @@ static int emulated_initfn(CCIDCardState *base)
> > >          printf("%s: failed to initialize vcard\n", EMULATED_DEV_NAME);
> > >          return -1;
> > >      }
> > > -    qemu_thread_create(&card->event_thread_id, event_thread, card,
> > > -                       QEMU_THREAD_JOINABLE);
> > > -    qemu_thread_create(&card->apdu_thread_id, handle_apdu_thread, card,
> > > -                       QEMU_THREAD_JOINABLE);
> > > +    qemu_thread_create(&card->event_thread_id, "ccid/event", event_thread,
> > > +                       card, QEMU_THREAD_JOINABLE);
> > > +    qemu_thread_create(&card->apdu_thread_id, "ccid/apdu", handle_apdu_thread,
> > > +                       card, QEMU_THREAD_JOINABLE);
> > >      return 0;
> > >  }
> > >  
> > > diff --git a/include/qemu/thread.h b/include/qemu/thread.h
> > > index bf1e110..f7e3b9b 100644
> > > --- a/include/qemu/thread.h
> > > +++ b/include/qemu/thread.h
> > > @@ -52,7 +52,7 @@ void qemu_event_reset(QemuEvent *ev);
> > >  void qemu_event_wait(QemuEvent *ev);
> > >  void qemu_event_destroy(QemuEvent *ev);
> > >  
> > > -void qemu_thread_create(QemuThread *thread,
> > > +void qemu_thread_create(QemuThread *thread, const char *name,
> > >                          void *(*start_routine)(void *),
> > >                          void *arg, int mode);
> > >  void *qemu_thread_join(QemuThread *thread);
> > > diff --git a/libcacard/vscclient.c b/libcacard/vscclient.c
> > > index 24f7088..3477ab3 100644
> > > --- a/libcacard/vscclient.c
> > > +++ b/libcacard/vscclient.c
> > > @@ -269,7 +269,7 @@ on_host_init(VSCMsgHeader *mhHeader, VSCMsgInit *incoming)
> > >      send_msg(VSC_ReaderRemove, VSCARD_MINIMAL_READER_ID, NULL, 0);
> > >      /* launch the event_thread. This will trigger reader adds for all the
> > >       * existing readers */
> > > -    qemu_thread_create(&thread_id, event_thread, NULL, 0);
> > > +    qemu_thread_create(&thread_id, "vsc/event", event_thread, NULL, 0);
> > >      return 0;
> > >  }
> > >  
> > > diff --git a/migration.c b/migration.c
> > > index 7235c23..bddec7e 100644
> > > --- a/migration.c
> > > +++ b/migration.c
> > > @@ -679,6 +679,6 @@ void migrate_fd_connect(MigrationState *s)
> > >      /* Notify before starting migration thread */
> > >      notifier_list_notify(&migration_state_notifiers, s);
> > >  
> > > -    qemu_thread_create(&s->thread, migration_thread, s,
> > > +    qemu_thread_create(&s->thread, "migration", migration_thread, s,
> > >                         QEMU_THREAD_JOINABLE);
> > >  }
> > > diff --git a/thread-pool.c b/thread-pool.c
> > > index 3735fd3..fbdd3ff 100644
> > > --- a/thread-pool.c
> > > +++ b/thread-pool.c
> > > @@ -140,7 +140,7 @@ static void do_spawn_thread(ThreadPool *pool)
> > >      pool->new_threads--;
> > >      pool->pending_threads++;
> > >  
> > > -    qemu_thread_create(&t, worker_thread, pool, QEMU_THREAD_DETACHED);
> > > +    qemu_thread_create(&t, "worker", worker_thread, pool, QEMU_THREAD_DETACHED);
> > >  }
> > >  
> > >  static void spawn_thread_bh_fn(void *opaque)
> > > diff --git a/ui/vnc-jobs.c b/ui/vnc-jobs.c
> > > index 2d3fce8..3f3c47b 100644
> > > --- a/ui/vnc-jobs.c
> > > +++ b/ui/vnc-jobs.c
> > > @@ -333,7 +333,8 @@ void vnc_start_worker_thread(void)
> > >          return ;
> > >  
> > >      q = vnc_queue_init();
> > > -    qemu_thread_create(&q->thread, vnc_worker_thread, q, QEMU_THREAD_DETACHED);
> > > +    qemu_thread_create(&q->thread, "vnc_worker", vnc_worker_thread, q,
> > > +                       QEMU_THREAD_DETACHED);
> > >      queue = q; /* Set global queue */
> > >  }
> > >  
> > > diff --git a/util/compatfd.c b/util/compatfd.c
> > > index 430a41c..341ada6 100644
> > > --- a/util/compatfd.c
> > > +++ b/util/compatfd.c
> > > @@ -88,7 +88,8 @@ static int qemu_signalfd_compat(const sigset_t *mask)
> > >      memcpy(&info->mask, mask, sizeof(*mask));
> > >      info->fd = fds[1];
> > >  
> > > -    qemu_thread_create(&thread, sigwait_compat, info, QEMU_THREAD_DETACHED);
> > > +    qemu_thread_create(&thread, "signalfd_compat", sigwait_compat, info,
> > > +                       QEMU_THREAD_DETACHED);
> > >  
> > >      return fds[0];
> > >  }
> > > diff --git a/util/qemu-thread-posix.c b/util/qemu-thread-posix.c
> > > index 0fa6c81..45113b4 100644
> > > --- a/util/qemu-thread-posix.c
> > > +++ b/util/qemu-thread-posix.c
> > > @@ -394,8 +394,7 @@ void qemu_event_wait(QemuEvent *ev)
> > >      }
> > >  }
> > >  
> > > -
> > > -void qemu_thread_create(QemuThread *thread,
> > > +void qemu_thread_create(QemuThread *thread, const char *name,
> > >                         void *(*start_routine)(void*),
> > >                         void *arg, int mode)
> > >  {
> > > @@ -421,6 +420,12 @@ void qemu_thread_create(QemuThread *thread,
> > >      if (err)
> > >          error_exit(err, __func__);
> > >  
> > > +#ifdef _GNU_SOURCE
> > > +    if (name_threads) {
> > > +        pthread_setname_np(thread->thread, name);
> > > +    }
> > > +#endif
> > > +
> > >      pthread_sigmask(SIG_SETMASK, &oldset, NULL);
> > >  
> > >      pthread_attr_destroy(&attr);
> > > diff --git a/util/qemu-thread-win32.c b/util/qemu-thread-win32.c
> > > index e42cb77..b9c957b 100644
> > > --- a/util/qemu-thread-win32.c
> > > +++ b/util/qemu-thread-win32.c
> > > @@ -333,7 +333,7 @@ void *qemu_thread_join(QemuThread *thread)
> > >      return ret;
> > >  }
> > >  
> > > -void qemu_thread_create(QemuThread *thread,
> > > +void qemu_thread_create(QemuThread *thread, const char *name,
> > >                         void *(*start_routine)(void *),
> > >                         void *arg, int mode)
> > >  {
> > > -- 
> > > 1.8.5.3
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
diff mbox

Patch

diff --git a/cpus.c b/cpus.c
index ca4c59f..d519b27 100644
--- a/cpus.c
+++ b/cpus.c
@@ -1125,7 +1125,7 @@  static void qemu_tcg_init_vcpu(CPUState *cpu)
         cpu->halt_cond = g_malloc0(sizeof(QemuCond));
         qemu_cond_init(cpu->halt_cond);
         tcg_halt_cond = cpu->halt_cond;
-        qemu_thread_create(cpu->thread, qemu_tcg_cpu_thread_fn, cpu,
+        qemu_thread_create(cpu->thread, "CPU/TCG", qemu_tcg_cpu_thread_fn, cpu,
                            QEMU_THREAD_JOINABLE);
 #ifdef _WIN32
         cpu->hThread = qemu_thread_get_handle(cpu->thread);
@@ -1145,7 +1145,7 @@  static void qemu_kvm_start_vcpu(CPUState *cpu)
     cpu->thread = g_malloc0(sizeof(QemuThread));
     cpu->halt_cond = g_malloc0(sizeof(QemuCond));
     qemu_cond_init(cpu->halt_cond);
-    qemu_thread_create(cpu->thread, qemu_kvm_cpu_thread_fn, cpu,
+    qemu_thread_create(cpu->thread, "CPU/KVM", qemu_kvm_cpu_thread_fn, cpu,
                        QEMU_THREAD_JOINABLE);
     while (!cpu->created) {
         qemu_cond_wait(&qemu_cpu_cond, &qemu_global_mutex);
@@ -1157,7 +1157,7 @@  static void qemu_dummy_start_vcpu(CPUState *cpu)
     cpu->thread = g_malloc0(sizeof(QemuThread));
     cpu->halt_cond = g_malloc0(sizeof(QemuCond));
     qemu_cond_init(cpu->halt_cond);
-    qemu_thread_create(cpu->thread, qemu_dummy_cpu_thread_fn, cpu,
+    qemu_thread_create(cpu->thread, "CPU/DUMMY", qemu_dummy_cpu_thread_fn, cpu,
                        QEMU_THREAD_JOINABLE);
     while (!cpu->created) {
         qemu_cond_wait(&qemu_cpu_cond, &qemu_global_mutex);
diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
index 456d437..980a684 100644
--- a/hw/block/dataplane/virtio-blk.c
+++ b/hw/block/dataplane/virtio-blk.c
@@ -358,7 +358,7 @@  static void start_data_plane_bh(void *opaque)
 
     qemu_bh_delete(s->start_bh);
     s->start_bh = NULL;
-    qemu_thread_create(&s->thread, data_plane_thread,
+    qemu_thread_create(&s->thread, "data_plane", data_plane_thread,
                        s, QEMU_THREAD_JOINABLE);
 }
 
diff --git a/hw/usb/ccid-card-emulated.c b/hw/usb/ccid-card-emulated.c
index aa913df..7213c89 100644
--- a/hw/usb/ccid-card-emulated.c
+++ b/hw/usb/ccid-card-emulated.c
@@ -546,10 +546,10 @@  static int emulated_initfn(CCIDCardState *base)
         printf("%s: failed to initialize vcard\n", EMULATED_DEV_NAME);
         return -1;
     }
-    qemu_thread_create(&card->event_thread_id, event_thread, card,
-                       QEMU_THREAD_JOINABLE);
-    qemu_thread_create(&card->apdu_thread_id, handle_apdu_thread, card,
-                       QEMU_THREAD_JOINABLE);
+    qemu_thread_create(&card->event_thread_id, "ccid/event", event_thread,
+                       card, QEMU_THREAD_JOINABLE);
+    qemu_thread_create(&card->apdu_thread_id, "ccid/apdu", handle_apdu_thread,
+                       card, QEMU_THREAD_JOINABLE);
     return 0;
 }
 
diff --git a/include/qemu/thread.h b/include/qemu/thread.h
index bf1e110..f7e3b9b 100644
--- a/include/qemu/thread.h
+++ b/include/qemu/thread.h
@@ -52,7 +52,7 @@  void qemu_event_reset(QemuEvent *ev);
 void qemu_event_wait(QemuEvent *ev);
 void qemu_event_destroy(QemuEvent *ev);
 
-void qemu_thread_create(QemuThread *thread,
+void qemu_thread_create(QemuThread *thread, const char *name,
                         void *(*start_routine)(void *),
                         void *arg, int mode);
 void *qemu_thread_join(QemuThread *thread);
diff --git a/libcacard/vscclient.c b/libcacard/vscclient.c
index 24f7088..3477ab3 100644
--- a/libcacard/vscclient.c
+++ b/libcacard/vscclient.c
@@ -269,7 +269,7 @@  on_host_init(VSCMsgHeader *mhHeader, VSCMsgInit *incoming)
     send_msg(VSC_ReaderRemove, VSCARD_MINIMAL_READER_ID, NULL, 0);
     /* launch the event_thread. This will trigger reader adds for all the
      * existing readers */
-    qemu_thread_create(&thread_id, event_thread, NULL, 0);
+    qemu_thread_create(&thread_id, "vsc/event", event_thread, NULL, 0);
     return 0;
 }
 
diff --git a/migration.c b/migration.c
index 7235c23..bddec7e 100644
--- a/migration.c
+++ b/migration.c
@@ -679,6 +679,6 @@  void migrate_fd_connect(MigrationState *s)
     /* Notify before starting migration thread */
     notifier_list_notify(&migration_state_notifiers, s);
 
-    qemu_thread_create(&s->thread, migration_thread, s,
+    qemu_thread_create(&s->thread, "migration", migration_thread, s,
                        QEMU_THREAD_JOINABLE);
 }
diff --git a/thread-pool.c b/thread-pool.c
index 3735fd3..fbdd3ff 100644
--- a/thread-pool.c
+++ b/thread-pool.c
@@ -140,7 +140,7 @@  static void do_spawn_thread(ThreadPool *pool)
     pool->new_threads--;
     pool->pending_threads++;
 
-    qemu_thread_create(&t, worker_thread, pool, QEMU_THREAD_DETACHED);
+    qemu_thread_create(&t, "worker", worker_thread, pool, QEMU_THREAD_DETACHED);
 }
 
 static void spawn_thread_bh_fn(void *opaque)
diff --git a/ui/vnc-jobs.c b/ui/vnc-jobs.c
index 2d3fce8..3f3c47b 100644
--- a/ui/vnc-jobs.c
+++ b/ui/vnc-jobs.c
@@ -333,7 +333,8 @@  void vnc_start_worker_thread(void)
         return ;
 
     q = vnc_queue_init();
-    qemu_thread_create(&q->thread, vnc_worker_thread, q, QEMU_THREAD_DETACHED);
+    qemu_thread_create(&q->thread, "vnc_worker", vnc_worker_thread, q,
+                       QEMU_THREAD_DETACHED);
     queue = q; /* Set global queue */
 }
 
diff --git a/util/compatfd.c b/util/compatfd.c
index 430a41c..341ada6 100644
--- a/util/compatfd.c
+++ b/util/compatfd.c
@@ -88,7 +88,8 @@  static int qemu_signalfd_compat(const sigset_t *mask)
     memcpy(&info->mask, mask, sizeof(*mask));
     info->fd = fds[1];
 
-    qemu_thread_create(&thread, sigwait_compat, info, QEMU_THREAD_DETACHED);
+    qemu_thread_create(&thread, "signalfd_compat", sigwait_compat, info,
+                       QEMU_THREAD_DETACHED);
 
     return fds[0];
 }
diff --git a/util/qemu-thread-posix.c b/util/qemu-thread-posix.c
index 0fa6c81..45113b4 100644
--- a/util/qemu-thread-posix.c
+++ b/util/qemu-thread-posix.c
@@ -394,8 +394,7 @@  void qemu_event_wait(QemuEvent *ev)
     }
 }
 
-
-void qemu_thread_create(QemuThread *thread,
+void qemu_thread_create(QemuThread *thread, const char *name,
                        void *(*start_routine)(void*),
                        void *arg, int mode)
 {
@@ -421,6 +420,12 @@  void qemu_thread_create(QemuThread *thread,
     if (err)
         error_exit(err, __func__);
 
+#ifdef _GNU_SOURCE
+    if (name_threads) {
+        pthread_setname_np(thread->thread, name);
+    }
+#endif
+
     pthread_sigmask(SIG_SETMASK, &oldset, NULL);
 
     pthread_attr_destroy(&attr);
diff --git a/util/qemu-thread-win32.c b/util/qemu-thread-win32.c
index e42cb77..b9c957b 100644
--- a/util/qemu-thread-win32.c
+++ b/util/qemu-thread-win32.c
@@ -333,7 +333,7 @@  void *qemu_thread_join(QemuThread *thread)
     return ret;
 }
 
-void qemu_thread_create(QemuThread *thread,
+void qemu_thread_create(QemuThread *thread, const char *name,
                        void *(*start_routine)(void *),
                        void *arg, int mode)
 {