Skip to contentSkip to conversation

make atomic_read() and atomic_set() behavior consistent across all architectures

PATCH In review Chris Snook 11 participants 50 messages Aug 13, 2007 – Aug 23, 2007

Subsystems (5)
Memory management Documentation Generic include/asm header files Slab allocator +1 more subsystems
lore
Share & tools
lore

Chris Snook

author
#0 lore raw

cover make atomic_read() and atomic_set() behavior consistent across all architectures

By popular demand, I've redone the patchset to include volatile casts in atomic_set as well. I've also converted the macros to inline functions, to help catch type mismatches at compile time.

This will do weird things on ia64 without Andreas Schwab's fix:

http://lkml.org/lkml/2007/8/10/410

Notably absent is a patch for powerpc. I expect Segher Boessenkool's assembly implementation should suffice there:

http://lkml.org/lkml/2007/8/10/470

Thanks to all who commented on previous incarnations.

-- Chris Snook

author
#1 lore raw

PATCH 1/23 document preferred use of volatile with atomic_t

From: Chris Snook <[email protected]>

Document proper use of volatile for atomic_t operations.

Signed-off-by Chris Snook <[email protected]>
Documentation/atomic_ops.txt +10 -3
@@ -12,13 +12,20 @@
 C integer type will fail.  Something like the following should
 suffice:
 
-	typedef struct { volatile int counter; } atomic_t;
+	typedef struct { int counter; } atomic_t;
+
+	Historically, counter has been declared as a volatile int.  This
+is now discouraged in favor of explicitly casting it as volatile where
+volatile behavior is required.  Most architectures will only require such
+a cast in atomic_read() and atomic_set(), as well as their 64-bit versions
+if applicable, since the more complex atomic operations directly or
+indirectly use assembly that results in volatile behavior.
 
 	The first operations to implement for atomic_t's are the
 initializers and plain reads.
 
 	#define ATOMIC_INIT(i)		{ (i) }
-	#define atomic_set(v, i)	((v)->counter = (i))
+	#define atomic_set(v, i)	(*(volatile int *)&(v)->counter = (i))
 
 The first macro is used in definitions, such as:
 

@@ -38,7 +45,7 @@
 
 Next, we have:
 
-	#define atomic_read(v)	((v)->counter)
+	#define atomic_read(v)	(*(volatile int *)&(v)->counter)
 
 which simply reads the current value of the counter.
Acked-by on v1 1/23
#2 lore raw

Re: [PATCH 1/23] document preferred use of volatile with atomic_t

Quoted 3 lines · Chris Snook

From: Chris Snook <[email protected]>

Document proper use of volatile for atomic_t operations.

Looks good, as did a once-over on the arch-specific files. Good stuff!!!

Acked-by Paul E. McKenney <[email protected]>
Quoted 36 lines
Signed-off-by Chris Snook <[email protected]>
Documentation/atomic_ops.txt +10 -3
@@ -12,13 +12,20 @@
 C integer type will fail.  Something like the following should
 suffice:
 
-	typedef struct { volatile int counter; } atomic_t;
+	typedef struct { int counter; } atomic_t;
+
+	Historically, counter has been declared as a volatile int.  This
+is now discouraged in favor of explicitly casting it as volatile where
+volatile behavior is required.  Most architectures will only require such
+a cast in atomic_read() and atomic_set(), as well as their 64-bit versions
+if applicable, since the more complex atomic operations directly or
+indirectly use assembly that results in volatile behavior.
 
 	The first operations to implement for atomic_t's are the
 initializers and plain reads.
 
 	#define ATOMIC_INIT(i)		{ (i) }
-	#define atomic_set(v, i)	((v)->counter = (i))
+	#define atomic_set(v, i)	(*(volatile int *)&(v)->counter = (i))
 
 The first macro is used in definitions, such as:
 

@@ -38,7 +45,7 @@
 
 Next, we have:
 
-	#define atomic_read(v)	((v)->counter)
+	#define atomic_read(v)	(*(volatile int *)&(v)->counter)
 
 which simply reads the current value of the counter.
#3 lore raw

Re: [PATCH 1/23] document preferred use of volatile with atomic_t

In reply to Chris Snook

Documentation/atomic_ops.txt line 50

Quoted 8 lines · Chris Snook
Documentation/atomic_ops.txt +1 -1
@@ -38,7 +45,7 @@
 
 Next, we have:
 
-	#define atomic_read(v)	((v)->counter)
+	#define atomic_read(v)	(*(volatile int *)&(v)->counter)
 
 which simply reads the current value of the counter.

volatile means that there is some vague notion of "read it now". But that really does not exist. Instead we control visibility via barriers (smp_wmb, smp_rmb). Would it not be best to not have volatile at all in atomic operations and let the barriers do the work?

author
#4 lore raw

Re: [PATCH 1/23] document preferred use of volatile with atomic_t

Quoted 4 lines · Christoph Lameter
Quoted 2 lines · Chris Snook
(quoted diff) +0 -0
@@ -38,7 +45,7 @@
Quoted 8 lines

Documentation/atomic_ops.txt line 50

Quoted 6 lines
Documentation/atomic_ops.txt +1 -1
 Next, we have:
 
-	#define atomic_read(v)	((v)->counter)
+	#define atomic_read(v)	(*(volatile int *)&(v)->counter)
 
 which simply reads the current value of the counter.

volatile means that there is some vague notion of "read it now". But that really does not exist. Instead we control visibility via barriers (smp_wmb, smp_rmb). Would it not be best to not have volatile at all in atomic operations and let the barriers do the work?

From my reply in the other thread...

But barriers force a flush of *everything* in scope, which we generally don't want. On the other hand, we pretty much always want to flush atomic_* operations. One way or another, we should be restricting the volatile behavior to the thing that needs it. On most architectures, this patch set just moves that from the declaration, where it is considered harmful, to the use, where it is considered an occasional necessary evil.

If you really, *really* distrust the compiler that much, you shouldn't be using barrier, since that uses volatile under the hood too. You should just go ahead and implement the atomic operations in assembler, like Segher Boessenkool did for powerpc in response to my previous patchset.

-- Chris

#5 lore raw

Re: [PATCH 1/23] document preferred use of volatile with atomic_t

Quoted 14 lines · Chris Snook
Quoted 4 lines

volatile means that there is some vague notion of "read it now". But that really does not exist. Instead we control visibility via barriers (smp_wmb, smp_rmb). Would it not be best to not have volatile at all in atomic operations and let the barriers do the work?

From my reply in the other thread...

But barriers force a flush of *everything* in scope, which we generally don't want. On the other hand, we pretty much always want to flush atomic_* operations. One way or another, we should be restricting the volatile behavior to the thing that needs it. On most architectures, this patch set just moves that from the declaration, where it is considered harmful, to the use, where it is considered an occasional necessary evil.

Quoted 4 lines

If you really, *really* distrust the compiler that much, you shouldn't be using barrier, since that uses volatile under the hood too. You should just go ahead and implement the atomic operations in assembler, like Segher Boessenkool did for powerpc in response to my previous patchset.

From my reply on the other thread:

Maybe we need two read functions? One volatile, one not?

The atomic_read()s that I have in slub really do not care about when the variables are read. And if volatile creates overhead then I rather not have it.

author
#6 lore raw

Re: [PATCH 1/23] document preferred use of volatile with atomic_t

Quoted 15 lines · Christoph Lameter
Quoted 13 lines · Chris Snook
Quoted 4 lines

volatile means that there is some vague notion of "read it now". But that really does not exist. Instead we control visibility via barriers (smp_wmb, smp_rmb). Would it not be best to not have volatile at all in atomic operations and let the barriers do the work?

From my reply in the other thread...

But barriers force a flush of *everything* in scope, which we generally don't want. On the other hand, we pretty much always want to flush atomic_* operations. One way or another, we should be restricting the volatile behavior to the thing that needs it. On most architectures, this patch set just moves that from the declaration, where it is considered harmful, to the use, where it is considered an occasional necessary evil.

