qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH v2 0/3] TLS abstraction layer for thread-local cpu_single_env on Linux
@ 2011-10-27 11:37 Peter Maydell
  2011-10-27 11:37 ` [Qemu-devel] [PATCH v2 1/3] qemu-tls.h: Add abstraction layer for TLS variables Peter Maydell
                   ` (2 more replies)
  0 siblings, 3 replies; 15+ messages in thread
From: Peter Maydell @ 2011-10-27 11:37 UTC (permalink / raw)
  To: qemu-devel
  Cc: Paolo Bonzini, Dr. David Alan Gilbert, Andreas Färber,
	Jan Kiszka, patches

These patches add enough of the TLS abstraction layer to allow us
to make cpu_single_env thread-local on Linux systems. This fixes
the regression described in bug 823902 for the 1.0 release; we
can add the Win32 and POSIX implementations later.

I haven't included Paolo's "Prepare Windows port for thread-local
cpu_single_env" patch -- it would be safe to do so but it isn't
necessary until we actually implement TLS for Win32.

Changes v1->v2:
 * fix Paolo's email address
 * split the darwin-user change out into a separate patch
 * drop the 'tls_' prefix from the cpu_single_env tls var name

Paolo Bonzini (2):
  darwin-user/main.c: Drop unused cpu_single_env definition
  Make cpu_single_env thread-local

Peter Maydell (1):
  qemu-tls.h: Add abstraction layer for TLS variables

 cpu-all.h          |    4 +++-
 darwin-user/main.c |    2 --
 exec.c             |    2 +-
 qemu-tls.h         |   51 +++++++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 55 insertions(+), 4 deletions(-)
 create mode 100644 qemu-tls.h

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [Qemu-devel] [PATCH v2 1/3] qemu-tls.h: Add abstraction layer for TLS variables
  2011-10-27 11:37 [Qemu-devel] [PATCH v2 0/3] TLS abstraction layer for thread-local cpu_single_env on Linux Peter Maydell
@ 2011-10-27 11:37 ` Peter Maydell
  2011-10-27 15:04   ` Andreas Färber
  2011-10-27 11:37 ` [Qemu-devel] [PATCH v2 2/3] darwin-user/main.c: Drop unused cpu_single_env definition Peter Maydell
  2011-10-27 11:37 ` [Qemu-devel] [PATCH v2 3/3] Make cpu_single_env thread-local Peter Maydell
  2 siblings, 1 reply; 15+ messages in thread
From: Peter Maydell @ 2011-10-27 11:37 UTC (permalink / raw)
  To: qemu-devel
  Cc: Paolo Bonzini, Dr. David Alan Gilbert, Andreas Färber,
	Jan Kiszka, patches

Add an abstraction layer for defining and using thread-local
variables. For the moment this is implemented only for Linux,
which means they can only be used in restricted circumstances.
The abstraction layer allows us to add POSIX and Win32 support
later.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 qemu-tls.h |   51 +++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 51 insertions(+), 0 deletions(-)
 create mode 100644 qemu-tls.h

