[RFC 0/2] tls: add macros for coroutine-safe TLS variables

[RFC 0/2] tls: add macros for coroutine-safe TLS variables

[Stefan Hajnoczi]

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

[RFC 1/2] tls: add macros for coroutine-safe TLS variables

[Stefan Hajnoczi]

Compiler optimizations can cache TLS values across coroutine yield
points, resulting in stale values from the previous thread when a
coroutine is re-entered by a new thread.

Serge Guelton developed an __attribute__((noinline)) wrapper and tested
it with clang and gcc. I formatted his idea according to QEMU's coding
style and wrote documentation.

These macros must be used instead of __thread from now on to prevent
coroutine TLS bugs.

Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1952483
Suggested-by: Serge Guelton <sguelton@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 MAINTAINERS        |   1 +
 include/qemu/tls.h | 142 +++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 143 insertions(+)
 create mode 100644 include/qemu/tls.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 894dc43105..cf225b7029 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2869,6 +2869,7 @@ M: Kevin Wolf <kwolf@redhat.com>
 S: Maintained
 F: util/*coroutine*
 F: include/qemu/coroutine*
+F: include/qemu/tls.h
 F: tests/unit/test-coroutine.c
 
 Buffers
diff --git a/include/qemu/tls.h b/include/qemu/tls.h
new file mode 100644
index 0000000000..9a5ccc6c52
--- /dev/null
+++ b/include/qemu/tls.h
@@ -0,0 +1,142 @@
+/*
+ * QEMU Thread Local Storage
+ *
+ * Copyright Red Hat
+ *
+ * SPDX-License-Identifier: LGPL-2.1-or-later
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2.1 or later.
+ * See the COPYING.LIB file in the top-level directory.
+ *
+ * It is forbidden to use __thread in QEMU because compiler optimizations may
+ * cause Thread Local Storage variables to be cached across coroutine re-entry.
+ * Coroutines can run in more than one thread through the course of their life,
+ * leading bugs when stale TLS values from the wrong thread are used as a
+ * result of compiler optimization.
+ *
+ * Although avoiding __thread is strictly only necessary when coroutines access
+ * the variable this is hard to audit or enforce in practice. Therefore
+ * __thread is forbidden everywhere. This prevents subtle bugs from creeping in
+ * if a variable that was previously not accessed from coroutines is now
+ * accessed from coroutines.
+ *
+ * An example is:
+ *
+ * ..code-block:: c
+ *   :caption: A coroutine that may see the wrong TLS value
+ *
+ *   static __thread AioContext *current_aio_context;
+ *   ...
+ *   static void coroutine_fn foo(void)
+ *   {
+ *       aio_notify(current_aio_context);
+ *       qemu_coroutine_yield();
+ *       aio_notify(current_aio_context); // <-- may be stale after yielding!
+ *   }
+ *
+ * This header provides macros for safely defining variables in Thread Local
+ * Storage:
+ *
+ * ..code-block:: c
+ *   :caption: A coroutine that safely uses TLS
+ *
+ *   QEMU_DECLARE_STATIC_TLS(AioContext *, current_aio_context)
+ *   ...
+ *   static void coroutine_fn foo(void)
+ *   {
+ *       aio_notify(get_current_aio_context());
+ *       qemu_coroutine_yield();
+ *       aio_notify(get_current_aio_context()); // <-- safe
+ *   }
+ */
+
+#ifndef QEMU_TLS_H
+#define QEMU_TLS_H
+
+/**
+ * QEMU_DECLARE_TLS:
+ * @type: the variable's C type
+ * @var: the variable name
+ *
+ * Declare an extern variable in Thread Local Storage from a header file:
+ *
+ * .. code-block:: c
+ *   :caption: Declaring an extern variable in Thread Local Storage
+ *
+ *   QEMU_DECLARE_TLS(int, my_count)
+ *   ...
+ *   int c = get_my_count();
+ *   set_my_count(c + 1);
+ *
+ * Use this instead of:
+ *
+ * .. code-block:: c
+ *   :caption: Declaring a TLS variable using __thread
+ *
+ *   extern __thread int my_count;
+ *   ...
+ *   int c = my_count;
+ *   my_count = c + 1;
+ */
+#define QEMU_DECLARE_TLS(type, var) \
+    __attribute__((noinline)) type get_##var(void); \
+    __attribute__((noinline)) void set_##var(type v); \
+
+/**
+ * QEMU_DEFINE_TLS:
+ * @type: the variable's C type
+ * @var: the variable name
+ *
+ * Define an variable in Thread Local Storage that was previously declared from
+ * a header file with QEMU_DECLARE_TLS():
+ *
+ * .. code-block:: c
+ *   :caption: Defining a variable in Thread Local Storage
+ *
+ *   QEMU_DEFINE_TLS(int, my_count)
+ *
+ * Use this instead of:
+ *
+ * .. code-block:: c
+ *   :caption: Defining a TLS variable using __thread
+ *
+ *   __thread int my_count;
+ */
+#define QEMU_DEFINE_TLS(type, var) \
+    __thread type qemu_tls_##var; \
+    type get_##var(void) { return qemu_tls_##var; } \
+    void set_##var(type v) { qemu_tls_##var = v; }
+
+/**
+ * QEMU_DEFINE_STATIC_TLS:
+ * @type: the variable's C type
+ * @var: the variable name
+ *
+ * Define a static variable in Thread Local Storage:
+ *
+ * .. code-block:: c
+ *   :caption: Defining a static variable in Thread Local Storage
+ *
+ *   QEMU_DEFINE_STATIC_TLS(int, my_count)
+ *   ...
+ *   int c = get_my_count();
+ *   set_my_count(c + 1);
+ *
+ * Use this instead of:
+ *
+ * .. code-block:: c
+ *   :caption: Defining a static TLS variable using __thread
+ *
+ *   static __thread int my_count;
+ *   ...
+ *   int c = my_count;
+ *   my_count = c + 1;
+ */
+#define QEMU_DEFINE_STATIC_TLS(type, var) \
+    static __thread type qemu_tls_##var; \
+    static __attribute__((noinline)) type get_##var(void); \
+    static type get_##var(void) { return qemu_tls_##var; } \
+    static __attribute__((noinline)) void set_##var(type v); \
+    static void set_##var(type v) { qemu_tls_##var = v; }
+
+#endif /* QEMU_TLS_H */
-- 
2.31.1

