Re: [Qemu-devel] [PATCH 3/3] Add a 'name' parameter to qemu_thread_create

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* 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