Quoted 8 lines
Quoted 4 lines

If you really, *really* distrust the compiler that much, you shouldn't be using barrier, since that uses volatile under the hood too. You should just go ahead and implement the atomic operations in assembler, like Segher Boessenkool did for powerpc in response to my previous patchset.

From my reply on the other thread:

Maybe we need two read functions? One volatile, one not?

If we're going to do this, and I don't think we need to, I'd prefer that atomic_read() be volatile, and something like atomic_read_opt() be non-volatile, to discourage premature optimization.

Quoted 1 line

The atomic_read()s that I have in slub really do not care about when the variables are read. And if volatile creates overhead then I rather not have it.

A single volatile access is no more expensive than a non-volatile access. It's when you have dependencies that you start to see overhead. If you're doing a bunch of atomic operations on the same atomic_t in quick succession, then you will see some overhead. Of course, if you're doing that, I think you have a design problem.

On modern, register-rich CPUs with cache latencies of a couple clock cycles, volatile generally isn't as much of a performance hit as it used to be. I think that going out of your way to avoid it would be premature optimization on modern hardware.

-- Chris

#7 lore raw

Re: [PATCH 1/23] document preferred use of volatile with atomic_t

In reply to Christoph Lameter
Quoted 21 lines · Christoph Lameter
Quoted 18 lines · Chris Snook
Quoted 4 lines

volatile means that there is some vague notion of "read it now". But that really does not exist. Instead we control visibility via barriers (smp_wmb, smp_rmb). Would it not be best to not have volatile at all in atomic operations and let the barriers do the work?

From my reply in the other thread...

But barriers force a flush of *everything* in scope, which we generally don't want. On the other hand, we pretty much always want to flush atomic_* operations. One way or another, we should be restricting the volatile behavior to the thing that needs it. On most architectures, this patch set just moves that from the declaration, where it is considered harmful, to the use, where it is considered an occasional necessary evil.

If you really, *really* distrust the compiler that much, you shouldn't be using barrier, since that uses volatile under the hood too. You should just go ahead and implement the atomic operations in assembler, like Segher Boessenkool did for powerpc in response to my previous patchset.

Quoted 6 lines
Quoted 1 line

From my reply on the other thread:

Maybe we need two read functions? One volatile, one not?

The atomic_read()s that I have in slub really do not care about when the variables are read. And if volatile creates overhead then I rather not have it.

The overhead due to volatile access is -way- small. Not like barrier(), which can flush out a fair fraction of the machine registers.

Thanx, Paul

#8 lore raw

Re: [PATCH 1/23] document preferred use of volatile with atomic_t

In reply to Chris Snook
Quoted 1 line

But barriers force a flush of *everything* in scope,

Nonsense, whatever "flush" is supposed to mean here.

Quoted 1 line

If you really, *really* distrust the compiler that much, you shouldn't be using barrier, since that uses volatile under the hood too. You should just go ahead and implement the atomic operations in assembler, like Segher Boessenkool did for powerpc in response to my previous patchset.

Puh-lease. I DO NOT DISTRUST THE COMPILER, I just don't assume it will do whatever I would like it to do without telling it. It's a machine you know, and it is very well documented.

