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
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
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~
On Mon, Oct 25, 2021 at 10:18 AM Richard Henderson < richard.henderson@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~ > >
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.henderson@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
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~
+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 >
© 2016 - 2024 Red Hat, Inc.