Patchwork [v4,08/16] QemuThread: make QemuThread as tls to store extra info

login
register
mail settings
Submitter pingfank@linux.vnet.ibm.com
Date Oct. 22, 2012, 9:23 a.m.
Message ID <1350897839-29593-9-git-send-email-pingfank@linux.vnet.ibm.com>
Download mbox | patch
Permalink /patch/193195/
State New
Headers show

Comments

pingfank@linux.vnet.ibm.com - Oct. 22, 2012, 9:23 a.m.
If mmio dispatch out of big lock, some function's calling context (ie,
holding big lock or not) are different. We need to trace these info in
runtime, and use tls to store them.
By this method, we can avoid to require big lock recursive.

Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
---
 cpus.c              |    1 +
 qemu-thread-posix.c |    7 +++++++
 qemu-thread-posix.h |    2 ++
 qemu-thread.h       |    1 +
 vl.c                |    6 ++++++
 5 files changed, 17 insertions(+), 0 deletions(-)
Peter Maydell - Oct. 22, 2012, 5:13 p.m.
On 22 October 2012 10:30, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> Can't we enhance qemu-tls.h to work via pthread_setspecific in case
> __thread is not working and use that abstraction (DECLARE/DEFINE_TLS)
> directly?

Agreed. (There were prototype patches floating around for Win32
at least). The only reason qemu-tls.h has the dummy not-actually-tls
code for non-linux is that IIRC we wanted to get the linux bits
in quickly before a release and we never got round to going back
and doing it properly for the other targets.

-- PMM
pingfan liu - Oct. 23, 2012, 5:58 a.m.
On Tue, Oct 23, 2012 at 1:13 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 22 October 2012 10:30, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>> Can't we enhance qemu-tls.h to work via pthread_setspecific in case
>> __thread is not working and use that abstraction (DECLARE/DEFINE_TLS)
>> directly?
>
> Agreed. (There were prototype patches floating around for Win32
> at least). The only reason qemu-tls.h has the dummy not-actually-tls
> code for non-linux is that IIRC we wanted to get the linux bits
> in quickly before a release and we never got round to going back
> and doing it properly for the other targets.
>
Oh, it seems that my need of it can be covered by __thread. I will
resort to it.  BTW, what is the exception that __thread can resolve
(if ELF can support this keyword)?

Thanks and regards,
pingfan
Paolo Bonzini - Oct. 23, 2012, 11:48 a.m.
Il 22/10/2012 19:13, Peter Maydell ha scritto:
>> > Can't we enhance qemu-tls.h to work via pthread_setspecific in case
>> > __thread is not working and use that abstraction (DECLARE/DEFINE_TLS)
>> > directly?
> Agreed. (There were prototype patches floating around for Win32
> at least). The only reason qemu-tls.h has the dummy not-actually-tls
> code for non-linux is that IIRC we wanted to get the linux bits
> in quickly before a release and we never got round to going back
> and doing it properly for the other targets.

Which will be "never" for OpenBSD.  It just doesn't have enough support.

Thread-wise OpenBSD is 100% crap, and we should stop supporting it IMHO
until they finish their "new" thread library that's been in the works
for 10 years or so.  FreeBSD is totally ok.

Paolo
Peter Maydell - Oct. 23, 2012, 11:50 a.m.
On 23 October 2012 12:48, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 22/10/2012 19:13, Peter Maydell ha scritto:
>>> > Can't we enhance qemu-tls.h to work via pthread_setspecific in case
>>> > __thread is not working and use that abstraction (DECLARE/DEFINE_TLS)
>>> > directly?
>> Agreed. (There were prototype patches floating around for Win32
>> at least). The only reason qemu-tls.h has the dummy not-actually-tls
>> code for non-linux is that IIRC we wanted to get the linux bits
>> in quickly before a release and we never got round to going back
>> and doing it properly for the other targets.
>
> Which will be "never" for OpenBSD.  It just doesn't have enough support.
>
> Thread-wise OpenBSD is 100% crap, and we should stop supporting it IMHO
> until they finish their "new" thread library that's been in the works
> for 10 years or so.  FreeBSD is totally ok.

It doesn't support any kind of TLS? Wow.