diff --git a/qemu-tls.h b/qemu-tls.h
new file mode 100644
index 0000000..d96a159
--- /dev/null
+++ b/qemu-tls.h
@@ -0,0 +1,51 @@
+/*
+ * Abstraction layer for defining and using TLS variables
+ *
+ * Copyright (c) 2011 Red Hat, Inc, 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_GCC_H
+#define QEMU_TLS_GCC_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 get_tls(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 get_tls(x)           tls__##x
+#endif
+
+#endif
-- 
1.7.1

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [Qemu-devel] [PATCH v2 2/3] darwin-user/main.c: Drop unused cpu_single_env definition
  2011-10-27 11:37 [Qemu-devel] [PATCH v2 0/3] TLS abstraction layer for thread-local cpu_single_env on Linux Peter Maydell
  2011-10-27 11:37 ` [Qemu-devel] [PATCH v2 1/3] qemu-tls.h: Add abstraction layer for TLS variables Peter Maydell
@ 2011-10-27 11:37 ` Peter Maydell
  2011-10-27 13:22   ` Andreas Färber
  2011-10-27 11:37 ` [Qemu-devel] [PATCH v2 3/3] Make cpu_single_env thread-local Peter Maydell
  2 siblings, 1 reply; 15+ messages in thread
From: Peter Maydell @ 2011-10-27 11:37 UTC (permalink / raw)
  To: qemu-devel
  Cc: Paolo Bonzini, Dr. David Alan Gilbert, Andreas Färber,
	Jan Kiszka, patches

From: Paolo Bonzini <pbonzini@redhat.com>

Drop the cpu_single_env definition as it is unused.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 darwin-user/main.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/darwin-user/main.c b/darwin-user/main.c
index 1a881a0..c0f14f8 100644
--- a/darwin-user/main.c
+++ b/darwin-user/main.c
@@ -729,8 +729,6 @@ static void usage(void)
 
 /* XXX: currently only used for async signals (see signal.c) */
 CPUState *global_env;
-/* used only if single thread */
-CPUState *cpu_single_env = NULL;
 
 /* used to free thread contexts */
 TaskState *first_task_state;
-- 
1.7.1

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [Qemu-devel] [PATCH v2 3/3] Make cpu_single_env thread-local
  2011-10-27 11:37 [Qemu-devel] [PATCH v2 0/3] TLS abstraction layer for thread-local cpu_single_env on Linux Peter Maydell
  2011-10-27 11:37 ` [Qemu-devel] [PATCH v2 1/3] qemu-tls.h: Add abstraction layer for TLS variables Peter Maydell
  2011-10-27 11:37 ` [Qemu-devel] [PATCH v2 2/3] darwin-user/main.c: Drop unused cpu_single_env definition Peter Maydell
@ 2011-10-27 11:37 ` Peter Maydell
  2011-10-27 15:10   ` Andreas Färber
  2 siblings, 1 reply; 15+ messages in thread
From: Peter Maydell @ 2011-10-27 11:37 UTC (permalink / raw)
  To: qemu-devel
  Cc: Paolo Bonzini, Dr. David Alan Gilbert, Andreas Färber,
	Jan Kiszka, patches

From: Paolo Bonzini <pbonzini@redhat.com>

Make cpu_single_env thread-local. This fixes a regression
in handling of multi-threaded programs in linux-user mode
(bug 823902).

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
[Peter Maydell: rename tls_cpu_single_env to cpu_single_env]
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 cpu-all.h |    4 +++-
 exec.c    |    2 +-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/cpu-all.h b/cpu-all.h
index 42a5fa0..5f47ab8 100644
--- a/cpu-all.h
+++ b/cpu-all.h
@@ -20,6 +20,7 @@
 #define CPU_ALL_H
 
 #include "qemu-common.h"
+#include "qemu-tls.h"
 #include "cpu-common.h"
 
 /* some important defines:
@@ -334,7 +335,8 @@ 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;
-extern CPUState *cpu_single_env;
+DECLARE_TLS(CPUState *,cpu_single_env);
+#define cpu_single_env get_tls(cpu_single_env)
 
 /* Flags for use in ENV->INTERRUPT_PENDING.
 
diff --git a/exec.c b/exec.c
index 9dc4edb..18e26cb 100644
--- a/exec.c
+++ b/exec.c
@@ -120,7 +120,7 @@ static MemoryRegion *system_io;
 CPUState *first_cpu;
 /* current CPU in the current thread. It is only valid inside
    cpu_exec() */
-CPUState *cpu_single_env;
+DEFINE_TLS(CPUState *,cpu_single_env);
 /* 0 = Do not count executed instructions.
    1 = Precise instruction counting.
    2 = Adaptive rate instruction counting.  */
