locking: Add volatile to arch_spinlock_t structures

NAK. Volatile on data structures (as opposed to in code) is a disease. This is wrong.

We very much pass spinlock values around. They aren't volatile. The only thing volatile is when you load the value from the lock itself. See for example the stuff we do when we load a reflock from memory. Once we've loaded the value into registers it's no longer volatile, and putting violators into the data structures is actively *wrong*.

So no, no, no. C got this wrong. Volatile data structures are a fundamental mistake and a bug. The only good volatile is in an access (ie like ACCESS_ONCE() and anything else is making things worse.

We're not catering to crazy compiler people that can't understand that fact. I'm not even seeing the point of your patch, since it seems to be entirely about "let's make excuses for crap compilers"

Linus

Quoted 206 lines · Paul E. McKenney

More concern about compilers...

Most architectures mark the fields in their arch_spinlock_t structures as volatile, which forces the compiler to respect the integrity of those fields. Without volatile markings, the compiler is within its rights to overwrite these fields, for example, by using a wide store to update an adjacent field as long as it fixes them up later. This sort of thing could come as a rather nasty surprise to any task attempting to concurrently acquire that lock.

For example, on x86 for smallish NR_CPUS, arch_spinlock_t is 16 bits wide. If there were three adjacent 16-bit fields in the structure containing the arch_spinlock_t, a compiler might reasonably use 64-bit accesses for those three fields. After a 64-bit load, the arch_spinlock_t's value would be available in a register, so that the compiler might use a 64-bit store followed by a 16-bit fixup store to update the three adjacent 16-bit fields.

This commit therefore adds volatile to the arch_spinlock_t and arch_rwlock_t fields that don't already have them.

Signed-off-by Paul E. McKenney <[email protected]>

Cc [email protected]

Cc Dmitry Vyukov <[email protected]>

Cc Davidlohr Bueso <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc Linus Torvalds <[email protected]>

arch/arm/include/asm/spinlock_types.h +6 -6

index 47663fcb10ad..7d3b6ecb5301 100644
@@ -9,14 +9,14 @@
 
 typedef struct {
        union {
-               u32 slock;
+               volatile u32 slock;
                struct __raw_tickets {
 #ifdef __ARMEB__
-                       u16 next;
-                       u16 owner;
+                       volatile u16 next;
+                       volatile u16 owner;
 #else
-                       u16 owner;
-                       u16 next;
+                       volatile u16 owner;
+                       volatile u16 next;
 #endif
                } tickets;
        };

@@ -25,7 +25,7 @@ typedef struct {
 #define __ARCH_SPIN_LOCK_UNLOCKED      { { 0 } }
 
 typedef struct {
-       u32 lock;
+       volatile u32 lock;
 } arch_rwlock_t;
 
 #define __ARCH_RW_LOCK_UNLOCKED                { 0 }

arch/arm64/include/asm/spinlock_types.h +4 -4

index b8d383665f56..0f841378f0f3 100644
@@ -24,11 +24,11 @@
 
 typedef struct {
 #ifdef __AARCH64EB__
-       u16 next;
-       u16 owner;
+       volatile u16 next;
+       volatile u16 owner;
 #else
-       u16 owner;
-       u16 next;
+       volatile u16 owner;
+       volatile u16 next;
 #endif
 } __aligned(4) arch_spinlock_t;
 

arch/mips/include/asm/spinlock_types.h +5 -5

index 9b2528e612c0..10f04a0482dd 100644
@@ -14,14 +14,14 @@ typedef union {
         * bits  0..15 : serving_now
         * bits 16..31 : ticket
         */
-       u32 lock;
+       volatile u32 lock;
        struct {
 #ifdef __BIG_ENDIAN
-               u16 ticket;
-               u16 serving_now;
+               volatile u16 ticket;
+               volatile u16 serving_now;
 #else
-               u16 serving_now;
-               u16 ticket;
+               volatile u16 serving_now;
+               volatile u16 ticket;
 #endif
        } h;
 } arch_spinlock_t;

arch/mn10300/include/asm/spinlock_types.h +2 -2

index 653dc519b405..04fd2c622f81 100644
@@ -6,13 +6,13 @@
 #endif
 
 typedef struct arch_spinlock {
-       unsigned int slock;
+       volatile unsigned int slock;
 } arch_spinlock_t;
 
 #define __ARCH_SPIN_LOCK_UNLOCKED      { 0 }
 
 typedef struct {
-       unsigned int lock;
+       volatile unsigned int lock;
 } arch_rwlock_t;
 
 #define __ARCH_RW_LOCK_UNLOCKED                { RW_LOCK_BIAS }

arch/s390/include/asm/spinlock_types.h +3 -3

index d84b6939237c..0ccdda3b6842 100644
@@ -6,14 +6,14 @@
 #endif
 
 typedef struct {
-       unsigned int lock;
+       volatile unsigned int lock;
 } __attribute__ ((aligned (4))) arch_spinlock_t;
 
 #define __ARCH_SPIN_LOCK_UNLOCKED { .lock = 0, }
 
 typedef struct {
-       unsigned int lock;
-       unsigned int owner;
+       volatile unsigned int lock;
+       volatile unsigned int owner;
 } arch_rwlock_t;
 
 #define __ARCH_RW_LOCK_UNLOCKED                { 0 }

arch/tile/include/asm/spinlock_types.h +4 -4

index a71f59b49c50..29f70a14b979 100644
@@ -23,14 +23,14 @@
 
 /* Low 15 bits are "next"; high 15 bits are "current". */
 typedef struct arch_spinlock {
-       unsigned int lock;
+       volatile unsigned int lock;
 } arch_spinlock_t;
 
 #define __ARCH_SPIN_LOCK_UNLOCKED      { 0 }
 
 /* High bit is "writer owns"; low 31 bits are a count of readers. */
 typedef struct arch_rwlock {
-       unsigned int lock;
+       volatile unsigned int lock;
 } arch_rwlock_t;
 
 #define __ARCH_RW_LOCK_UNLOCKED                { 0 }

@@ -39,9 +39,9 @@ typedef struct arch_rwlock {
 
 typedef struct arch_spinlock {
        /* Next ticket number to hand out. */
-       int next_ticket;
+       volatile int next_ticket;
        /* The ticket number that currently owns this lock. */
-       int current_ticket;
+       volatile int current_ticket;
 } arch_spinlock_t;
 
 #define __ARCH_SPIN_LOCK_UNLOCKED      { 0, 0 }

arch/x86/include/asm/spinlock_types.h +2 -2

index 5f9d7572d82b..fead0ca82468 100644
@@ -25,9 +25,9 @@ typedef u32 __ticketpair_t;
 
 typedef struct arch_spinlock {
        union {
-               __ticketpair_t head_tail;
+               volatile __ticketpair_t head_tail;
                struct __raw_tickets {
-                       __ticket_t head, tail;
+                       volatile __ticket_t head, tail;
                } tickets;
        };
 } arch_spinlock_t;

Quoted 2 lines · Linus Torvalds

So no, no, no. C got this wrong. Volatile data structures are a

fundamental mistake and a bug.

BTW, I'm not at all interested in language lawyering and people who say "but but we can do x". A compiler that modifies adjacent fields because the standard leaves is open is a crap compiler, and we won't use it, or disable the broken optimization. It is wrong from a concurrency standpoint anyway, and adding broken volatiles is just making things worse.

Quoted 11 lines · Linus Torvalds

Quoted 2 lines · Linus Torvalds

So no, no, no. C got this wrong. Volatile data structures are a

fundamental mistake and a bug.

BTW, I'm not at all interested in language lawyering and people who say "but but we can do x". A compiler that modifies adjacent fields because the standard leaves is open is a crap compiler, and we won't use it, or disable the broken optimization. It is wrong from a concurrency standpoint anyway, and adding broken volatiles is just making things worse.

Understood, for example, adjacent fields protected by different locks as one example, where adjacent-field overwriting completely breaks even very conservatively designed code.

Quoted 13 lines · Paul E. McKenney

Quoted 11 lines · Linus Torvalds

Quoted 2 lines · Linus Torvalds

So no, no, no. C got this wrong. Volatile data structures are a

fundamental mistake and a bug.

BTW, I'm not at all interested in language lawyering and people who say "but but we can do x". A compiler that modifies adjacent fields because the standard leaves is open is a crap compiler, and we won't use it, or disable the broken optimization. It is wrong from a concurrency standpoint anyway, and adding broken volatiles is just making things worse.

Quoted 3 lines

Understood, for example, adjacent fields protected by different locks as one example, where adjacent-field overwriting completely breaks even very conservatively designed code.

Exactly. Compilers that "optimize" things to touch fields that aren't touched by the source code are simply inherently buggy shit. I'm not at all interested in catering to their insanity.

It doesn't matter one whit if they can point to the legacy C "virtual machine" definition and say that those accesses are invisible in the virtual machine. They are not invisible in real life, and it is entirely possible that two adjacent variables or fields are protected by different locks - even in non-kernel code. Claiming that they need to be marked volatile is a symptom of a diseased compiler writer.

Now, the one exception to this is generally bitfields, because there the programmer knowingly and intentionally puts the fields together in the same storage unit. I also think that volatile bitfields are an insane concept, even if I think that the standard allows them. So I am not saying that compilers should try to magically make bitfield members not access the members around them.

I also accept that some architectures are broken. Old non-byte/word-access alpha being the really canonical example. It's not the compilers fault if the architecture is broken, and the compiler cannot magically fix it.

But compilers that think that "hey, vectorization is cool, and I can do load-stores and mask things dynamically" are misguided crap. It may be fancy, it may be really cool compiler technology, but it's fundamentally wrong unless the programmer told it was safe some way (be it with a "pragma" or "restrict" or a compile-time switch or whatever).

I have to ask... Does this mean we can remove the current restrictions against 8-bit and 16-bit access from smp_load_acquire() and smp_store_release()?

Quoted 4 lines · Paul E. McKenney

I have to ask... Does this mean we can remove the current restrictions against 8-bit and 16-bit access from smp_load_acquire() and smp_store_release()?

I'd certainly be ok with it. Alpha doesn't have acquire/release semantics anyway (really, it's the worst memory ordering model *ever*), so those will end up being just plain (access-once) loads and stores, followed/preceded by a memory barrier anyway. So it arguably is no worse than the existing situation with ACCESS_ONCE() on alpha.

And quite frankly, I simply don't think that an old broken alpha architecture should be something we worry about. Remember: the byte and word ops were introduced in 21164, and released in 1996, so it's not even like "alpha has broken behavior". It's literally just "the very earliest alphas were broken", and I suspect most of those machines (at least running Linux) weren't even SMP (ie somebody may still have a Multia around for sentimental reasons, but SMP? No).

Of course, I'd like there to be a real reason to do so, not just "who cares about really old alphas, nyaah, nyaah, nyaah"? But if there is a clear case where a byte load-acquire and store-release would improve on something important, then yes, I think we should do it.

We dropped support for the original i386, we can drop support for old broken alphas. People running them for sentimental reasons might as well be sentimental about software too, and run old kernels ;)

So any compiler that clobbers some adjacent non-bitfield variable or field that is accessible by other threads is not just despicable, it fails to conform to the standard.

Whew! ;-)

anywhere in that translation unit. After all, any non-static function in that translation unit might be called from some other translation unit that -did- use locking or whatever.

I will let you know how it goes. ;-)

Quoted 5 lines

anywhere in that translation unit. After all, any non-static function in that translation unit might be called from some other translation unit that -did- use locking or whatever.

I will let you know how it goes. ;-)

It breaks DEC10 ;-)

If there is kickback over things like optimisation perhaps the gcc maintainers could at least consider something like

	int __attribute((threadsafe)) fred;

??