| Message ID | 20111003163352.GA22822@davesworkthinkpad |
|---|---|
| State | New |
| Headers | show |
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
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
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 --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. */
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>