coherent ops and mb() revisited

Hi all,
I haven't tested the following patch and just wanted to get feedback if it is "the right thing" or not.

John Marvin was clear in previous discussion we don't need to use coherent stores but I've heard we should follow the arch anyway so it doesn't bite us later. I find it highly unlikely but see no harm in "doing it right".

The second bit is if adding "memory" reference in the ldcw and _raw_spin_unlock() could replace the mb() instructions which James Bottomley added to prevent gcc from re-ordering instructions outside the critical section.

opinions?

thanks,
grant

Index: include/asm-parisc/spinlock.h
===================================================================
RCS file: /var/cvs/linux-2.6/include/asm-parisc/spinlock.h,v retrieving revision 1.6

diff -u -p -r1.6 spinlock.h
@@ -29,20 +29,26 @@ static inline void _raw_spin_lock(spinlo
 {
 	volatile unsigned int *a;
 
-	mb();
 	a = __ldcw_align(x);
 	while (__ldcw(a) == 0)
 		while (*a == 0);
-	mb();
 }
 
 static inline void _raw_spin_unlock(spinlock_t *x)
 {
 	volatile unsigned int *a;
-	mb();
+	register unsigned tmp=1;
+
 	a = __ldcw_align(x);
-	*a = 1;
-	mb();
+	/* use a coherent store. PA1.1 is always strongly ordered.
+	 * Even though no PA2.0 implementation is weakly ordered,
+	 * jda would prefer we use coherent stores (",ma" with zero offset
+	 * is the same thing but PA1.1 compatible).
+	 * Key here is "memory" - prevent gcc from re-ordering memory
+	 * accesses below releasing the lock.
+	 */
+	__asm__ __volatile__ ("stw,ma %1,0(%0)"
+			: : "r" (a), "r" (tmp) : "memory");
 }
 
 static inline int _raw_spin_trylock(spinlock_t *x)

Index: include/asm-parisc/system.h
===================================================================
RCS file: /var/cvs/linux-2.6/include/asm-parisc/system.h,v retrieving revision 1.7

diff -u -p -r1.7 system.h
@@ -141,7 +141,7 @@ static inline void set_eiem(unsigned lon
 /* 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(%1),%0" : "=r" (__ret) : "r" (a) : "memory"); \
 	__ret; \
 })
 

_______________________________________________
parisc-linux mailing list
[email protected]
http://lists.parisc-linux.org/mailman/listinfo/parisc-linux

It should work, but I don't think it's the right thing to do.

__ldcw already insists on operating on volatile data, which is the correct thing to do. Adding a memory barrier is probably harmless, but it crimps any optimisations gcc might like to try with that function (not that there really are any with such a simple loop). Indeed, we could even remove the __volatile__ from the asm.

I think the best implementation is probably

barrier();
a = __ldcw_align(x);
while (__ldcw(a) == 0)

	while(*a == 0)
		;

mb();

Which makes it totally clear what barriers we need where in the spinlock (the first being simply a compiler ordering barrier and the second actually a processor memory barrier).

James

_______________________________________________
parisc-linux mailing list
[email protected]
http://lists.parisc-linux.org/mailman/listinfo/parisc-linux

I don't understand why you put the barrier before the __ldcw_align operation. The alignment operation is non-critical and could be done much earlier if that's desireable from a scheduling standpoint.

Dave

J. David Anglin                                  [email protected]
National Research Council of Canada              (613) 990-0752 (FAX: 952-6602)
_______________________________________________
parisc-linux mailing list
[email protected]
http://lists.parisc-linux.org/mailman/listinfo/parisc-linux

Actually, you're right. I think all we need is the mb() after the spin_lock code and another mb before the spin_unlock code. How the taking or releasing of the lock is optimised by gcc should be irrelevant as long as it's locked when we cross the barrier.

James

_______________________________________________
parisc-linux mailing list
[email protected]
http://lists.parisc-linux.org/mailman/listinfo/parisc-linux

Actaully, I don't think it's irrelevant. If a lock is contended for, re-ordering by gcc could excerbate the problem by adding additional instructions (good for the instruction pipeline) to the "critical section" (the period we actually hold the lock).

I know, lock contention is bad and it should never happen. Reality is some workload will contend for a lock. I want to have some confidence gcc is not making it any worse. This is why I'm asking about use of "memory" in the actual asm instruction that either acquire or release the lock instead of using mb().

thanks,
grant

_______________________________________________
parisc-linux mailing list
[email protected]
http://lists.parisc-linux.org/mailman/listinfo/parisc-linux

I don't have any particular insight here beyond what's in the manual:

If your assembler instructions access memory in an unpredictable fashion, add @samp{memory} to the list of clobbered registers. This will cause GCC to not keep memory values cached in registers across the assembler instruction and not optimize stores or loads to that memory. You will also want to add the @code{volatile} keyword if the memory affected is not listed in the inputs or outputs of the @code{asm}, as the @samp{memory} clobber does not count as a side-effect of the @code{asm}. If you know how large the accessed memory is, you can add it as input or output but if this is not known, you should add @samp{memory}. As an example, if you access ten bytes of a string, you can use a memory input like:

@example
@{"m"( (@{ struct @{ char x[10]; @} *p = (void *)ptr ; *p; @}) )@}. @end example

Note that in the following example the memory input is necessary, otherwise GCC might optimize the store to @code{x} away:
@example
int foo ()
@{
int x = 42;
int *y = &x;
int result;
asm ("magic stuff accessing an 'int' pointed to by '%1'"

       "=&d" (r) : "a" (y), "m" (*y));

return result;
@}
@end example

@noindent
If you write an @code{asm} instruction with no outputs, GCC will know the instruction has side-effects and will not delete the instruction or move it outside of loops.

The @code{volatile} keyword indicates that the instruction has important side-effects. GCC will not delete a volatile @code{asm} if it is reachable. (The instruction can still be deleted if GCC can prove that control-flow will never reach the location of the instruction.) In addition, GCC will not reschedule instructions across a volatile @code{asm} instruction. For example:

@smallexample
*(volatile int *)addr = foo;
asm volatile ("eieio" : : );
@end smallexample

@noindent
Assume @code{addr} contains the address of a memory mapped device register. The PowerPC @code{eieio} instruction (Enforce In-order Execution of I/O) tells the CPU to make sure that the store to that device register happens before it issues any other I/O@.

Note that even a volatile @code{asm} instruction can be moved in ways that appear insignificant to the compiler, such as across jump instructions. You can't expect a sequence of volatile @code{asm} instructions to remain perfectly consecutive. If you want consecutive output, use a single @code{asm}. Also, GCC will perform some optimizations across a volatile @code{asm} instruction; GCC does not ``forget everything'' when it encounters a volatile @code{asm} instruction the way some other compilers do.

Dave

J. David Anglin                                  [email protected]
National Research Council of Canada              (613) 990-0752 (FAX: 952-6602)
_______________________________________________
parisc-linux mailing list
[email protected]
http://lists.parisc-linux.org/mailman/listinfo/parisc-linux

But that's what placing the mb() after the spinlock code does: confines the critical section and prevents gcc reordering around it.

You probably get slightly more confinement by using mb() instead of the "memory" clobber in __ldcw(). But really it's insignificant. What appeals to me is that the barrier in the spinlocks is explicit. If the spinlocks were fully asm coded, like for example x86, then adding a "memory" clobber would make sense, but they're not, they're coded in C with a little asm help.

James

_______________________________________________
parisc-linux mailing list
[email protected]
http://lists.parisc-linux.org/mailman/listinfo/parisc-linux

Maybe I'm just confused again. I was thinking gcc could move instructions between the asm("ldcw") and mb(). And the same thing between mb() and "lock = 1".

In other words, I'm worried about the "distance" bewteen lock/unlock ops is greater than the "distance" between two mb() that contain the critical section of code.

Erm, how?
Our mb() definition uses asm(:::"memory").
Is that different from using "memory" in the actual bit of assembly?

I just noticed set_eiem(), mtsp() and IRQ support in our system.h do exactly what I proposed.

Yes, me tool.
But I'm suggesting those are superfluous when using "memory" in the core asm() lock operations.

thanks,
grant
_______________________________________________
parisc-linux mailing list
[email protected]
http://lists.parisc-linux.org/mailman/listinfo/parisc-linux

It can, in a restricted fashion (the asm volatile ensures this restriction). But that's OK. What we want is that when we complete the spin_lock code, the spin is locked and memory clobbered. That's why the only necessary mb() is at the end.

Well, the point about using an optimising compiler is that you're supposed to give it the information necessary to do its job. In this case, the correct information is that a is volatile. And further, after it's finished the spin lock code we prevent no reordering leaks and clobber memory.

Because in your implementation you have the barrier in the middle of a loop; that means it's crossed many times in the contended case. In mine it's only crossed once.

James

_______________________________________________
parisc-linux mailing list
[email protected]
http://lists.parisc-linux.org/mailman/listinfo/parisc-linux

yes - I understand that part and agree the current code is correct.

Yeah - there is a tradeoff between optimizing the pipeline between cases where the lock is contended and where it is not. The compiler does not have that info unless it could consume Profile info. (ie "Profile Based Optimization").
I was thinking we should favor the contended case to make it less painful. I've changed my mind and will not push this patch.

Yes - no argument about correctness of current code.

But the _ldcw() is part of a tight "while" loop.
What's the penalty for "crossing" the barrier?
I don't see one.

BTW, do you think we should use coherent loads/stores for locks? That's the other aspect of the patch that I think jda would like to see incorporated.

thanks,
grant
_______________________________________________
parisc-linux mailing list
[email protected]
http://lists.parisc-linux.org/mailman/listinfo/parisc-linux

Erm, no, optimisation of the pipeline is something different again. If we want to hint to the compiler that the lock will be uncontended, then our code should read:

        while (unlikely(__ldcw(a) == 0))
                while (likely(*a == 0));

In this case, probably not much ... it would forbid clever optimisations of the while loops, but I'm sure the compiler isn't actually optimising them very much. However, the *principle* is that you want your barriers where they're effective but do the least damage to the compiler's ability to optimise.

I'm not really sure what the coherent hint actually does.

James

_______________________________________________
parisc-linux mailing list
[email protected]
http://lists.parisc-linux.org/mailman/listinfo/parisc-linux

This is general and applies to all uses of locks.
PBO would determine which is the common path at each usage. But hints are right most of the time - included in the new patch below.

ok. That makes sense. I just didn't want an overly clever compiler doing optimizations which are good for the pipeline but bad for the algorithm.

coherent hint is an architecturally (PA 2.0) defined mechanism to enforce ordering of memory accesses - ie makes memory writes visible to other CPUs in order. This assumes PA2.0 CPU implements weakly ordered memory (which is not the case and I'll continue to assert is unlikely to ever happen). But I'm happy to include this if people think it's the "right thing".

The patch below:
o removes unneeded mb()
o adds likely/unlikely to spinlock loop as suggest by James Bottomley
o adds coherent hints to unlock "store" instruction as suggested by John David Anglin.

(o and drops the "memory" asm directives from the previous version)

thanks,
grant

Index: include/asm-parisc/spinlock.h
===================================================================
RCS file: /var/cvs/linux-2.6/include/asm-parisc/spinlock.h,v retrieving revision 1.6

diff -u -p -r1.6 spinlock.h
@@ -2,6 +2,7 @@
 #define __ASM_SPINLOCK_H
 
 #include <asm/system.h>
+#include <linux/compiler.h>	/* for likely/unlikely ops */
 
 /* Note that PA-RISC has to use `1' to mean unlocked and `0' to mean locked
  * since it only has load-and-zero. Moreover, at least on some PA processors,

@@ -29,20 +30,28 @@ static inline void _raw_spin_lock(spinlo
 {
 	volatile unsigned int *a;
 
-	mb();
 	a = __ldcw_align(x);
-	while (__ldcw(a) == 0)
-		while (*a == 0);
+	while (unlikely(__ldcw(a) == 0))
+		while (likely(*a == 0));
 	mb();
 }
 
 static inline void _raw_spin_unlock(spinlock_t *x)
 {
 	volatile unsigned int *a;
+
 	mb();
 	a = __ldcw_align(x);
-	*a = 1;
-	mb();
+
+	/* use a coherent store. PA1.1 is always strongly ordered.
+	* The idea here is stores to locks will enforce memory ordering
+	* should any PA20 chip ever implement weakly ordered memory.
+	* To date, no PA2.0 implementation is weakly ordered.
+	* jda would prefer we use coherent stores (",ma" with zero offset
+	* is the same thing but PA1.1 compatible).
+	*/
+	__asm__ __volatile__ ("stw,ma %1,0(%0)"
+			: : "r" (a), "r" (1) );
 }
 
 static inline int _raw_spin_trylock(spinlock_t *x)

@@ -50,7 +59,6 @@ static inline int _raw_spin_trylock(spin
 	volatile unsigned int *a;
 	int ret;
 
-	mb();
 	a = __ldcw_align(x);
         ret = __ldcw(a) != 0;
 	mb();

_______________________________________________
parisc-linux mailing list
[email protected]
http://lists.parisc-linux.org/mailman/listinfo/parisc-linux