[05/30] exec: do not use qemu/tls.h

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 <pbonzini@redhat.com>
---
 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

Paolo Bonzini <pbonzini@redhat.com> writes:

> 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 <pbonzini@redhat.com>

Reviewed-by: Anthony Liguori <aliguori@us.ibm.com>

Regards,

Anthony Liguori

> ---
>  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
> +
>  /* 0 = Do not count executed instructions.
>     1 = Precise instruction counting.
>     2 = Adaptive rate instruction counting.  */
> diff --git a/include/exec/cpu-all.h b/include/exec/cpu-all.h
> index e9c3717..2202ba3 100644
> --- a/include/exec/cpu-all.h
> +++ b/include/exec/cpu-all.h
> @@ -20,7 +20,6 @@
>  #define CPU_ALL_H
>  
>  #include "qemu-common.h"
> -#include "qemu/tls.h"
>  #include "exec/cpu-common.h"
>  #include "qemu/thread.h"
>  
> @@ -368,8 +367,17 @@ void cpu_dump_statistics(CPUArchState *env, FILE *f, fprintf_function cpu_fprint
>  void QEMU_NORETURN cpu_abort(CPUArchState *env, const char *fmt, ...)
>      GCC_FMT_ATTR(2, 3);
>  extern CPUArchState *first_cpu;
> -DECLARE_TLS(CPUArchState *,cpu_single_env);
> -#define cpu_single_env tls_var(cpu_single_env)
> +
> +/* This is thread-local depending on __linux__ because:
> + *  - the only -user mode supporting multiple VCPU threads is linux-user
> + *  - TCG system mode is single-threaded regarding VCPUs
> + *  - KVM system mode is multi-threaded but limited to Linux
> + */
> +#if defined CONFIG_KVM || (defined CONFIG_USER_ONLY && defined CONFIG_USE_NPTL)
> +extern __thread CPUArchState *cpu_single_env;
> +#else
> +extern CPUArchState *cpu_single_env;
> +#endif
>  
>  /* Flags for use in ENV->INTERRUPT_PENDING.
>  
> diff --git a/include/qemu/tls.h b/include/qemu/tls.h
> deleted file mode 100644
> index b92ea9d..0000000
> --- a/include/qemu/tls.h
> +++ /dev/null
> @@ -1,52 +0,0 @@
> -/*
> - * Abstraction layer for defining and using TLS variables
> - *
> - * Copyright (c) 2011 Red Hat, Inc
> - * Copyright (c) 2011 Linaro Limited
> - *
> - * Authors:
> - *  Paolo Bonzini <pbonzini@redhat.com>
> - *  Peter Maydell <peter.maydell@linaro.org>
> - *
> - * This program is free software; you can redistribute it and/or
> - * modify it under the terms of the GNU General Public License as
> - * published by the Free Software Foundation; either version 2 of
> - * the License, or (at your option) any later version.
> - *
> - * This program is distributed in the hope that it will be useful,
> - * but WITHOUT ANY WARRANTY; without even the implied warranty of
> - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> - * GNU General Public License for more details.
> - *
> - * You should have received a copy of the GNU General Public License along
> - * with this program; if not, see <http://www.gnu.org/licenses/>.
> - */
> -
> -#ifndef QEMU_TLS_H
> -#define QEMU_TLS_H
> -
> -/* Per-thread variables. Note that we only have implementations
> - * which are really thread-local on Linux; the dummy implementations
> - * define plain global variables.
> - *
> - * This means that for the moment use should be restricted to
> - * per-VCPU variables, which are OK because:
> - *  - the only -user mode supporting multiple VCPU threads is linux-user
> - *  - TCG system mode is single-threaded regarding VCPUs
> - *  - KVM system mode is multi-threaded but limited to Linux
> - *
> - * TODO: proper implementations via Win32 .tls sections and
> - * POSIX pthread_getspecific.
> - */
> -#ifdef __linux__
> -#define DECLARE_TLS(type, x) extern DEFINE_TLS(type, x)
> -#define DEFINE_TLS(type, x)  __thread __typeof__(type) tls__##x
> -#define tls_var(x)           tls__##x
> -#else
> -/* Dummy implementations which define plain global variables */
> -#define DECLARE_TLS(type, x) extern DEFINE_TLS(type, x)
> -#define DEFINE_TLS(type, x)  __typeof__(type) tls__##x
> -#define tls_var(x)           tls__##x
> -#endif
> -
> -#endif
> -- 
> 1.8.1.4

On 28 June 2013 14:26, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> +/* This is thread-local depending on __linux__ because:

Is the comment perhaps unchanged from an earlier revision that used a
different test?  It seems odd to me to reference __linux__ here.

> + *  - the only -user mode supporting multiple VCPU threads is linux-user
> + *  - TCG system mode is single-threaded regarding VCPUs
> + *  - KVM system mode is multi-threaded but limited to Linux
> + */
> +#if defined CONFIG_KVM || (defined CONFIG_USER_ONLY && defined CONFIG_USE_NPTL)

Also, in discussion on the FreeBSD bsd-user patch set the suggestion
was made that we do away with a flag, and just have thread support
always enabled.  Would you suggest this test then become KVM ||
(USER_ONLY && (USE_NPTL || __FreeBSD__))?

On 28 June 2013 19:26, Paolo Bonzini <pbonzini@redhat.com> 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 <pbonzini@redhat.com>
> ---
>  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.

thanks
-- PMM

Il 29/06/2013 01:53, Ed Maste ha scritto:
> On 28 June 2013 14:26, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>
>> +/* This is thread-local depending on __linux__ because:
> 
> Is the comment perhaps unchanged from an earlier revision that used a
> different test?  It seems odd to me to reference __linux__ here.
> 
>> + *  - the only -user mode supporting multiple VCPU threads is linux-user
>> + *  - TCG system mode is single-threaded regarding VCPUs
>> + *  - KVM system mode is multi-threaded but limited to Linux
>> + */
>> +#if defined CONFIG_KVM || (defined CONFIG_USER_ONLY && defined CONFIG_USE_NPTL)
> 
> Also, in discussion on the FreeBSD bsd-user patch set the suggestion
> was made that we do away with a flag, and just have thread support
> always enabled.  Would you suggest this test then become KVM ||
> (USER_ONLY && (USE_NPTL || __FreeBSD__))?

I would suggest that you have something like CONFIG_USER_THREADS that
can be used by both linux-user and bsd-user.

Paolo

Il 29/06/2013 12:55, Peter Maydell ha scritto:
> On 28 June 2013 19:26, Paolo Bonzini <pbonzini@redhat.com> 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 <pbonzini@redhat.com>
>> ---
>>  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

On 1 July 2013 11:45, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 29/06/2013 12:55, Peter Maydell ha scritto:
>> 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.

MacOSX 10.8 supports __thread if you build with clang (not if you
build with gcc), though I can't speak to its performance since it
compiles to a function call to get the variable's address.

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

That is making things worse! Non-TLS is the untested minority
case, we want to be taking things out of it, not pushing
configs that were previously TLS into it.

-- PMM

Il 01/07/2013 13:05, Peter Maydell ha scritto:
>> > 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.
> That is making things worse! Non-TLS is the untested minority
> case, we want to be taking things out of it, not pushing
> configs that were previously TLS into it.

I think we should strive for one of these two:

(1) all targets are TLS;

(2) all targets are non-TLS if this is possible.

Either maximizes the homogeneity across platforms.

Paolo

On 1 July 2013 17:21, Paolo Bonzini <pbonzini@redhat.com> wrote:
> I think we should strive for one of these two:
>
> (1) all targets are TLS;
>
> (2) all targets are non-TLS if this is possible.
>
> Either maximizes the homogeneity across platforms.

Since the two largest cases are both "cpu_single_env must be TLS"
(ie (a) system emulation built with KVM support and (b) linux-user),
the set of targets which can be non-TLS is really really small,
and I think (1) makes much more sense.

(I'm assuming you don't want to try to support cpu_single_env
being both per-thread and not-per-thread in a single binary
depending on whether the user passes -enable-kvm or not.)

-- PMM

Il 01/07/2013 18:26, Peter Maydell ha scritto:
> > I think we should strive for one of these two:
> >
> > (1) all targets are TLS;
> >
> > (2) all targets are non-TLS if this is possible.
> >
> > Either maximizes the homogeneity across platforms.
> 
> Since the two largest cases are both "cpu_single_env must be TLS"
> (ie (a) system emulation built with KVM support and (b) linux-user),
> the set of targets which can be non-TLS is really really small,
> and I think (1) makes much more sense.

Not many linux-user targets support threads (including not i386).

> (I'm assuming you don't want to try to support cpu_single_env
> being both per-thread and not-per-thread in a single binary
> depending on whether the user passes -enable-kvm or not.)

No, of course not.

On 1 July 2013 21:52, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 01/07/2013 18:26, Peter Maydell ha scritto:
>> Since the two largest cases are both "cpu_single_env must be TLS"
>> (ie (a) system emulation built with KVM support and (b) linux-user),
>> the set of targets which can be non-TLS is really really small,
>> and I think (1) makes much more sense.
>
> Not many linux-user targets support threads (including not i386).

i386 guest is a comparatively rare case for linux-user (because
most people have an i386 box they can run them on). Also I'm
hoping to get most of the linux-user guests up to the point where
we can just have CONFIG_NPTL be true for all of them -- I have
several patches on-list which are trying to head in that direction.

thanks
-- PMM

Am 01.07.2013 23:34, schrieb Peter Maydell:
> On 1 July 2013 21:52, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> Il 01/07/2013 18:26, Peter Maydell ha scritto:
>>> Since the two largest cases are both "cpu_single_env must be TLS"
>>> (ie (a) system emulation built with KVM support and (b) linux-user),
>>> the set of targets which can be non-TLS is really really small,
>>> and I think (1) makes much more sense.
>>
>> Not many linux-user targets support threads (including not i386).
> 
> i386 guest is a comparatively rare case for linux-user (because
> most people have an i386 box they can run them on). Also I'm
> hoping to get most of the linux-user guests up to the point where
> we can just have CONFIG_NPTL be true for all of them -- I have
> several patches on-list which are trying to head in that direction.

Alex had posted a patch which implements NPTL for i386, tested with
WINE, I believe.

Andreas

On 07/02/2013 03:40 PM, Andreas Färber wrote:
> Am 01.07.2013 23:34, schrieb Peter Maydell:
>> On 1 July 2013 21:52, Paolo Bonzini<pbonzini@redhat.com>  wrote:
>>> Il 01/07/2013 18:26, Peter Maydell ha scritto:
>>>> Since the two largest cases are both "cpu_single_env must be TLS"
>>>> (ie (a) system emulation built with KVM support and (b) linux-user),
>>>> the set of targets which can be non-TLS is really really small,
>>>> and I think (1) makes much more sense.
>>> Not many linux-user targets support threads (including not i386).
>> i386 guest is a comparatively rare case for linux-user (because
>> most people have an i386 box they can run them on). Also I'm
>> hoping to get most of the linux-user guests up to the point where
>> we can just have CONFIG_NPTL be true for all of them -- I have
>> several patches on-list which are trying to head in that direction.
> Alex had posted a patch which implements NPTL for i386, tested with
> WINE, I believe.

Not sure I posted it, but I do have it ready. I'll rebase it once the 
NPTL refactorings are in. They look very sane to me.


Alex