Re: [RFC 1/2] tls: add macros for coroutine-safe TLS variables

[Daniel P. Berrangé]

On Mon, Oct 25, 2021 at 03:07:15PM +0100, Stefan Hajnoczi wrote:
> Compiler optimizations can cache TLS values across coroutine yield
> points, resulting in stale values from the previous thread when a
> coroutine is re-entered by a new thread.
> 
> Serge Guelton developed an __attribute__((noinline)) wrapper and tested
> it with clang and gcc. I formatted his idea according to QEMU's coding
> style and wrote documentation.
> 
> These macros must be used instead of __thread from now on to prevent
> coroutine TLS bugs.

Does this apply to all  __thread usage in the QEMU process that can
be used from coroutine context, or just certain __thread usage ?

Mostly I'm wondering if this is going to have implications on external
libraries we use. eg if block layer is using librbd.so APIs, is librbd.sp
safe to use __thread directly in any way it desires ?


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [RFC 1/2] tls: add macros for coroutine-safe TLS variables

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1708 bytes --]

On Mon, Oct 25, 2021 at 03:14:36PM +0100, Daniel P. Berrangé wrote:
> On Mon, Oct 25, 2021 at 03:07:15PM +0100, Stefan Hajnoczi wrote:
> > Compiler optimizations can cache TLS values across coroutine yield
> > points, resulting in stale values from the previous thread when a
> > coroutine is re-entered by a new thread.
> > 
> > Serge Guelton developed an __attribute__((noinline)) wrapper and tested
> > it with clang and gcc. I formatted his idea according to QEMU's coding
> > style and wrote documentation.
> > 
> > These macros must be used instead of __thread from now on to prevent
> > coroutine TLS bugs.
> 
> Does this apply to all  __thread usage in the QEMU process that can
> be used from coroutine context, or just certain __thread usage ?
> 
> Mostly I'm wondering if this is going to have implications on external
> libraries we use. eg if block layer is using librbd.so APIs, is librbd.sp
> safe to use __thread directly in any way it desires ?