-- 
1.7.1

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [Qemu-devel] [PATCH v2 2/3] darwin-user/main.c: Drop unused cpu_single_env definition
  2011-10-27 11:37 ` [Qemu-devel] [PATCH v2 2/3] darwin-user/main.c: Drop unused cpu_single_env definition Peter Maydell
@ 2011-10-27 13:22   ` Andreas Färber
  0 siblings, 0 replies; 15+ messages in thread
From: Andreas Färber @ 2011-10-27 13:22 UTC (permalink / raw)
  To: Peter Maydell
  Cc: Paolo Bonzini, Dr. David Alan Gilbert, qemu-devel, Jan Kiszka,
	patches

Am 27.10.2011 13:37, schrieb Peter Maydell:
> From: Paolo Bonzini <pbonzini@redhat.com>
> 
> Drop the cpu_single_env definition as it is unused.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Acked-by: Andreas Färber <afaerber@suse.de>

> ---
>  darwin-user/main.c |    2 --
>  1 files changed, 0 insertions(+), 2 deletions(-)
> 
> diff --git a/darwin-user/main.c b/darwin-user/main.c
> index 1a881a0..c0f14f8 100644
> --- a/darwin-user/main.c
> +++ b/darwin-user/main.c
> @@ -729,8 +729,6 @@ static void usage(void)
>  
>  /* XXX: currently only used for async signals (see signal.c) */
>  CPUState *global_env;
> -/* used only if single thread */
> -CPUState *cpu_single_env = NULL;
>  
>  /* used to free thread contexts */
>  TaskState *first_task_state;


-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Qemu-devel] [PATCH v2 1/3] qemu-tls.h: Add abstraction layer for TLS variables
  2011-10-27 11:37 ` [Qemu-devel] [PATCH v2 1/3] qemu-tls.h: Add abstraction layer for TLS variables Peter Maydell
@ 2011-10-27 15:04   ` Andreas Färber
  2011-10-27 15:08     ` Paolo Bonzini
                       ` (3 more replies)
  0 siblings, 4 replies; 15+ messages in thread
From: Andreas Färber @ 2011-10-27 15:04 UTC (permalink / raw)
  To: Peter Maydell
  Cc: Paolo Bonzini, patches, Dr. David Alan Gilbert, qemu-devel,
	Jan Kiszka

Am 27.10.2011 13:37, schrieb Peter Maydell:
> Add an abstraction layer for defining and using thread-local
> variables. For the moment this is implemented only for Linux,
> which means they can only be used in restricted circumstances.
> The abstraction layer allows us to add POSIX and Win32 support
> later.
> 

