| Message ID | 50ACDE8F.5010507@siemens.com |
|---|---|
| State | New |
| Headers | show |
Il 21/11/2012 15:00, Jan Kiszka ha scritto: > + 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
On 2012-11-21 15:08, Paolo Bonzini wrote: > Il 21/11/2012 15:00, Jan Kiszka ha scritto: >> + 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
On Wed, 21 Nov 2012, Jan Kiszka wrote: > On 2012-11-21 15:08, Paolo Bonzini wrote: > > Il 21/11/2012 15:00, Jan Kiszka ha scritto: > >> + 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. > Dlls receive something like THREAD_DETTACH in it's startup routine or something like that if my memory serves me.
Il 21/11/2012 15:33, malc ha scritto: >>> > > 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
On 2012-11-21 15:38, Paolo Bonzini wrote: > Il 21/11/2012 15:33, malc ha scritto: >>>>>> 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? Current Debian delivers 4.4-based mingw unfortunately. Jan
On 2012-11-21 15:49, Jan Kiszka wrote: > On 2012-11-21 15:38, Paolo Bonzini wrote: >> Il 21/11/2012 15:33, malc ha scritto: >>>>>>> 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? > > 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
Il 21/11/2012 16:44, Jan Kiszka ha scritto: >>>>>>>> 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? >> > >> > 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
On 2012-11-21 16:54, Paolo Bonzini wrote: > Il 21/11/2012 16:44, Jan Kiszka ha scritto: >>>>>>>>> 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? >>>> >>>> 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. So it's a non-leak for current QEMU. :) Jan
Am 21.11.2012 15:38, schrieb Paolo Bonzini: > Il 21/11/2012 15:33, malc ha scritto: >>>>>> 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 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
Il 21/11/2012 20:11, Stefan Weil ha scritto: > > 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
On 2012-11-21 20:11, Stefan Weil wrote: > Am 21.11.2012 15:38, schrieb Paolo Bonzini: >> Il 21/11/2012 15:33, malc ha scritto: >>>>>>> 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 > > 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. > > 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. > > 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. 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
diff --git a/coroutine-win32.c b/coroutine-win32.c index 4179609..bf0aeac 100644 --- a/coroutine-win32.c +++ b/coroutine-win32.c @@ -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; }
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 <jan.kiszka@siemens.com> --- coroutine-win32.c | 29 ++++++++++++++++++++++------- 1 files changed, 22 insertions(+), 7 deletions(-)