There is a theoretical problem but I'm not sure if it will occur in
practice:

If a __thread variable is in an extern function then there's little
risk of the value being cached. The function executes and returns back
to QEMU, never yielding.

The only case I can think of is when the library accesses a __thread
variable, invokes a callback into QEMU, and that callback yields. If the
coroutine is re-entered from another thread and returns back to the
library, then we have a problem. This seems like a very rare case
though.

It gets trickier if the library has the __thread variable in a header
file, because then the compiler may optimize it into a QEMU coroutine
function and cache its value.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC 1/2] tls: add macros for coroutine-safe TLS variables

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 297 bytes --]

Actually, nevermind what I said about the callback scenario. I don't
think that is a problem because the compiler cannot assume the __thread
variable remains unchanged across the callback. Therefore it cannot
safely cache the value.

So I think only the header file scenario is a problem.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC 1/2] tls: add macros for coroutine-safe TLS variables

[Kevin Wolf]

[-- Attachment #1: Type: text/plain, Size: 724 bytes --]

Am 26.10.2021 um 15:41 hat Stefan Hajnoczi geschrieben:
> Actually, nevermind what I said about the callback scenario. I don't
> think that is a problem because the compiler cannot assume the __thread
> variable remains unchanged across the callback. Therefore it cannot
> safely cache the value.

And additionally, I think callbacks are never coroutine_fn (especially
not if they come from an external library), so they must not yield
anyway.

> So I think only the header file scenario is a problem.

The mere existence of a __thread variable in the header file isn't a
problem either, but if QEMU accesses it, we would have to implement
wrappers similar to what you're proposing for QEMU's thread local
variables.

Kevin

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [RFC 1/2] tls: add macros for coroutine-safe TLS variables

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1218 bytes --]

On Tue, Oct 26, 2021 at 04:10:16PM +0200, Kevin Wolf wrote:
> Am 26.10.2021 um 15:41 hat Stefan Hajnoczi geschrieben:
> > Actually, nevermind what I said about the callback scenario. I don't
> > think that is a problem because the compiler cannot assume the __thread
> > variable remains unchanged across the callback. Therefore it cannot
> > safely cache the value.
> 
> And additionally, I think callbacks are never coroutine_fn (especially
> not if they come from an external library), so they must not yield
> anyway.

There's a tiny chance that the third-party library was called from
coroutine context and the callback invokes a non-coroutine_fn that uses
qemu_in_coroutine() to dynamically decide it's possible to yield. But it
seems very unlikely.

> > So I think only the header file scenario is a problem.
> 
> The mere existence of a __thread variable in the header file isn't a
> problem either, but if QEMU accesses it, we would have to implement
> wrappers similar to what you're proposing for QEMU's thread local
> variables.

There could be static inline functions that access it in the header
file. If QEMU calls those functions then the compiler can optimize that.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC 1/2] tls: add macros for coroutine-safe TLS variables

[Richard Henderson]

On 10/25/21 7:07 AM, Stefan Hajnoczi wrote:
> Compiler optimizations can cache TLS values across coroutine yield
> points, resulting in stale values from the previous thread when a
> coroutine is re-entered by a new thread.
...
>   include/qemu/tls.h | 142 +++++++++++++++++++++++++++++++++++++++++++++

Better as qemu/coroutine-tls.h, since it is needed for no other purpose.

> +#define QEMU_DEFINE_TLS(type, var) \
> +    __thread type qemu_tls_##var; \
> +    type get_##var(void) { return qemu_tls_##var; } \
> +    void set_##var(type v) { qemu_tls_##var = v; }

You might as well make the variable static, since it may only be referenced by these two 
functions.

> +#define QEMU_DEFINE_STATIC_TLS(type, var) \
> +    static __thread type qemu_tls_##var; \
> +    static __attribute__((noinline)) type get_##var(void); \
> +    static type get_##var(void) { return qemu_tls_##var; } \
> +    static __attribute__((noinline)) void set_##var(type v); \
> +    static void set_##var(type v) { qemu_tls_##var = v; }