[Paolo's SoB missing]

> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  qemu-tls.h |   51 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 51 insertions(+), 0 deletions(-)
>  create mode 100644 qemu-tls.h
> 
> diff --git a/qemu-tls.h b/qemu-tls.h
> new file mode 100644
> index 0000000..d96a159
> --- /dev/null
> +++ b/qemu-tls.h
> @@ -0,0 +1,51 @@
> +/*
> + * Abstraction layer for defining and using TLS variables
> + *
> + * Copyright (c) 2011 Red Hat, Inc, Linaro Limited

The concatenation looks kind of funny. ;)

> + *
> + * 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_GCC_H
> +#define QEMU_TLS_GCC_H

Extra _GCC. But does no harm.

> +
> +/* 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 get_tls(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 get_tls(x)           tls__##x
> +#endif
> +
> +#endif

Looks okay to me.

I assume __typeof__() is a GCCism, indicated by ..._GCC_H, to ensure
type is actually a valid type?

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Qemu-devel] [PATCH v2 1/3] qemu-tls.h: Add abstraction layer for TLS variables
  2011-10-27 15:04   ` Andreas Färber
@ 2011-10-27 15:08     ` Paolo Bonzini
  2011-10-27 15:15     ` Peter Maydell
                       ` (2 subsequent siblings)
  3 siblings, 0 replies; 15+ messages in thread
From: Paolo Bonzini @ 2011-10-27 15:08 UTC (permalink / raw)
  To: Andreas Färber
  Cc: Peter Maydell, patches, Dr. David Alan Gilbert, qemu-devel,
	Jan Kiszka

On 10/27/2011 05:04 PM, Andreas Färber wrote:
> [Paolo's SoB missing]

There's really 1-2 lines of my code left, but anyway

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Paolo

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Qemu-devel] [PATCH v2 3/3] Make cpu_single_env thread-local
  2011-10-27 11:37 ` [Qemu-devel] [PATCH v2 3/3] Make cpu_single_env thread-local Peter Maydell
@ 2011-10-27 15:10   ` Andreas Färber
  2011-10-27 15:12     ` Paolo Bonzini
  2011-10-27 16:20     ` Andreas Färber
  0 siblings, 2 replies; 15+ messages in thread
From: Andreas Färber @ 2011-10-27 15:10 UTC (permalink / raw)
  To: Peter Maydell
  Cc: Paolo Bonzini, patches, Dr. David Alan Gilbert, qemu-devel,
	Jan Kiszka

Am 27.10.2011 13:37, schrieb Peter Maydell:
> From: Paolo Bonzini <pbonzini@redhat.com>
> 
> Make cpu_single_env thread-local. This fixes a regression
> in handling of multi-threaded programs in linux-user mode
> (bug 823902).
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> [Peter Maydell: rename tls_cpu_single_env to cpu_single_env]
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  cpu-all.h |    4 +++-
>  exec.c    |    2 +-
>  2 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/cpu-all.h b/cpu-all.h
> index 42a5fa0..5f47ab8 100644
> --- a/cpu-all.h
> +++ b/cpu-all.h
> @@ -20,6 +20,7 @@
>  #define CPU_ALL_H
>  
>  #include "qemu-common.h"
> +#include "qemu-tls.h"
>  #include "cpu-common.h"
>  
>  /* some important defines:
[8<]

Is there a reason not to include this from qemu-common.h? To avoid
accidental use in unsuited cases? If so, add an explicit comment to
either qemu-common.h or Jan's comment in qemu-tls.h to avoid someone
doing such a cleanup/simplification patch?

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Qemu-devel] [PATCH v2 3/3] Make cpu_single_env thread-local
  2011-10-27 15:10   ` Andreas Färber
@ 2011-10-27 15:12     ` Paolo Bonzini
  2011-10-27 16:20     ` Andreas Färber
  1 sibling, 0 replies; 15+ messages in thread
From: Paolo Bonzini @ 2011-10-27 15:12 UTC (permalink / raw)
  To: Andreas Färber
  Cc: Peter Maydell, patches, Dr. David Alan Gilbert, qemu-devel,
	Jan Kiszka

On 10/27/2011 05:10 PM, Andreas Färber wrote:
> Am 27.10.2011 13:37, schrieb Peter Maydell:
>> From: Paolo Bonzini<pbonzini@redhat.com>
>>
>> Make cpu_single_env thread-local. This fixes a regression
>> in handling of multi-threaded programs in linux-user mode
>> (bug 823902).
>>
>> Signed-off-by: Paolo Bonzini<pbonzini@redhat.com>
>> [Peter Maydell: rename tls_cpu_single_env to cpu_single_env]
>> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
>> ---
>>   cpu-all.h |    4 +++-
>>   exec.c    |    2 +-
>>   2 files changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/cpu-all.h b/cpu-all.h
>> index 42a5fa0..5f47ab8 100644
>> --- a/cpu-all.h
>> +++ b/cpu-all.h
>> @@ -20,6 +20,7 @@
>>   #define CPU_ALL_H
>>
>>   #include "qemu-common.h"
>> +#include "qemu-tls.h"
>>   #include "cpu-common.h"
>>
>>   /* some important defines:
> [8<]
>
> Is there a reason not to include this from qemu-common.h? To avoid
> accidental use in unsuited cases? If so, add an explicit comment to
> either qemu-common.h or Jan's comment in qemu-tls.h to avoid someone
> doing such a cleanup/simplification patch?

I think we're evolving towards having qemu-common.h as a common place 
for system header inclusions, but not really for QEMU-specific headers.

Paolo

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Qemu-devel] [PATCH v2 1/3] qemu-tls.h: Add abstraction layer for TLS variables
  2011-10-27 15:04   ` Andreas Färber
  2011-10-27 15:08     ` Paolo Bonzini
@ 2011-10-27 15:15     ` Peter Maydell
  2011-10-27 15:18       ` Paolo Bonzini
  2011-10-27 16:18     ` Andreas Färber
  2011-10-28  7:27     ` Markus Armbruster
  3 siblings, 1 reply; 15+ messages in thread
From: Peter Maydell @ 2011-10-27 15:15 UTC (permalink / raw)
  To: Andreas Färber
  Cc: Paolo Bonzini, patches, Dr. David Alan Gilbert, qemu-devel,
	Jan Kiszka

On 27 October 2011 16:04, Andreas Färber <afaerber@suse.de> wrote:
>> + * Copyright (c) 2011 Red Hat, Inc, Linaro Limited
>
> The concatenation looks kind of funny. ;)

Yeah, I know :-) Hope nobody in five years time goes looking for
those Inc guys to try to track them down for a licensing change...

>> +#ifndef QEMU_TLS_GCC_H
>> +#define QEMU_TLS_GCC_H
>
> Extra _GCC. But does no harm.

That's because I cut-and-pasted it from Paolo's qemu-tls-gcc.h...

>> +#define DEFINE_TLS(type, x)  __thread __typeof__(type) tls__##x

> I assume __typeof__() is a GCCism, indicated by ..._GCC_H, to ensure
> type is actually a valid type?

Paolo?

-- PMM

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Qemu-devel] [PATCH v2 1/3] qemu-tls.h: Add abstraction layer for TLS variables
  2011-10-27 15:15     ` Peter Maydell
@ 2011-10-27 15:18       ` Paolo Bonzini
  0 siblings, 0 replies; 15+ messages in thread
From: Paolo Bonzini @ 2011-10-27 15:18 UTC (permalink / raw)
  To: Peter Maydell
  Cc: Jan Kiszka, patches, Dr. David Alan Gilbert, Andreas Färber,
	qemu-devel

On 10/27/2011 05:15 PM, Peter Maydell wrote:
>
>>> >>  +#define DEFINE_TLS(type, x)  __thread __typeof__(type) tls__##x
>> >  I assume __typeof__() is a GCCism, indicated by ..._GCC_H, to ensure
>> >  type is actually a valid type?
> Paolo?

No, _GCC_H has nothing to do with GCCisms.  __typeof__ is needed so that 
"type" can be "int[5]":

     __thread __typeof__ (int[5]) foo; /* works */
     __thread int[5] foo; /* fails */

(even without __thread of course).

In fact, it has the opposite side effect: type can be any expression, 
but the assumption is that DEFINE_TLS(123, x) will be caught by review.

Paolo

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Qemu-devel] [PATCH v2 1/3] qemu-tls.h: Add abstraction layer for TLS variables
  2011-10-27 15:04   ` Andreas Färber
  2011-10-27 15:08     ` Paolo Bonzini
  2011-10-27 15:15     ` Peter Maydell
@ 2011-10-27 16:18     ` Andreas Färber
  2011-10-28  7:27     ` Markus Armbruster
  3 siblings, 0 replies; 15+ messages in thread
From: Andreas Färber @ 2011-10-27 16:18 UTC (permalink / raw)
  To: Peter Maydell
  Cc: Paolo Bonzini, Dr. David Alan Gilbert, qemu-devel, Jan Kiszka,
	patches

Am 27.10.2011 17:04, schrieb Andreas Färber:
> Am 27.10.2011 13:37, schrieb Peter Maydell:
>> Add an abstraction layer for defining and using thread-local
>> variables. For the moment this is implemented only for Linux,
>> which means they can only be used in restricted circumstances.
>> The abstraction layer allows us to add POSIX and Win32 support
>> later.
>>
> 
> [Paolo's SoB missing]
> 
>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>> ---
>>  qemu-tls.h |   51 +++++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 files changed, 51 insertions(+), 0 deletions(-)
>>  create mode 100644 qemu-tls.h
>>
>> diff --git a/qemu-tls.h b/qemu-tls.h
>> new file mode 100644
>> index 0000000..d96a159
>> --- /dev/null
>> +++ b/qemu-tls.h
>> @@ -0,0 +1,51 @@
>> +/*
>> + * Abstraction layer for defining and using TLS variables
>> + *
>> + * Copyright (c) 2011 Red Hat, Inc, Linaro Limited
> 
> The concatenation looks kind of funny. ;)
> 
>> + *
>> + * 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_GCC_H
>> +#define QEMU_TLS_GCC_H
> 
> Extra _GCC. But does no harm.
> 
>> +
>> +/* 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 get_tls(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 get_tls(x)           tls__##x
>> +#endif
>> +
>> +#endif
> 
> Looks okay to me.

Reviewed-by: Andreas Färber <afaerber@suse.de>

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Qemu-devel] [PATCH v2 3/3] Make cpu_single_env thread-local
  2011-10-27 15:10   ` Andreas Färber
  2011-10-27 15:12     ` Paolo Bonzini
@ 2011-10-27 16:20     ` Andreas Färber
  1 sibling, 0 replies; 15+ messages in thread
