From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:41441) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WCQpH-0004Dj-4Z for qemu-devel@nongnu.org; Sun, 09 Feb 2014 04:37:29 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WCQpB-0005CZ-2v for qemu-devel@nongnu.org; Sun, 09 Feb 2014 04:37:23 -0500 Received: from mx1.redhat.com ([209.132.183.28]:23676) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WCQpA-0005CV-Ra for qemu-devel@nongnu.org; Sun, 09 Feb 2014 04:37:17 -0500 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id s199bGhg001948 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Sun, 9 Feb 2014 04:37:16 -0500 Message-ID: <52F74C4A.6050001@redhat.com> Date: Sun, 09 Feb 2014 10:37:14 +0100 From: Laszlo Ersek MIME-Version: 1.0 References: <1391077232-14649-1-git-send-email-dgilbert@redhat.com> <1391077232-14649-4-git-send-email-dgilbert@redhat.com> In-Reply-To: <1391077232-14649-4-git-send-email-dgilbert@redhat.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v2 3/3] Add a 'name' parameter to qemu_thread_create List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Dr. David Alan Gilbert (git)" Cc: pbonzini@redhat.com, qemu-devel@nongnu.org, mst@redhat.com comments below On 01/30/14 11:20, Dr. David Alan Gilbert (git) wrote: > From: "Dr. David Alan Gilbert" > > 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 > --- > cpus.c | 25 ++++++++++++++++++++----- > 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, 41 insertions(+), 19 deletions(-) > > diff --git a/cpus.c b/cpus.c > index ca4c59f..e5bb271 100644 > --- a/cpus.c > +++ b/cpus.c > @@ -1117,16 +1117,23 @@ void resume_all_vcpus(void) > } > } > > +/* For temporary buffers for forming a name */ > +#define TMP_THREAD_NAME_LEN 16 This should be called _SIZE in my opinion, because it seems to include the trailing NUL character. Alternatively, as much as I hate glib's presence in qemu, you could replace each (fixed size buffer, snprintf()) pair with a (pointer, g_strdup_printf(), g_free()) triplet. > + > static void qemu_tcg_init_vcpu(CPUState *cpu) > { > + char thread_name[TMP_THREAD_NAME_LEN]; > + > /* share a single thread for all cpus with TCG */ > if (!tcg_cpu_thread) { > cpu->thread = g_malloc0(sizeof(QemuThread)); > 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_JOINABLE); > + snprintf(thread_name, TMP_THREAD_NAME_LEN, "CPU %d/TCG", > + cpu->cpu_index); > + qemu_thread_create(cpu->thread, thread_name, qemu_tcg_cpu_thread_fn, > + cpu, QEMU_THREAD_JOINABLE); > #ifdef _WIN32 > cpu->hThread = qemu_thread_get_handle(cpu->thread); > #endif > @@ -1142,11 +1149,15 @@ static void qemu_tcg_init_vcpu(CPUState *cpu) > > static void qemu_kvm_start_vcpu(CPUState *cpu) > { > + char thread_name[TMP_THREAD_NAME_LEN]; > + > 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_JOINABLE); > + snprintf(thread_name, TMP_THREAD_NAME_LEN, "CPU %d/KVM", > + cpu->cpu_index); > + qemu_thread_create(cpu->thread, thread_name, qemu_kvm_cpu_thread_fn, > + cpu, QEMU_THREAD_JOINABLE); > while (!cpu->created) { > qemu_cond_wait(&qemu_cpu_cond, &qemu_global_mutex); > } > @@ -1154,10 +1165,14 @@ static void qemu_kvm_start_vcpu(CPUState *cpu) > > static void qemu_dummy_start_vcpu(CPUState *cpu) > { > + char thread_name[TMP_THREAD_NAME_LEN]; > + > 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, > + snprintf(thread_name, TMP_THREAD_NAME_LEN, "CPU %d/DUMMY", > + cpu->cpu_index); > + qemu_thread_create(cpu->thread, thread_name, 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); > } Don't really want to suggest feature creep, but since we can have several dataplane threads as well, I suggest appending the serial number of the virtio-blk device, s->blk->serial (can be NULL). Example for access is in do_get_id_cmd(). Of course with this free-form string, g_strdup_printf() becomes more attractive. > > 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; > } No clue how many there can be of these. > > 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; > } Ditto. > > 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); > } At most one such thread at a time, right? > 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); > } I do think these guys should be numbered somehow. "pending_threads" looks like an obvious choice, but it might be incorrect. > > 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 */ > } Probably one thread only. > > 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]; > } Ditto. > 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 > + Maybe a retval check? Or do we ignore ERANGE on purpose? > 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) > { > Anyway this is for helping debugging, and there's nothing in here that I'd call a bug, so I won't waste your time with asking for a new version. :) Reviewed-by: Laszlo Ersek Thanks! Laszlo