Re: [RFC 0/2] tls: add macros for coroutine-safe TLS variables
On Tue, Oct 26, 2021 at 10:10:44AM -0700, Richard Henderson wrote: > On 10/26/21 9:34 AM, Stefan Hajnoczi wrote: > > On Tue, Oct 26, 2021 at 08:10:16AM -0700, Richard Henderson wrote: > > > On 10/26/21 6:22 AM, Stefan Hajnoczi wrote: > > > > If "safe" TLS variables are opt-in then we'll likely have obscure bugs > > > > when code changes to access a TLS variable that was previously never > > > > accessed from a coroutine. There is no compiler error and no way to > > > > detect this. When it happens debugging it is painful. > > > > > > Co-routines are never used in user-only builds. > > > > If developers have the choice of using __thread then bugs can slip > > through. > > Huh? How. No, really. If there is no checkpatch.pl error then more instances of __thread will slip in. Not everyone in the QEMU community will be aware of this issue, so it's likely that code with __thread will get merged. Subsystems that use coroutines today include block, 9p, mpqemu, io channels, migration, colo, and monitor commands. I understand that qemu-user is particularly unlikely to use coroutines. Thomas' suggestion sounds good to me. Let's allow __thread only in subsystems where it's safe. Stefan signature.asc Description: PGP signature
Re: [RFC 0/2] tls: add macros for coroutine-safe TLS variables
Am 26.10.2021 um 19:10 hat Richard Henderson geschrieben: > On 10/26/21 9:34 AM, Stefan Hajnoczi wrote: > > On Tue, Oct 26, 2021 at 08:10:16AM -0700, Richard Henderson wrote: > > > On 10/26/21 6:22 AM, Stefan Hajnoczi wrote: > > > > If "safe" TLS variables are opt-in then we'll likely have obscure bugs > > > > when code changes to access a TLS variable that was previously never > > > > accessed from a coroutine. There is no compiler error and no way to > > > > detect this. When it happens debugging it is painful. > > > > > > Co-routines are never used in user-only builds. > > > > If developers have the choice of using __thread then bugs can slip > > through. > > Huh? How. No, really. > > > Are you concerned about performance, the awkwardness of calling > > getters/setters, or something else for qemu-user? > > Awkwardness first, performance second. > > I'll also note that coroutines never run on vcpu threads, only io threads. > So I'll resist any use of these interfaces in TCG as well. This is wrong. Device emulation is called from vcpu threads, and it calls into code using coroutines. Using dedicated iothreads is possible for some devices, but not the default. In fact, this is probably where the most visible case of the bug comes from: A coroutine is created in the vcpu thread (while holding the BQL) and after yielding first, it is subsequently reentered from the main thread. This is exactly the same pattern as you often get with callbacks, where the request is made in the vcpu thread and the callback is run in the main thread. Kevin
Re: [RFC 0/2] tls: add macros for coroutine-safe TLS variables
On 10/26/21 10:26 AM, Thomas Huth wrote: Would it maybe make sense to tweak check_patch.pl to forbid __thread in certain folders only, e.g. block/ and util/ (i.e. where we know that there might be code that the iothreads are using)? This sounds plausible; hw/ too, perhaps. r~
Re: [RFC 0/2] tls: add macros for coroutine-safe TLS variables
On 26/10/2021 19.10, Richard Henderson wrote: On 10/26/21 9:34 AM, Stefan Hajnoczi wrote: On Tue, Oct 26, 2021 at 08:10:16AM -0700, Richard Henderson wrote: On 10/26/21 6:22 AM, Stefan Hajnoczi wrote: If "safe" TLS variables are opt-in then we'll likely have obscure bugs when code changes to access a TLS variable that was previously never accessed from a coroutine. There is no compiler error and no way to detect this. When it happens debugging it is painful. Co-routines are never used in user-only builds. If developers have the choice of using __thread then bugs can slip through. Huh? How. No, really. Are you concerned about performance, the awkwardness of calling getters/setters, or something else for qemu-user? Awkwardness first, performance second. I'll also note that coroutines never run on vcpu threads, only io threads. So I'll resist any use of these interfaces in TCG as well. Would it maybe make sense to tweak check_patch.pl to forbid __thread in certain folders only, e.g. block/ and util/ (i.e. where we know that there might be code that the iothreads are using)? Thomas
Re: [RFC 0/2] tls: add macros for coroutine-safe TLS variables
On 10/26/21 9:34 AM, Stefan Hajnoczi wrote: On Tue, Oct 26, 2021 at 08:10:16AM -0700, Richard Henderson wrote: On 10/26/21 6:22 AM, Stefan Hajnoczi wrote: If "safe" TLS variables are opt-in then we'll likely have obscure bugs when code changes to access a TLS variable that was previously never accessed from a coroutine. There is no compiler error and no way to detect this. When it happens debugging it is painful. Co-routines are never used in user-only builds. If developers have the choice of using __thread then bugs can slip through. Huh? How. No, really. Are you concerned about performance, the awkwardness of calling getters/setters, or something else for qemu-user? Awkwardness first, performance second. I'll also note that coroutines never run on vcpu threads, only io threads. So I'll resist any use of these interfaces in TCG as well. r~
Re: [RFC 0/2] tls: add macros for coroutine-safe TLS variables
On Tue, Oct 26, 2021 at 08:10:16AM -0700, Richard Henderson wrote: > On 10/26/21 6:22 AM, Stefan Hajnoczi wrote: > > If "safe" TLS variables are opt-in then we'll likely have obscure bugs > > when code changes to access a TLS variable that was previously never > > accessed from a coroutine. There is no compiler error and no way to > > detect this. When it happens debugging it is painful. > > Co-routines are never used in user-only builds. If developers have the choice of using __thread then bugs can slip through. Your assembly get_addr() approach reduces the performance overhead of TLS getters/setters. Are you concerned about performance, the awkwardness of calling getters/setters, or something else for qemu-user? Stefan signature.asc Description: PGP signature
Re: [RFC 0/2] tls: add macros for coroutine-safe TLS variables
On 10/26/21 6:22 AM, Stefan Hajnoczi wrote: If "safe" TLS variables are opt-in then we'll likely have obscure bugs when code changes to access a TLS variable that was previously never accessed from a coroutine. There is no compiler error and no way to detect this. When it happens debugging it is painful. Co-routines are never used in user-only builds. r~
Re: [RFC 0/2] tls: add macros for coroutine-safe TLS variables
On Mon, Oct 25, 2021 at 05:27:29PM -0600, Warner Losh wrote: > On Mon, Oct 25, 2021 at 10:18 AM Richard Henderson < > richard.hender...@linaro.org> wrote: > > > On 10/25/21 7:07 AM, Stefan Hajnoczi wrote: > > > This is a preview of how we can solve the coroutines TLS problem. > > Coroutines > > > re-entered from another thread sometimes see stale TLS values. This > > happens > > > because compilers may cache values across yield points, so a value from > > the > > > previous thread will be used when the coroutine is re-entered in another > > > thread. > > > > I'm not thrilled by this, but I guess it does work. > > > > It could be worthwhile to add some inline asm instead for specific hosts > > -- one > > instruction instead of an out-of-line call. > > > > > > > Serge Guelton developed this technique, see the first patch for details. > > I'm > > > submitting it for discussion before I go ahead with a full conversion of > > the > > > source tree. > > > > > > Todo: > > > - Convert all uses of __thread > > > - Extend checkpatch.pl to reject code that uses __thread > > > > Absolutely not. *Perhaps* one or two tls variables which are accessible > > by coroutines, > > but there are plenty that have absolutely no relation. Especially > > everything related to > > user-only execution. > > > > I had the same worry. I'd also worry that the hoops that are jumped through > for > coroutines would somehow conflict with the low-level user-only execution > environment. I mean, it should be fine, but I know I'd be cranky if I traced > obscure regressions to being forced to use this construct... I have the opposite worry: If "safe" TLS variables are opt-in then we'll likely have obscure bugs when code changes to access a TLS variable that was previously never accessed from a coroutine. There is no compiler error and no way to detect this. When it happens debugging it is painful. That's why I think all __thread variables should be converted. Stefan signature.asc Description: PGP signature
Re: [RFC 0/2] tls: add macros for coroutine-safe TLS variables
On Mon, Oct 25, 2021 at 10:18 AM Richard Henderson < richard.hender...@linaro.org> wrote: > On 10/25/21 7:07 AM, Stefan Hajnoczi wrote: > > This is a preview of how we can solve the coroutines TLS problem. > Coroutines > > re-entered from another thread sometimes see stale TLS values. This > happens > > because compilers may cache values across yield points, so a value from > the > > previous thread will be used when the coroutine is re-entered in another > > thread. > > I'm not thrilled by this, but I guess it does work. > > It could be worthwhile to add some inline asm instead for specific hosts > -- one > instruction instead of an out-of-line call. > > > > Serge Guelton developed this technique, see the first patch for details. > I'm > > submitting it for discussion before I go ahead with a full conversion of > the > > source tree. > > > > Todo: > > - Convert all uses of __thread > > - Extend checkpatch.pl to reject code that uses __thread > > Absolutely not. *Perhaps* one or two tls variables which are accessible > by coroutines, > but there are plenty that have absolutely no relation. Especially > everything related to > user-only execution. > I had the same worry. I'd also worry that the hoops that are jumped through for coroutines would somehow conflict with the low-level user-only execution environment. I mean, it should be fine, but I know I'd be cranky if I traced obscure regressions to being forced to use this construct... Warner > r~ > >
Re: [RFC 0/2] tls: add macros for coroutine-safe TLS variables
On 10/25/21 7:07 AM, Stefan Hajnoczi wrote: This is a preview of how we can solve the coroutines TLS problem. Coroutines re-entered from another thread sometimes see stale TLS values. This happens because compilers may cache values across yield points, so a value from the previous thread will be used when the coroutine is re-entered in another thread. I'm not thrilled by this, but I guess it does work. It could be worthwhile to add some inline asm instead for specific hosts -- one instruction instead of an out-of-line call. Serge Guelton developed this technique, see the first patch for details. I'm submitting it for discussion before I go ahead with a full conversion of the source tree. Todo: - Convert all uses of __thread - Extend checkpatch.pl to reject code that uses __thread Absolutely not. *Perhaps* one or two tls variables which are accessible by coroutines, but there are plenty that have absolutely no relation. Especially everything related to user-only execution. r~
Re: [RFC 0/2] tls: add macros for coroutine-safe TLS variables
+Richard/Peter On 10/25/21 16:07, Stefan Hajnoczi wrote: > This is a preview of how we can solve the coroutines TLS problem. Coroutines > re-entered from another thread sometimes see stale TLS values. This happens > because compilers may cache values across yield points, so a value from the > previous thread will be used when the coroutine is re-entered in another > thread. > > Serge Guelton developed this technique, see the first patch for details. I'm > submitting it for discussion before I go ahead with a full conversion of the > source tree. Beside the point Daniel raised (shared libs) this sensible approach LGTM. > > Todo: > - Convert all uses of __thread $ git grep __thread | wc -l 55 :/ > - Extend checkpatch.pl to reject code that uses __thread > > Stefan Hajnoczi (2): > tls: add macros for coroutine-safe TLS variables > util/async: replace __thread with QEMU TLS macros > > MAINTAINERS| 1 + > include/qemu/tls.h | 142 + > util/async.c | 12 ++-- > 3 files changed, 150 insertions(+), 5 deletions(-) > create mode 100644 include/qemu/tls.h >
[RFC 0/2] tls: add macros for coroutine-safe TLS variables
This is a preview of how we can solve the coroutines TLS problem. Coroutines re-entered from another thread sometimes see stale TLS values. This happens because compilers may cache values across yield points, so a value from the previous thread will be used when the coroutine is re-entered in another thread. Serge Guelton developed this technique, see the first patch for details. I'm submitting it for discussion before I go ahead with a full conversion of the source tree. Todo: - Convert all uses of __thread - Extend checkpatch.pl to reject code that uses __thread Stefan Hajnoczi (2): tls: add macros for coroutine-safe TLS variables util/async: replace __thread with QEMU TLS macros MAINTAINERS| 1 + include/qemu/tls.h | 142 + util/async.c | 12 ++-- 3 files changed, 150 insertions(+), 5 deletions(-) create mode 100644 include/qemu/tls.h -- 2.31.1