From: Andreas Färber @ 2011-10-27 16:20 UTC (permalink / raw)
  To: Peter Maydell
  Cc: Paolo Bonzini, Dr. David Alan Gilbert, qemu-devel, Jan Kiszka,
	patches

Am 27.10.2011 17:10, schrieb Andreas Färber:
> Am 27.10.2011 13:37, schrieb Peter Maydell:
>> From: Paolo Bonzini <pbonzini@redhat.com>
>>
>> Make cpu_single_env thread-local. This fixes a regression
>> in handling of multi-threaded programs in linux-user mode
>> (bug 823902).
>>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> [Peter Maydell: rename tls_cpu_single_env to cpu_single_env]
>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Andreas Färber <afaerber@suse.de>

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Qemu-devel] [PATCH v2 1/3] qemu-tls.h: Add abstraction layer for TLS variables
  2011-10-27 15:04   ` Andreas Färber
                       ` (2 preceding siblings ...)
  2011-10-27 16:18     ` Andreas Färber
@ 2011-10-28  7:27     ` Markus Armbruster
  2011-10-28  7:45       ` Peter Maydell
  3 siblings, 1 reply; 15+ messages in thread
From: Markus Armbruster @ 2011-10-28  7:27 UTC (permalink / raw)
  To: Andreas Färber
  Cc: Peter Maydell, Dr. David Alan Gilbert, patches, Jan Kiszka,
	qemu-devel, Paolo Bonzini

Andreas Färber <afaerber@suse.de> writes:

> Am 27.10.2011 13:37, schrieb Peter Maydell:
>> Add an abstraction layer for defining and using thread-local
>> variables. For the moment this is implemented only for Linux,
>> which means they can only be used in restricted circumstances.
>> The abstraction layer allows us to add POSIX and Win32 support
>> later.
>> 
>
> [Paolo's SoB missing]
>
>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>> ---
>>  qemu-tls.h |   51 +++++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 files changed, 51 insertions(+), 0 deletions(-)
>>  create mode 100644 qemu-tls.h
>> 
>> diff --git a/qemu-tls.h b/qemu-tls.h
>> new file mode 100644
>> index 0000000..d96a159
>> --- /dev/null
>> +++ b/qemu-tls.h
>> @@ -0,0 +1,51 @@
>> +/*
>> + * Abstraction layer for defining and using TLS variables
>> + *
>> + * Copyright (c) 2011 Red Hat, Inc, Linaro Limited
>
> The concatenation looks kind of funny. ;)

I'd split into

 * 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_GCC_H
>> +#define QEMU_TLS_GCC_H
>
> Extra _GCC. But does no harm.

Unless you file "unusual" under harm, which I happen to do :)

>> +
>> +/* 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 get_tls(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 get_tls(x)           tls__##x
>> +#endif

Any particular reason for pasting tls__ onto the identifier?

>> +
>> +#endif
[...]

I wish we'll be able to ditch these cumbersome, ugly macros in favor of
C1X threading support.  It'll probably remain a wish forever, given the
lengths we go to support systems that stubbornly refuse to update their
C programming environment for the 21st century.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Qemu-devel] [PATCH v2 1/3] qemu-tls.h: Add abstraction layer for TLS variables
  2011-10-28  7:27     ` Markus Armbruster
