[Qemu-devel] [PATCH 0/3] Name threads

[Qemu-devel] [PATCH 0/3] Name threads

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

This series uses pthread_setname_np (when available) to set the names on
threads that QEMU creates to make life easier when debugging.

It's turned off by default (because there were worries that it might break
tools that relied on process names) but is enabled by adding

   namethreads=on

to the --name option.

Note that the initial thread still has the default name (or the value passed
as the process= parameter to --name).

The naming of the individual threads is not meant to form an API; tools
shall not rely on the names.

The first patch converts --name to use QemuOpts, a side effect of this is that
  --name process=foo,bar
no longer allows a process name of 'foo,bar', since ',' is a separator.

With this enabled in gdb we see:

(gdb) info threads
  Id   Target Id         Frame 
  5    Thread 0x7fb6670f9700 (LWP 18243) "worker" 0x00007fb66b8fcec0 in sem_timedwait () from /lib64/libpthread.so.0
  4    Thread 0x7fb666515700 (LWP 18244) "CPU/KVM" 0x00007fb669ab0117 in ioctl () from /lib64/libc.so.6
  3    Thread 0x7fb665d14700 (LWP 18245) "CPU/KVM" 0x00007fb669ab0117 in ioctl () from /lib64/libc.so.6
  2    Thread 0x7fb5d7fff700 (LWP 18247) "vnc_worker" 0x00007fb66b8fad20 in pthread_cond_wait@@GLIBC_2.3.2 () from /lib64/libpthread.so.0
* 1    Thread 0x7fb66d451a00 (LWP 18242) "qemuprocess" 0x00007fb669aaeb4f in ppoll () from /lib64/libc.so.6

and it also shows up well in 'top' using the H and V options.

Dr. David Alan Gilbert (3):
  Rework --name to use QemuOpts
  Add 'namethreads' suboption to --name
  Add a 'name' parameter to qemu_thread_create

 cpus.c                          |  6 ++---
 hw/block/dataplane/virtio-blk.c |  2 +-
 hw/usb/ccid-card-emulated.c     |  8 +++---
 include/qemu/thread.h           |  3 ++-
 libcacard/vscclient.c           |  2 +-
 migration.c                     |  2 +-
 qemu-options.hx                 |  6 +++--
 thread-pool.c                   |  2 +-
 ui/vnc-jobs.c                   |  3 ++-
 util/compatfd.c                 |  3 ++-
 util/qemu-thread-posix.c        | 16 +++++++++--
 util/qemu-thread-win32.c        | 10 ++++++-
 vl.c                            | 59 ++++++++++++++++++++++++++++++++---------
 13 files changed, 90 insertions(+), 32 deletions(-)

-- 
1.8.5.3

[Qemu-devel] [PATCH 1/3] Rework --name to use QemuOpts

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 vl.c | 52 +++++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 39 insertions(+), 13 deletions(-)

diff --git a/vl.c b/vl.c
index 7f4fe0d..5f993e4 100644
--- a/vl.c
+++ b/vl.c
@@ -531,6 +531,27 @@ static QemuOptsList qemu_msg_opts = {
     },
 };
 
