diff mbox

Make cpu_single_env thread local (Linux only for now)

Message ID 20111003163352.GA22822@davesworkthinkpad
State Superseded
Headers show

Commit Message

Dr. David Alan Gilbert Oct. 3, 2011, 4:33 p.m. UTC
Make cpu_single_env thread local (Linux only for now)
  * Fixes some user space threading issues (esp those triggered
    by bug 823902)

Against rev d11cf8cc..., tested on ARM user mode, and ARM Vexpress
system mode (with Blue Swirl's fix from yesterday) - only
tested on Linux host.  Lets me run ARM userspace firefox.

Signed-off-by: Dr. David Alan Gilbert <david.gilbert@linaro.org>

Comments

Jan Kiszka Oct. 3, 2011, 5:51 p.m. UTC | #1
On 2011-10-03 18:33, Dr. David Alan Gilbert wrote:
>   Make cpu_single_env thread local (Linux only for now)
>   * Fixes some user space threading issues (esp those triggered
>     by bug 823902)
> 
> Against rev d11cf8cc..., tested on ARM user mode, and ARM Vexpress
> system mode (with Blue Swirl's fix from yesterday) - only
> tested on Linux host.  Lets me run ARM userspace firefox.
> 
> Signed-off-by: Dr. David Alan Gilbert <david.gilbert@linaro.org>
> 
> diff --git a/cpu-all.h b/cpu-all.h
> index 42a5fa0..d895ee6 100644
> --- a/cpu-all.h
> +++ b/cpu-all.h
> @@ -334,7 +334,13 @@ void cpu_dump_statistics(CPUState *env, FILE *f, fprintf_function cpu_fprintf,
>  void QEMU_NORETURN cpu_abort(CPUState *env, const char *fmt, ...)
>      GCC_FMT_ATTR(2, 3);
>  extern CPUState *first_cpu;
> +
> +#ifdef __linux__
> +/* DAG: Only tested thread local on Linux, feel free to add others */
> +extern __thread CPUState *cpu_single_env;
> +#else
>  extern CPUState *cpu_single_env;
> +#endif

We need this for all platforms in order to skip qemu_global_mutex while
manipulating some CPUState. And leaving some platforms with non-TLS will
eventually break them when code is added that assumes TLS.

However, it's not unlikely that some weird platforms / ancient
toolchains still have problems with __thread - even on Linux. We may
want to play safe and use pthread_key on POSIX.

Jan
Lluís Vilanova Oct. 4, 2011, 3:10 p.m. UTC | #2
Jan Kiszka writes:

> On 2011-10-03 18:33, Dr. David Alan Gilbert wrote:
>> Make cpu_single_env thread local (Linux only for now)
>> * Fixes some user space threading issues (esp those triggered
>> by bug 823902)
>> 
>> Against rev d11cf8cc..., tested on ARM user mode, and ARM Vexpress
>> system mode (with Blue Swirl's fix from yesterday) - only
>> tested on Linux host.  Lets me run ARM userspace firefox.
>> 
>> Signed-off-by: Dr. David Alan Gilbert <david.gilbert@linaro.org>
>> 
>> diff --git a/cpu-all.h b/cpu-all.h
>> index 42a5fa0..d895ee6 100644
>> --- a/cpu-all.h
>> +++ b/cpu-all.h
>> @@ -334,7 +334,13 @@ void cpu_dump_statistics(CPUState *env, FILE *f, fprintf_function cpu_fprintf,
>> void QEMU_NORETURN cpu_abort(CPUState *env, const char *fmt, ...)
>> GCC_FMT_ATTR(2, 3);
>> extern CPUState *first_cpu;
>> +
>> +#ifdef __linux__
>> +/* DAG: Only tested thread local on Linux, feel free to add others */
>> +extern __thread CPUState *cpu_single_env;
>> +#else
>> extern CPUState *cpu_single_env;
>> +#endif

> We need this for all platforms in order to skip qemu_global_mutex while
> manipulating some CPUState. And leaving some platforms with non-TLS will
> eventually break them when code is added that assumes TLS.

> However, it's not unlikely that some weird platforms / ancient
> toolchains still have problems with __thread - even on Linux. We may
> want to play safe and use pthread_key on POSIX.

Why not make this kind of annotations available in qemu-commmon.h; or even
better somewhere less... "generic".

#if defined(CONFIG_SUPPORTS_THREAD_KEYWORD)

// use __thread
#define QEMU_DECL_TLS(type, name) \
        extern __thread type name;
#define QEMU_DEF_TLS(type, name) \
        __thread type name;
#define QEMU_SET_TLS(name, value) \
        do { name = value; } while (0)
#define QEMU_GET_TLS(name) \
        name

#elif defined(CONFIG_SUPPORTS_PTHREAD_KEY)

// use pthread_key_t
#define QEMU_DECL_TLS(type, name) \
        extern type _type_##name; \
        extern pthread_key_t name;
#define QEMU_DEF_TLS(type, name) \
        pthread_key_t name; \
        void _init_##name (void) __attribute__((constructor)); \
        void _init_##name (void) { pthread_key_create(&name, NULL); }
#define QEMU_SET_TLS(name, value) \
        do { pthread_setspecific(name, (void*)value); } while (0)
#define QEMU_GET_TLS(name) \
        ((typeof(_type_##name))pthread_getspecific(name))

#else
#error Go home
#endif


Lluis
Jan Kiszka Oct. 4, 2011, 5:26 p.m. UTC | #3
On 2011-10-04 17:10, Lluís Vilanova wrote:
> Jan Kiszka writes:
> 
>> On 2011-10-03 18:33, Dr. David Alan Gilbert wrote:
>>> Make cpu_single_env thread local (Linux only for now)
>>> * Fixes some user space threading issues (esp those triggered
>>> by bug 823902)
>>>
>>> Against rev d11cf8cc..., tested on ARM user mode, and ARM Vexpress
>>> system mode (with Blue Swirl's fix from yesterday) - only
>>> tested on Linux host.  Lets me run ARM userspace firefox.
>>>
>>> Signed-off-by: Dr. David Alan Gilbert <david.gilbert@linaro.org>
>>>
>>> diff --git a/cpu-all.h b/cpu-all.h
>>> index 42a5fa0..d895ee6 100644
>>> --- a/cpu-all.h
>>> +++ b/cpu-all.h
>>> @@ -334,7 +334,13 @@ void cpu_dump_statistics(CPUState *env, FILE *f, fprintf_function cpu_fprintf,
>>> void QEMU_NORETURN cpu_abort(CPUState *env, const char *fmt, ...)
>>> GCC_FMT_ATTR(2, 3);
>>> extern CPUState *first_cpu;
>>> +
>>> +#ifdef __linux__
>>> +/* DAG: Only tested thread local on Linux, feel free to add others */
>>> +extern __thread CPUState *cpu_single_env;
>>> +#else
>>> extern CPUState *cpu_single_env;
>>> +#endif
> 
>> We need this for all platforms in order to skip qemu_global_mutex while
>> manipulating some CPUState. And leaving some platforms with non-TLS will
>> eventually break them when code is added that assumes TLS.
> 
>> However, it's not unlikely that some weird platforms / ancient
>> toolchains still have problems with __thread - even on Linux. We may
>> want to play safe and use pthread_key on POSIX.
> 
> Why not make this kind of annotations available in qemu-commmon.h; or even
> better somewhere less... "generic".

Yes, we will need some encapsulation for this.

> 
> #if defined(CONFIG_SUPPORTS_THREAD_KEYWORD)
> 
> // use __thread
> #define QEMU_DECL_TLS(type, name) \
>         extern __thread type name;
> #define QEMU_DEF_TLS(type, name) \
>         __thread type name;
> #define QEMU_SET_TLS(name, value) \
>         do { name = value; } while (0)
> #define QEMU_GET_TLS(name) \
>         name
> 
> #elif defined(CONFIG_SUPPORTS_PTHREAD_KEY)
> 
> // use pthread_key_t
> #define QEMU_DECL_TLS(type, name) \
>         extern type _type_##name; \
>         extern pthread_key_t name;
> #define QEMU_DEF_TLS(type, name) \
>         pthread_key_t name; \
>         void _init_##name (void) __attribute__((constructor)); \
>         void _init_##name (void) { pthread_key_create(&name, NULL); }
> #define QEMU_SET_TLS(name, value) \
>         do { pthread_setspecific(name, (void*)value); } while (0)
> #define QEMU_GET_TLS(name) \
>         ((typeof(_type_##name))pthread_getspecific(name))
> 
> #else
> #error Go home
> #endif

Looks like a start. But I would avoid macros and go for (static inline)
functions where possible. And initialization should be explicit (so that
you can start using TLS already inside constructors).

Jan
diff mbox

Patch

diff --git a/cpu-all.h b/cpu-all.h
index 42a5fa0..d895ee6 100644
--- a/cpu-all.h
+++ b/cpu-all.h
@@ -334,7 +334,13 @@  void cpu_dump_statistics(CPUState *env, FILE *f, fprintf_function cpu_fprintf,
 void QEMU_NORETURN cpu_abort(CPUState *env, const char *fmt, ...)
     GCC_FMT_ATTR(2, 3);
 extern CPUState *first_cpu;
+
+#ifdef __linux__
+/* DAG: Only tested thread local on Linux, feel free to add others */
+extern __thread CPUState *cpu_single_env;
+#else
 extern CPUState *cpu_single_env;
+#endif
 
 /* Flags for use in ENV->INTERRUPT_PENDING.
 
diff --git a/exec.c b/exec.c
index d0cbf15..b82d8e4 100644
--- a/exec.c
+++ b/exec.c
@@ -120,7 +120,12 @@  static MemoryRegion *system_io;
 CPUState *first_cpu;
 /* current CPU in the current thread. It is only valid inside
    cpu_exec() */
+#ifdef __linux__
+/* DAG: Only tested thread local on Linux, feel free to add others */
+__thread CPUState *cpu_single_env;
+#else
 CPUState *cpu_single_env;
+#endif
 /* 0 = Do not count executed instructions.
    1 = Precise instruction counting.
    2 = Adaptive rate instruction counting.  */