@ 2011-10-28  7:45       ` Peter Maydell
  0 siblings, 0 replies; 15+ messages in thread
From: Peter Maydell @ 2011-10-28  7:45 UTC (permalink / raw)
  To: Markus Armbruster
  Cc: Dr. David Alan Gilbert, patches, Jan Kiszka, qemu-devel,
	Paolo Bonzini, Andreas Färber

On 28 October 2011 08:27, Markus Armbruster <armbru@redhat.com> wrote:
> Andreas Färber <afaerber@suse.de> writes:
>
>> Am 27.10.2011 13:37, schrieb Peter Maydell:
>>> + * Copyright (c) 2011 Red Hat, Inc, Linaro Limited
>>
>> The concatenation looks kind of funny. ;)
>
> I'd split into
>
>  * Copyright (c) 2011 Red Hat, Inc
>  * Copyright (c) 2011 Linaro Limited

>>> +#ifndef QEMU_TLS_GCC_H
>>> +#define QEMU_TLS_GCC_H
>>
>> Extra _GCC. But does no harm.
>
> Unless you file "unusual" under harm, which I happen to do :)

OK, I'll fix these nits and resend later this morning.

>>> +#ifdef __linux__
>>> +#define DECLARE_TLS(type, x) extern DEFINE_TLS(type, x)
>>> +#define DEFINE_TLS(type, x)  __thread __typeof__(type) tls__##x
>>> +#define get_tls(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 get_tls(x)           tls__##x
>>> +#endif
>
> Any particular reason for pasting tls__ onto the identifier?

It means we catch accidentally using the identifier directly
rather than via get_tls() at compile time. (That doesn't matter
for the __thread case, obviously, but does for other implementations.)
(also it means we get out of the way of the cpu_single_env macro
we define in patch 3.)

-- PMM

^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2011-10-28  7:45 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-10-27 11:37 [Qemu-devel] [PATCH v2 0/3] TLS abstraction layer for thread-local cpu_single_env on Linux Peter Maydell
2011-10-27 11:37 ` [Qemu-devel] [PATCH v2 1/3] qemu-tls.h: Add abstraction layer for TLS variables Peter Maydell
2011-10-27 15:04   ` Andreas Färber
2011-10-27 15:08     ` Paolo Bonzini
2011-10-27 15:15     ` Peter Maydell
2011-10-27 15:18       ` Paolo Bonzini
2011-10-27 16:18     ` Andreas Färber
2011-10-28  7:27     ` Markus Armbruster
2011-10-28  7:45       ` Peter Maydell
2011-10-27 11:37 ` [Qemu-devel] [PATCH v2 2/3] darwin-user/main.c: Drop unused cpu_single_env definition Peter Maydell
2011-10-27 13:22   ` Andreas Färber
2011-10-27 11:37 ` [Qemu-devel] [PATCH v2 3/3] Make cpu_single_env thread-local Peter Maydell
2011-10-27 15:10   ` Andreas Färber
2011-10-27 15:12     ` Paolo Bonzini
2011-10-27 16:20     ` Andreas Färber

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).