+static QemuOptsList qemu_name_opts = {
+    .name = "name",
+    .implied_opt_name = "guest",
+    .merge_lists = true,
+    .head = QTAILQ_HEAD_INITIALIZER(qemu_name_opts.head),
+    .desc = {
+        {
+            .name = "guest",
+            .type = QEMU_OPT_STRING,
+            .help = "Sets the name of the guest.\n"
+                    "This name will be displayed in the SDL window caption.\n"
+                    "The name will also be used for the VNC server",
+        }, {
+            .name = "process",
+            .type = QEMU_OPT_STRING,
+            .help = "Sets the name of the QEMU process, as shown in top etc",
+        },
+        { /* End of list */ }
+    },
+};
+
 /**
  * Get machine options
  *
@@ -981,6 +1002,18 @@ static int parse_sandbox(QemuOpts *opts, void *opaque)
     return 0;
 }
 
+static void parse_name(QemuOpts *opts)
+{
+    const char *proc_name;
+
+    qemu_name = qemu_opt_get(opts, "guest");
+
+    proc_name = qemu_opt_get(opts, "process");
+    if (proc_name) {
+        os_set_proc_name(proc_name);
+    }
+}
+
 bool usb_enabled(bool default_usb)
 {
     return qemu_opt_get_bool(qemu_get_machine_opts(), "usb", default_usb);
@@ -2895,6 +2928,7 @@ int main(int argc, char **argv, char **envp)
     qemu_add_opts(&qemu_tpmdev_opts);
     qemu_add_opts(&qemu_realtime_opts);
     qemu_add_opts(&qemu_msg_opts);
+    qemu_add_opts(&qemu_name_opts);
 
     runstate_init();
 
@@ -3630,19 +3664,11 @@ int main(int argc, char **argv, char **envp)
                                 "is no longer supported.\n");
                 break;
             case QEMU_OPTION_name:
-                qemu_name = g_strdup(optarg);
-		 {
-		     char *p = strchr(qemu_name, ',');
-		     if (p != NULL) {
-		        *p++ = 0;
-			if (strncmp(p, "process=", 8)) {
-			    fprintf(stderr, "Unknown subargument %s to -name\n", p);
-			    exit(1);
-			}
-			p += 8;
-			os_set_proc_name(p);
-		     }
-		 }
+                opts = qemu_opts_parse(qemu_find_opts("name"), optarg, 1);
+                if (!opts) {
+                    exit(1);
+                }
+                parse_name(opts);
                 break;
             case QEMU_OPTION_prom_env:
                 if (nb_prom_envs >= MAX_PROM_ENVS) {
-- 
1.8.5.3

Re: [Qemu-devel] [PATCH 1/3] Rework --name to use QemuOpts

[Alex Bennée]

dgilbert@redhat.com writes:

> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> ---
>  vl.c | 52 +++++++++++++++++++++++++++++++++++++++-------------
>  1 file changed, 39 insertions(+), 13 deletions(-)
>
> diff --git a/vl.c b/vl.c
> index 7f4fe0d..5f993e4 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -531,6 +531,27 @@ static QemuOptsList qemu_msg_opts = {
>      },
>  };
>  
> +static QemuOptsList qemu_name_opts = {
> +    .name = "name",
> +    .implied_opt_name = "guest",
> +    .merge_lists = true,
> +    .head = QTAILQ_HEAD_INITIALIZER(qemu_name_opts.head),
> +    .desc = {
> +        {
> +            .name = "guest",
> +            .type = QEMU_OPT_STRING,
> +            .help = "Sets the name of the guest.\n"
> +                    "This name will be displayed in the SDL window caption.\n"
> +                    "The name will also be used for the VNC server",
> +        }, {
> +            .name = "process",
> +            .type = QEMU_OPT_STRING,
> +            .help = "Sets the name of the QEMU process, as shown in top etc",
> +        },
> +        { /* End of list */ }
> +    },
> +};
> +
<snip>
>                  break;
>              case QEMU_OPTION_name:
> -                qemu_name = g_strdup(optarg);
> -		 {
> -		     char *p = strchr(qemu_name, ',');
> -		     if (p != NULL) {
> -		        *p++ = 0;
> -			if (strncmp(p, "process=", 8)) {
> -			    fprintf(stderr, "Unknown subargument %s to -name\n", p);
> -			    exit(1);
> -			}
> -			p += 8;
> -			os_set_proc_name(p);
> -		     }
> -		 }
> +                opts = qemu_opts_parse(qemu_find_opts("name"), optarg, 1);
> +                if (!opts) {
> +                    exit(1);
> +                }
> +                parse_name(opts);
<snip>

Always good to see hand parsing replace with proper option handling ;-)

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

-- 
Alex Bennée

[Qemu-devel] [PATCH 2/3] Add 'namethreads' suboption to --name

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

Add flag storage to qemu-thread-* to store the namethreads flag

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 include/qemu/thread.h    | 1 +
 qemu-options.hx          | 6 ++++--
 util/qemu-thread-posix.c | 7 +++++++
 util/qemu-thread-win32.c | 8 ++++++++
 vl.c                     | 7 +++++++
 5 files changed, 27 insertions(+), 2 deletions(-)

diff --git a/include/qemu/thread.h b/include/qemu/thread.h
index 3e32c65..bf1e110 100644
--- a/include/qemu/thread.h
+++ b/include/qemu/thread.h
@@ -59,5 +59,6 @@ void *qemu_thread_join(QemuThread *thread);
 void qemu_thread_get_self(QemuThread *thread);
 bool qemu_thread_is_self(QemuThread *thread);
 void qemu_thread_exit(void *retval);
+void qemu_thread_naming(bool enable);
 
 #endif
