ldcw inline assembler patch
I'm wondering if this patch might help people who are seeing locking problems on SMP boxes ? Helge
include/asm-parisc/system.h +3 -2
@@ -168,8 +168,9 @@ static inline void set_eiem(unsigned long val)/* LDCW, the only atomic read-write operation PA-RISC has. *sigh*. */ #define __ldcw (a) ({ \ unsigned __ret; \ - __asm__ __volatile__(__LDCW " 0(%1),%0" \ - : "=r" (__ret) : "r" (a)); \ + __asm__ __volatile__(__LDCW " 0(%2),%0" \ + : "=r" (__ret), "=m" (*(a)) \ + : "r" (a), "m" (*(a)) ); \ __ret; \ })
Quoted 9 lines · Helge Deller
I'm wondering if this patch might help people who are seeing locking problems on SMP boxes ?
Helgeinclude/asm-parisc/system.h +0
-0
include/asm-parisc/system.h static inline void set_eiem(unsigned long val) · line 175
Quoted 10 lines
include/asm-parisc/system.h +3
-2@@ -168,8 +168,9 @@ static inline void set_eiem(unsigned long val)/* LDCW, the only atomic read-write operation PA-RISC has. *sigh*. */ #define __ldcw (a) ({ \ unsigned __ret; \ - __asm__ __volatile__(__LDCW " 0(%1),%0" \ - : "=r" (__ret) : "r" (a)); \ + __asm__ __volatile__(__LDCW " 0(%2),%0" \ + : "=r" (__ret), "=m" (*(a)) \ + : "r" (a), "m" (*(a)) ); \ __ret; \ })
You don't want to do that, the compiler might hold "=m" (*(a)) in a temporary memory location.
e.g.temp_mem = *a;
reg2 = &temp_mem;
... operation ...
*a = temp_mem;
You would be atomic with regards to the store to temp_mem, but not a. Infact this could always be the case.
I'm now of the opinion that we need a "memory" clobber in the original expression to prevent this from ever happening.
Cheers,
Carlos.
Hi Carlos,
Quoted 30 lines · Carlos O'Donell
Quoted 9 lines · Helge Deller
I'm wondering if this patch might help people who are seeing locking problems on SMP boxes ?
Helgeinclude/asm-parisc/system.h +0
-0include/asm-parisc/system.h static inline void set_eiem(unsigned long val) · line 175
Quoted 10 lines
include/asm-parisc/system.h +3
-2@@ -168,8 +168,9 @@ static inline void set_eiem(unsigned long val)/* LDCW, the only atomic read-write operation PA-RISC has. *sigh*. */ #define __ldcw (a) ({ \ unsigned __ret; \ - __asm__ __volatile__(__LDCW " 0(%1),%0" \ - : "=r" (__ret) : "r" (a)); \ + __asm__ __volatile__(__LDCW " 0(%2),%0" \ + : "=r" (__ret), "=m" (*(a)) \ + : "r" (a), "m" (*(a)) ); \ __ret; \ })You don't want to do that, the compiler might hold "=m" (*(a)) in a temporary memory location.
e.g.
temp_mem= *a;
reg2 =&temp_mem;
... operation ...
*a =temp_mem;
Quoted 5 lines
You would be atomic with regards to the store to
temp_mem, but not a. Infact this could always be the case.I'm now of the opinion that we need a "memory" clobber in the original expression to prevent this from ever happening.
So, your proposal is (copy-and-pasted in here) the following ?
include/asm-parisc/system.h +1 -1
@@ -169,7 +169,7 @@ static inline void set_eiem(unsigned long val)#define __ldcw (a) ({ \ unsigned __ret; \ __asm__ __volatile__(__LDCW " 0(%1),%0" \ - : "=r" (__ret) : "r" (a)); \ + : "=r" (__ret) : "r" (a) : "memory" ); \ __ret; \ })
Quoted 7 lines · Helge Deller
So, your proposal is (copy-and-pasted in here) the following ?
include/asm-parisc/system.h +0
-0
include/asm-parisc/system.h static inline void set_eiem(unsigned long val) · line 174
Quoted 7 lines
include/asm-parisc/system.h +1
-1@@ -169,7 +169,7 @@ static inline void set_eiem(unsigned long val)#define __ldcw (a) ({ \ unsigned __ret; \ __asm__ __volatile__(__LDCW " 0(%1),%0" \ - : "=r" (__ret) : "r" (a)); \ + : "=r" (__ret) : "r" (a) : "memory" ); \ __ret; \ })
Yes. The asm should clobber memory thus forcing the compiler to avoid memory temporaries.
Dave may have other opinions.
I think this solution is the safest.
Cheers,
Carlos.
Quoted 19 lines · Carlos O'Donell
Quoted 7 lines · Helge Deller
So, your proposal is (copy-and-pasted in here) the following ?
include/asm-parisc/system.h +0
-0include/asm-parisc/system.h static inline void set_eiem(unsigned long val) · line 174
Quoted 7 lines
include/asm-parisc/system.h +1
-1@@ -169,7 +169,7 @@ static inline void set_eiem(unsigned long val)#define __ldcw (a) ({ \ unsigned __ret; \ __asm__ __volatile__(__LDCW " 0(%1),%0" \ - : "=r" (__ret) : "r" (a)); \ + : "=r" (__ret) : "r" (a) : "memory" ); \ __ret; \ })Yes. The asm should clobber memory thus forcing the compiler to avoid memory temporaries.
It shouldn't need to, since we're only ever accessing one word (the one specified in the operand.)
Otherwise basically every inline asm everywhere ever is going to need a memory clobber, and that's just BROKEN.
r, Kyle
Quoted 21 lines · Kyle McMartin
Quoted 19 lines · Carlos O'Donell
Quoted 7 lines · Helge Deller
So, your proposal is (copy-and-pasted in here) the following ?
include/asm-parisc/system.h +0
-0include/asm-parisc/system.h static inline void set_eiem(unsigned long val) · line 174
Quoted 7 lines
include/asm-parisc/system.h +1
-1@@ -169,7 +169,7 @@ static inline void set_eiem(unsigned long val)#define __ldcw (a) ({ \ unsigned __ret; \ __asm__ __volatile__(__LDCW " 0(%1),%0" \ - : "=r" (__ret) : "r" (a)); \ + : "=r" (__ret) : "r" (a) : "memory" ); \ __ret; \ })Yes. The asm should clobber memory thus forcing the compiler to avoid memory temporaries.
Quoted 6 lines
It shouldn't need to, since we're only ever accessing one word (the one specified in the operand.)
Otherwise basically every inline asm everywhere ever is going to need a memory clobber, and that's just BROKEN.
Willy points out the caching of the locked data across the lock, but we surround the inline in a memory barrier, so we're fine.
r, Kyle
Quoted 20 lines · Kyle McMartin
Quoted 18 lines · Carlos O'Donell
Quoted 7 lines · Helge Deller
So, your proposal is (copy-and-pasted in here) the following ?
include/asm-parisc/system.h +0
-0include/asm-parisc/system.h static inline void set_eiem(unsigned long val) · line 174
Quoted 7 lines
include/asm-parisc/system.h +1
-1@@ -169,7 +169,7 @@ static inline void set_eiem(unsigned long val)#define __ldcw (a) ({ \ unsigned __ret; \ __asm__ __volatile__(__LDCW " 0(%1),%0" \ - : "=r" (__ret) : "r" (a)); \ + : "=r" (__ret) : "r" (a) : "memory" ); \ __ret; \ })Yes. The asm should clobber memory thus forcing the compiler to avoid memory temporaries.
Quoted 5 lines
It shouldn't need to, since we're only ever accessing one word (the one specified in the operand.)
Otherwise basically every inline asm everywhere ever is going to need a memory clobber, and that's just BROKEN.
Carlos' and Helge's point (I think) is that the __ldcw() doesn't clobber memory, so gcc can cache other things in registers across a call to __ldcw(). While this is true, our definition of __raw_spin_lock() has two calls to mb() in it, which is defined to clobber memory.
The only users of __ldcw() are in spinlock.h which has the mb()s in place. I don't think there's a problem here.
Signature · 4 lines
Intel are signing my paycheques ... these opinions are still mine "Bill, look, we understand that you're interested in selling us this operating system, but compare it to ours. We can't possibly take such a retrograde step."
Quoted 2 lines · Matthew Wilcox
The only users of
__ldcw()are inspinlock.hwhich has the mb()s in place. I don't think there's a problem here.
Given:mb()__ldcw(a)mb()
What stops the compiler from doing?
mb()*stack_slot = *a
reg1 = *stack_slot
reg2 = __ldcw(reg1)
*a = *stack_slotmb()
Memory is consistent before and after the memory barriers, but the operation is not atomic?
While this seems stupid, the compile may create a memory temporary to shuffle things around.
Cheers,
Carlos.
Quoted 10 lines · Carlos O'Donell
Given:
mb()__ldcw(a)mb()What stops the compiler from doing?
mb()*stack_slot= *a
reg1 =*stack_slot
This is a mistake, and should read:
reg1 = stack_slot
e.g. Copy the address of the stack slot into reg1.
Quoted 9 lines
reg2 =
__ldcw(reg1)
*a =*stack_slotmb()Memory is consistent before and after the memory barriers, but the operation is not atomic?
While this seems stupid, the compile may create a memory temporary to shuffle things around.
The compiler creates memory temporaries all the times and expects the optimizers to see that the temporaries are not needed. However, sometimes the temporaries aren't removed.
Cheers,
Carlos.
Quoted 3 lines · Carlos O'Donell
The compiler creates memory temporaries all the times and expects the optimizers to see that the temporaries are not needed. However, sometimes the temporaries aren't removed.
It turns out this should never happen because raw_spinlock_t->lock is volatile.
Therefore the memory clobber is not required.
The memory barriers will prevent code from being moved before the lock or past the unlock.
Cheers,
Carlos.
Quoted 19 lines
Quoted 5 lines
include/asm-parisc/system.h +0
-0include/asm-parisc/system.h static inline void set_eiem(unsigned long val) · line 175
Quoted 10 lines
include/asm-parisc/system.h +3
-2@@ -168,8 +168,9 @@ static inline void set_eiem(unsigned long val)/* LDCW, the only atomic read-write operation PA-RISC has. *sigh*. */ #define __ldcw (a) ({ \ unsigned __ret; \ - __asm__ __volatile__(__LDCW " 0(%1),%0" \ - : "=r" (__ret) : "r" (a)); \ + __asm__ __volatile__(__LDCW " 0(%2),%0" \ + : "=r" (__ret), "=m" (*(a)) \ + : "r" (a), "m" (*(a)) ); \ __ret; \ })You don't want to do that, the compiler might hold "=m" (*(a)) in a temporary memory location.
Quoted 5 lines
e.g.
temp_mem= *a;
reg2 =&temp_mem;
... operation ...
*a =temp_mem;
The operation uses "a" not "reg2". Listing *a as written in the asm should stop the compiler from caching *a in temp_mem.
I don't believe the compiler ever caches values in "memory". However, it does cache memory values in registers. So, we need to tell GCC not to cache *a in a register. This can be done either by explicitly listing the affected locations or by clobbering all memory.
I believe using the volatile keyword and a memory clobber is ok but overkill. Explicitly listing the affected memory as suggested by Helge appears to be the way to go. There is an example of this in the GCC manual.
I also believe we can use the "+m" constraint so that *a doesn't need to be listed twice.
It might be possible to remove the volatile keyword when the affected memory is explicitly listed. However, I doubt there are any situations where the asm can be optimized away.
Dave
Signature · 2 lines
J. David Anglin [email protected] National Research Council of Canada (613) 990-0752 (FAX: 952-6602)