| Message ID | 531F3083.9090304@siemens.com |
|---|---|
| State | New |
| Headers | show |
On 11 March 2014 15:49, Jan Kiszka <jan.kiszka@siemens.com> wrote: > pthread_setname_np was introduced with 2.12. > > Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> > --- > util/qemu-thread-posix.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/util/qemu-thread-posix.c b/util/qemu-thread-posix.c > index 45113b4..960d7f5 100644 > --- a/util/qemu-thread-posix.c > +++ b/util/qemu-thread-posix.c > @@ -420,7 +420,7 @@ void qemu_thread_create(QemuThread *thread, const char *name, > if (err) > error_exit(err, __func__); > > -#ifdef _GNU_SOURCE > +#if defined(__GLIBC__) && (__GLIBC__ > 2 || (__GLIBC__ == 2 && __GLIBC_MINOR__ >= 12)) > if (name_threads) { > pthread_setname_np(thread->thread, name); > } > -- > 1.8.1.1.298.ge7eed54 Can we have a configure test for the function instead, please? We don't generally do tests for specific glibc versions (among other things, glibc is not the only C library we might be compiled against -- consider MacOSX, the BSDs, and occasionally somebody tries to compile against one of the embedded libcs). thanks -- PMM
* Peter Maydell (peter.maydell@linaro.org) wrote: > On 11 March 2014 15:49, Jan Kiszka <jan.kiszka@siemens.com> wrote: > > pthread_setname_np was introduced with 2.12. > > > > Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> > > --- > > util/qemu-thread-posix.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/util/qemu-thread-posix.c b/util/qemu-thread-posix.c > > index 45113b4..960d7f5 100644 > > --- a/util/qemu-thread-posix.c > > +++ b/util/qemu-thread-posix.c > > @@ -420,7 +420,7 @@ void qemu_thread_create(QemuThread *thread, const char *name, > > if (err) > > error_exit(err, __func__); > > > > -#ifdef _GNU_SOURCE > > +#if defined(__GLIBC__) && (__GLIBC__ > 2 || (__GLIBC__ == 2 && __GLIBC_MINOR__ >= 12)) > > if (name_threads) { > > pthread_setname_np(thread->thread, name); > > } > > -- > > 1.8.1.1.298.ge7eed54 > > Can we have a configure test for the function instead, please? > We don't generally do tests for specific glibc versions > (among other things, glibc is not the only C library we > might be compiled against -- consider MacOSX, the BSDs, > and occasionally somebody tries to compile against one > of the embedded libcs). Except pthread_setname_np is not portable and was previously ifdef'd _GNU_SOURCE anyway, and the parameters on other OSs maybe different (freebsd has got a 3rd parameter for no apparent reason). Dave -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On 11 March 2014 16:13, Dr. David Alan Gilbert <dgilbert@redhat.com> wrote: > * Peter Maydell (peter.maydell@linaro.org) wrote: >> Can we have a configure test for the function instead, please? >> We don't generally do tests for specific glibc versions >> (among other things, glibc is not the only C library we >> might be compiled against -- consider MacOSX, the BSDs, >> and occasionally somebody tries to compile against one >> of the embedded libcs). > > Except pthread_setname_np is not portable and was previously > ifdef'd _GNU_SOURCE anyway I still think it's a generally worse way to do checks. We currently have only one check against __GLIBC_MINOR__ in the tree (and that's in extremely Linux-specific code). > the parameters on other OSs > maybe different (freebsd has got a 3rd parameter for no > apparent reason). This rather suggests we should abstract the "set thread name" functionality out into its own function so we can easily provide other implementations for those other OSes later. thanks -- PMM
* Peter Maydell (peter.maydell@linaro.org) wrote: > On 11 March 2014 16:13, Dr. David Alan Gilbert <dgilbert@redhat.com> wrote: > > * Peter Maydell (peter.maydell@linaro.org) wrote: > >> Can we have a configure test for the function instead, please? > >> We don't generally do tests for specific glibc versions > >> (among other things, glibc is not the only C library we > >> might be compiled against -- consider MacOSX, the BSDs, > >> and occasionally somebody tries to compile against one > >> of the embedded libcs). > > > > Except pthread_setname_np is not portable and was previously > > ifdef'd _GNU_SOURCE anyway > > I still think it's a generally worse way to do checks. > We currently have only one check against __GLIBC_MINOR__ > in the tree (and that's in extremely Linux-specific > code). > > > the parameters on other OSs > > maybe different (freebsd has got a 3rd parameter for no > > apparent reason). > > This rather suggests we should abstract the "set thread > name" functionality out into its own function so we > can easily provide other implementations for those > other OSes later. OK, well there is already a void os_set_proc_name(const char *s) in os-posix.c but it has ~2 problems: 1) If the OS doesn't support doing it that way it exit(1)'s 2) it uses prctl to set it on the current thread, where pthread_setname_np has the advantage that it sets it on something you have a pthread* for. 3) There isn't a matching entry in os-win32.c is the best thing to add another function to os-posix and os-win then? Dave -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On 11 March 2014 16:36, Dr. David Alan Gilbert <dgilbert@redhat.com> wrote: > * Peter Maydell (peter.maydell@linaro.org) wrote: >> On 11 March 2014 16:13, Dr. David Alan Gilbert <dgilbert@redhat.com> wrote: >> > * Peter Maydell (peter.maydell@linaro.org) wrote: >> >> Can we have a configure test for the function instead, please? >> >> We don't generally do tests for specific glibc versions >> >> (among other things, glibc is not the only C library we >> >> might be compiled against -- consider MacOSX, the BSDs, >> >> and occasionally somebody tries to compile against one >> >> of the embedded libcs). >> > >> > Except pthread_setname_np is not portable and was previously >> > ifdef'd _GNU_SOURCE anyway >> >> I still think it's a generally worse way to do checks. >> We currently have only one check against __GLIBC_MINOR__ >> in the tree (and that's in extremely Linux-specific >> code). >> >> > the parameters on other OSs >> > maybe different (freebsd has got a 3rd parameter for no >> > apparent reason). >> >> This rather suggests we should abstract the "set thread >> name" functionality out into its own function so we >> can easily provide other implementations for those >> other OSes later. > > OK, well there is already a > > void os_set_proc_name(const char *s) > > in os-posix.c but it has ~2 problems: > 1) If the OS doesn't support doing it that way it exit(1)'s > 2) it uses prctl to set it on the current thread, where > pthread_setname_np has the advantage that it sets it on > something you have a pthread* for. > 3) There isn't a matching entry in os-win32.c > > is the best thing to add another function to os-posix and os-win > then? Given this function's only for use in qemu-thread-posix.c I think it would be reasonable to just have it live there. It would be nicer to have an static void qemu_thread_set_name(QemuThread *thread, const char *name) { #ifdef CONFIG_THREAD_SETNAME_NP pthread_setname_np(thread->thread, name); #endif } rather than #ifdefs in the middle of functions, that's all. thanks -- PMM
* Jan Kiszka (jan.kiszka@siemens.com) wrote: > pthread_setname_np was introduced with 2.12. > > Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com> I'll polish a configure/separate function as suggested by Peter; let's get the simple fix in quickly though to unbreak people while I do that. Dave > --- > util/qemu-thread-posix.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/util/qemu-thread-posix.c b/util/qemu-thread-posix.c > index 45113b4..960d7f5 100644 > --- a/util/qemu-thread-posix.c > +++ b/util/qemu-thread-posix.c > @@ -420,7 +420,7 @@ void qemu_thread_create(QemuThread *thread, const char *name, > if (err) > error_exit(err, __func__); > > -#ifdef _GNU_SOURCE > +#if defined(__GLIBC__) && (__GLIBC__ > 2 || (__GLIBC__ == 2 && __GLIBC_MINOR__ >= 12)) > if (name_threads) { > pthread_setname_np(thread->thread, name); > } > -- > 1.8.1.1.298.ge7eed54 > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On Tue, Mar 11, 2014 at 05:51:16PM +0000, Peter Maydell wrote: > On 11 March 2014 16:36, Dr. David Alan Gilbert <dgilbert@redhat.com> wrote: > > * Peter Maydell (peter.maydell@linaro.org) wrote: > >> On 11 March 2014 16:13, Dr. David Alan Gilbert <dgilbert@redhat.com> wrote: > >> > * Peter Maydell (peter.maydell@linaro.org) wrote: > >> >> Can we have a configure test for the function instead, please? > >> >> We don't generally do tests for specific glibc versions > >> >> (among other things, glibc is not the only C library we > >> >> might be compiled against -- consider MacOSX, the BSDs, > >> >> and occasionally somebody tries to compile against one > >> >> of the embedded libcs). > >> > > >> > Except pthread_setname_np is not portable and was previously > >> > ifdef'd _GNU_SOURCE anyway > >> > >> I still think it's a generally worse way to do checks. > >> We currently have only one check against __GLIBC_MINOR__ > >> in the tree (and that's in extremely Linux-specific > >> code). > >> > >> > the parameters on other OSs > >> > maybe different (freebsd has got a 3rd parameter for no > >> > apparent reason). > >> > >> This rather suggests we should abstract the "set thread > >> name" functionality out into its own function so we > >> can easily provide other implementations for those > >> other OSes later. > > > > OK, well there is already a > > > > void os_set_proc_name(const char *s) > > > > in os-posix.c but it has ~2 problems: > > 1) If the OS doesn't support doing it that way it exit(1)'s > > 2) it uses prctl to set it on the current thread, where > > pthread_setname_np has the advantage that it sets it on > > something you have a pthread* for. > > 3) There isn't a matching entry in os-win32.c > > > > is the best thing to add another function to os-posix and os-win > > then? > > Given this function's only for use in qemu-thread-posix.c > I think it would be reasonable to just have it live there. > It would be nicer to have an > > static void qemu_thread_set_name(QemuThread *thread, const char *name) > { > #ifdef CONFIG_THREAD_SETNAME_NP > pthread_setname_np(thread->thread, name); > #endif > } > > rather than #ifdefs in the middle of functions, > that's all. > > thanks > -- PMM Shouldn't we prevent the unsupported option from appearing in qemu output though? Otherwise how does user know whether it works?
On 11 March 2014 18:08, Dr. David Alan Gilbert <dgilbert@redhat.com> wrote: > * Jan Kiszka (jan.kiszka@siemens.com) wrote: >> pthread_setname_np was introduced with 2.12. >> >> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> > > Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > > I'll polish a configure/separate function as suggested by Peter; > let's get the simple fix in quickly though to unbreak people > while I do that. Applied to master as a buildfix; thanks. -- PMM
* Michael S. Tsirkin (mst@redhat.com) wrote: > Shouldn't we prevent the unsupported option from > appearing in qemu output though? > Otherwise how does user know whether it works? I'll make it spit a warning if you try and enable it on a platform that doesn't have a way of doing it; ifdef all the option parsing would just make everywhere messy and in the end we defined it as a debug option. Dave -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On 11 March 2014 19:46, Dr. David Alan Gilbert <dgilbert@redhat.com> wrote: > * Michael S. Tsirkin (mst@redhat.com) wrote: > >> Shouldn't we prevent the unsupported option from >> appearing in qemu output though? >> Otherwise how does user know whether it works? > > I'll make it spit a warning if you try and enable it on a platform > that doesn't have a way of doing it; ifdef all the option parsing > would just make everywhere messy and in the end we defined it > as a debug option. When would you want to not enable it? If this is purely for debug convenience why not just have "always set thread names if the platform allows it" ? thanks -- PMM
On Tue, Mar 11, 2014 at 07:53:20PM +0000, Peter Maydell wrote: > On 11 March 2014 19:46, Dr. David Alan Gilbert <dgilbert@redhat.com> wrote: > > * Michael S. Tsirkin (mst@redhat.com) wrote: > > > >> Shouldn't we prevent the unsupported option from > >> appearing in qemu output though? > >> Otherwise how does user know whether it works? > > > > I'll make it spit a warning if you try and enable it on a platform > > that doesn't have a way of doing it; ifdef all the option parsing > > would just make everywhere messy and in the end we defined it > > as a debug option. > > When would you want to not enable it? If this is > purely for debug convenience why not just have "always > set thread names if the platform allows it" ? > > thanks > -- PMM Because we don't want people to start relying on thread naming to manage the threads outside qemu. Then we won't be able to change the threading model. In particular that was the reason we don't have a monitor command to expose thread IDs for threads except VCPUs (for which the reasoning is 1 thread per VCPU is the only thing allowed by KVM). There also can be existing tools that would break if we changed thread names.
* Peter Maydell (peter.maydell@linaro.org) wrote: > On 11 March 2014 19:46, Dr. David Alan Gilbert <dgilbert@redhat.com> wrote: > > * Michael S. Tsirkin (mst@redhat.com) wrote: > > > >> Shouldn't we prevent the unsupported option from > >> appearing in qemu output though? > >> Otherwise how does user know whether it works? > > > > I'll make it spit a warning if you try and enable it on a platform > > that doesn't have a way of doing it; ifdef all the option parsing > > would just make everywhere messy and in the end we defined it > > as a debug option. > > When would you want to not enable it? If this is > purely for debug convenience why not just have "always > set thread names if the platform allows it" ? See the original thread; there was some undefined worry that some tools somewhere might break if it was enabled. http://lists.gnu.org/archive/html/qemu-devel/2014-01/msg03819.html Dave > thanks > -- PMM -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On 11 March 2014 20:03, Michael S. Tsirkin <mst@redhat.com> wrote: > Because we don't want people to start relying on thread naming to > manage the threads outside qemu. Then document that it isn't stable; anybody who relies on it deserves what they get. If we put a command line argument on it then people will still be able to incorrectly rely on thread naming by using the command line option. > There also can be existing tools that would break if > we changed thread names. In this case surely we are already stuck with our current threading model? I would be anyway happy to say "you relied on undefined and unstable behaviour so tough"... On the other hand, a new command line switch really is user interface which we have to carry around and support for ever. thanks -- PMM
On Tue, Mar 11, 2014 at 08:27:58PM +0000, Peter Maydell wrote: > On 11 March 2014 20:03, Michael S. Tsirkin <mst@redhat.com> wrote: > > Because we don't want people to start relying on thread naming to > > manage the threads outside qemu. > > Then document that it isn't stable; anybody who relies > on it deserves what they get. > If we put a command line > argument on it then people will still be able to incorrectly > rely on thread naming by using the command line option. Where would you document this? The idea was that "debug" in name is a hint. > > There also can be existing tools that would break if > > we changed thread names. > > In this case surely we are already stuck with our current > threading model? I would be anyway happy to say "you > relied on undefined and unstable behaviour so tough"... > > On the other hand, a new command line switch really is > user interface which we have to carry around and support > for ever. > > thanks > -- PMM
On 03/11/2014 02:35 PM, Michael S. Tsirkin wrote: >> If we put a command line >> argument on it then people will still be able to incorrectly >> rely on thread naming by using the command line option. > > > Where would you document this? > > The idea was that "debug" in name is a hint. Or even name it -x-debug, since we've already documented that any API prefixed with x- is unstable and may go away in future releases.
On 11 March 2014 12:13, Dr. David Alan Gilbert <dgilbert@redhat.com> wrote: > Except pthread_setname_np is not portable and was previously > ifdef'd _GNU_SOURCE anyway, and the parameters on other OSs > maybe different (freebsd has got a 3rd parameter for no > apparent reason). No, glibc's arguments are identical to FreeBSD's. The difference is that it's pthread_set_name_np (with an extra underscore). FreeBSD: void pthread_set_name_np(pthread_t, const char *); glibc: int pthread_setname_np(pthread_t, const char *); NetBSD: int pthread_setname_np(pthread_t, const char *, void *); Darwin: pthread_setname_np(const char *);
* Ed Maste (emaste@freebsd.org) wrote: > On 11 March 2014 12:13, Dr. David Alan Gilbert <dgilbert@redhat.com> wrote: > > > Except pthread_setname_np is not portable and was previously > > ifdef'd _GNU_SOURCE anyway, and the parameters on other OSs > > maybe different (freebsd has got a 3rd parameter for no > > apparent reason). > > No, glibc's arguments are identical to FreeBSD's. The difference is > that it's pthread_set_name_np (with an extra underscore). > > FreeBSD: void pthread_set_name_np(pthread_t, const char *); > glibc: int pthread_setname_np(pthread_t, const char *); > NetBSD: int pthread_setname_np(pthread_t, const char *, void *); > Darwin: pthread_setname_np(const char *); Ah, apologies - I got the wrong *BSD for the 3rd argument stuff. Anyway, feel free to add your favorite OS to the configure check/use. Dave -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
diff --git a/util/qemu-thread-posix.c b/util/qemu-thread-posix.c index 45113b4..960d7f5 100644 --- a/util/qemu-thread-posix.c +++ b/util/qemu-thread-posix.c @@ -420,7 +420,7 @@ void qemu_thread_create(QemuThread *thread, const char *name, if (err) error_exit(err, __func__); -#ifdef _GNU_SOURCE +#if defined(__GLIBC__) && (__GLIBC__ > 2 || (__GLIBC__ == 2 && __GLIBC_MINOR__ >= 12)) if (name_threads) { pthread_setname_np(thread->thread, name); }
pthread_setname_np was introduced with 2.12. Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> --- util/qemu-thread-posix.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)