diff --git a/qemu-options.hx b/qemu-options.hx
index 56e5fdf..d53343a 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -328,9 +328,10 @@ possible drivers and properties, use @code{-device help} and
 ETEXI
 
 DEF("name", HAS_ARG, QEMU_OPTION_name,
-    "-name string1[,process=string2]\n"
+    "-name string1[,process=string2][,namethreads=on|off]\n"
     "                set the name of the guest\n"
-    "                string1 sets the window title and string2 the process name (on Linux)\n",
+    "                string1 sets the window title and string2 the process name (on Linux)\n"
+    "                When namethreads is enabled, individual threads are given a separate name (on Linux)\n",
     QEMU_ARCH_ALL)
 STEXI
 @item -name @var{name}
@@ -339,6 +340,7 @@ Sets the @var{name} of the guest.
 This name will be displayed in the SDL window caption.
 The @var{name} will also be used for the VNC server.
 Also optionally set the top visible process name in Linux.
+Naming of individual threads can also be enabled on Linux.
 ETEXI
 
 DEF("uuid", HAS_ARG, QEMU_OPTION_uuid,
diff --git a/util/qemu-thread-posix.c b/util/qemu-thread-posix.c
index 37dd298..0fa6c81 100644
--- a/util/qemu-thread-posix.c
+++ b/util/qemu-thread-posix.c
@@ -27,6 +27,13 @@
 #include "qemu/thread.h"
 #include "qemu/atomic.h"
 
+static bool name_threads;
+
+void qemu_thread_naming(bool enable)
+{
+    name_threads = enable;
+}
+
 static void error_exit(int err, const char *msg)
 {
     fprintf(stderr, "qemu: %s: %s\n", msg, strerror(err));
diff --git a/util/qemu-thread-win32.c b/util/qemu-thread-win32.c
index 27a5217..e42cb77 100644
--- a/util/qemu-thread-win32.c
+++ b/util/qemu-thread-win32.c
@@ -16,6 +16,14 @@
 #include <assert.h>
 #include <limits.h>
 
+static bool name_threads;
+
+void qemu_thread_naming(bool enable)
+{
+    /* But note we don't actually name them on Windows yet */
+    name_threads = enable;
+}
+
 static void error_exit(int err, const char *msg)
 {
     char *pstr;
diff --git a/vl.c b/vl.c
index 5f993e4..8c515ae 100644
--- a/vl.c
+++ b/vl.c
@@ -547,6 +547,10 @@ static QemuOptsList qemu_name_opts = {
             .name = "process",
             .type = QEMU_OPT_STRING,
             .help = "Sets the name of the QEMU process, as shown in top etc",
+        }, {
+            .name = "namethreads",
+            .type = QEMU_OPT_BOOL,
+            .help = "When enabled, name the individual threads; defaults off",
         },
         { /* End of list */ }
     },
@@ -1006,6 +1010,9 @@ static void parse_name(QemuOpts *opts)
 {
     const char *proc_name;
 
+    if (qemu_opt_get(opts, "namethreads")) {
+        qemu_thread_naming(qemu_opt_get_bool(opts, "namethreads", false));
+    }
     qemu_name = qemu_opt_get(opts, "guest");
 
     proc_name = qemu_opt_get(opts, "process");
-- 
1.8.5.3

Re: [Qemu-devel] [PATCH 2/3] Add 'namethreads' suboption to --name

[Michael S. Tsirkin]

On Tue, Jan 28, 2014 at 03:20:38PM +0000, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> 
> Add flag storage to qemu-thread-* to store the namethreads flag
> 
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

How about a pattern?  threads='vmfoo-%s' and have qemu fill in
specific strings for %s.

> ---
>  include/qemu/thread.h    | 1 +
>  qemu-options.hx          | 6 ++++--
>  util/qemu-thread-posix.c | 7 +++++++
>  util/qemu-thread-win32.c | 8 ++++++++
>  vl.c                     | 7 +++++++
>  5 files changed, 27 insertions(+), 2 deletions(-)
> 
> diff --git a/include/qemu/thread.h b/include/qemu/thread.h
> index 3e32c65..bf1e110 100644
> --- a/include/qemu/thread.h
> +++ b/include/qemu/thread.h
> @@ -59,5 +59,6 @@ void *qemu_thread_join(QemuThread *thread);
>  void qemu_thread_get_self(QemuThread *thread);
>  bool qemu_thread_is_self(QemuThread *thread);
>  void qemu_thread_exit(void *retval);
> +void qemu_thread_naming(bool enable);
>  
>  #endif
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 56e5fdf..d53343a 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -328,9 +328,10 @@ possible drivers and properties, use @code{-device help} and
>  ETEXI
>  
>  DEF("name", HAS_ARG, QEMU_OPTION_name,
> -    "-name string1[,process=string2]\n"
> +    "-name string1[,process=string2][,namethreads=on|off]\n"
>      "                set the name of the guest\n"
> -    "                string1 sets the window title and string2 the process name (on Linux)\n",
> +    "                string1 sets the window title and string2 the process name (on Linux)\n"
> +    "                When namethreads is enabled, individual threads are given a separate name (on Linux)\n",
>      QEMU_ARCH_ALL)
>  STEXI
>  @item -name @var{name}
> @@ -339,6 +340,7 @@ Sets the @var{name} of the guest.
>  This name will be displayed in the SDL window caption.
>  The @var{name} will also be used for the VNC server.
>  Also optionally set the top visible process name in Linux.
> +Naming of individual threads can also be enabled on Linux.
>  ETEXI
>  
>  DEF("uuid", HAS_ARG, QEMU_OPTION_uuid,
> diff --git a/util/qemu-thread-posix.c b/util/qemu-thread-posix.c
> index 37dd298..0fa6c81 100644
> --- a/util/qemu-thread-posix.c
> +++ b/util/qemu-thread-posix.c
> @@ -27,6 +27,13 @@
>  #include "qemu/thread.h"
>  #include "qemu/atomic.h"
>  
> +static bool name_threads;
> +
> +void qemu_thread_naming(bool enable)
> +{
> +    name_threads = enable;
> +}
> +
>  static void error_exit(int err, const char *msg)
>  {
>      fprintf(stderr, "qemu: %s: %s\n", msg, strerror(err));
> diff --git a/util/qemu-thread-win32.c b/util/qemu-thread-win32.c
> index 27a5217..e42cb77 100644
> --- a/util/qemu-thread-win32.c
> +++ b/util/qemu-thread-win32.c
> @@ -16,6 +16,14 @@
>  #include <assert.h>
>  #include <limits.h>
>  
> +static bool name_threads;
> +
> +void qemu_thread_naming(bool enable)
> +{
> +    /* But note we don't actually name them on Windows yet */
> +    name_threads = enable;
> +}
> +
>  static void error_exit(int err, const char *msg)
>  {
>      char *pstr;
> diff --git a/vl.c b/vl.c
> index 5f993e4..8c515ae 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -547,6 +547,10 @@ static QemuOptsList qemu_name_opts = {
>              .name = "process",
>              .type = QEMU_OPT_STRING,
>              .help = "Sets the name of the QEMU process, as shown in top etc",
> +        }, {
> +            .name = "namethreads",
> +            .type = QEMU_OPT_BOOL,
> +            .help = "When enabled, name the individual threads; defaults off",
>          },
>          { /* End of list */ }
>      },
> @@ -1006,6 +1010,9 @@ static void parse_name(QemuOpts *opts)
>  {
>      const char *proc_name;
>  
> +    if (qemu_opt_get(opts, "namethreads")) {
> +        qemu_thread_naming(qemu_opt_get_bool(opts, "namethreads", false));
> +    }
>      qemu_name = qemu_opt_get(opts, "guest");
>  
>      proc_name = qemu_opt_get(opts, "process");
> -- 
> 1.8.5.3

Re: [Qemu-devel] [PATCH 2/3] Add 'namethreads' suboption to --name

[Dr. David Alan Gilbert]

* Michael S. Tsirkin (mst@redhat.com) wrote:
> On Tue, Jan 28, 2014 at 03:20:38PM +0000, Dr. David Alan Gilbert (git) wrote:
> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> > 
> > Add flag storage to qemu-thread-* to store the namethreads flag
> > 
> > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> 
> How about a pattern?  threads='vmfoo-%s' and have qemu fill in
> specific strings for %s.

The size of the name is limited to 16bytes according to the manpage
for pthread_setname_sn, so things start to get a bit squashed
if you try and have a vmname and anything else useful in the one name.

Dave
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH 2/3] Add 'namethreads' suboption to --name

[Michael S. Tsirkin]

On Tue, Jan 28, 2014 at 06:09:06PM +0000, Dr. David Alan Gilbert wrote:
> * Michael S. Tsirkin (mst@redhat.com) wrote:
> > On Tue, Jan 28, 2014 at 03:20:38PM +0000, Dr. David Alan Gilbert (git) wrote:
> > > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> > > 
> > > Add flag storage to qemu-thread-* to store the namethreads flag
> > > 
> > > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > 
> > How about a pattern?  threads='vmfoo-%s' and have qemu fill in
> > specific strings for %s.
> 
> The size of the name is limited to 16bytes according to the manpage
> for pthread_setname_sn, so things start to get a bit squashed
> if you try and have a vmname and anything else useful in the one name.
> 
> Dave

I see. Well, as long as it's a debug only flag, I guess that's fine.

> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

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

[Dr. David Alan Gilbert (git)]

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(-)

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

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

[Michael S. Tsirkin]

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

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

[Dr. David Alan Gilbert]

* 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

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

[Laszlo Ersek]

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

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

[Michael S. Tsirkin]

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

Re: [Qemu-devel] [PATCH 0/3] Name threads

[Paolo Bonzini]

Il 28/01/2014 16:20, Dr. David Alan Gilbert (git) ha scritto:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>
> This series uses pthread_setname_np (when available) to set the names on
> threads that QEMU creates to make life easier when debugging.
>
> It's turned off by default (because there were worries that it might break
> tools that relied on process names) but is enabled by adding
>
>    namethreads=on
>
> to the --name option.
>
> Note that the initial thread still has the default name (or the value passed
> as the process= parameter to --name).

I plead guilty to bikeshedding and propose s/namethreads/threads/.

Also, how did you test the merging of options?  Perhaps parse_name could 
itself call qemu_find_opts("name"), and could be called just once at the 
end of option parsing.  But that's just how *I* would have written the 
code, and what you have is fine if it works with multiple occurrences of 
-name.

Thanks for submitting this patch!

Paolo

Re: [Qemu-devel] [PATCH 0/3] Name threads

[Dr. David Alan Gilbert]

* Paolo Bonzini (pbonzini@redhat.com) wrote:
> Il 28/01/2014 16:20, Dr. David Alan Gilbert (git) ha scritto:
> >From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> >
> >This series uses pthread_setname_np (when available) to set the names on
> >threads that QEMU creates to make life easier when debugging.
> >
> >It's turned off by default (because there were worries that it might break
> >tools that relied on process names) but is enabled by adding
> >
> >   namethreads=on
> >
> >to the --name option.
> >
> >Note that the initial thread still has the default name (or the value passed
> >as the process= parameter to --name).
> 
> I plead guilty to bikeshedding and propose s/namethreads/threads/.

If I need to reroll anyway I'll change that; although I just suggested
making it debug-thread in my reply to Michael to make it clear it wasn't
an API.

> Also, how did you test the merging of options?  Perhaps parse_name
> could itself call qemu_find_opts("name"), and could be called just
> once at the end of option parsing.  But that's just how *I* would
> have written the code, and what you have is fine if it works with
> multiple occurrences of -name.

I was trying to follow the closest example I could find; but
the multiple --name seems to woke out ok; with a bit of debug
if I do:

 ./bin/qemu-system-x86_64 --name foo,namethreads=off --name process=bar --name namethreads=on
parse_name: setting namethreads=0
parse_name: qemu_name=foo proc_name=(null)
parse_name: setting namethreads=0
parse_name: qemu_name=foo proc_name=bar
parse_name: setting namethreads=1
parse_name: qemu_name=foo proc_name=bar

So I think that ends up with the right result; it doesn't stop you from repeating
options, but I think takes the last one.

Dave
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH 0/3] Name threads

[Paolo Bonzini]

Il 28/01/2014 17:31, Dr. David Alan Gilbert ha scritto:
>> > I plead guilty to bikeshedding and propose s/namethreads/threads/.
> If I need to reroll anyway I'll change that; although I just suggested
> making it debug-thread in my reply to Michael to make it clear it wasn't
> an API.

Even better.

>> > Also, how did you test the merging of options?  Perhaps parse_name
>> > could itself call qemu_find_opts("name"), and could be called just
>> > once at the end of option parsing.  But that's just how *I* would
>> > have written the code, and what you have is fine if it works with
>> > multiple occurrences of -name.
> I was trying to follow the closest example I could find; but
> the multiple --name seems to woke out ok; with a bit of debug
> if I do:
>
>  ./bin/qemu-system-x86_64 --name foo,namethreads=off --name process=bar --name namethreads=on
> parse_name: setting namethreads=0
> parse_name: qemu_name=foo proc_name=(null)
> parse_name: setting namethreads=0
> parse_name: qemu_name=foo proc_name=bar
> parse_name: setting namethreads=1
> parse_name: qemu_name=foo proc_name=bar
>
> So I think that ends up with the right result; it doesn't stop you from repeating
> options, but I think takes the last one.

Good, thanks!

Paolo