You don't need separate function declarations; you can fold them together.

If would be nice to inline this when possible,

#if defined(__aarch64__)
#define QEMU_COROUTINE_TLS_ADDR(RET, VAR)                       \
     asm volatile("mrs %0, tpidr_el0\n\t"                        \
                  "add %0, %0, #:tprel_hi12:"#VAR", lsl #12\n\t" \
                  "add %0, %0, #:tprel_lo12_nc:"#VAR             \
                  : "=r"(RET))
#elif defined(__powerpc64__)
#define QEMU_COROUTINE_TLS_ADDR(RET, VAR)                       \
     asm volatile("addis %0,13,"#VAR"@tprel@ha\n\t"              \
                  "add   %0,%0,"#VAR"@tprel@l"                   \
                  : "=r"(RET))
#elif defined(__riscv)
#define QEMU_COROUTINE_TLS_ADDR(RET, VAR)                       \
     asm volatile("lui  %0,%%tprel_hi("#VAR")\n\t"               \
                  "add  %0,%0,%%tprel_add("#VAR")\n\t"           \
                  "addi %0,%0,%%tprel_lo("#VAR")"                \
                  : "=r"(RET))
#elif defined(__x86_64__)
#define QEMU_COROUTINE_TLS_ADDR(RET, VAR)                       \
     asm volatile("mov %%fs:"#VAR"@tpoff, %0" : "=r"(RET))
#endif

#ifdef QEMU_COROUTINE_TLS_ADDR
#define QEMU_COROUTINE_TLS_DECLARE(TYPE, VAR)                   \
     extern __thread TYPE co_tls_##VAR;                          \
     static inline TYPE get_##VAR(void)                          \
     { TYPE *p; QEMU_COROUTINE_TLS_ADDR(p, co_tls_##VAR); return *p; } \
     static inline void set_##VAR(TYPE v)                        \
     { TYPE *p; QEMU_COROUTINE_TLS_ADDR(p, co_tls_##VAR); *p = v; }
#else
     etc
#endif


r~

Re: [RFC 1/2] tls: add macros for coroutine-safe TLS variables

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 3297 bytes --]

On Mon, Oct 25, 2021 at 10:19:04AM -0700, Richard Henderson wrote:
> On 10/25/21 7:07 AM, Stefan Hajnoczi wrote:
> > Compiler optimizations can cache TLS values across coroutine yield
> > points, resulting in stale values from the previous thread when a
> > coroutine is re-entered by a new thread.
> ...
> >   include/qemu/tls.h | 142 +++++++++++++++++++++++++++++++++++++++++++++
> 
> Better as qemu/coroutine-tls.h, since it is needed for no other purpose.
> 
> > +#define QEMU_DEFINE_TLS(type, var) \
> > +    __thread type qemu_tls_##var; \
> > +    type get_##var(void) { return qemu_tls_##var; } \
> > +    void set_##var(type v) { qemu_tls_##var = v; }
> 
> You might as well make the variable static, since it may only be referenced
> by these two functions.

Oops, that's a bug. It should be declared extern. QEMU_DEFINE_TLS() is
meant for use in header files.

> 
> > +#define QEMU_DEFINE_STATIC_TLS(type, var) \
> > +    static __thread type qemu_tls_##var; \
> > +    static __attribute__((noinline)) type get_##var(void); \
> > +    static type get_##var(void) { return qemu_tls_##var; } \
> > +    static __attribute__((noinline)) void set_##var(type v); \
> > +    static void set_##var(type v) { qemu_tls_##var = v; }
> 
> You don't need separate function declarations; you can fold them together.
> 
> If would be nice to inline this when possible,
> 
> #if defined(__aarch64__)
> #define QEMU_COROUTINE_TLS_ADDR(RET, VAR)                       \
>     asm volatile("mrs %0, tpidr_el0\n\t"                        \
>                  "add %0, %0, #:tprel_hi12:"#VAR", lsl #12\n\t" \
>                  "add %0, %0, #:tprel_lo12_nc:"#VAR             \
>                  : "=r"(RET))
> #elif defined(__powerpc64__)
> #define QEMU_COROUTINE_TLS_ADDR(RET, VAR)                       \
>     asm volatile("addis %0,13,"#VAR"@tprel@ha\n\t"              \
>                  "add   %0,%0,"#VAR"@tprel@l"                   \
>                  : "=r"(RET))
> #elif defined(__riscv)
> #define QEMU_COROUTINE_TLS_ADDR(RET, VAR)                       \
>     asm volatile("lui  %0,%%tprel_hi("#VAR")\n\t"               \
>                  "add  %0,%0,%%tprel_add("#VAR")\n\t"           \
>                  "addi %0,%0,%%tprel_lo("#VAR")"                \
>                  : "=r"(RET))
> #elif defined(__x86_64__)
> #define QEMU_COROUTINE_TLS_ADDR(RET, VAR)                       \
>     asm volatile("mov %%fs:"#VAR"@tpoff, %0" : "=r"(RET))
> #endif
> 
> #ifdef QEMU_COROUTINE_TLS_ADDR
> #define QEMU_COROUTINE_TLS_DECLARE(TYPE, VAR)                   \
>     extern __thread TYPE co_tls_##VAR;                          \
>     static inline TYPE get_##VAR(void)                          \
>     { TYPE *p; QEMU_COROUTINE_TLS_ADDR(p, co_tls_##VAR); return *p; } \
>     static inline void set_##VAR(TYPE v)                        \
>     { TYPE *p; QEMU_COROUTINE_TLS_ADDR(p, co_tls_##VAR); *p = v; }
> #else
>     etc
> #endif

Nice. That makes me wonder if it's possible to write a portable version:

  static inline TYPE get_##VAR(void) \
  { volatile TYPE *p = &co_tls_##VAR; return *p; }

But the trouble is we need &co_tls_##VAR to be "volatile" and I don't
think there is a way to express that?

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC 1/2] tls: add macros for coroutine-safe TLS variables

[Richard Henderson]

On 10/26/21 6:30 AM, Stefan Hajnoczi wrote:
> On Mon, Oct 25, 2021 at 10:19:04AM -0700, Richard Henderson wrote:
>> On 10/25/21 7:07 AM, Stefan Hajnoczi wrote:
>>> Compiler optimizations can cache TLS values across coroutine yield
>>> points, resulting in stale values from the previous thread when a
>>> coroutine is re-entered by a new thread.
>> ...
>>>    include/qemu/tls.h | 142 +++++++++++++++++++++++++++++++++++++++++++++
>>
>> Better as qemu/coroutine-tls.h, since it is needed for no other purpose.
>>
>>> +#define QEMU_DEFINE_TLS(type, var) \
>>> +    __thread type qemu_tls_##var; \
>>> +    type get_##var(void) { return qemu_tls_##var; } \
>>> +    void set_##var(type v) { qemu_tls_##var = v; }
>>
>> You might as well make the variable static, since it may only be referenced
>> by these two functions.
> 
> Oops, that's a bug. It should be declared extern. QEMU_DEFINE_TLS() is
> meant for use in header files.

No, QEMU_DECLARE_TLS was for use in header files, and it of course does not globally 
declare the tls variable at all.  Only the definition here requires the tls variable.


> Nice. That makes me wonder if it's possible to write a portable version:
> 
>    static inline TYPE get_##VAR(void) \
>    { volatile TYPE *p = &co_tls_##VAR; return *p; }
> 
> But the trouble is we need &co_tls_##VAR to be "volatile" and I don't
> think there is a way to express that?

No, there's not.

Anyway, with those four hosts we get coverage of almost all users.  I'll note that both 
arm32 and s390x use the constant pool in computing these tls addresses, so they'll need to 
keep using your functional version.  So we'll still have testing of that path as well.


r~

Re: [RFC 1/2] tls: add macros for coroutine-safe TLS variables

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1968 bytes --]

On Tue, Oct 26, 2021 at 08:32:11AM -0700, Richard Henderson wrote:
> On 10/26/21 6:30 AM, Stefan Hajnoczi wrote:
> > On Mon, Oct 25, 2021 at 10:19:04AM -0700, Richard Henderson wrote:
> > > On 10/25/21 7:07 AM, Stefan Hajnoczi wrote:
> > > > Compiler optimizations can cache TLS values across coroutine yield
> > > > points, resulting in stale values from the previous thread when a
> > > > coroutine is re-entered by a new thread.
> > > ...
> > > >    include/qemu/tls.h | 142 +++++++++++++++++++++++++++++++++++++++++++++
> > > 
> > > Better as qemu/coroutine-tls.h, since it is needed for no other purpose.
> > > 
> > > > +#define QEMU_DEFINE_TLS(type, var) \
> > > > +    __thread type qemu_tls_##var; \
> > > > +    type get_##var(void) { return qemu_tls_##var; } \
> > > > +    void set_##var(type v) { qemu_tls_##var = v; }
> > > 
> > > You might as well make the variable static, since it may only be referenced
> > > by these two functions.
> > 
> > Oops, that's a bug. It should be declared extern. QEMU_DEFINE_TLS() is
> > meant for use in header files.
> 
> No, QEMU_DECLARE_TLS was for use in header files, and it of course does not
> globally declare the tls variable at all.  Only the definition here requires
> the tls variable.

You are right, thanks for pointing this out.

> 
> > Nice. That makes me wonder if it's possible to write a portable version:
> > 
> >    static inline TYPE get_##VAR(void) \
> >    { volatile TYPE *p = &co_tls_##VAR; return *p; }
> > 
> > But the trouble is we need &co_tls_##VAR to be "volatile" and I don't
> > think there is a way to express that?
> 
> No, there's not.
> 
> Anyway, with those four hosts we get coverage of almost all users.  I'll
> note that both arm32 and s390x use the constant pool in computing these tls
> addresses, so they'll need to keep using your functional version.  So we'll
> still have testing of that path as well.

Okay, thanks!

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[RFC 2/2] util/async: replace __thread with QEMU TLS macros

[Stefan Hajnoczi]

QEMU TLS macros must be used to make TLS variables safe with coroutines.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 util/async.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/util/async.c b/util/async.c
index 6f6717a34b..9033f22a20 100644
--- a/util/async.c
+++ b/util/async.c
@@ -32,6 +32,7 @@
 #include "qemu/rcu_queue.h"
 #include "block/raw-aio.h"
 #include "qemu/coroutine_int.h"
+#include "qemu/tls.h"
 #include "trace.h"
 
 /***********************************************************/
@@ -669,12 +670,13 @@ void aio_context_release(AioContext *ctx)
     qemu_rec_mutex_unlock(&ctx->lock);
 }
 
-static __thread AioContext *my_aiocontext;
+QEMU_DEFINE_STATIC_TLS(AioContext *, my_aiocontext)
 
 AioContext *qemu_get_current_aio_context(void)
 {
-    if (my_aiocontext) {
-        return my_aiocontext;
+    AioContext *ctx = get_my_aiocontext();
+    if (ctx) {
+        return ctx;
     }
     if (qemu_mutex_iothread_locked()) {
         /* Possibly in a vCPU thread.  */
@@ -685,6 +687,6 @@ AioContext *qemu_get_current_aio_context(void)
 
 void qemu_set_current_aio_context(AioContext *ctx)
 {
-    assert(!my_aiocontext);
-    my_aiocontext = ctx;
+    assert(!get_my_aiocontext());
+    set_my_aiocontext(ctx);
 }
-- 
2.31.1

Re: [RFC 0/2] tls: add macros for coroutine-safe TLS variables

[Philippe Mathieu-Daudé]

+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
> 

Re: [RFC 0/2] tls: add macros for coroutine-safe TLS variables

[Richard Henderson]

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

[Warner Losh]

[-- Attachment #1: Type: text/plain, Size: 1481 bytes --]

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~
>
>

[-- Attachment #2: Type: text/html, Size: 2208 bytes --]

Re: [RFC 0/2] tls: add macros for coroutine-safe TLS variables

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 2024 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC 0/2] tls: add macros for coroutine-safe TLS variables

[Richard Henderson]

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

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 755 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC 0/2] tls: add macros for coroutine-safe TLS variables

[Richard Henderson]

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

[Thomas Huth]

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

[Richard Henderson]

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

[Kevin Wolf]

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

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1230 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]