coroutine: Fix win32 variant for older mingw32 compilers

PATCH In review Jan Kiszka 4 participants 12 messages Nov 21, 2012 – Nov 22, 2012

Share & tools

Jan Kiszka

author

2012-11-21 14:01 UTC #0 lore raw

PATCH 1/1 coroutine: Fix win32 variant for older mingw32 compilers

mingw32 with gcc up to (at least) 4.4 has broken __thread support. This means all __thread variables silently become global ones.

Address this by switching the coroutine implementation for win32 to Get/SetTlsValue.

Signed-off-by Jan Kiszka <[email protected]>

---
 coroutine-win32.c |   29 ++++++++++++++++++++++-------
 1 files changed, 22 insertions(+), 7 deletions(-)

coroutine-win32.c +22 -7

index 4179609..bf0aeac 100644
@@ -33,8 +33,7 @@ typedef struct
     CoroutineAction action;
 } CoroutineWin32;
 
-static __thread CoroutineWin32 leader;
-static __thread Coroutine *current;
+static int current_tls_index = TLS_OUT_OF_INDEXES;
 
 CoroutineAction qemu_coroutine_switch(Coroutine *from_, Coroutine *to_,
                                       CoroutineAction action)

@@ -42,7 +41,7 @@ CoroutineAction qemu_coroutine_switch(Coroutine *from_, Coroutine *to_,
     CoroutineWin32 *from = DO_UPCAST(CoroutineWin32, base, from_);
     CoroutineWin32 *to = DO_UPCAST(CoroutineWin32, base, to_);
 
-    current = to_;
+    TlsSetValue(current_tls_index, to_);
 
     to->action = action;
     SwitchToFiber(to->fiber);

@@ -79,14 +78,30 @@ void qemu_coroutine_delete(Coroutine *co_)
 
 Coroutine *qemu_coroutine_self(void)
 {
-    if (!current) {
-        current = &leader.base;
-        leader.fiber = ConvertThreadToFiber(NULL);
+    CoroutineWin32 *leader;
+
+    if (current_tls_index == TLS_OUT_OF_INDEXES) {
+        current_tls_index = TlsAlloc();
+        if (current_tls_index == TLS_OUT_OF_INDEXES) {
+            fprintf(stderr, "qemu: %s: out of TLS handles\n", __func__);
+            abort();
+        }
+    }
+    if (!TlsGetValue(current_tls_index)) {
+        leader = g_malloc0(sizeof(*leader));
+        leader->fiber = ConvertThreadToFiber(NULL);
+        TlsSetValue(current_tls_index, &leader->base);
     }
-    return current;
+    return TlsGetValue(current_tls_index);
 }
 
 bool qemu_in_coroutine(void)
 {
+    Coroutine *current;
+
+    if (current_tls_index == TLS_OUT_OF_INDEXES) {
+        return false;
+    }
+    current = TlsGetValue(current_tls_index);
     return current && current->caller;
 }

Signature · 1 line

1.7.3.4

Paolo Bonzini

2012-11-21 14:08 UTC #1 lore raw

Il 21/11/2012 15:00, Jan Kiszka ha scritto:

coroutine-win32.c void qemu_coroutine_delete(Coroutine *co_) · line 94

Quoted 5 lines

coroutine-win32.c +4 -0

+    if (!TlsGetValue(current_tls_index)) {
+        leader = g_malloc0(sizeof(*leader));
+        leader->fiber = ConvertThreadToFiber(NULL);
+        TlsSetValue(current_tls_index, &leader->base);
     }

Leaking leader is a bit bad, but it looks ok for 1.3.

For 1.4 we really should bring back a portable solution for TLS that includes statically-allocated data.

Paolo

Jan Kiszka

author

2012-11-21 14:30 UTC #2 lore raw

Quoted 8 lines · Paolo Bonzini

Il 21/11/2012 15:00, Jan Kiszka ha scritto:
coroutine-win32.c void qemu_coroutine_delete(Coroutine *co_) · line 94
Quoted 5 lines
coroutine-win32.c +4 -0
+    if (!TlsGetValue(current_tls_index)) {
+        leader = g_malloc0(sizeof(*leader));
+        leader->fiber = ConvertThreadToFiber(NULL);
+        TlsSetValue(current_tls_index, &leader->base);
     }
Leaking leader is a bit bad, but it looks ok for 1.3.

Hmm. A TLS destructor is apparently not available. Is there some "on thread termination" callback mechanism on Windows? Didn't find one on first glance.

Jan

Signature · 2 lines

Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux

malc

2012-11-21 14:34 UTC #3 lore raw

Quoted 8 lines · Jan Kiszka

Quoted 7 lines · Paolo Bonzini

Il 21/11/2012 15:00, Jan Kiszka ha scritto:

Quoted 5 lines

coroutine-win32.c +4 -0

+    if (!TlsGetValue(current_tls_index)) {
+        leader = g_malloc0(sizeof(*leader));
+        leader->fiber = ConvertThreadToFiber(NULL);
+        TlsSetValue(current_tls_index, &leader->base);
     }

Quoted 6 lines

Quoted 1 line
Leaking leader is a bit bad, but it looks ok for 1.3.
Hmm. A TLS destructor is apparently not available. Is there some "on thread termination" callback mechanism on Windows? Didn't find one on first glance.

Dlls receive something like THREAD_DETTACH in it's startup routine or something like that if my memory serves me.

Signature · 1 line

mailto:[email protected]

Paolo Bonzini

2012-11-21 14:38 UTC #4 lore raw

Il 21/11/2012 15:33, malc ha scritto:

Quoted 8 lines

Quoted 6 lines
Quoted 6 lines
Quoted 1 line
Quoted 1 line
Leaking leader is a bit bad, but it looks ok for 1.3.
Hmm. A TLS destructor is apparently not available. Is there some "on thread termination" callback mechanism on Windows? Didn't find one on first glance.
Dlls receive something like THREAD_DETTACH in it's startup routine or something like that if my memory serves me.

Only DLLs.

But this sounds like deja-vu. I'm pretty sure in the past we just decided that this compiler is not supported (of course it's bad that it's silent). Stefan, do you remember the details?

Paolo

Jan Kiszka

author

2012-11-21 14:49 UTC #5 lore raw

Quoted 7 lines · Paolo Bonzini

Il 21/11/2012 15:33, malc ha scritto:
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 1 line
Quoted 1 line
Leaking leader is a bit bad, but it looks ok for 1.3.
Hmm. A TLS destructor is apparently not available. Is there some "on thread termination" callback mechanism on Windows? Didn't find one on first glance.

Quoted 8 lines

Quoted 2 lines
Dlls receive something like THREAD_DETTACH in it's startup routine or something like that if my memory serves me.
Only DLLs.
But this sounds like deja-vu. I'm pretty sure in the past we just decided that this compiler is not supported (of course it's bad that it's silent). Stefan, do you remember the details?

Current Debian delivers 4.4-based mingw unfortunately.

Jan

Signature · 2 lines

Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux

Jan Kiszka

author

2012-11-21 15:44 UTC #6 lore raw

Quoted 11 lines · Jan Kiszka

Quoted 10 lines · Paolo Bonzini
Il 21/11/2012 15:33, malc ha scritto:
Quoted 8 lines
Quoted 6 lines
Quoted 6 lines
Quoted 1 line
Quoted 1 line
Leaking leader is a bit bad, but it looks ok for 1.3.
Hmm. A TLS destructor is apparently not available. Is there some "on thread termination" callback mechanism on Windows? Didn't find one on first glance.
Dlls receive something like THREAD_DETTACH in it's startup routine or something like that if my memory serves me.

Quoted 7 lines

Quoted 5 lines
Only DLLs.
But this sounds like deja-vu. I'm pretty sure in the past we just decided that this compiler is not supported (of course it's bad that it's silent). Stefan, do you remember the details?
Current Debian delivers 4.4-based mingw unfortunately.

I think we practically do not leak, at least as long as we continue to use coroutines only over cpu and iothread context. Those threads stay as long as qemu is running. And to my understanding, those contexts are the only target of coroutines anyway. Anything that already uses its own proper threads has no need for this problematic concept, no?

Jan

Signature · 2 lines

Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux

Paolo Bonzini

2012-11-21 15:55 UTC #7 lore raw

Il 21/11/2012 16:44, Jan Kiszka ha scritto:

Quoted 15 lines

Quoted 15 lines
Quoted 15 lines
Quoted 14 lines
Quoted 14 lines
Quoted 8 lines
Quoted 8 lines
Quoted 6 lines
Leaking leader is a bit bad, but it looks ok for 1.3.
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Hmm. A TLS destructor is apparently not available. Is there some "on thread termination" callback mechanism on Windows? Didn't find one on first glance.
Dlls receive something like THREAD_DETTACH in it's startup routine or something like that if my memory serves me.
Only DLLs.
But this sounds like deja-vu. I'm pretty sure in the past we just decided that this compiler is not supported (of course it's bad that it's silent). Stefan, do you remember the details?

Quoted 6 lines

Quoted 1 line
Quoted 1 line
Current Debian delivers 4.4-based mingw unfortunately.
I think we practically do not leak, at least as long as we continue to use coroutines only over cpu and iothread context. Those threads stay as long as qemu is running. And to my understanding, those contexts are the only target of coroutines anyway. Anything that already uses its own proper threads has no need for this problematic concept, no?

Kind of... when Stefan (Hajnoczi) finishes the full version of virtio-blk dataplane, there will be one thread per device running coroutines.

But it's still a minor leak, it's ok for 1.3 and we can get it right later using the Windows run-time linker's TLS support, like on Linux.

Paolo

Jan Kiszka

author

2012-11-21 16:00 UTC #8 lore raw

Quoted 23 lines · Paolo Bonzini

Il 21/11/2012 16:44, Jan Kiszka ha scritto:
Quoted 15 lines
Quoted 15 lines
Quoted 15 lines
Quoted 14 lines
Quoted 14 lines
Quoted 8 lines
Quoted 8 lines
Quoted 6 lines
Leaking leader is a bit bad, but it looks ok for 1.3.
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Hmm. A TLS destructor is apparently not available. Is there some "on thread termination" callback mechanism on Windows? Didn't find one on first glance.
Dlls receive something like THREAD_DETTACH in it's startup routine or something like that if my memory serves me.
Only DLLs.
But this sounds like deja-vu. I'm pretty sure in the past we just decided that this compiler is not supported (of course it's bad that it's silent). Stefan, do you remember the details?
Quoted 6 lines
Quoted 1 line
Quoted 1 line
Current Debian delivers 4.4-based mingw unfortunately.
I think we practically do not leak, at least as long as we continue to use coroutines only over cpu and iothread context. Those threads stay as long as qemu is running. And to my understanding, those contexts are the only target of coroutines anyway. Anything that already uses its own proper threads has no need for this problematic concept, no?

Quoted 6 lines

Kind of... when Stefan (Hajnoczi) finishes the full version of virtio-blk dataplane, there will be one thread per device running coroutines.
But it's still a minor leak, it's ok for 1.3 and we can get it right later using the Windows run-time linker's TLS support, like on Linux.

So it's a non-leak for current QEMU. :)

Jan

Signature · 2 lines

Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux

Stefan Weil

2012-11-21 19:11 UTC #9 lore raw

In reply to Paolo Bonzini

Am 21.11.2012 15:38, schrieb Paolo Bonzini:

Quoted 8 lines

Il 21/11/2012 15:33, malc ha scritto:
Quoted 7 lines
Quoted 5 lines
Quoted 5 lines
Quoted 1 line
Quoted 1 line
Leaking leader is a bit bad, but it looks ok for 1.3.
Hmm. A TLS destructor is apparently not available. Is there some "on thread termination" callback mechanism on Windows? Didn't find one on first glance.
Dlls receive something like THREAD_DETTACH in it's startup routine or something like that if my memory serves me.

Quoted 7 lines

Only DLLs.
But this sounds like deja-vu. I'm pretty sure in the past we just decided that this compiler is not supported (of course it's bad that it's silent). Stefan, do you remember the details?
Paolo

Debian cross works with -mthread.The issue was discussed here:

https://bugs.launchpad.net/qemu/+bug/932487

Jan, I don't think your patch should be applied.

Current MinGW / MinGW-w64 compilers work, so those users which compile and use QEMU on Windows won't have a problem. With MinGW-w64, it is even possible to compile QEMU with nearly no warnings :-)

Debian cross development is full of difficulties. Passing an extra compiler option like -mthread is only one of these difficulties. I updated http://wiki.qemu.org/Hosts/W32, so anybody who really wants to run cross compilations on Debian can get more information there.

We could add a check to configure and add -mthread automatically. Up to now, there was no consensus whether this is wanted because -mthread adds a library to QEMU's dependencies.

Stefan

Paolo Bonzini

2012-11-22 09:02 UTC #10 lore raw

Il 21/11/2012 20:11, Stefan Weil ha scritto:

Quoted 6 lines

Debian cross development is full of difficulties. Passing an extra compiler option like -mthread is only one of these difficulties. I updated http://wiki.qemu.org/Hosts/W32, so anybody who really wants to run cross compilations on Debian can get more information there.

Thanks very much!

Paolo

Jan Kiszka

author

2012-11-22 12:07 UTC #11 lore raw

In reply to Stefan Weil

Quoted 11 lines · Stefan Weil

Am 21.11.2012 15:38, schrieb Paolo Bonzini:
Quoted 10 lines
Il 21/11/2012 15:33, malc ha scritto:
Quoted 7 lines
Quoted 5 lines
Quoted 5 lines
Quoted 1 line
Quoted 1 line
Leaking leader is a bit bad, but it looks ok for 1.3.
Hmm. A TLS destructor is apparently not available. Is there some "on thread termination" callback mechanism on Windows? Didn't find one on first glance.
Dlls receive something like THREAD_DETTACH in it's startup routine or something like that if my memory serves me.
Only DLLs.

Quoted 7 lines

Quoted 5 lines
But this sounds like deja-vu. I'm pretty sure in the past we just decided that this compiler is not supported (of course it's bad that it's silent). Stefan, do you remember the details?
Paolo
Debian cross works with -mthread.The issue was discussed here:

Yep, works here as well.

With this knowledge, why not convert the TLS usage in qemu-thread-win32.c as well? That's what motivated me to go this path.

Quoted 4 lines

https://bugs.launchpad.net/qemu/+bug/932487
Jan, I don't think your patch should be applied.

I agree, but we need a different solution for this subtle failure.

Quoted 12 lines

Current MinGW / MinGW-w64 compilers work, so those users which compile and use QEMU on Windows won't have a problem. With MinGW-w64, it is even possible to compile QEMU with nearly no warnings :-)
Debian cross development is full of difficulties. Passing an extra compiler option like -mthread is only one of these difficulties. I updated http://wiki.qemu.org/Hosts/W32, so anybody who really wants to run cross compilations on Debian can get more information there.

Quoted 3 lines

We could add a check to configure and add -mthread automatically. Up to now, there was no consensus whether this is wanted because -mthread adds a library to QEMU's dependencies.

As the alternative to this is a crashing QEMU, I doubt there is much to discuss.

Do we know which mingw version is fine without -mthreads? Or do we have to test this during configure?

Jan

Signature · 2 lines

Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux