* [PATCH 1/2]: atomic_t: Cast to volatile when accessing atomic variables
@ 2010-05-17 4:33 Anton Blanchard
2010-05-17 4:34 ` [PATCH 2/2]: atomic_t: Remove volatile from atomic_t definition Anton Blanchard
0 siblings, 1 reply; 16+ messages in thread
From: Anton Blanchard @ 2010-05-17 4:33 UTC (permalink / raw)
To: torvalds, akpm, willy, benh, paulus; +Cc: linux-arch, linux-kernel
In preparation for removing volatile from the atomic_t definition, this
patch adds a volatile cast to all the atomic read functions.
Signed-off-by: Anton Blanchard <anton@samba.org>
---
I did my best to fix every architecture, but I'd appreciate it if arch
maintainers could double check this catches everything.
Index: linux-cpumask/arch/alpha/include/asm/atomic.h
===================================================================
--- linux-cpumask.orig/arch/alpha/include/asm/atomic.h 2010-04-08 19:46:03.177952918 +1000
+++ linux-cpumask/arch/alpha/include/asm/atomic.h 2010-05-17 13:21:55.353455034 +1000
@@ -17,8 +17,8 @@
#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)
+#define atomic_read(v) (*(volatile int *)&(v)->counter)
+#define atomic64_read(v) (*(volatile long *)&(v)->counter)
#define atomic_set(v,i) ((v)->counter = (i))
#define atomic64_set(v,i) ((v)->counter = (i))
Index: linux-cpumask/arch/arm/include/asm/atomic.h
===================================================================
--- linux-cpumask.orig/arch/arm/include/asm/atomic.h 2010-04-08 19:46:03.197946415 +1000
+++ linux-cpumask/arch/arm/include/asm/atomic.h 2010-05-17 13:21:55.364704766 +1000
@@ -24,7 +24,7 @@
* strex/ldrex monitor on some implementations. The reason we can use it for
* atomic_set() is the clrex or dummy strex done on every exception return.
*/
-#define atomic_read(v) ((v)->counter)
+#define atomic_read(v) (*(volatile int *)&(v)->counter)
#define atomic_set(v,i) (((v)->counter) = (i))
#if __LINUX_ARM_ARCH__ >= 6
Index: linux-cpumask/arch/avr32/include/asm/atomic.h
===================================================================
--- linux-cpumask.orig/arch/avr32/include/asm/atomic.h 2010-04-08 19:46:03.167946279 +1000
+++ linux-cpumask/arch/avr32/include/asm/atomic.h 2010-05-17 13:21:55.384708819 +1000
@@ -19,7 +19,7 @@
#define ATOMIC_INIT(i) { (i) }
-#define atomic_read(v) ((v)->counter)
+#define atomic_read(v) (*(volatile int *)&(v)->counter)
#define atomic_set(v, i) (((v)->counter) = i)
/*
Index: linux-cpumask/arch/cris/include/asm/atomic.h
===================================================================
--- linux-cpumask.orig/arch/cris/include/asm/atomic.h 2010-04-08 19:46:03.147945641 +1000
+++ linux-cpumask/arch/cris/include/asm/atomic.h 2010-05-17 13:21:55.404706041 +1000
@@ -15,7 +15,7 @@
#define ATOMIC_INIT(i) { (i) }
-#define atomic_read(v) ((v)->counter)
+#define atomic_read(v) (*(volatile int *)&(v)->counter)
#define atomic_set(v,i) (((v)->counter) = (i))
/* These should be written in asm but we do it in C for now. */
Index: linux-cpumask/arch/frv/include/asm/atomic.h
===================================================================
--- linux-cpumask.orig/arch/frv/include/asm/atomic.h 2010-04-08 19:46:03.397951974 +1000
+++ linux-cpumask/arch/frv/include/asm/atomic.h 2010-05-17 13:21:55.424704874 +1000
@@ -36,7 +36,7 @@
#define smp_mb__after_atomic_inc() barrier()
#define ATOMIC_INIT(i) { (i) }
-#define atomic_read(v) ((v)->counter)
+#define atomic_read(v) (*(volatile int *)&(v)->counter)
#define atomic_set(v, i) (((v)->counter) = (i))
#ifndef CONFIG_FRV_OUTOFLINE_ATOMIC_OPS
Index: linux-cpumask/arch/h8300/include/asm/atomic.h
===================================================================
--- linux-cpumask.orig/arch/h8300/include/asm/atomic.h 2010-04-08 19:46:03.237946283 +1000
+++ linux-cpumask/arch/h8300/include/asm/atomic.h 2010-05-17 13:21:55.445954164 +1000
@@ -10,7 +10,7 @@
#define ATOMIC_INIT(i) { (i) }
-#define atomic_read(v) ((v)->counter)
+#define atomic_read(v) (*(volatile int *)&(v)->counter)
#define atomic_set(v, i) (((v)->counter) = i)
#include <asm/system.h>
Index: linux-cpumask/arch/ia64/include/asm/atomic.h
===================================================================
--- linux-cpumask.orig/arch/ia64/include/asm/atomic.h 2010-04-08 19:46:03.207982670 +1000
+++ linux-cpumask/arch/ia64/include/asm/atomic.h 2010-05-17 13:21:55.453760038 +1000
@@ -21,8 +21,8 @@
#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)
+#define atomic_read(v) (*(volatile int *)&(v)->counter)
+#define atomic64_read(v) (*(volatile long *)&(v)->counter)
#define atomic_set(v,i) (((v)->counter) = (i))
#define atomic64_set(v,i) (((v)->counter) = (i))
Index: linux-cpumask/arch/m32r/include/asm/atomic.h
===================================================================
--- linux-cpumask.orig/arch/m32r/include/asm/atomic.h 2010-04-08 19:46:03.377953123 +1000
+++ linux-cpumask/arch/m32r/include/asm/atomic.h 2010-05-17 13:21:55.464742675 +1000
@@ -26,7 +26,7 @@
*
* Atomically reads the value of @v.
*/
-#define atomic_read(v) ((v)->counter)
+#define atomic_read(v) (*(volatile int *)&(v)->counter)
/**
* atomic_set - set atomic variable
Index: linux-cpumask/arch/m68k/include/asm/atomic_mm.h
===================================================================
--- linux-cpumask.orig/arch/m68k/include/asm/atomic_mm.h 2010-05-05 23:02:20.013473674 +1000
+++ linux-cpumask/arch/m68k/include/asm/atomic_mm.h 2010-05-17 13:21:55.485966391 +1000
@@ -15,7 +15,7 @@
#define ATOMIC_INIT(i) { (i) }
-#define atomic_read(v) ((v)->counter)
+#define atomic_read(v) (*(volatile int *)&(v)->counter)
#define atomic_set(v, i) (((v)->counter) = i)
static inline void atomic_add(int i, atomic_t *v)
Index: linux-cpumask/arch/m68k/include/asm/atomic_no.h
===================================================================
--- linux-cpumask.orig/arch/m68k/include/asm/atomic_no.h 2010-04-08 19:46:03.277950527 +1000
+++ linux-cpumask/arch/m68k/include/asm/atomic_no.h 2010-05-17 13:21:55.485966391 +1000
@@ -15,7 +15,7 @@
#define ATOMIC_INIT(i) { (i) }
-#define atomic_read(v) ((v)->counter)
+#define atomic_read(v) (*(volatile int *)&(v)->counter)
#define atomic_set(v, i) (((v)->counter) = i)
static __inline__ void atomic_add(int i, atomic_t *v)
Index: linux-cpumask/arch/mips/include/asm/atomic.h
===================================================================
--- linux-cpumask.orig/arch/mips/include/asm/atomic.h 2010-04-08 19:46:03.337946398 +1000
+++ linux-cpumask/arch/mips/include/asm/atomic.h 2010-05-17 13:21:55.523485056 +1000
@@ -29,7 +29,7 @@
*
* Atomically reads the value of @v.
*/
-#define atomic_read(v) ((v)->counter)
+#define atomic_read(v) (*(volatile int *)&(v)->counter)
/*
* atomic_set - set atomic variable
@@ -410,7 +410,7 @@ static __inline__ int atomic_add_unless(
* @v: pointer of type atomic64_t
*
*/
-#define atomic64_read(v) ((v)->counter)
+#define atomic64_read(v) (*(volatile long *)&(v)->counter)
/*
* atomic64_set - set atomic variable
Index: linux-cpumask/arch/mn10300/include/asm/atomic.h
===================================================================
--- linux-cpumask.orig/arch/mn10300/include/asm/atomic.h 2010-04-08 19:46:03.327946770 +1000
+++ linux-cpumask/arch/mn10300/include/asm/atomic.h 2010-05-17 13:21:55.613465776 +1000
@@ -31,7 +31,7 @@
* Atomically reads the value of @v. Note that the guaranteed
* useful range of an atomic_t is only 24 bits.
*/
-#define atomic_read(v) ((v)->counter)
+#define atomic_read(v) (*(volatile int *)&(v)->counter)
/**
* atomic_set - set atomic variable
Index: linux-cpumask/arch/parisc/include/asm/atomic.h
===================================================================
--- linux-cpumask.orig/arch/parisc/include/asm/atomic.h 2010-04-08 19:46:03.347946599 +1000
+++ linux-cpumask/arch/parisc/include/asm/atomic.h 2010-05-17 13:21:55.643453602 +1000
@@ -189,7 +189,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 */
@@ -286,7 +286,7 @@ atomic64_set(atomic64_t *v, s64 i)
static __inline__ s64
atomic64_read(const atomic64_t *v)
{
- return v->counter;
+ return (*(volatile long *)&(v)->counter);
}
#define atomic64_add(i,v) ((void)(__atomic64_add_return( ((s64)(i)),(v))))
Index: linux-cpumask/arch/x86/include/asm/atomic64_64.h
===================================================================
--- linux-cpumask.orig/arch/x86/include/asm/atomic64_64.h 2010-04-08 19:46:03.297951878 +1000
+++ linux-cpumask/arch/x86/include/asm/atomic64_64.h 2010-05-17 13:21:55.673453935 +1000
@@ -18,7 +18,7 @@
*/
static inline long atomic64_read(const atomic64_t *v)
{
- return v->counter;
+ return (*(volatile long *)&(v)->counter);
}
/**
Index: linux-cpumask/arch/sh/include/asm/atomic.h
===================================================================
--- linux-cpumask.orig/arch/sh/include/asm/atomic.h 2010-04-08 19:46:03.227947464 +1000
+++ linux-cpumask/arch/sh/include/asm/atomic.h 2010-05-17 13:21:55.703454497 +1000
@@ -13,7 +13,7 @@
#define ATOMIC_INIT(i) ( (atomic_t) { (i) } )
-#define atomic_read(v) ((v)->counter)
+#define atomic_read(v) (*(volatile int *)&(v)->counter)
#define atomic_set(v,i) ((v)->counter = (i))
#if defined(CONFIG_GUSA_RB)
Index: linux-cpumask/arch/sparc/include/asm/atomic_32.h
===================================================================
--- linux-cpumask.orig/arch/sparc/include/asm/atomic_32.h 2010-04-08 19:46:03.247946656 +1000
+++ linux-cpumask/arch/sparc/include/asm/atomic_32.h 2010-05-17 13:21:55.723453281 +1000
@@ -25,7 +25,7 @@ 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)
+#define atomic_read(v) (*(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)))
Index: linux-cpumask/arch/sparc/include/asm/atomic_64.h
===================================================================
--- linux-cpumask.orig/arch/sparc/include/asm/atomic_64.h 2010-04-08 19:46:03.257945857 +1000
+++ linux-cpumask/arch/sparc/include/asm/atomic_64.h 2010-05-17 13:21:55.743456221 +1000
@@ -13,8 +13,8 @@
#define ATOMIC_INIT(i) { (i) }
#define ATOMIC64_INIT(i) { (i) }
-#define atomic_read(v) ((v)->counter)
-#define atomic64_read(v) ((v)->counter)
+#define atomic_read(v) (*(volatile int *)&(v)->counter)
+#define atomic64_read(v) (*(volatile long *)&(v)->counter)
#define atomic_set(v, i) (((v)->counter) = i)
#define atomic64_set(v, i) (((v)->counter) = i)
Index: linux-cpumask/arch/x86/include/asm/atomic.h
===================================================================
--- linux-cpumask.orig/arch/x86/include/asm/atomic.h 2010-04-08 19:46:03.307951966 +1000
+++ linux-cpumask/arch/x86/include/asm/atomic.h 2010-05-17 13:21:55.743456221 +1000
@@ -22,7 +22,7 @@
*/
static inline int atomic_read(const atomic_t *v)
{
- return v->counter;
+ return (*(volatile int *)&(v)->counter);
}
/**
Index: linux-cpumask/arch/xtensa/include/asm/atomic.h
===================================================================
--- linux-cpumask.orig/arch/xtensa/include/asm/atomic.h 2010-04-08 19:46:03.367946559 +1000
+++ linux-cpumask/arch/xtensa/include/asm/atomic.h 2010-05-17 13:21:55.753456882 +1000
@@ -46,7 +46,7 @@
*
* Atomically reads the value of @v.
*/
-#define atomic_read(v) ((v)->counter)
+#define atomic_read(v) (*(volatile int *)&(v)->counter)
/**
* atomic_set - set atomic variable
Index: linux-cpumask/include/asm-generic/atomic.h
===================================================================
--- linux-cpumask.orig/include/asm-generic/atomic.h 2010-04-08 19:46:03.137946549 +1000
+++ linux-cpumask/include/asm-generic/atomic.h 2010-05-17 13:21:55.773453759 +1000
@@ -33,7 +33,7 @@
* Atomically reads the value of @v. Note that the guaranteed
* useful range of an atomic_t is only 24 bits.
*/
-#define atomic_read(v) ((v)->counter)
+#define atomic_read(v) (*(volatile int *)&(v)->counter)
/**
* atomic_set - set atomic variable
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 2/2]: atomic_t: Remove volatile from atomic_t definition
2010-05-17 4:33 [PATCH 1/2]: atomic_t: Cast to volatile when accessing atomic variables Anton Blanchard
@ 2010-05-17 4:34 ` Anton Blanchard
2010-05-17 8:58 ` Heiko Carstens
2010-05-17 15:01 ` Linus Torvalds
0 siblings, 2 replies; 16+ messages in thread
From: Anton Blanchard @ 2010-05-17 4:34 UTC (permalink / raw)
To: torvalds, akpm, willy, benh, paulus; +Cc: linux-arch, linux-kernel
When looking at a performance problem on PowerPC, I noticed some awful code
generation:
c00000000051fc98: 3b 60 00 01 li r27,1
...
c00000000051fca0: 3b 80 00 00 li r28,0
...
c00000000051fcdc: 93 61 00 70 stw r27,112(r1)
c00000000051fce0: 93 81 00 74 stw r28,116(r1)
c00000000051fce4: 81 21 00 70 lwz r9,112(r1)
c00000000051fce8: 80 01 00 74 lwz r0,116(r1)
c00000000051fcec: 7d 29 07 b4 extsw r9,r9
c00000000051fcf0: 7c 00 07 b4 extsw r0,r0
c00000000051fcf4: 7c 20 04 ac lwsync
c00000000051fcf8: 7d 60 f8 28 lwarx r11,0,r31
c00000000051fcfc: 7c 0b 48 00 cmpw r11,r9
c00000000051fd00: 40 c2 00 10 bne- c00000000051fd10
c00000000051fd04: 7c 00 f9 2d stwcx. r0,0,r31
c00000000051fd08: 40 c2 ff f0 bne+ c00000000051fcf8
c00000000051fd0c: 4c 00 01 2c isync
We create two constants, write them out to the stack, read them straight back
in and sign extend them. What a mess.
It turns out this bad code is a result of us defining atomic_t as a
volatile int.
We removed the volatile attribute from the powerpc atomic_t definition years
ago, but commit ea435467500612636f8f4fb639ff6e76b2496e4b (atomic_t: unify all
arch definitions) added it back in.
To dig up an old quote from Linus:
> The fact is, volatile on data structures is a bug. It's a wart in the C
> language. It shouldn't be used.
>
> Volatile accesses in *code* can be ok, and if we have "atomic_read()"
> expand to a "*(volatile int *)&(x)->value", then I'd be ok with that.
>
> But marking data structures volatile just makes the compiler screw up
> totally, and makes code for initialization sequences etc much worse.
And screw up it does :)
With the volatile removed, we see much more reasonable code generation:
c00000000051f5b8: 3b 60 00 01 li r27,1
...
c00000000051f5c0: 3b 80 00 00 li r28,0
...
c00000000051fc7c: 7c 20 04 ac lwsync
c00000000051fc80: 7c 00 f8 28 lwarx r0,0,r31
c00000000051fc84: 7c 00 d8 00 cmpw r0,r27
c00000000051fc88: 40 c2 00 10 bne- c00000000051fc98
c00000000051fc8c: 7f 80 f9 2d stwcx. r28,0,r31
c00000000051fc90: 40 c2 ff f0 bne+ c00000000051fc80
c00000000051fc94: 4c 00 01 2c isync
Six instructions less.
Signed-off-by: Anton Blanchard <anton@samba.org>
---
Index: linux-cpumask/include/linux/types.h
===================================================================
--- linux-cpumask.orig/include/linux/types.h 2010-04-08 19:46:02.916696580 +1000
+++ linux-cpumask/include/linux/types.h 2010-05-17 13:21:59.135964013 +1000
@@ -188,12 +188,12 @@ typedef u32 phys_addr_t;
typedef phys_addr_t resource_size_t;
typedef struct {
- volatile int counter;
+ int counter;
} atomic_t;
#ifdef CONFIG_64BIT
typedef struct {
- volatile long counter;
+ long counter;
} atomic64_t;
#endif
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2]: atomic_t: Remove volatile from atomic_t definition
2010-05-17 4:34 ` [PATCH 2/2]: atomic_t: Remove volatile from atomic_t definition Anton Blanchard
@ 2010-05-17 8:58 ` Heiko Carstens
2010-05-17 15:01 ` Linus Torvalds
1 sibling, 0 replies; 16+ messages in thread
From: Heiko Carstens @ 2010-05-17 8:58 UTC (permalink / raw)
To: Anton Blanchard
Cc: torvalds, akpm, willy, benh, paulus, linux-arch, linux-kernel,
Martin Schwidefsky
On Mon, May 17, 2010 at 02:34:57PM +1000, Anton Blanchard wrote:
>
> When looking at a performance problem on PowerPC, I noticed some awful code
> generation:
>
> c00000000051fc98: 3b 60 00 01 li r27,1
> ...
> c00000000051fca0: 3b 80 00 00 li r28,0
> ...
> c00000000051fcdc: 93 61 00 70 stw r27,112(r1)
> c00000000051fce0: 93 81 00 74 stw r28,116(r1)
> c00000000051fce4: 81 21 00 70 lwz r9,112(r1)
> c00000000051fce8: 80 01 00 74 lwz r0,116(r1)
> c00000000051fcec: 7d 29 07 b4 extsw r9,r9
> c00000000051fcf0: 7c 00 07 b4 extsw r0,r0
>
> c00000000051fcf4: 7c 20 04 ac lwsync
> c00000000051fcf8: 7d 60 f8 28 lwarx r11,0,r31
> c00000000051fcfc: 7c 0b 48 00 cmpw r11,r9
> c00000000051fd00: 40 c2 00 10 bne- c00000000051fd10
> c00000000051fd04: 7c 00 f9 2d stwcx. r0,0,r31
> c00000000051fd08: 40 c2 ff f0 bne+ c00000000051fcf8
> c00000000051fd0c: 4c 00 01 2c isync
>
> We create two constants, write them out to the stack, read them straight back
> in and sign extend them. What a mess.
>
> It turns out this bad code is a result of us defining atomic_t as a
> volatile int.
>
> We removed the volatile attribute from the powerpc atomic_t definition years
> ago, but commit ea435467500612636f8f4fb639ff6e76b2496e4b (atomic_t: unify all
> arch definitions) added it back in.
With your patches we can also revert 39475179d40996b4efa662e3825735a84d2526d1
"[S390] Improve code generated by atomic operations." which was created for
the very same reason.
Thanks!
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2]: atomic_t: Remove volatile from atomic_t definition
2010-05-17 4:34 ` [PATCH 2/2]: atomic_t: Remove volatile from atomic_t definition Anton Blanchard
2010-05-17 8:58 ` Heiko Carstens
@ 2010-05-17 15:01 ` Linus Torvalds
2010-05-17 20:13 ` Jamie Lokier
2010-05-19 13:03 ` Nick Piggin
1 sibling, 2 replies; 16+ messages in thread
From: Linus Torvalds @ 2010-05-17 15:01 UTC (permalink / raw)
To: Anton Blanchard; +Cc: akpm, willy, benh, paulus, linux-arch, linux-kernel
On Mon, 17 May 2010, Anton Blanchard wrote:
>
> It turns out this bad code is a result of us defining atomic_t as a
> volatile int.
Heh. Ok, as you point out in the commit message, I obviously agree with
this patch. "volatile" on data is evil, with the possible exception of
"jiffies" type things.
So applied.
Linus
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2]: atomic_t: Remove volatile from atomic_t definition
2010-05-17 15:01 ` Linus Torvalds
@ 2010-05-17 20:13 ` Jamie Lokier
2010-05-17 20:20 ` Linus Torvalds
2010-05-19 13:03 ` Nick Piggin
1 sibling, 1 reply; 16+ messages in thread
From: Jamie Lokier @ 2010-05-17 20:13 UTC (permalink / raw)
To: Linus Torvalds
Cc: Anton Blanchard, akpm, willy, benh, paulus, linux-arch, linux-kernel
Linus Torvalds wrote:
>
>
> On Mon, 17 May 2010, Anton Blanchard wrote:
> >
> > It turns out this bad code is a result of us defining atomic_t as a
> > volatile int.
>
> Heh. Ok, as you point out in the commit message, I obviously agree with
> this patch. "volatile" on data is evil, with the possible exception of
> "jiffies" type things.
I wonder if
extern unsigned long __nv_jiffies;
#define jiffies (*(volatile unsigned long *)*__nv_jiffies)
would improve any code in the same way as this atomic_t change.
-- Jamie
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2]: atomic_t: Remove volatile from atomic_t definition
2010-05-17 20:13 ` Jamie Lokier
@ 2010-05-17 20:20 ` Linus Torvalds
0 siblings, 0 replies; 16+ messages in thread
From: Linus Torvalds @ 2010-05-17 20:20 UTC (permalink / raw)
To: Jamie Lokier
Cc: Anton Blanchard, akpm, willy, benh, paulus, linux-arch, linux-kernel
On Mon, 17 May 2010, Jamie Lokier wrote:
>
> I wonder if
>
> extern unsigned long __nv_jiffies;
> #define jiffies (*(volatile unsigned long *)*__nv_jiffies)
>
> would improve any code in the same way as this atomic_t change.
We effectively already do that. The full 64-bit jiffies isn't volatile.
But for legacy reasons, we have that "word-sized subpart" of the full
counter that is volatile - and we're using linker tricks rather the the C
preprocessor to do the above thing.
So no, there's no room for improvement for jiffies - the only people who
ever use the volatile jiffies are things that read it. There are no
initializers that matter, and the updates (which also tend to suck with
"volatile" because gcc just doesn't do even the obvious optimizations) are
all done locked and to the non-volatile jiffies_64.
Linus
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2]: atomic_t: Remove volatile from atomic_t definition
2010-05-17 15:01 ` Linus Torvalds
2010-05-17 20:13 ` Jamie Lokier
@ 2010-05-19 13:03 ` Nick Piggin
2010-05-19 14:55 ` Linus Torvalds
2010-05-19 15:01 ` Paul E. McKenney
1 sibling, 2 replies; 16+ messages in thread
From: Nick Piggin @ 2010-05-19 13:03 UTC (permalink / raw)
To: Linus Torvalds
Cc: Anton Blanchard, akpm, willy, benh, paulus, linux-arch,
linux-kernel, Paul E. McKenney
On Mon, May 17, 2010 at 08:01:54AM -0700, Linus Torvalds wrote:
>
>
> On Mon, 17 May 2010, Anton Blanchard wrote:
> >
> > It turns out this bad code is a result of us defining atomic_t as a
> > volatile int.
>
> Heh. Ok, as you point out in the commit message, I obviously agree with
> this patch. "volatile" on data is evil, with the possible exception of
> "jiffies" type things.
>
> So applied.
I wonder, Linus, is there a good reason to use volatile for these at
all?
I asked you about it quite a while back, and IIRC you said it might
be OK to remove volatile from bitops, provided that callers were audited
(ie. that nobody used bitops on volatile variables).
For atomic_read it shouldn't matter unless gcc is *really* bad at it.
Ah, for atomic_read, the required semantic is surely ACCESS_ONCE, so
that's where the volatile is needed? (maybe it would be clearer to
explicitly use ACCESS_ONCE?)
The case I was thinking about for bitops was for multiple non-atomic
bitops, which would be nice to combine. In reality a lot of performance
critical code (like page allocator) bites the bullet and does the
open-coded bitwise ops. But it would be nice if that just worked for
__set_bit / __clear_bit too.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2]: atomic_t: Remove volatile from atomic_t definition
2010-05-19 13:03 ` Nick Piggin
@ 2010-05-19 14:55 ` Linus Torvalds
2010-05-19 15:01 ` Paul E. McKenney
1 sibling, 0 replies; 16+ messages in thread
From: Linus Torvalds @ 2010-05-19 14:55 UTC (permalink / raw)
To: Nick Piggin
Cc: Anton Blanchard, akpm, willy, benh, paulus, linux-arch,
linux-kernel, Paul E. McKenney
On Wed, 19 May 2010, Nick Piggin wrote:
>
> I wonder, Linus, is there a good reason to use volatile for these at
> all?
>
> I asked you about it quite a while back, and IIRC you said it might
> be OK to remove volatile from bitops, provided that callers were audited
> (ie. that nobody used bitops on volatile variables).
The bitops volatiles are different. They are there to allow for the C type
system (ie "const volatile *" just means that it accepts any kind of
pointer without complaining about implicit casting of const -> non-const
or volatile -> non-volatile).
For atomic_read(), and for the test_bit(), the _internal_ volatiles are
there just to get that ACCESS_ONCE() behavior, so that you can do things
like
while (test_bit(..)) {
..
}
and know that the compiler doesn't think it can do things like move the
atomic or bit read outside the loop or whatever.
Now, I do agree that _normally_ we should have memory barriers or similar
that guarantee that the compiler won't do odd things, but atomics and the
bitops are basically designed to work in the _absense_ of any other
serialization, so that's why it makes sense to have ACCESS_ONCE()
semantics for them.
> For atomic_read it shouldn't matter unless gcc is *really* bad at it.
> Ah, for atomic_read, the required semantic is surely ACCESS_ONCE, so
> that's where the volatile is needed? (maybe it would be clearer to
> explicitly use ACCESS_ONCE?)
Exactly. The volatile access on read inside those macros/functions (as
opposed to the "volatiles" that are there for C type reasons) is basically
the same as ACCESS_ONCE(). We could replace it with ACCESS_ONCE, although
I don't think it makes much difference as long as you always just think of
volatile as ACCESS_ONCE and just always put it in code (rather than on the
data structures)).
And replacing it with ACCESS_ONCE always has the header file dependency
issues, so..
> The case I was thinking about for bitops was for multiple non-atomic
> bitops, which would be nice to combine. In reality a lot of performance
> critical code (like page allocator) bites the bullet and does the
> open-coded bitwise ops. But it would be nice if that just worked for
> __set_bit / __clear_bit too.
__set_bit / __clear_bit should probably just be done as regular C code.
And yeah, we should remove the volatile from them. They aren't even valid
on anything that isn't locked anyway, so if somebody uses them on
something they have marked volatile, it's a bug.
Linus
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2]: atomic_t: Remove volatile from atomic_t definition
2010-05-19 13:03 ` Nick Piggin
2010-05-19 14:55 ` Linus Torvalds
@ 2010-05-19 15:01 ` Paul E. McKenney
2010-05-19 19:54 ` David Miller
1 sibling, 1 reply; 16+ messages in thread
From: Paul E. McKenney @ 2010-05-19 15:01 UTC (permalink / raw)
To: Nick Piggin
Cc: Linus Torvalds, Anton Blanchard, akpm, willy, benh, paulus,
linux-arch, linux-kernel
On Wed, May 19, 2010 at 11:03:27PM +1000, Nick Piggin wrote:
> On Mon, May 17, 2010 at 08:01:54AM -0700, Linus Torvalds wrote:
> >
> >
> > On Mon, 17 May 2010, Anton Blanchard wrote:
> > >
> > > It turns out this bad code is a result of us defining atomic_t as a
> > > volatile int.
> >
> > Heh. Ok, as you point out in the commit message, I obviously agree with
> > this patch. "volatile" on data is evil, with the possible exception of
> > "jiffies" type things.
> >
> > So applied.
>
> I wonder, Linus, is there a good reason to use volatile for these at
> all?
>
> I asked you about it quite a while back, and IIRC you said it might
> be OK to remove volatile from bitops, provided that callers were audited
> (ie. that nobody used bitops on volatile variables).
>
> For atomic_read it shouldn't matter unless gcc is *really* bad at it.
> Ah, for atomic_read, the required semantic is surely ACCESS_ONCE, so
> that's where the volatile is needed? (maybe it would be clearer to
> explicitly use ACCESS_ONCE?)
Explicit use of ACCESS_ONCE() where needed makes a lot of sense to me,
and allows better code to be generated for initialization and cleanup
code where no other task has access to the atomic_t.
> The case I was thinking about for bitops was for multiple non-atomic
> bitops, which would be nice to combine. In reality a lot of performance
> critical code (like page allocator) bites the bullet and does the
> open-coded bitwise ops. But it would be nice if that just worked for
> __set_bit / __clear_bit too.
FWIW, a similar debate in the C-language standards committee seems to
be headed in the direction of allowing combining of adjacent atomic
operations.
Thanx, Paul
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2]: atomic_t: Remove volatile from atomic_t definition
2010-05-19 15:01 ` Paul E. McKenney
@ 2010-05-19 19:54 ` David Miller
2010-05-19 22:50 ` Paul E. McKenney
0 siblings, 1 reply; 16+ messages in thread
From: David Miller @ 2010-05-19 19:54 UTC (permalink / raw)
To: paulmck
Cc: npiggin, torvalds, anton, akpm, willy, benh, paulus, linux-arch,
linux-kernel
From: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
Date: Wed, 19 May 2010 08:01:32 -0700
> On Wed, May 19, 2010 at 11:03:27PM +1000, Nick Piggin wrote:
>> For atomic_read it shouldn't matter unless gcc is *really* bad at it.
>> Ah, for atomic_read, the required semantic is surely ACCESS_ONCE, so
>> that's where the volatile is needed? (maybe it would be clearer to
>> explicitly use ACCESS_ONCE?)
>
> Explicit use of ACCESS_ONCE() where needed makes a lot of sense to me,
> and allows better code to be generated for initialization and cleanup
> code where no other task has access to the atomic_t.
I agree and I want to see this too, but I think with the tree the size
that it is we have to work backwards at this point.
Existing behavior by default, and optimized cases get tagged by using
a new interface (atomic_read_light(), test_bit{,s}_light(), etc.)
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2]: atomic_t: Remove volatile from atomic_t definition
2010-05-19 19:54 ` David Miller
@ 2010-05-19 22:50 ` Paul E. McKenney
2010-05-21 5:27 ` Nick Piggin
0 siblings, 1 reply; 16+ messages in thread
From: Paul E. McKenney @ 2010-05-19 22:50 UTC (permalink / raw)
To: David Miller
Cc: npiggin, torvalds, anton, akpm, willy, benh, paulus, linux-arch,
linux-kernel
On Wed, May 19, 2010 at 12:54:49PM -0700, David Miller wrote:
> From: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
> Date: Wed, 19 May 2010 08:01:32 -0700
>
> > On Wed, May 19, 2010 at 11:03:27PM +1000, Nick Piggin wrote:
> >> For atomic_read it shouldn't matter unless gcc is *really* bad at it.
> >> Ah, for atomic_read, the required semantic is surely ACCESS_ONCE, so
> >> that's where the volatile is needed? (maybe it would be clearer to
> >> explicitly use ACCESS_ONCE?)
> >
> > Explicit use of ACCESS_ONCE() where needed makes a lot of sense to me,
> > and allows better code to be generated for initialization and cleanup
> > code where no other task has access to the atomic_t.
>
> I agree and I want to see this too, but I think with the tree the size
> that it is we have to work backwards at this point.
>
> Existing behavior by default, and optimized cases get tagged by using
> a new interface (atomic_read_light(), test_bit{,s}_light(), etc.)
Fair enough!
Thanx, Paul
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2]: atomic_t: Remove volatile from atomic_t definition
2010-05-19 22:50 ` Paul E. McKenney
@ 2010-05-21 5:27 ` Nick Piggin
2010-05-21 5:54 ` David Miller
0 siblings, 1 reply; 16+ messages in thread
From: Nick Piggin @ 2010-05-21 5:27 UTC (permalink / raw)
To: Paul E. McKenney
Cc: David Miller, torvalds, anton, akpm, willy, benh, paulus,
linux-arch, linux-kernel
On Wed, May 19, 2010 at 03:50:46PM -0700, Paul E. McKenney wrote:
> On Wed, May 19, 2010 at 12:54:49PM -0700, David Miller wrote:
> > From: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
> > Date: Wed, 19 May 2010 08:01:32 -0700
> >
> > > On Wed, May 19, 2010 at 11:03:27PM +1000, Nick Piggin wrote:
> > >> For atomic_read it shouldn't matter unless gcc is *really* bad at it.
> > >> Ah, for atomic_read, the required semantic is surely ACCESS_ONCE, so
> > >> that's where the volatile is needed? (maybe it would be clearer to
> > >> explicitly use ACCESS_ONCE?)
> > >
> > > Explicit use of ACCESS_ONCE() where needed makes a lot of sense to me,
> > > and allows better code to be generated for initialization and cleanup
> > > code where no other task has access to the atomic_t.
> >
> > I agree and I want to see this too, but I think with the tree the size
> > that it is we have to work backwards at this point.
> >
> > Existing behavior by default, and optimized cases get tagged by using
> > a new interface (atomic_read_light(), test_bit{,s}_light(), etc.)
>
> Fair enough!
Hmm, I'm missing something. David, back up a second, as far as I can see,
with Anton's patches, atomic_read() *is* effectively just ACCESS_ONCE()
now. Linus pointed out that header tangle is the reason not to just use
the macro.
Am I wrong, or is it that ACCESS_ONCE has a more relaxed semantic *in
theory* that allows a future more aggressive implementation if the
compiler supports it?
do {
done = ACCESS_ONCE(blah);
} while (!done);
Is this (in theory) allowed to be turned into a branch into an infinite
loop? Wheras the volatile deref would require it be reloaded each time?
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2]: atomic_t: Remove volatile from atomic_t definition
2010-05-21 5:27 ` Nick Piggin
@ 2010-05-21 5:54 ` David Miller
2010-05-21 6:06 ` Nick Piggin
0 siblings, 1 reply; 16+ messages in thread
From: David Miller @ 2010-05-21 5:54 UTC (permalink / raw)
To: npiggin
Cc: paulmck, torvalds, anton, akpm, willy, benh, paulus, linux-arch,
linux-kernel
From: Nick Piggin <npiggin@suse.de>
Date: Fri, 21 May 2010 15:27:46 +1000
> Hmm, I'm missing something. David, back up a second, as far as I can see,
> with Anton's patches, atomic_read() *is* effectively just ACCESS_ONCE()
> now. Linus pointed out that header tangle is the reason not to just use
> the macro.
My bad, I was under the impression that the proposal was to remove
volatile usage and also not even do ACCESS_ONCE() in atomic_read().
And then explicitly annotate call sits that actually need the
ACCESS_ONCE() semantic.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2]: atomic_t: Remove volatile from atomic_t definition
2010-05-21 5:54 ` David Miller
@ 2010-05-21 6:06 ` Nick Piggin
2010-05-21 6:10 ` David Miller
0 siblings, 1 reply; 16+ messages in thread
From: Nick Piggin @ 2010-05-21 6:06 UTC (permalink / raw)
To: David Miller
Cc: paulmck, torvalds, anton, akpm, willy, benh, paulus, linux-arch,
linux-kernel
On Thu, May 20, 2010 at 10:54:54PM -0700, David Miller wrote:
> From: Nick Piggin <npiggin@suse.de>
> Date: Fri, 21 May 2010 15:27:46 +1000
>
> > Hmm, I'm missing something. David, back up a second, as far as I can see,
> > with Anton's patches, atomic_read() *is* effectively just ACCESS_ONCE()
> > now. Linus pointed out that header tangle is the reason not to just use
> > the macro.
>
> My bad, I was under the impression that the proposal was to remove
> volatile usage and also not even do ACCESS_ONCE() in atomic_read().
>
> And then explicitly annotate call sits that actually need the
> ACCESS_ONCE() semantic.
Ah ok, no. I see ACCESS_ONCE is a fundamental ("obvious") property of
atomic_read, so we definitely should keep it, even if we could audit
everyone.
Actually, I bet we have a lot of bugs there with loading integers and
pointers atomically, where the code assumes the loaded value will not
be reloaded by the compiler, because it is an easy thing to assume.
atomic_read_light could be useful though, for sure.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2]: atomic_t: Remove volatile from atomic_t definition
2010-05-21 6:06 ` Nick Piggin
@ 2010-05-21 6:10 ` David Miller
2010-05-21 6:44 ` Eric Dumazet
0 siblings, 1 reply; 16+ messages in thread
From: David Miller @ 2010-05-21 6:10 UTC (permalink / raw)
To: npiggin
Cc: paulmck, torvalds, anton, akpm, willy, benh, paulus, linux-arch,
linux-kernel
From: Nick Piggin <npiggin@suse.de>
Date: Fri, 21 May 2010 16:06:00 +1000
> Actually, I bet we have a lot of bugs there with loading integers and
> pointers atomically, where the code assumes the loaded value will not
> be reloaded by the compiler, because it is an easy thing to assume.
Alexey Kuznetsov was aware of this problem 8+ years ago when we were
first adding fine-grained locking the the networking.
> atomic_read_light could be useful though, for sure.
I definitely think so. And every usage of it should have a big fat
comment right next to it explaining how it's usage is valid in that
spot :-)
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2]: atomic_t: Remove volatile from atomic_t definition
2010-05-21 6:10 ` David Miller
@ 2010-05-21 6:44 ` Eric Dumazet
0 siblings, 0 replies; 16+ messages in thread
From: Eric Dumazet @ 2010-05-21 6:44 UTC (permalink / raw)
To: David Miller
Cc: npiggin, paulmck, torvalds, anton, akpm, willy, benh, paulus,
linux-arch, linux-kernel
Le jeudi 20 mai 2010 à 23:10 -0700, David Miller a écrit :
> From: Nick Piggin <npiggin@suse.de>
> Date: Fri, 21 May 2010 16:06:00 +1000
>
> > Actually, I bet we have a lot of bugs there with loading integers and
> > pointers atomically, where the code assumes the loaded value will not
> > be reloaded by the compiler, because it is an easy thing to assume.
>
> Alexey Kuznetsov was aware of this problem 8+ years ago when we were
> first adding fine-grained locking the the networking.
>
> > atomic_read_light could be useful though, for sure.
>
> I definitely think so. And every usage of it should have a big fat
> comment right next to it explaining how it's usage is valid in that
> spot :-)
> -
I really doubt a valid (and dully commented) usage of
atomic_read_light() will bring anything, but added complexity to API.
Generated code will be the same than atomic_read() in these cases,
unless some real factorization can be done inside a loop.
We still have some uneeded RMW atomic ops to remove, before even
thinking to optimise reads ;)
^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2010-05-21 6:44 UTC | newest]
Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-05-17 4:33 [PATCH 1/2]: atomic_t: Cast to volatile when accessing atomic variables Anton Blanchard
2010-05-17 4:34 ` [PATCH 2/2]: atomic_t: Remove volatile from atomic_t definition Anton Blanchard
2010-05-17 8:58 ` Heiko Carstens
2010-05-17 15:01 ` Linus Torvalds
2010-05-17 20:13 ` Jamie Lokier
2010-05-17 20:20 ` Linus Torvalds
2010-05-19 13:03 ` Nick Piggin
2010-05-19 14:55 ` Linus Torvalds
2010-05-19 15:01 ` Paul E. McKenney
2010-05-19 19:54 ` David Miller
2010-05-19 22:50 ` Paul E. McKenney
2010-05-21 5:27 ` Nick Piggin
2010-05-21 5:54 ` David Miller
2010-05-21 6:06 ` Nick Piggin
2010-05-21 6:10 ` David Miller
2010-05-21 6:44 ` Eric Dumazet
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.