Re: [Qemu-devel] [PATCH 05/30] exec: do not use qemu/tls.h

[Paolo Bonzini]

Il 29/06/2013 12:55, Peter Maydell ha scritto:
> On 28 June 2013 19:26, Paolo Bonzini <address@hidden> wrote:
>> The next patch will change qemu/tls.h to support more platforms, but at
>> some performance cost.  Declare cpu_single_env directly instead of using
>> the tls.h abstractions.
>>
>> Signed-off-by: Paolo Bonzini <address@hidden>
>> ---
>>  exec.c                 | 10 ++++++++--
>>  include/exec/cpu-all.h | 14 +++++++++++---
>>  include/qemu/tls.h     | 52 
>> --------------------------------------------------
>>  3 files changed, 19 insertions(+), 57 deletions(-)
>>  delete mode 100644 include/qemu/tls.h
>>
>> diff --git a/exec.c b/exec.c
>> index d28403b..a788981 100644
>> --- a/exec.c
>> +++ b/exec.c
>> @@ -70,9 +70,15 @@ static MemoryRegion io_mem_unassigned;
>>  #endif
>>
>>  CPUArchState *first_cpu;
>> +
>>  /* current CPU in the current thread. It is only valid inside
>> -   cpu_exec() */
>> -DEFINE_TLS(CPUArchState *,cpu_single_env);
>> + * cpu_exec().  See comment in include/exec/cpu-all.h.  */
>> +#if defined CONFIG_KVM || (defined CONFIG_USER_ONLY && defined 
>> CONFIG_USE_NPTL)
>> +__thread CPUArchState *cpu_single_env;
>> +#else
>> +CPUArchState *cpu_single_env;
>> +#endif
> 
> I don't like having the semantics of this variable differ
> depending on whether CONFIG_KVM was defined. In particular
> this means that the variable is per-thread if you're running
> TCG on a QEMU that was configured with KVM support, but
> not per-thread if you're running TCG on a QEMU that was
> configured without per-thread support. That's just bizarre
> and a recipe for confusion and for bugs creeping in in the
> less-well-tested config combinations.
> 
> We should just be consistent and always make this be
> per-thread.

If it's okay to make cpu_single_env accesses more expensive by a factor
of 4 on TLS-deficient hosts (at least OpenBSD; do Darwin and NetBSD
support thread-local storage?), I'm all for it.  I (and I guess Stefan
too) do not want to introduce performance regressions in these patches.
 Making it simpler is something that one would do after having tested at
least one of OpenBSD/Darwin/whatever.

This patch does not make things worse than before.  If anything, it's
better because *more* targets have non-TLS semantics: namely non-KVM
targets on Linux become non-TLS.

Paolo