From 59f4a9f62d1ab9656b02eef240d7aac65669262d Mon Sep 17 00:00:00 2001 From: Gabor Melis Date: Fri, 9 Jan 2009 16:43:56 +0000 Subject: [PATCH] 1.0.24.26: fix release spinlock Both in the runtime and in Lisp releasing a spinlock was a simple assignment. That doesn't work because the new value may not make it to main memory by the time another CPU wants to acquire it making it needlessly slow. Worse, it also allows the CPU to reorder instructions from the critical section after the release. There is a spinlock implementation for MIPS in the runtime, but it's not used as we don't have threads on that platform. I don't know if it's broken too. --- src/code/target-thread.lisp | 11 +++++++++-- src/runtime/x86-64-arch.h | 14 ++++++++------ src/runtime/x86-arch.h | 14 ++++++++------ version.lisp-expr | 2 +- 4 files changed, 26 insertions(+), 15 deletions(-) diff --git a/src/code/target-thread.lisp b/src/code/target-thread.lisp index 3493cfc..fac5220 100644 --- a/src/code/target-thread.lisp +++ b/src/code/target-thread.lisp @@ -218,8 +218,15 @@ in future versions." (defun release-spinlock (spinlock) (declare (optimize (speed 3) (safety 0))) - (setf (spinlock-value spinlock) nil) - nil) + ;; 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))) ;;;; mutexes diff --git a/src/runtime/x86-64-arch.h b/src/runtime/x86-64-arch.h index 4c5d1a7..2538ef7 100644 --- a/src/runtime/x86-64-arch.h +++ b/src/runtime/x86-64-arch.h @@ -49,12 +49,6 @@ get_spinlock(volatile lispobj *word,long value) #endif } -static inline void -release_spinlock(volatile lispobj *word) -{ - *word=0; -} - static inline lispobj swap_lispobjs(volatile lispobj *dest, lispobj value) { @@ -67,4 +61,12 @@ 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 index 1a51e6b..2dfac47 100644 --- a/src/runtime/x86-arch.h +++ b/src/runtime/x86-arch.h @@ -48,12 +48,6 @@ get_spinlock(volatile lispobj *word, unsigned long value) #endif } -static inline void -release_spinlock(volatile lispobj *word) -{ - *word=0; -} - #include static inline lispobj @@ -74,6 +68,14 @@ 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 index 3120660..e844c56 100644 --- 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.25" +"1.0.24.26" -- 1.7.10.4