include/qemu/coroutine-tls.h | 205 +++++++++++++++++++++++++++++++++++ include/qemu/rcu.h | 7 +- softmmu/cpus.c | 8 +- tests/unit/rcutorture.c | 10 +- tests/unit/test-rcu-list.c | 4 +- util/async.c | 12 +- util/rcu.c | 10 +- 7 files changed, 232 insertions(+), 24 deletions(-) create mode 100644 include/qemu/coroutine-tls.h
v3: - Added __attribute__((weak)) to get_ptr_*() [Florian] - Replace rdfsbase with mov %%fs:0,%0 [Florian] This patch series solves 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 a portable technique and Richard Henderson developed an inline-friendly architecture-specific technique, see the first patch for details. I have audited all __thread variables in QEMU and converted those that can be used from coroutines. Most actually look safe to me. This patch does not include a checkpatch.pl rule that requires coroutine-tls.h usage, but perhaps we should add one for block/ at least? Stefan Hajnoczi (4): tls: add macros for coroutine-safe TLS variables util/async: replace __thread with QEMU TLS macros rcu: use coroutine TLS macros cpus: use coroutine TLS macros for iothread_locked include/qemu/coroutine-tls.h | 205 +++++++++++++++++++++++++++++++++++ include/qemu/rcu.h | 7 +- softmmu/cpus.c | 8 +- tests/unit/rcutorture.c | 10 +- tests/unit/test-rcu-list.c | 4 +- util/async.c | 12 +- util/rcu.c | 10 +- 7 files changed, 232 insertions(+), 24 deletions(-) create mode 100644 include/qemu/coroutine-tls.h -- 2.33.1
On Mon, 6 Dec 2021 at 14:33, Stefan Hajnoczi <stefanha@redhat.com> wrote: > > v3: > - Added __attribute__((weak)) to get_ptr_*() [Florian] Do we really need it *only* on get_ptr_*() ? If we need to noinline the other two we probably also should use the same attribute weak to force no optimizations at all. -- PMM
On Mon, Dec 06, 2021 at 02:34:45PM +0000, Peter Maydell wrote: > On Mon, 6 Dec 2021 at 14:33, Stefan Hajnoczi <stefanha@redhat.com> wrote: > > > > v3: > > - Added __attribute__((weak)) to get_ptr_*() [Florian] > > Do we really need it *only* on get_ptr_*() ? If we need to > noinline the other two we probably also should use the same > attribute weak to force no optimizations at all. I don't know but it does seem safer to use weak in all cases. Florian and others? Stefan
On Mon, Dec 06, 2021 at 02:34:45PM +0000, Peter Maydell wrote:
> On Mon, 6 Dec 2021 at 14:33, Stefan Hajnoczi <stefanha@redhat.com> wrote:
> >
> > v3:
> > - Added __attribute__((weak)) to get_ptr_*() [Florian]
>
> Do we really need it *only* on get_ptr_*() ? If we need to
> noinline the other two we probably also should use the same
> attribute weak to force no optimizations at all.
The weak attribute can't be used on static functions, so I think we need
a different approach:
In file included from ../util/async.c:35:
/builds/stefanha/qemu/include/qemu/coroutine-tls.h:201:11: error: weak declaration of 'get_ptr_my_aiocontext' must be public
type *get_ptr_##var(void) \
^~~~~~~~
../util/async.c:673:1: note: in expansion of macro 'QEMU_DEFINE_STATIC_CO_TLS'
QEMU_DEFINE_STATIC_CO_TLS(AioContext *, my_aiocontext)
^~~~~~~~~~~~~~~~~~~~~~~~~
Adding asm volatile("") seems to work though:
https://godbolt.org/z/3hn8Gh41d
The GCC documentation mentions combining noinline with asm(""):
https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html#index-noinline-function-attribute
Stefan
On Tue, 7 Dec 2021 at 13:53, Stefan Hajnoczi <stefanha@redhat.com> wrote:
>
> On Mon, Dec 06, 2021 at 02:34:45PM +0000, Peter Maydell wrote:
> > On Mon, 6 Dec 2021 at 14:33, Stefan Hajnoczi <stefanha@redhat.com> wrote:
> > >
> > > v3:
> > > - Added __attribute__((weak)) to get_ptr_*() [Florian]
> >
> > Do we really need it *only* on get_ptr_*() ? If we need to
> > noinline the other two we probably also should use the same
> > attribute weak to force no optimizations at all.
>
> The weak attribute can't be used on static functions, so I think we need
> a different approach:
>
> In file included from ../util/async.c:35:
> /builds/stefanha/qemu/include/qemu/coroutine-tls.h:201:11: error: weak declaration of 'get_ptr_my_aiocontext' must be public
> type *get_ptr_##var(void) \
> ^~~~~~~~
> ../util/async.c:673:1: note: in expansion of macro 'QEMU_DEFINE_STATIC_CO_TLS'
> QEMU_DEFINE_STATIC_CO_TLS(AioContext *, my_aiocontext)
> ^~~~~~~~~~~~~~~~~~~~~~~~~
>
> Adding asm volatile("") seems to work though:
> https://godbolt.org/z/3hn8Gh41d
You can see in the clang disassembly there that this isn't
sufficient. The compiler puts in both calls, but it ignores
the return results and always returns "true" from the function.
-- PMM
On Tue, Dec 07, 2021 at 01:55:34PM +0000, Peter Maydell wrote:
> On Tue, 7 Dec 2021 at 13:53, Stefan Hajnoczi <stefanha@redhat.com> wrote:
> >
> > On Mon, Dec 06, 2021 at 02:34:45PM +0000, Peter Maydell wrote:
> > > On Mon, 6 Dec 2021 at 14:33, Stefan Hajnoczi <stefanha@redhat.com> wrote:
> > > >
> > > > v3:
> > > > - Added __attribute__((weak)) to get_ptr_*() [Florian]
> > >
> > > Do we really need it *only* on get_ptr_*() ? If we need to
> > > noinline the other two we probably also should use the same
> > > attribute weak to force no optimizations at all.
> >
> > The weak attribute can't be used on static functions, so I think we need
> > a different approach:
> >
> > In file included from ../util/async.c:35:
> > /builds/stefanha/qemu/include/qemu/coroutine-tls.h:201:11: error: weak declaration of 'get_ptr_my_aiocontext' must be public
> > type *get_ptr_##var(void) \
> > ^~~~~~~~
> > ../util/async.c:673:1: note: in expansion of macro 'QEMU_DEFINE_STATIC_CO_TLS'
> > QEMU_DEFINE_STATIC_CO_TLS(AioContext *, my_aiocontext)
> > ^~~~~~~~~~~~~~~~~~~~~~~~~
> >
> > Adding asm volatile("") seems to work though:
> > https://godbolt.org/z/3hn8Gh41d
>
> You can see in the clang disassembly there that this isn't
> sufficient. The compiler puts in both calls, but it ignores
> the return results and always returns "true" from the function.
You're right! I missed that the return value of the call isn't used >_<.
Stefan
On Tue, Dec 07, 2021 at 01:55:34PM +0000, Peter Maydell wrote:
> On Tue, 7 Dec 2021 at 13:53, Stefan Hajnoczi <stefanha@redhat.com> wrote:
> >
> > On Mon, Dec 06, 2021 at 02:34:45PM +0000, Peter Maydell wrote:
> > > On Mon, 6 Dec 2021 at 14:33, Stefan Hajnoczi <stefanha@redhat.com> wrote:
> > > >
> > > > v3:
> > > > - Added __attribute__((weak)) to get_ptr_*() [Florian]
> > >
> > > Do we really need it *only* on get_ptr_*() ? If we need to
> > > noinline the other two we probably also should use the same
> > > attribute weak to force no optimizations at all.
> >
> > The weak attribute can't be used on static functions, so I think we need
> > a different approach:
> >
> > In file included from ../util/async.c:35:
> > /builds/stefanha/qemu/include/qemu/coroutine-tls.h:201:11: error: weak declaration of 'get_ptr_my_aiocontext' must be public
> > type *get_ptr_##var(void) \
> > ^~~~~~~~
> > ../util/async.c:673:1: note: in expansion of macro 'QEMU_DEFINE_STATIC_CO_TLS'
> > QEMU_DEFINE_STATIC_CO_TLS(AioContext *, my_aiocontext)
> > ^~~~~~~~~~~~~~~~~~~~~~~~~
> >
> > Adding asm volatile("") seems to work though:
> > https://godbolt.org/z/3hn8Gh41d
>
> You can see in the clang disassembly there that this isn't
> sufficient. The compiler puts in both calls, but it ignores
> the return results and always returns "true" from the function.
Specifying a input operand forces the compiler to evaluate the return
value of get_ptr_i():
static __attribute__((noinline)) long get_ptr_i()
{
long l = (long)&i;
asm volatile("" : "+rm" (l));
return l;
}
https://godbolt.org/z/e6ddGdPMv
Stefan
© 2016 - 2024 Red Hat, Inc.