(And most barriers don't (need to) use volatile).

Implementing the atomic ops in asm loses exactly *no* semantics, and it doesn't add restrictions either; it does allow you however to access an atomic_t with normal loads/stores independently, if you so choose. The only valid issue brought up so far is Russell King's comment that GCC cannot schedule the machine instructions in and around an asm() perfectly, it doesn't look inside the asm() after all; but the situation isn't quite as sever as he suggests (GCC _does_ know you are performing loads/stores, etc.); more about that later, when I've finished researching the current state of that stuff.

Segher

author
#9 lore raw

PATCH 2/23 make atomic_read() and atomic_set() behavior consistent on alpha

In reply to Chris Snook

From: Chris Snook <[email protected]>

Use volatile consistently in atomic.h on alpha.

Signed-off-by Chris Snook <[email protected]>
include/asm-alpha/atomic.h +23 -9
@@ -14,21 +14,35 @@
 
 
 /*
- * Counter is volatile to make sure gcc doesn't try to be clever
- * and move things around on us. We need to use _exactly_ the address
- * the user gave us, not some alias that contains the same information.
+ * Make sure gcc doesn't try to be clever and move things around
+ * on us. We need to use _exactly_ the address the user gave us,
+ * not some alias that contains the same information.
  */
-typedef struct { volatile int counter; } atomic_t;
-typedef struct { volatile long counter; } atomic64_t;
+typedef struct { int counter; } atomic_t;
+typedef struct { long counter; } atomic64_t;
 
 #define ATOMIC_INIT(i)		( (atomic_t) { (i) } )
 #define ATOMIC64_INIT(i)	( (atomic64_t) { (i) } )
 
-#define atomic_read(v)		((v)->counter + 0)
-#define atomic64_read(v)	((v)->counter + 0)
+static __inline__ int atomic_read(atomic_t *v)
+{
+	return *(volatile int *)&v->counter + 0;
+}
+
+static __inline__ long atomic64_read(atomic64_t *v)
+{
+	return *(volatile long *)&v->counter + 0;
+}
 
-#define atomic_set(v,i)		((v)->counter = (i))
-#define atomic64_set(v,i)	((v)->counter = (i))
+static __inline__ void atomic_set(atomic_t *v, int i)
+{
+	*(volatile int *)&v->counter = i;
+}
+
+static __inline__ void atomic64_set(atomic64_t *v, long i)
+{
+	*(volatile long *)&v->counter = i;
+}
 
 /*
  * To get proper branch prediction for the main line, we must branch
author
#10 lore raw

PATCH 3/23 make atomic_read() and atomic_set() behavior consistent on arm

In reply to Chris Snook

From: Chris Snook <[email protected]>

Use volatile consistently in atomic.h on arm.

Signed-off-by Chris Snook <[email protected]>
include/asm-arm/atomic.h +5 -2
@@ -14,13 +14,16 @@
 #include <linux/compiler.h>
 #include <asm/system.h>
 
-typedef struct { volatile int counter; } atomic_t;
+typedef struct { int counter; } atomic_t;
 
 #define ATOMIC_INIT(i)	{ (i) }
 
 #ifdef __KERNEL__
 
-#define atomic_read(v)	((v)->counter)
+static inline int atomic_read(atomic_t *v)
+{
+        return *(volatile int *)&v->counter;
+}
 
 #if __LINUX_ARM_ARCH__ >= 6
#11 lore raw

Re: [PATCH 3/23] make atomic_read() and atomic_set() behavior consistent on arm

Quoted 17 lines · Chris Snook

From: Chris Snook <[email protected]>

Use volatile consistently in atomic.h on arm.

Signed-off-by Chris Snook <[email protected]>
include/asm-arm/atomic.h +1 -1
@@ -14,13 +14,16 @@
 #include <linux/compiler.h>
 #include <asm/system.h>
 
-typedef struct { volatile int counter; } atomic_t;
+typedef struct { int counter; } atomic_t;
 
 #define ATOMIC_INIT(i)	{ (i) }

include/asm-arm/atomic.h line 29

Quoted 10 lines
include/asm-arm/atomic.h +4 -1
 #ifdef __KERNEL__
 
-#define atomic_read(v)	((v)->counter)
+static inline int atomic_read(atomic_t *v)
+{
+        return *(volatile int *)&v->counter;
+}
 
 #if __LINUX_ARM_ARCH__ >= 6

...
In the first email of the series:

Quoted 3 lines

By popular demand, I've redone the patchset to include volatile casts in atomic_set as well. I've also converted the macros to inline functions, to help catch type mismatches at compile time.

and in the rest of include/asm-arm/atomic.h:

#else /* ARM_ARCH_6 */

#include <asm/system.h>

#ifdef CONFIG_SMP
#error SMP not supported on pre-ARMv6 CPUs
#endif

#define atomic_set(v,i) (((v)->counter) = (i))

Seems you missed that. Grep is a wonderful tool.

Signature · 3 lines
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:
author
#12 lore raw

Re: [PATCH 3/23] make atomic_read() and atomic_set() behavior consistent on arm

Quoted 11 lines · Russell King
Quoted 3 lines · Chris Snook

By popular demand, I've redone the patchset to include volatile casts in atomic_set as well. I've also converted the macros to inline functions, to help catch type mismatches at compile time.

and in the rest of include/asm-arm/atomic.h:

#else /* ARM_ARCH_6 */

#include <asm/system.h>

Quoted 7 lines

#ifdef CONFIG_SMP
#error SMP not supported on pre-ARMv6 CPUs
#endif

#define atomic_set(v,i) (((v)->counter) = (i))

Seems you missed that. Grep is a wonderful tool.

D'oh! Try this.

-- Chris

include/asm-arm/atomic.h +9 -3
@@ -14,13 +14,16 @@
 #include <linux/compiler.h>
 #include <asm/system.h>
 
-typedef struct { volatile int counter; } atomic_t;
+typedef struct { int counter; } atomic_t;
 
 #define ATOMIC_INIT(i)	{ (i) }
 
 #ifdef __KERNEL__
 
-#define atomic_read(v)	((v)->counter)
+static inline int atomic_read(atomic_t *v)
+{
+        return *(volatile int *)&v->counter;
+}
 
 #if __LINUX_ARM_ARCH__ >= 6
 

@@ -122,7 +125,10 @@ static inline void atomic_clear_mask(uns
 #error SMP not supported on pre-ARMv6 CPUs
 #endif
 
-#define atomic_set(v,i)	(((v)->counter) = (i))
+static inline void atomic_set(atomic_t *v, int i)
+{
+        *(volatile int *)&v->counter = i;
+}
 
 static inline int atomic_add_return(int i, atomic_t *v)
 {
Acked-by
#13 lore raw

Re: [PATCH 3/23] make atomic_read() and atomic_set() behavior consistent on arm

Quoted 16 lines · Chris Snook
Quoted 15 lines · Russell King
Quoted 3 lines · Chris Snook

By popular demand, I've redone the patchset to include volatile casts in atomic_set as well. I've also converted the macros to inline functions, to help catch type mismatches at compile time.

and in the rest of include/asm-arm/atomic.h:

#else /* ARM_ARCH_6 */

#include <asm/system.h>

#ifdef CONFIG_SMP
#error SMP not supported on pre-ARMv6 CPUs
#endif

Quoted 5 lines
Quoted 3 lines

#define atomic_set(v,i) (((v)->counter) = (i))

Seems you missed that. Grep is a wonderful tool.

D'oh! Try this.

Thanks.

Acked-by Russell King <[email protected]>
Quoted 37 lines
include/asm-arm/atomic.h +9 -4
@@ -14,13 +14,16 @@
 #include <linux/compiler.h>
 #include <asm/system.h>
 
-typedef struct { volatile int counter; } atomic_t;
+typedef struct { int counter; } atomic_t;
 
 #define ATOMIC_INIT(i)	{ (i) }
 
 #ifdef __KERNEL__
 
-#define atomic_read(v)	((v)->counter)
+static inline int atomic_read(atomic_t *v)
+{
+        return *(volatile int *)&v->counter;
+}
 
 #if __LINUX_ARM_ARCH__ >= 6
 

@@ -122,7 +125,10 @@ static inline void atomic_clear_mask(uns
 #error SMP not supported on pre-ARMv6 CPUs
 #endif
 
-#define atomic_set(v,i)	(((v)->counter) = (i))
+static inline void atomic_set(atomic_t *v, int i)
+{
+        *(volatile int *)&v->counter = i;
+}
 
 static inline int atomic_add_return(int i, atomic_t *v)
 {
-

To unsubscribe from this list: send the line "unsubscribe linux-arch" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Signature · 3 lines
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:
author
#14 lore raw

PATCH 4/23 make atomic_read() and atomic_set() behavior consistent on avr32

In reply to Chris Snook

From: Chris Snook <[email protected]>

Use volatile consistently in atomic.h on avr32.

Signed-off-by Chris Snook <[email protected]>
include/asm-avr32/atomic.h +10 -3
@@ -16,11 +16,18 @@
 
 #include <asm/system.h>
 
-typedef struct { volatile int counter; } atomic_t;
+typedef struct { int counter; } atomic_t;
 #define ATOMIC_INIT(i)  { (i) }
 
-#define atomic_read(v)		((v)->counter)
-#define atomic_set(v, i)	(((v)->counter) = i)
+static inline int atomic_read(atomic_t *v)
+{
+        return *(volatile int *)&v->counter;
+}
+
+static inline void atomic_set(atomic_t *v, int i)
+{
+        *(volatile int *)&v->counter = i;
+}
 
 /*
  * atomic_sub_return - subtract the atomic variable
author
#15 lore raw

PATCH 5/23 make atomic_read() and atomic_set() behavior consistent on blackfin

In reply to Chris Snook

From: Chris Snook <[email protected]>

Use volatile consistently in atomic.h on blackfin.

Signed-off-by Chris Snook <[email protected]>
include/asm-blackfin/atomic.h +9 -2
@@ -18,8 +18,15 @@ typedef struct {
 } atomic_t;
 #define ATOMIC_INIT(i)	{ (i) }
 
-#define atomic_read(v)		((v)->counter)
-#define atomic_set(v, i)	(((v)->counter) = i)
+static inline int atomic_read(atomic_t *v)
+{
+        return *(volatile int *)&v->counter;
+}
+
+static inline void atomic_set(atomic_t *v, int i)
+{
+        *(volatile int *)&v->counter = i;
+}
 
 static __inline__ void atomic_add(int i, atomic_t * v)
 {
author
#16 lore raw

PATCH 6/23 make atomic_read() and atomic_set() behavior consistent on cris

In reply to Chris Snook

From: Chris Snook <[email protected]>

Use volatile consistently in atomic.h on cris.

Signed-off-by Chris Snook <[email protected]>
include/asm-cris/atomic.h +10 -3
@@ -11,12 +11,19 @@
  * resource counting etc..
  */
 
-typedef struct { volatile int counter; } atomic_t;
+typedef struct { int counter; } atomic_t;
 
 #define ATOMIC_INIT(i)  { (i) }
 
-#define atomic_read(v) ((v)->counter)
-#define atomic_set(v,i) (((v)->counter) = (i))
+static inline int atomic_read(atomic_t *v)
+{
+        return *(volatile int *)&v->counter;
+}
+
+static inline void atomic_set(atomic_t *v, int i)
+{
+        *(volatile int *)&v->counter = i;
+}
 
 /* These should be written in asm but we do it in C for now. */
author
#17 lore raw

PATCH 7/23 make atomic_read() and atomic_set() behavior consistent on frv

In reply to Chris Snook

From: Chris Snook <[email protected]>

Use volatile consistently in atomic.h on frv.

Signed-off-by Chris Snook <[email protected]>
include/asm-frv/atomic.h +10 -2
@@ -40,8 +40,16 @@ typedef struct {
 } atomic_t;
 
 #define ATOMIC_INIT(i)		{ (i) }
-#define atomic_read(v)		((v)->counter)
-#define atomic_set(v, i)	(((v)->counter) = (i))
+
+static inline int atomic_read(atomic_t *v)
+{
+        return *(volatile int *)&v->counter;
+}
+
+static inline void atomic_set(atomic_t *v, int i)
+{
+        *(volatile int *)&v->counter = i;
+}
 
 #ifndef CONFIG_FRV_OUTOFLINE_ATOMIC_OPS
 static inline int atomic_add_return(int i, atomic_t *v)
author
#19 lore raw

PATCH 8/23 make atomic_read() and atomic_set() behavior consistent on h8300

In reply to Chris Snook

From: Chris Snook <[email protected]>

Use volatile consistently in atomic.h on h8300.

Signed-off-by Chris Snook <[email protected]>
include/asm-h8300/atomic.h +9 -2
@@ -9,8 +9,15 @@
 typedef struct { int counter; } atomic_t;
 #define ATOMIC_INIT(i)	{ (i) }
 
-#define atomic_read(v)		((v)->counter)
-#define atomic_set(v, i)	(((v)->counter) = i)
+static inline int atomic_read(atomic_t *v)
+{
+        return *(volatile int *)&v->counter;
+}
+
+static inline void atomic_set(atomic_t *v, int i)
+{
+        *(volatile int *)&v->counter = i;
+}
 
 #include <asm/system.h>
 #include <linux/kernel.h>
author
#20 lore raw

PATCH 9/23 make atomic_read() and atomic_set() behavior consistent on i386

In reply to Chris Snook

From: Chris Snook <[email protected]>

Use volatile consistently in atomic.h on i386.

Signed-off-by Chris Snook <[email protected]>
include/asm-i386/atomic.h +8 -2
@@ -25,7 +25,10 @@ typedef struct { int counter; } atomic_t
  * 
  * Atomically reads the value of @v.
  */ 
-#define atomic_read(v)		((v)->counter)
+static __inline__ int atomic_read(atomic_t *v)
+{
+        return *(volatile int *)&v->counter;
+}
 
 /**
  * atomic_set - set atomic variable

@@ -34,7 +37,10 @@ typedef struct { int counter; } atomic_t
  * 
  * Atomically sets the value of @v to @i.
  */ 
-#define atomic_set(v,i)		(((v)->counter) = (i))
+static __inline__ void atomic_set(atomic_t *v, int i)
+{
+        *(volatile int *)&v->counter = i;
+}
 
 /**
  * atomic_add - add integer to atomic variable
author
#21 lore raw

PATCH 10/23 make atomic_read() and atomic_set() behavior consistent on ia64

In reply to Chris Snook

From: Chris Snook <[email protected]>

Use volatile consistently in atomic.h on ia64. This will do weird things without Andreas Schwab's fix:
http://lkml.org/lkml/2007/8/10/410

Signed-off-by Chris Snook <[email protected]>
include/asm-ia64/atomic.h +22 -7
@@ -19,19 +19,34 @@
 
 /*
  * On IA-64, counter must always be volatile to ensure that that the
- * memory accesses are ordered.
+ * memory accesses are ordered.  This must be enforced each time that
+ * counter is read or written.
  */
-typedef struct { volatile __s32 counter; } atomic_t;
-typedef struct { volatile __s64 counter; } atomic64_t;
+typedef struct { __s32 counter; } atomic_t;
+typedef struct { __s64 counter; } atomic64_t;
 
 #define ATOMIC_INIT(i)		((atomic_t) { (i) })
 #define ATOMIC64_INIT(i)	((atomic64_t) { (i) })
 
-#define atomic_read(v)		((v)->counter)
-#define atomic64_read(v)	((v)->counter)
+static inline __s32 atomic_read(atomic_t *v)
+{
+        return *(volatile __s32 *)&v->counter;
+}
+
+static inline void atomic_set(atomic_t *v, __s32 i)
+{
+        *(volatile __s32 *)&v->counter = i;
+}
 
-#define atomic_set(v,i)		(((v)->counter) = (i))
-#define atomic64_set(v,i)	(((v)->counter) = (i))
+static inline __s64 atomic64_read(atomic64_t *v)
+{
+        return *(volatile __s64 *)&v->counter;
+}
+
+static inline void atomic64_set(atomic64_t *v, __s64 i)
+{
+        *(volatile __s64 *)&v->counter = i;
+}
 
 static __inline__ int
 ia64_atomic_add (int i, atomic_t *v)
#22 lore raw

RE: [PATCH 10/23] make atomic_read() and atomic_set() behavior consistent on ia64

Quoted 3 lines

Use volatile consistently in atomic.h on ia64. This will do weird things without Andreas Schwab's fix:
http://lkml.org/lkml/2007/8/10/410

The build is very noisy with the inline versions of atomic_{read,set} and their 64-bit siblings. Here are the prime culprits (some of them repeat >100 times).

include/linux/skbuff.h:521: warning: passing arg 1 of atomic_read' discards qualifiers from pointer target type include/net/sock.h:1244: warning: passing arg 1 of atomic_read' discards qualifiers from pointer target type include/net/tcp.h:958: warning: passing arg 1 of atomic_read' discards qualifiers from pointer target type mm/slub.c:3115: warning: passing arg 1 of atomic_read' from incompatible pointer type mm/slub.c:3250: warning: passing arg 1 of atomic_read' from incompatible pointer type mm/slub.c:3286: warning: passing arg 1 of atomic_read' from incompatible pointer type

The inline versions also result in some structural changes in the object file that make it difficult to compare with the original. Text size is 96 bytes smaller ... but even after I use sed(1) to exclude the most obvious instructions that differ, I still find big blocks of code with changes. Perhaps even more surprising there are entire functions that are optimized out in either the 'before' or 'after' binary. E.g. lookup_pi_state() was optimized away (or completely inlined?) before this patch, but the function appears as standalone in the 'after' version. The reverse is true for fixup_pi_state_owner().

The binary does boot ... but I haven't run any tests to see whether there are any problems.

-Tony

author
#23 lore raw

Re: [PATCH 10/23] make atomic_read() and atomic_set() behavior consistent on ia64

Quoted 7 lines · Tony
Quoted 3 lines

Use volatile consistently in atomic.h on ia64. This will do weird things without Andreas Schwab's fix:
http://lkml.org/lkml/2007/8/10/410

The build is very noisy with the inline versions of atomic_{read,set} and their 64-bit siblings. Here are the prime culprits (some of them repeat >100 times).

Part of the motivation for using inline functions was to expose places where we've been lazy, so this isn't unexpected. We need to work on clearing up those callers.

Quoted 6 lines

include/linux/skbuff.h:521: warning: passing arg 1 of atomic_read' discards qualifiers from pointer target type include/net/sock.h:1244: warning: passing arg 1 of atomic_read' discards qualifiers from pointer target type include/net/tcp.h:958: warning: passing arg 1 of atomic_read' discards qualifiers from pointer target type mm/slub.c:3115: warning: passing arg 1 of atomic_read' from incompatible pointer type mm/slub.c:3250: warning: passing arg 1 of atomic_read' from incompatible pointer type mm/slub.c:3286: warning: passing arg 1 of atomic_read' from incompatible pointer type

Do you get any warnings other than those two?

Quoted 11 lines

The inline versions also result in some structural changes in the object file that make it difficult to compare with the original. Text size is 96 bytes smaller ... but even after I use sed(1) to exclude the most obvious instructions that differ, I still find big blocks of code with changes. Perhaps even more surprising there are entire functions that are optimized out in either the 'before' or 'after' binary. E.g. lookup_pi_state() was optimized away (or completely inlined?) before this patch, but the function appears as standalone in the 'after' version. The reverse is true for fixup_pi_state_owner().

IIRC, when you applied a version which used macros instead, there was no change.
It would seem that inlining changed the optimization behavior of the compiler. If you turn down the optimization level, do the macro and inline versions look
the same, or at least more similar?

Quoted 2 lines

The binary does boot ... but I haven't run any tests to see whether there are any problems.

The only part of the patch that I was really worried about breaking anything was the removal of the volatile declaration, in case there was some other access that needed a cast. Since the macro version didn't change anything, that's covered. Converting from a macro to an inline shouldn't really change anything in this case, except perhaps for how the compiler optimizes it. If something *does* break, I'd suspect compiler bugs.

-- Chris

#24 lore raw

RE: [PATCH 10/23] make atomic_read() and atomic_set() behavior consistent on ia64

Quoted 8 lines
Quoted 6 lines

include/linux/skbuff.h:521: warning: passing arg 1 of atomic_read' discards qualifiers from pointer target type include/net/sock.h:1244: warning: passing arg 1 of atomic_read' discards qualifiers from pointer target type include/net/tcp.h:958: warning: passing arg 1 of atomic_read' discards qualifiers from pointer target type mm/slub.c:3115: warning: passing arg 1 of atomic_read' from incompatible pointer type mm/slub.c:3250: warning: passing arg 1 of atomic_read' from incompatible pointer type mm/slub.c:3286: warning: passing arg 1 of atomic_read' from incompatible pointer type

Do you get any warnings other than those two?

That looks like six, not two. But that's the whole list.

Quoted 4 lines

IIRC, when you applied a version which used macros instead, there was no change.
It would seem that inlining changed the optimization behavior of the compiler. If you turn down the optimization level, do the macro and inline versions look the same, or at least more similar?

I re-tried the macros ... the three warnings from mm/slub.c all result in broken code ... and quite rightly too, they all come from code that does:

	atomic_read(&n->nr_slabs)

But the nr_slabs field is an atomic_long_t, so we shouldn't be using atomic_read(). I didn't spot these last time around because I was using slab, not slub for the previous build.

I think that I'll run into other build issues if I turn down the optimization level (there are lots of places where the kernel relies on optimizing away impossible cases in switch statements.

Quoted 2 lines

The binary does boot ... but I haven't run any tests to see whether there are any problems.

-Tony

#25 lore raw

RE: [PATCH 10/23] make atomic_read() and atomic_set() behavior consistent on ia64

Quoted 8 lines · Tony

I re-tried the macros ... the three warnings from mm/slub.c all result in broken code ... and quite rightly too, they all come from code that does:

	atomic_read(&n->nr_slabs)

But the nr_slabs field is an atomic_long_t, so we shouldn't be using atomic_read(). I didn't spot these last time around because I was using slab, not slub for the previous build.

Hmmmm... Strange that this did not cause failures before on any other platforms?

Fix atomic_read's in slub

Signed-off-by Christoph Lameter <[email protected]>
mm/slub.c +3 -3
index 69d02e3..0c106d7 100644
@@ -3112,7 +3112,7 @@ static int list_locations(struct kmem_cache *s, char *buf,
 		unsigned long flags;
 		struct page *page;
 
-		if (!atomic_read(&n->nr_slabs))
+		if (!atomic_long_read(&n->nr_slabs))
 			continue;
 
 		spin_lock_irqsave(&n->list_lock, flags);

@@ -3247,7 +3247,7 @@ static unsigned long slab_objects(struct kmem_cache *s,
 		}
 
 		if (flags & SO_FULL) {
-			int full_slabs = atomic_read(&n->nr_slabs)
+			int full_slabs = atomic_long_read(&n->nr_slabs)
 					- per_cpu[node]
 					- n->nr_partial;
 

@@ -3283,7 +3283,7 @@ static int any_slab_objects(struct kmem_cache *s)
 	for_each_node(node) {
 		struct kmem_cache_node *n = get_node(s, node);
 
-		if (n->nr_partial || atomic_read(&n->nr_slabs))
+		if (n->nr_partial || atomic_long_read(&n->nr_slabs))
 			return 1;
 	}
 	return 0;
author
#26 lore raw

Re: [PATCH 10/23] make atomic_read() and atomic_set() behavior consistent on ia64

Quoted 5 lines · Christoph Lameter
Quoted 3 lines · Tony

I re-tried the macros ... the three warnings from mm/slub.c all result in broken code ... and quite rightly too, they all come from code that does:

Quoted 7 lines
Quoted 5 lines
	atomic_read(&n->nr_slabs)

But the nr_slabs field is an atomic_long_t, so we shouldn't be using atomic_read(). I didn't spot these last time around because I was using slab, not slub for the previous build.

Hmmmm... Strange that this did not cause failures before on any other platforms?

Prior to the patch in question, atomic_read was a macro. I didn't use slub in my cursory testing. Tony had ia64 under a microscope because of the tricky memory access ordering semantics of that architecture.

-- Chris

author
#27 lore raw

PATCH 11/23 make atomic_read() and atomic_set() behavior consistent on m32r

In reply to Chris Snook

From: Chris Snook <[email protected]>

Use volatile consistently in atomic.h on m32r.

Signed-off-by Chris Snook <[email protected]>
include/asm-m32r/atomic.h +9 -3
@@ -22,7 +22,7 @@
  * on us. We need to use _exactly_ the address the user gave us,
  * not some alias that contains the same information.
  */
-typedef struct { volatile int counter; } atomic_t;
+typedef struct { int counter; } atomic_t;
 
 #define ATOMIC_INIT(i)	{ (i) }
 

@@ -32,7 +32,10 @@ typedef struct { volatile int counter; }
  *
  * Atomically reads the value of @v.
  */
-#define atomic_read(v)	((v)->counter)
+static __inline__ int atomic_read(atomic_t *v)
+{
+        return *(volatile int *)&v->counter;
+}
 
 /**
  * atomic_set - set atomic variable

@@ -41,7 +44,10 @@ typedef struct { volatile int counter; }
  *
  * Atomically sets the value of @v to @i.
  */
-#define atomic_set(v,i)	(((v)->counter) = (i))
+static __inline__ void atomic_set(atomic_t *v, int i)
+{
+        *(volatile int *)&v->counter = i;
+}
 
 /**
  * atomic_add_return - add integer to atomic variable and return it
#29 lore raw

Re: [PATCH 11/23] make atomic_read() and atomic_set() behavior consistent on m32r

Hi, Chris,

From: Hirokazu Takata <[email protected]>
Date: Wed, 22 Aug 2007 10:56:54 +0900

Quoted 11 lines

From: Chris Snook <[email protected]>
Date: Mon, 13 Aug 2007 07:24:52 -0400

Quoted 5 lines

From: Chris Snook <[email protected]>

Use volatile consistently in atomic.h on m32r.

Signed-off-by Chris Snook <[email protected]>

Thanks,

Acked-by Hirokazu Takata <[email protected]>

Hmmm.. It seems my reply was overhasty.

Applying the above patch, I have many warning messages like this:

<-- snip -->
...
CC kernel/sched.o
In file included from /project/m32r-linux/kernel/work/linux-2.6_dev.git/include/linux/netlink.h:139,

                 from /project/m32r-linux/kernel/work/linux-2.6_dev.git/include/linux/genetlink.h:4,
                 from /project/m32r-linux/kernel/work/linux-2.6_dev.git/include/net/genetlink.h:4,
                 from /project/m32r-linux/kernel/work/linux-2.6_dev.git/include/linux/taskstats_kern.h:12,
                 from /project/m32r-linux/kernel/work/linux-2.6_dev.git/include/linux/delayacct.h:21,
                 from /project/m32r-linux/kernel/work/linux-2.6_dev.git/kernel/sched.c:61:

/project/m32r-linux/kernel/work/linux-2.6_dev.git/include/linux/skbuff.h: In function 'skb_shared':
/project/m32r-linux/kernel/work/linux-2.6_dev.git/include/linux/skbuff.h:521: warning: passing argument 1 of 'atomic_read' discards qualifiers from pointer target type
...
<-- snip -->

In this case, it is because stb_shared() is defined with a parameter with "const" qualifier, in include/linux/skbuff.h.

	static inline int skb_shared(const struct sk_buff *skb)
	{
	        return atomic_read(&skb->users) != 1;
	}

I think the parameter of atomic_read() should have "const" qualifier to avoid these warnings, and IMHO this modification might be worth applying on other archs.

Here is an additional patch to revise the previous one for m32r. I also tried to rewrite it with inline asm code, but the kernel text size bacame roughly 2kB larger. So, I prefer C version.

Thanks,

-- Takata

[PATCH] m32r: Add "const" qualifier to the parameter of atomic_read()

Update atomic_read() to avoid the following warning of gcc-4.1.x:
warning: passing argument 1 of 'atomic_read' discards qualifiers from pointer target type

Signed-off-by Hirokazu Takata <[email protected]>
Cc Chris Snook <[email protected]>
---
 include/asm-m32r/atomic.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
include/asm-m32r/atomic.h +1 -1
index ba19689..9d46f86 100644
@@ -32,7 +32,7 @@ typedef struct { int counter; } atomic_t;
  *
  * Atomically reads the value of @v.
  */
-static __inline__ int atomic_read(atomic_t *v)
+static __inline__ int atomic_read(const atomic_t *v)
 {
         return *(volatile int *)&v->counter;
 }
Signature · 5 lines
1.5.2.4

--
Hirokazu Takata <[email protected]>
Linux/M32R Project:  http://www.linux-m32r.org/
author
#30 lore raw

Re: [PATCH 11/23] make atomic_read() and atomic_set() behavior consistent on m32r

Quoted 2 lines · Hirokazu Takata

I think the parameter of atomic_read() should have "const" qualifier to avoid these warnings, and IMHO this modification might be worth applying on other archs.

I agree.

Quoted 1 line

Here is an additional patch to revise the previous one for m32r.

I'll incorporate this change if we get enough consensus to justify a re-re-re-submit. Since the patch is intended to be a functional no-op on m32r, I'm inclined to leave it alone at the moment.

Quoted 2 lines

I also tried to rewrite it with inline asm code, but the kernel text size bacame roughly 2kB larger. So, I prefer C version.

You're not the only arch maintainer who prefers doing it in C. If you trust your compiler (a big "if", apparently), inline asm only improves code generation if you have a whole bunch of general purpose registers for the optimizer to play with.

-- Chris

#31 lore raw

Re: [PATCH 11/23] make atomic_read() and atomic_set() behavior consistent on m32r

Quoted 2 lines
Quoted 2 lines

I also tried to rewrite it with inline asm code, but the kernel text size bacame roughly 2kB larger. So, I prefer C version.

Could you send me the source code diff between the two versions you tested? 2kB difference is way too much, the asm version should be smaller if anything.

Quoted 1 line

You're not the only arch maintainer who prefers doing it in C. If you trust your compiler (a big "if", apparently), inline asm only improves code generation if you have a whole bunch of general purpose registers for the optimizer to play with.

No. The only real difference between the *(volatile *)& version and the volatile asm() version is that the volatile asm() version has defined semantics. There will be some code generation differences too, but they should be in the noise, unless GCC generates really bad code for either case. We know it sometimes does that for the *(volatile *)& thing; if the asm() version does something bad, I'd like to know about that too.

Segher

#32 lore raw

Re: [PATCH 11/23] make atomic_read() and atomic_set() behavior consistent on m32r

Quoted 6 lines · Segher Boessenkool
Quoted 2 lines
Quoted 2 lines

I also tried to rewrite it with inline asm code, but the kernel text size bacame roughly 2kB larger. So, I prefer C version.

Could you send me the source code diff between the two versions you tested? 2kB difference is way too much, the asm version should be smaller if anything.

Segher, can you please drop out of this discussion?

Your points are all wrong.

The inline asm version has the EXACT SAME PROBLEM as the "volatile" version has: it means that the compiler is unable to combine trivial instructions. There's no way that is acceptable.

So why the *hell* you'd expect the asm version to be smaller, I can't imagine. It's obvkously not going to be true.

So here's a clue to everybody in this thread:

THE CURRENT x86 BEHAVIOUR IS THE CORRECT ONE

where we don't have any "volatile" and no inline asm and no barriers, and we let the compiler do the right thing. If the code needs barriers, the code should damn well add them.

It's worked for the past 8 months, on the most common platform there is.

Stop this *idiotic* thread already.

Linus

#33 lore raw

Re: [PATCH 11/23] make atomic_read() and atomic_set() behavior consistent on m32r

Quoted 3 lines

The inline asm version has the EXACT SAME PROBLEM as the "volatile" version has: it means that the compiler is unable to combine trivial instructions.

This simply isn't true. The compiler *can* combine asm stuff:

typedef struct { int counter; } atomic_t;

static inline __attribute__((pure)) int atomic_read(const atomic_t *v)

{
        int x;
        asm("ld %0,@%1" : "=r"(x) : "r"(&v->counter), "m"(v->counter));
        return x;
}

int f(atomic_t *x)

{
        return atomic_read(x) + atomic_read(x);
}

int g(atomic_t *x)

{
        return 0 * atomic_read(x);
}

generates

f:

        ld r0,@r0
        slli r0,#1
        jmp lr

g:

        ldi r0,#0
        jmp lr
Quoted 2 lines

So why the *hell* you'd expect the asm version to be smaller, I can't imagine.

I expect it to be smaller than the current code, which uses the "big hammer" volatile version. We're talking about m32r here, not x86. Even when using "volatile asm" it shouldn't generate much bigger code.

Anyhow, I answered my own question: on m32r, you cannot use "m" with ld/st insns, since autoincrement modes don't work there (the assembler complains, at least). So you have to force the address into a reg instead, and _that_ causes the size increase.

Quoted 1 line

If the code needs barriers, the code should damn well add them.

Sure. I'm not suggesting otherwise.

Segher

#34 lore raw

Re: [PATCH 11/23] make atomic_read() and atomic_set() behavior consistent on m32r

Quoted 11 lines · Segher Boessenkool

This simply isn't true. The compiler *can* combine asm stuff:

typedef struct { int counter; } atomic_t;

static inline __attribute__((pure)) int atomic_read(const atomic_t *v)

{
        int x;
        asm("ld %0,@%1" : "=r"(x) : "r"(&v->counter), "m"(v->counter));
        return x;
}

That's not precisely combining asm stuff. The compiler is ditching a whole function because you've told it it can cache the result.

David

#35 lore raw

Re: [PATCH 11/23] make atomic_read() and atomic_set() behavior consistent on m32r

In reply to Segher Boessenkool
Quoted 2 lines · Segher Boessenkool

This simply isn't true. The compiler *can* combine asm stuff:

Please Segher, just shut up.

The combining, which I've mentioned *multiple*times* is

	if (atomic_read(&x) <= 1)

and dammit, if that doesn't result in a *single* instruction, the code generation is pure and utter crap.

It should result in

	cmpl $1,offset(reg)

and nothing else. And there is no way in hell you are doing that with "atomic_read()" being inline asm.

So can you now just go away?

Linus

#36 lore raw

RE: [PATCH 11/23] make atomic_read() and atomic_set() behavior consistent on m32r

Quoted 10 lines · Segher Boessenkool

The combining, which I've mentioned *multiple*times* is

	if (atomic_read(&x) <= 1)

and dammit, if that doesn't result in a *single* instruction, the code generation is pure and utter crap.

Quoted 6 lines

It should result in

	cmpl $1,offset(reg)

and nothing else. And there is no way in hell you are doing that with "atomic_read()" being inline asm.

Sorry, Linus, I don't agree. The whole point of 'volatile' is to say to the compiler, "DO NOT OPTIMIZE THIS. What you think is harmless may break things you do not and should not understand." The combination of the read and the compare into a single operation is a compiler optimization.

While it would not be unreasonable to still allow this optimization (since I cannot think of any situations in which it could be harmful), it is just as reasonable not to.

DS

#37 lore raw

Re: [PATCH 11/23] make atomic_read() and atomic_set() behavior consistent on m32r

In reply to Segher Boessenkool
Quoted 8 lines · Segher Boessenkool

int f(atomic_t *x)

{
        return atomic_read(x) + atomic_read(x);
}
         ld r0,@r0
         slli r0,#1
         jmp lr

Looks like peephole optimization at work.

author
#38 lore raw

PATCH 12/23 make atomic_read() and atomic_set() behavior consistent on m68knommu

In reply to Chris Snook

From: Chris Snook <[email protected]>

Use volatile consistently in atomic.h on m68knommu.

Signed-off-by Chris Snook <[email protected]>
include/asm-m68knommu/atomic.h +9 -2
@@ -15,8 +15,15 @@
 typedef struct { int counter; } atomic_t;
 #define ATOMIC_INIT(i)	{ (i) }
 
-#define atomic_read(v)		((v)->counter)
-#define atomic_set(v, i)	(((v)->counter) = i)
+static inline int atomic_read(atomic_t *v)
+{
+        return *(volatile int *)&v->counter;
+}
+
+static inline void atomic_set(atomic_t *v, int i)
+{
+        *(volatile int *)&v->counter = i;
+}
 
 static __inline__ void atomic_add(int i, atomic_t *v)
 {
author
#39 lore raw

PATCH 13/23 make atomic_read() and atomic_set() behavior consistent on m68k

In reply to Chris Snook

From: Chris Snook <[email protected]>

Use volatile consistently in atomic.h on m68k.

Signed-off-by Chris Snook <[email protected]>
include/asm-m68k/atomic.h +9 -2
@@ -16,8 +16,15 @@
 typedef struct { int counter; } atomic_t;
 #define ATOMIC_INIT(i)	{ (i) }
 
-#define atomic_read(v)		((v)->counter)
-#define atomic_set(v, i)	(((v)->counter) = i)
+static inline int atomic_read(atomic_t *v)
+{
+        return *(volatile int *)&v->counter;
+}
+
+static inline void atomic_set(atomic_t *v, int i)
+{
+        *(volatile int *)&v->counter = i;
+}
 
 static inline void atomic_add(int i, atomic_t *v)
 {
author
#40 lore raw

PATCH 14/23 make atomic_read() and atomic_set() behavior consistent on mips

In reply to Chris Snook

From: Chris Snook <[email protected]>

Use volatile consistently in atomic.h on mips.

Signed-off-by Chris Snook <[email protected]>
include/asm-mips/atomic.h +18 -6
@@ -20,7 +20,7 @@
 #include <asm/war.h>
 #include <asm/system.h>
 
-typedef struct { volatile int counter; } atomic_t;
+typedef struct { int counter; } atomic_t;
 
 #define ATOMIC_INIT(i)    { (i) }
 

@@ -30,7 +30,10 @@ typedef struct { volatile int counter; }
  *
  * Atomically reads the value of @v.
  */
-#define atomic_read(v)		((v)->counter)
+static __inline__ int atomic_read(atomic_t *v)
+{
+        return *(volatile int *)&v->counter;
+}
 
 /*
  * atomic_set - set atomic variable

@@ -39,7 +42,10 @@ typedef struct { volatile int counter; }
  *
  * Atomically sets the value of @v to @i.
  */
-#define atomic_set(v,i)		((v)->counter = (i))
+static __inline__ void atomic_set(atomic_t *v, int i)
+{
+        *(volatile int *)&v->counter = i;
+}
 
 /*
  * atomic_add - add integer to atomic variable

@@ -404,7 +410,7 @@ static __inline__ int atomic_add_unless(
 
 #ifdef CONFIG_64BIT
 
-typedef struct { volatile long counter; } atomic64_t;
+typedef struct { long counter; } atomic64_t;
 
 #define ATOMIC64_INIT(i)    { (i) }
 

@@ -413,14 +419,20 @@ typedef struct { volatile long counter; 
  * @v: pointer of type atomic64_t
  *
  */
-#define atomic64_read(v)	((v)->counter)
+static __inline__ long atomic64_read(atomic64_t *v)
+{
+        return *(volatile long *)&v->counter;
+}
 
 /*
  * atomic64_set - set atomic variable
  * @v: pointer of type atomic64_t
  * @i: required value
  */
-#define atomic64_set(v,i)	((v)->counter = (i))
+static __inline__ void atomic64_set(atomic64_t *v, long i)
+{
+        *(volatile long *)&v->counter = i;
+}
 
 /*
  * atomic64_add - add integer to atomic variable
author
#41 lore raw

PATCH 15/23 make atomic_read() and atomic_set() behavior consistent on parisc

In reply to Chris Snook

From: Chris Snook <[email protected]>

Use volatile consistently in atomic.h on parisc.

Signed-off-by Chris Snook <[email protected]>
include/asm-parisc/atomic.h +4 -4
@@ -128,7 +128,7 @@ __cmpxchg(volatile void *ptr, unsigned l
  * Cache-line alignment would conflict with, for example, linux/module.h
  */
 
-typedef struct { volatile int counter; } atomic_t;
+typedef struct { int counter; } atomic_t;
 
 /* It's possible to reduce all atomic operations to either
  * __atomic_add_return, atomic_set and atomic_read (the latter

@@ -159,7 +159,7 @@ static __inline__ void atomic_set(atomic
 
 static __inline__ int atomic_read(const atomic_t *v)
 {
-	return v->counter;
+	return *(volatile int *)&v->counter;
 }
 
 /* exported interface */

@@ -227,7 +227,7 @@ static __inline__ int atomic_add_unless(
 
 #ifdef CONFIG_64BIT
 
-typedef struct { volatile s64 counter; } atomic64_t;
+typedef struct { s64 counter; } atomic64_t;
 
 #define ATOMIC64_INIT(i) ((atomic64_t) { (i) })
 

@@ -258,7 +258,7 @@ atomic64_set(atomic64_t *v, s64 i)
 static __inline__ s64
 atomic64_read(const atomic64_t *v)
 {
-	return v->counter;
+	return *(volatile s64 *)&v->counter;
 }
 
 #define atomic64_add(i,v)	((void)(__atomic64_add_return( ((s64)i),(v))))
author
#42 lore raw

PATCH 16/23 make atomic_read() and atomic_set() behavior consistent on s390

In reply to Chris Snook

From: Chris Snook <[email protected]>

Use volatile consistently in atomic.h on s390.

Signed-off-by Chris Snook <[email protected]>
include/asm-s390/atomic.h +18 -4
@@ -67,8 +67,15 @@ typedef struct {
 
 #endif /* __GNUC__ */
 
-#define atomic_read(v)          ((v)->counter)
-#define atomic_set(v,i)         (((v)->counter) = (i))
+static __inline__ int atomic_read(atomic_t *v)
+{
+        return *(volatile int *)&v->counter;
+}
+
+static __inline__ void atomic_set(atomic_t *v, int i)
+{
+        *(volatile int *)&v->counter = i;
+}
 
 static __inline__ int atomic_add_return(int i, atomic_t * v)
 {

@@ -182,8 +189,15 @@ typedef struct {
 
 #endif /* __GNUC__ */
 
-#define atomic64_read(v)          ((v)->counter)
-#define atomic64_set(v,i)         (((v)->counter) = (i))
+static __inline__ long long atomic64_read(atomic64_t *v)
+{
+        return *(volatile long long *)&v->counter;
+}
+
+static __inline__ void atomic64_set(atomic64_t *v, long long i)
+{
+        *(volatile long long *)&v->counter = i;
+}
 
 static __inline__ long long atomic64_add_return(long long i, atomic64_t * v)
 {
author
#43 lore raw

PATCH 17/23 make atomic_read() and atomic_set() behavior consistent on sh64

In reply to Chris Snook

From: Chris Snook <[email protected]>

Use volatile consistently in atomic.h on sh64.

Signed-off-by Chris Snook <[email protected]>
include/asm-sh64/atomic.h +10 -3
@@ -19,12 +19,19 @@
  *
  */
 
-typedef struct { volatile int counter; } atomic_t;
+typedef struct { int counter; } atomic_t;
 
 #define ATOMIC_INIT(i)	( (atomic_t) { (i) } )
 
-#define atomic_read(v)		((v)->counter)
-#define atomic_set(v,i)		((v)->counter = (i))
+static inline int atomic_read(atomic_t *v)
+{
+        return *(volatile int *)&v->counter;
+}
+
+static inline void atomic_set(atomic_t *v, int i)
+{
+        *(volatile int *)&v->counter = i;
+}
 
 #include <asm/system.h>
author
#44 lore raw

PATCH 18/23 make atomic_read() and atomic_set() behavior consistent on sh

In reply to Chris Snook

From: Chris Snook <[email protected]>

Use volatile consistently in atomic.h on sh.

Signed-off-by Chris Snook <[email protected]>
include/asm-sh/atomic.h +10 -3
@@ -7,12 +7,19 @@
  *
  */
 
-typedef struct { volatile int counter; } atomic_t;
+typedef struct { int counter; } atomic_t;
 
 #define ATOMIC_INIT(i)	( (atomic_t) { (i) } )
 
-#define atomic_read(v)		((v)->counter)
-#define atomic_set(v,i)		((v)->counter = (i))
+static inline int atomic_read(atomic_t *v)
+{
+        return *(volatile int *)&v->counter;
+}
+
+static inline void atomic_set(atomic_t *v, int i)
+{
+        *(volatile int *)&v->counter = i;
+}
 
 #include <linux/compiler.h>
 #include <asm/system.h>
author
#45 lore raw

PATCH 19/23 make atomic_read() and atomic_set() behavior consistent on sparc64

In reply to Chris Snook

From: Chris Snook <[email protected]>

Use volatile consistently in atomic.h on sparc64.

Signed-off-by Chris Snook <[email protected]>
include/asm-sparc64/atomic.h +20 -6
@@ -11,17 +11,31 @@
 #include <linux/types.h>
 #include <asm/system.h>
 
-typedef struct { volatile int counter; } atomic_t;
-typedef struct { volatile __s64 counter; } atomic64_t;
+typedef struct { int counter; } atomic_t;
+typedef struct { __s64 counter; } atomic64_t;
 
 #define ATOMIC_INIT(i)		{ (i) }
 #define ATOMIC64_INIT(i)	{ (i) }
 
-#define atomic_read(v)		((v)->counter)
-#define atomic64_read(v)	((v)->counter)
+static __inline__ int atomic_read(atomic_t *v)
+{
+        return *(volatile int *)&v->counter;
+}
+
+static __inline__ __s64 atomic64_read(atomic64_t *v)
+{
+        return *(volatile __s64 *)&v->counter;
+}
 
-#define atomic_set(v, i)	(((v)->counter) = i)
-#define atomic64_set(v, i)	(((v)->counter) = i)
+static __inline__ void atomic_set(atomic_t *v, int i)
+{
+        *(volatile int *)&v->counter = i;
+}
+
+static __inline__ void atomic64_set(atomic64_t *v, __s64 i)
+{
+        *(volatile __s64 *)&v->counter = i;
+}
 
 extern void atomic_add(int, atomic_t *);
 extern void atomic64_add(int, atomic64_t *);
author
#46 lore raw

PATCH 20/23 make atomic_read() and atomic_set() behavior consistent on sparc

In reply to Chris Snook

From: Chris Snook <[email protected]>

Use volatile consistently in atomic.h on alpha.
Leave sparc-internal atomic24_t type alone.

Signed-off-by Chris Snook <[email protected]>
include/asm-sparc/atomic.h +5 -2
@@ -13,7 +13,7 @@
 
 #include <linux/types.h>
 
-typedef struct { volatile int counter; } atomic_t;
+typedef struct { int counter; } atomic_t;
 
 #ifdef __KERNEL__
 

@@ -61,7 +61,10 @@ extern int atomic_cmpxchg(atomic_t *, in
 extern int atomic_add_unless(atomic_t *, int, int);
 extern void atomic_set(atomic_t *, int);
 
-#define atomic_read(v)          ((v)->counter)
+static inline int atomic_read(atomic_t *v)
+{
+        return *(volatile int *)&v->counter;
+}
 
 #define atomic_add(i, v)	((void)__atomic_add_return( (int)(i), (v)))
 #define atomic_sub(i, v)	((void)__atomic_add_return(-(int)(i), (v)))
author
#47 lore raw

PATCH 21/23 make atomic_read() and atomic_set() behavior consistent on v850

In reply to Chris Snook

From: Chris Snook <[email protected]>

Use volatile consistently in atomic.h on v850.

Signed-off-by Chris Snook <[email protected]>
include/asm-v850/atomic.h +9 -2
@@ -27,8 +27,15 @@ typedef struct { int counter; } atomic_t
 
 #ifdef __KERNEL__
 
-#define atomic_read(v)		((v)->counter)
-#define atomic_set(v,i)		(((v)->counter) = (i))
+static inline int atomic_read(atomic_t *v)
+{
+        return *(volatile int *)&v->counter;
+}
+
+static inline void atomic_set(atomic_t *v, int i)
+{
+        *(volatile int *)&v->counter = i;
+}
 
 static inline int atomic_add_return (int i, volatile atomic_t *v)
 {
author
#48 lore raw

PATCH 22/23 make atomic_read() and atomic_set() behavior consistent on x86_64

In reply to Chris Snook

From: Chris Snook <[email protected]>

Use volatile consistently in atomic.h on x86_64.

Signed-off-by Chris Snook <[email protected]>
include/asm-x86_64/atomic.h +17 -5
@@ -32,7 +32,10 @@ typedef struct { int counter; } atomic_t
  * 
  * Atomically reads the value of @v.
  */ 
-#define atomic_read(v)		((v)->counter)
+static __inline__ int atomic_read(atomic_t *v)
+{
+        return *(volatile int *)&v->counter;
+}
 
 /**
  * atomic_set - set atomic variable

@@ -41,7 +44,10 @@ typedef struct { int counter; } atomic_t
  * 
  * Atomically sets the value of @v to @i.
  */ 
-#define atomic_set(v,i)		(((v)->counter) = (i))
+static __inline__ void atomic_set(atomic_t *v, int i)
+{
+        *(volatile int *)&v->counter = i;
+}
 
 /**
  * atomic_add - add integer to atomic variable

@@ -206,7 +212,7 @@ static __inline__ int atomic_sub_return(
 
 /* An 64bit atomic type */
 
-typedef struct { volatile long counter; } atomic64_t;
+typedef struct { long counter; } atomic64_t;
 
 #define ATOMIC64_INIT(i)	{ (i) }
 

@@ -217,7 +223,10 @@ typedef struct { volatile long counter; 
  * Atomically reads the value of @v.
  * Doesn't imply a read memory barrier.
  */
-#define atomic64_read(v)		((v)->counter)
+static __inline__ long atomic64_read(atomic64_t *v)
+{
+        return *(volatile long *)&v->counter;
+}
 
 /**
  * atomic64_set - set atomic64 variable

@@ -226,7 +235,10 @@ typedef struct { volatile long counter; 
  *
  * Atomically sets the value of @v to @i.
  */
-#define atomic64_set(v,i)		(((v)->counter) = (i))
+static __inline__ void atomic64_set(atomic64_t *v, long i)
+{
+        *(volatile long *)&v->counter = i;
+}
 
 /**
  * atomic64_add - add integer to atomic64 variable
author
#49 lore raw

PATCH 23/23 make atomic_read() and atomic_set() behavior consistent on xtensa

In reply to Chris Snook

From: Chris Snook <[email protected]>

Use volatile consistently in atomic.h on xtensa.

Signed-off-by Chris Snook <[email protected]>
include/asm-xtensa/atomic.h +9 -3
@@ -15,7 +15,7 @@
 
 #include <linux/stringify.h>
 
-typedef struct { volatile int counter; } atomic_t;
+typedef struct { int counter; } atomic_t;
 
 #ifdef __KERNEL__
 #include <asm/processor.h>

@@ -47,7 +47,10 @@ typedef struct { volatile int counter; }
  *
  * Atomically reads the value of @v.
  */
-#define atomic_read(v)		((v)->counter)
+static inline int atomic_read(atomic_t *v)
+{
+        return *(volatile int *)&v->counter;
+}
 
 /**
  * atomic_set - set atomic variable

@@ -56,7 +59,10 @@ typedef struct { volatile int counter; }
  *
  * Atomically sets the value of @v to @i.
  */
-#define atomic_set(v,i)		((v)->counter = (i))
+static inline void atomic_set(atomic_t *v, int i)
+{
+        *(volatile int *)&v->counter = i;
+}
 
 /**
  * atomic_add - add integer to atomic variable