-- PMM
Jan Kiszka - Oct. 23, 2012, 11:51 a.m.
On 2012-10-23 13:50, Peter Maydell wrote:
> On 23 October 2012 12:48, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> Il 22/10/2012 19:13, Peter Maydell ha scritto:
>>>>> Can't we enhance qemu-tls.h to work via pthread_setspecific in case
>>>>> __thread is not working and use that abstraction (DECLARE/DEFINE_TLS)
>>>>> directly?
>>> Agreed. (There were prototype patches floating around for Win32
>>> at least). The only reason qemu-tls.h has the dummy not-actually-tls
>>> code for non-linux is that IIRC we wanted to get the linux bits
>>> in quickly before a release and we never got round to going back
>>> and doing it properly for the other targets.
>>
>> Which will be "never" for OpenBSD.  It just doesn't have enough support.
>>
>> Thread-wise OpenBSD is 100% crap, and we should stop supporting it IMHO
>> until they finish their "new" thread library that's been in the works
>> for 10 years or so.  FreeBSD is totally ok.
> 
> It doesn't support any kind of TLS? Wow.

It's probably more secure.

Jan
Paolo Bonzini - Oct. 23, 2012, noon
Il 23/10/2012 13:50, Peter Maydell ha scritto:
> On 23 October 2012 12:48, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> Il 22/10/2012 19:13, Peter Maydell ha scritto:
>>>>> Can't we enhance qemu-tls.h to work via pthread_setspecific in case
>>>>> __thread is not working and use that abstraction (DECLARE/DEFINE_TLS)
>>>>> directly?
>>> Agreed. (There were prototype patches floating around for Win32
>>> at least). The only reason qemu-tls.h has the dummy not-actually-tls
>>> code for non-linux is that IIRC we wanted to get the linux bits
>>> in quickly before a release and we never got round to going back
>>> and doing it properly for the other targets.
>>
>> Which will be "never" for OpenBSD.  It just doesn't have enough support.
>>
>> Thread-wise OpenBSD is 100% crap, and we should stop supporting it IMHO
>> until they finish their "new" thread library that's been in the works
>> for 10 years or so.  FreeBSD is totally ok.
> 
> It doesn't support any kind of TLS? Wow.

It does support pthread_get/setspecific, but it didn't support something
else so the qemu-tls.h variant that used pthread_get/setspecific didn't
work either.

And it doesn't support sigaltstack in threads, so it's the only platform
where the gthread-based coroutines are used.  Those are buggy because
the coroutines tend to get random signal masks.

Paolo
Peter Maydell - Oct. 23, 2012, 12:27 p.m.
On 23 October 2012 13:00, Paolo Bonzini <pbonzini@redhat.com> wrote:
> It does support pthread_get/setspecific, but it didn't support something
> else so the qemu-tls.h variant that used pthread_get/setspecific didn't
> work either.
>
> And it doesn't support sigaltstack in threads, so it's the only platform
> where the gthread-based coroutines are used.  Those are buggy because
> the coroutines tend to get random signal masks.

MacOS uses the gthread version too. In fact anything that doesn't
doesn't use makecontext will use gthread -- you won't get the
sigaltstack version unless you explicitly ask configure for it.

[insert usual rant here about what a bad idea coroutines are]

-- PMM
Brad - Nov. 18, 2012, 10:02 a.m.
On 10/23/12 08:00, Paolo Bonzini wrote:
> Il 23/10/2012 13:50, Peter Maydell ha scritto:
>> On 23 October 2012 12:48, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>> Il 22/10/2012 19:13, Peter Maydell ha scritto:
>>>>>> Can't we enhance qemu-tls.h to work via pthread_setspecific in case
>>>>>> __thread is not working and use that abstraction (DECLARE/DEFINE_TLS)
>>>>>> directly?
>>>> Agreed. (There were prototype patches floating around for Win32
>>>> at least). The only reason qemu-tls.h has the dummy not-actually-tls
>>>> code for non-linux is that IIRC we wanted to get the linux bits
>>>> in quickly before a release and we never got round to going back
>>>> and doing it properly for the other targets.
>>>
>>> Which will be "never" for OpenBSD.  It just doesn't have enough support.
>>>
>>> Thread-wise OpenBSD is 100% crap, and we should stop supporting it IMHO
>>> until they finish their "new" thread library that's been in the works
>>> for 10 years or so.  FreeBSD is totally ok.
>>
>> It doesn't support any kind of TLS? Wow.
>
> It does support pthread_get/setspecific, but it didn't support something
> else so the qemu-tls.h variant that used pthread_get/setspecific didn't
> work either.
>
> And it doesn't support sigaltstack in threads, so it's the only platform
> where the gthread-based coroutines are used.  Those are buggy because
> the coroutines tend to get random signal masks.
>
> Paolo

I'd love to know what that something else is.

