1.0.24.32: undo parts of 1.0.24.26

No need for memory barriers when unlocking a spinlock on x86/x86-64. The ordering rules and the cache coherency mechanism together guarantee this. However, the compiler must be prevented from reordering instructions with the unlock (at least in one direction). This is now done in the runtime, but not in Lisp as the Lisp compiler does no reordering.
dmitryvk · Jan 11, 2009 · b71ea3a · b71ea3a
1 parent 9bdba41
commit b71ea3a
Show file tree

Hide file tree

Showing 6 changed files with 36 additions and 29 deletions.
diff --git a/src/assembly/x86-64/alloc.lisp b/src/assembly/x86-64/alloc.lisp
@@ -112,6 +112,7 @@
                      (fixnumize 1))
                (storew target other symbol-tls-index-slot other-pointer-lowtag)
                (emit-label release-tls-index-lock)
+               ;; No need for barriers on x86/x86-64 on unlock.
                (store-symbol-value 0 *tls-index-lock*)
                ;; Restore OTHER.
                (inst pop other))

diff --git a/src/assembly/x86/alloc.lisp b/src/assembly/x86/alloc.lisp
@@ -120,6 +120,7 @@
                      (fixnumize 1))
                (storew target other symbol-tls-index-slot other-pointer-lowtag)
                (emit-label release-tls-index-lock)
+               ;; No need for barriers on x86/x86-64 on unlock.
                (store-symbol-value 0 *tls-index-lock*)
                ;; Restore OTHER.
                (inst pop other))

diff --git a/src/code/target-thread.lisp b/src/code/target-thread.lisp
@@ -224,15 +224,18 @@ in future versions."
 
 (defun release-spinlock (spinlock)
   (declare (optimize (speed 3) (safety 0)))
-  ;; Simply setting SPINLOCK-VALUE to NIL is not enough as it does not
-  ;; propagate to other processors, plus without a memory barrier the
-  ;; CPU might reorder instructions allowing code from the critical
-  ;; section to leak out. Use COMPARE-AND-SWAP for the memory barrier
-  ;; effect and do some sanity checking while we are at it.
-  (unless (eq *current-thread*
-              (sb!ext:compare-and-swap (spinlock-value spinlock)
-                                       *current-thread* nil))
-    (error "Only the owner can release the spinlock ~S." spinlock)))
+  ;; On x86 and x86-64 we can get away with no memory barriers, (see
+  ;; Linux kernel mailing list "spin_unlock optimization(i386)"
+  ;; thread, summary at
+  ;; http://kt.iserv.nl/kernel-traffic/kt19991220_47.html#1.
+  ;;
+  ;; If the compiler may reorder this with other instructions, insert
+  ;; compiler barrier here.
+  ;;
+  ;; FIXME: this does not work on SMP Pentium Pro and OOSTORE systems,
+  ;; neither on most non-x86 architectures (but we don't have threads
+  ;; on those).
+  (setf (spinlock-value spinlock) nil))
 
 
 ;;;; Mutexes

diff --git a/src/runtime/x86-64-arch.h b/src/runtime/x86-64-arch.h
@@ -18,7 +18,8 @@
  * here? (The answer wasn't obvious to me when merging the
  * architecture-abstracting patches for CSR's SPARC port. -- WHN 2002-02-15) */
 
-#include "interr.h"
+#define COMPILER_BARRIER \
+    do { __asm__ __volatile__ ( "" : : : "memory"); } while (0)
 
 static inline void
 get_spinlock(volatile lispobj *word,long value)
@@ -49,6 +50,15 @@ get_spinlock(volatile lispobj *word,long value)
 #endif
 }
 
+static inline void
+release_spinlock(volatile lispobj *word)
+{
+    /* See comment in RELEASE-SPINLOCK in target-thread.lisp. */
+    COMPILER_BARRIER;
+    *word=0;
+    COMPILER_BARRIER;
+}
+
 static inline lispobj
 swap_lispobjs(volatile lispobj *dest, lispobj value)
 {
@@ -61,12 +71,4 @@ swap_lispobjs(volatile lispobj *dest, lispobj value)
     return old_value;
 }
 
-static inline void
-release_spinlock(volatile lispobj *word)
-{
-    /* A memory barrier is needed, use swap_lispobjs. See comment in
-     * RELEASE-SPINLOCK in target-thread.lisp. */
-    swap_lispobjs(word,0);
-}
-
 #endif /* _X86_64_ARCH_H */
diff --git a/src/runtime/x86-arch.h b/src/runtime/x86-arch.h
@@ -18,7 +18,8 @@
  * here? (The answer wasn't obvious to me when merging the
  * architecture-abstracting patches for CSR's SPARC port. -- WHN 2002-02-15) */
 
-#include "interr.h"
+#define COMPILER_BARRIER \
+    do { __asm__ __volatile__ ( "" : : : "memory"); } while (0)
 
 static inline void
 get_spinlock(volatile lispobj *word, unsigned long value)
@@ -48,7 +49,14 @@ get_spinlock(volatile lispobj *word, unsigned long value)
 #endif
 }
 
-#include <stdio.h>
+static inline void
+release_spinlock(volatile lispobj *word)
+{
+    /* See comment in RELEASE-SPINLOCK in target-thread.lisp. */
+    COMPILER_BARRIER;
+    *word=0;
+    COMPILER_BARRIER;
+}
 
 static inline lispobj
 swap_lispobjs(volatile lispobj *dest, lispobj value)
@@ -68,14 +76,6 @@ swap_lispobjs(volatile lispobj *dest, lispobj value)
     return old_value;
 }
 
-static inline void
-release_spinlock(volatile lispobj *word)
-{
-    /* A memory barrier is needed, use swap_lispobjs. See comment in
-     * RELEASE-SPINLOCK in target-thread.lisp. */
-    swap_lispobjs(word,0);
-}
-
 extern void fast_bzero_detect(void *, size_t);
 extern void (*fast_bzero_pointer)(void *, size_t);
 

diff --git a/version.lisp-expr b/version.lisp-expr
@@ -17,4 +17,4 @@
 ;;; checkins which aren't released. (And occasionally for internal
 ;;; versions, especially for internal versions off the main CVS
 ;;; branch, it gets hairier, e.g. "0.pre7.14.flaky4.13".)
-"1.0.24.31"
+"1.0.24.32"