There is a diff pending to fix sigaltstack in threads which should
be going into -current very soon.
Paolo Bonzini - Nov. 18, 2012, 4:14 p.m.
> > It does support pthread_get/setspecific, but it didn't support something
> > else so the qemu-tls.h variant that used pthread_get/setspecific didn't
> > work either.
> >
> > And it doesn't support sigaltstack in threads, so it's the only platform
> > where the gthread-based coroutines are used.  Those are buggy because
> > the coroutines tend to get random signal masks.
> 
> I'd love to know what that something else is.

I think it is constructor priorities.  Probably not needed if I
look at the code again with a fresh mind. :)

But yes
Paolo
Paolo Bonzini - Nov. 18, 2012, 4:15 p.m.
> > > It does support pthread_get/setspecific, but it didn't support
> > > something
> > > else so the qemu-tls.h variant that used pthread_get/setspecific
> > > didn't
> > > work either.
> > >
> > > And it doesn't support sigaltstack in threads, so it's the only
> > > platform
> > > where the gthread-based coroutines are used.  Those are buggy
> > > because
> > > the coroutines tend to get random signal masks.
> > 
> > I'd love to know what that something else is.
> 
> I think it is constructor priorities.  Probably not needed if I
> look at the code again with a fresh mind. :)
> 
> But yes

... now that real pthreads are supported in OpenBSD it's a wholly
different story, and we should simply (in 1.4) stop supporting older
versions.  (Sincere) congratulations to the OpenBSD devs!

Paolo

Patch

diff --git a/cpus.c b/cpus.c
index e476a3c..4cd7f85 100644
--- a/cpus.c
+++ b/cpus.c
@@ -735,6 +735,7 @@  static void *qemu_kvm_cpu_thread_fn(void *arg)
     CPUState *cpu = ENV_GET_CPU(env);
     int r;
 
+    pthread_setspecific(qemu_thread_key, cpu->thread);
     qemu_mutex_lock(&qemu_global_mutex);
     qemu_thread_get_self(cpu->thread);
     env->thread_id = qemu_get_thread_id();
diff --git a/qemu-thread-posix.c b/qemu-thread-posix.c
index 8fbabda..f448fcb 100644
--- a/qemu-thread-posix.c
+++ b/qemu-thread-posix.c
@@ -19,6 +19,8 @@ 
 #include <string.h>
 #include "qemu-thread.h"
 
+pthread_key_t qemu_thread_key;
+
 static void error_exit(int err, const char *msg)
 {
     fprintf(stderr, "qemu: %s: %s\n", msg, strerror(err));
@@ -151,6 +153,11 @@  void qemu_thread_get_self(QemuThread *thread)
     thread->thread = pthread_self();
 }
 
+void qemu_thread_key_create(void)
+{
+    pthread_key_create(&qemu_thread_key, NULL);
+}
+
 bool qemu_thread_is_self(QemuThread *thread)
 {
    return pthread_equal(pthread_self(), thread->thread);
diff --git a/qemu-thread-posix.h b/qemu-thread-posix.h
index ee4618e..2607b1c 100644
--- a/qemu-thread-posix.h
+++ b/qemu-thread-posix.h
@@ -14,4 +14,6 @@  struct QemuThread {
     pthread_t thread;
 };
 
+extern pthread_key_t qemu_thread_key;
+
 #endif
diff --git a/qemu-thread.h b/qemu-thread.h
index 05fdaaf..4a6427d 100644
--- a/qemu-thread.h
+++ b/qemu-thread.h
@@ -46,4 +46,5 @@  void qemu_thread_get_self(QemuThread *thread);
 bool qemu_thread_is_self(QemuThread *thread);
 void qemu_thread_exit(void *retval);
 
+void qemu_thread_key_create(void);
 #endif
diff --git a/vl.c b/vl.c
index 7c577fa..442479a 100644
--- a/vl.c
+++ b/vl.c
@@ -149,6 +149,7 @@  int main(int argc, char **argv)
 #include "qemu-options.h"
 #include "qmp-commands.h"
 #include "main-loop.h"
+#include "qemu-thread.h"
 #ifdef CONFIG_VIRTFS
 #include "fsdev/qemu-fsdev.h"
 #endif
@@ -2342,6 +2343,7 @@  int qemu_init_main_loop(void)
     return main_loop_init();
 }
 
+
 int main(int argc, char **argv, char **envp)
 {
     int i;
@@ -3483,6 +3485,10 @@  int main(int argc, char **argv, char **envp)
         exit(1);
     }
 
+    qemu_thread_key_create();
+    QemuThread *ioctx = g_malloc0(sizeof(QemuThread));
+    pthread_setspecific(qemu_thread_key, ioctx);
+
     os_set_line_buffering();
 
     if (init_timer_alarm() < 0) {