1.0.10.29: MUTEX refactoring & optimization
authorNikodemus Siivola <nikodemus@random-state.net>
Fri, 5 Oct 2007 14:48:40 +0000 (14:48 +0000)
committerNikodemus Siivola <nikodemus@random-state.net>
Fri, 5 Oct 2007 14:48:40 +0000 (14:48 +0000)
 * (SETF MUTEX-VALUE) removed entirely. Using it was never sane,
   and in the new setup entirely broken.

 * On futex platforms, keep track of state of the mutex: free, taken,
   or contested (taken and may have one ore more threads waiting).

 * On release, if the mutex is not contested, there is no need to
   perform the wakeup.

 * Document assumptions made by GET-MUTEX & RELEASE-MUTEX better.

 * Update FASL version.

NEWS
src/code/early-fasl.lisp
src/code/gc.lisp
src/code/target-thread.lisp
src/code/thread.lisp
version.lisp-expr

diff --git a/NEWS b/NEWS
index 7ec96bc..e6b1d1e 100644 (file)
--- a/NEWS
+++ b/NEWS
@@ -7,6 +7,8 @@ changes in sbcl-1.0.11 relative to sbcl-1.0.10:
     locking at the correct granularity. In the current implementation it is
     still safe to have multiple readers access the same table, but it's not
     guaranteed that this property will be maintained in future releases.
+  * minor incompatible change: (SETF MUTEX-VALUE) is not longer supported,
+    and will signal an error at runtime.
   * enhancement: SB-THREAD package now exports a semaphore interface.
   * enhancement: CONS can now stack-allocate on x86 and
     x86-64. (Earlier LIST and LIST* supported stack-allocation, but
@@ -16,6 +18,8 @@ changes in sbcl-1.0.11 relative to sbcl-1.0.10:
   * enhancement: dynamic-extent support has been extended to support
     cases where there are multiple possible sources for the stack
     allocated value.
+  * optimization: RELEASE-MUTEX no longer needs to perform a syscall
+    if the mutex is uncontested on Linux.
   * bug fix: symbol-macro expansion now uses the *MACROEXPAND-HOOK*
     as specified by the CLHS. (thanks to Tobias Rittweiler)
 
index a051605..f65a958 100644 (file)
@@ -76,7 +76,7 @@
 ;;; versions which break binary compatibility. But it certainly should
 ;;; be incremented for release versions which break binary
 ;;; compatibility.
-(def!constant +fasl-file-version+ 75)
+(def!constant +fasl-file-version+ 76)
 ;;; (description of versions before 0.9.0.1 deleted in 0.9.17)
 ;;; 56: (2005-05-22) Something between 0.9.0.1 and 0.9.0.14. My money is
 ;;;     on 0.9.0.6 (MORE CASE CONSISTENCY).
 ;;; 73: (2007-04-13) Changed a hash function
 ;;; 74: (2007-06-05) UNWIND-TO-FRAME-AND-CALL
 ;;; 75: (2007-08-06) FD-STREAM layout changes
+;;; 76: (2007-05-10) MUTEX layout changes
 
 ;;; the conventional file extension for our fasl files
 (declaim (type simple-string *fasl-file-type*))
index bc6f6cd..7ba3b3b 100644 (file)
@@ -198,7 +198,7 @@ run in any thread.")
 
 (defun sub-gc (&key (gen 0))
   (unless (eq sb!thread:*current-thread*
-              (sb!thread::mutex-value *already-in-gc*))
+              (sb!thread:mutex-value *already-in-gc*))
     ;; With gencgc, unless *GC-PENDING* every allocation in this
     ;; function triggers another gc, potentially exceeding maximum
     ;; interrupt nesting. If *GC-INHIBIT* is not true, however,
index 1509e01..ec94be8 100644 (file)
@@ -215,38 +215,48 @@ in future versions."
 (setf (fdocumentation 'make-mutex 'function)
       "Create a mutex."
       (fdocumentation 'mutex-name 'function)
-      "The name of the mutex. Setfable."
-      (fdocumentation 'mutex-value 'function)
-      "The value of the mutex. NIL if the mutex is free. Setfable.")
+      "The name of the mutex. Setfable.")
 
 #!+(and sb-thread (not sb-lutex))
-(define-structure-slot-addressor mutex-value-address
+(progn
+  (define-structure-slot-addressor mutex-state-address
       :structure mutex
-      :slot value)
-
-(defun get-mutex (mutex &optional (new-value *current-thread*) (waitp t))
+      :slot state)
+  ;; Important: current code assumes these are fixnums or other
+  ;; lisp objects that don't need pinning.
+  (defconstant +lock-free+ 0)
+  (defconstant +lock-taken+ 1)
+  (defconstant +lock-contested+ 2))
+
+(defun get-mutex (mutex &optional (new-owner *current-thread*) (waitp t))
   #!+sb-doc
-  "Acquire MUTEX, setting it to NEW-VALUE or some suitable default value if
-NIL. If WAITP is non-NIL and the mutex is in use, sleep until it is available."
+  "Acquire MUTEX for NEW-OWNER, which must be a thread or NIL. If
+NEW-OWNER is NIL, it defaults to the current thread. If WAITP is
+non-NIL and the mutex is in use, sleep until it is available.
+
+Note: using GET-MUTEX to assign a MUTEX to another thread then the
+current one is not recommended, and liable to be deprecated.
+
+GET-MUTEX is not interrupt safe. The correct way to call it is:
+
+ (WITHOUT-INTERRUPTS
+   ...
+   (ALLOW-WITH-INTERRUPTS (GET-MUTEX ...))
+   ...)
+
+WITHOUT-INTERRUPTS is necessary to avoid an interrupt unwinding the
+call while the mutex is in an inconsistent state while
+ALLOW-WITH-INTERRUPTS allows the call to be interrupted from sleep.
+
+It is recommended that you use WITH-MUTEX instead of calling GET-MUTEX
+directly."
   (declare (type mutex mutex) (optimize (speed 3)))
-  (/show0 "Entering GET-MUTEX")
-  (unless new-value
-    (setq new-value *current-thread*))
-  #!-sb-thread
-  (let ((old (mutex-value mutex)))
-    (when (and old waitp)
-      (error "In unithread mode, mutex ~S was requested with WAITP ~S and ~
-              new-value ~S, but has already been acquired (with value ~S)."
-             mutex waitp new-value old))
-    (setf (mutex-value mutex) new-value)
-    t)
+  (unless new-owner
+    (setq new-owner *current-thread*))
+  (when (eql new-owner (mutex-%owner mutex))
+    (error "Recursive lock attempt ~S." mutex))
   #!+sb-thread
   (progn
-    (when (eql new-value (mutex-value mutex))
-      (warn "recursive lock attempt ~S~%" mutex)
-      (format *debug-io* "Thread: ~A~%" *current-thread*)
-      (sb!debug:backtrace most-positive-fixnum *debug-io*)
-      (force-output *debug-io*))
     ;; FIXME: Lutexes do not currently support deadlines, as at least
     ;; on Darwin pthread_foo_timedbar functions are not supported:
     ;; this means that we probably need to use the Carbon multiprocessing
@@ -255,42 +265,80 @@ NIL. If WAITP is non-NIL and the mutex is in use, sleep until it is available."
     ;; FIXME: This is definitely not interrupt safe: what happens if
     ;; we get hit (1) during the lutex calls (ok, they may be safe,
     ;; but has that been checked?) (2) after the lutex call, but
-    ;; before setting the mutex value.
+    ;; before setting the mutex owner.
     #!+sb-lutex
     (when (zerop (with-lutex-address (lutex (mutex-lutex mutex))
                    (if waitp
                        (with-interrupts (%lutex-lock lutex))
                        (%lutex-trylock lutex))))
-      (setf (mutex-value mutex) new-value))
+      (setf (mutex-%owner mutex) new-owner)
+      t)
     #!-sb-lutex
-    (let (old)
-      (when (and (setf old (sb!ext:compare-and-swap (mutex-value mutex) nil new-value))
-                 waitp)
-        (loop while old
-              do (multiple-value-bind (to-sec to-usec) (decode-timeout nil)
-                   (when (= 1 (with-pinned-objects (mutex old)
-                                (futex-wait (mutex-value-address mutex)
-                                            (get-lisp-obj-address old)
-                                            (or to-sec -1)
-                                            (or to-usec 0))))
-                     (signal-deadline)))
-              (setf old (sb!ext:compare-and-swap (mutex-value mutex) nil new-value))))
-      (not old))))
+    (let ((old (sb!ext:compare-and-swap (mutex-state mutex)
+                                        +lock-free+
+                                        +lock-taken+)))
+      (unless (or (eql +lock-free+ old) (not waitp))
+        (tagbody
+         :retry
+           (when (or (eql +lock-contested+ old)
+                     (not (eql +lock-free+
+                               (sb!ext:compare-and-swap (mutex-state mutex)
+                                                        +lock-taken+
+                                                        +lock-contested+))))
+             ;; Wait on the contested lock.
+             (multiple-value-bind (to-sec to-usec) (decode-timeout nil)
+               (when (= 1 (with-pinned-objects (mutex)
+                            (futex-wait (mutex-state-address mutex)
+                                        (get-lisp-obj-address +lock-contested+)
+                                        (or to-sec -1)
+                                        (or to-usec 0))))
+                 (signal-deadline))))
+           (setf old (sb!ext:compare-and-swap (mutex-state mutex)
+                                              +lock-free+
+                                              +lock-contested+))
+           ;; Did we get it?
+           (unless (eql +lock-free+ old)
+             (go :retry))))
+      (cond ((eql +lock-free+ old)
+             (let ((prev (sb!ext:compare-and-swap (mutex-%owner mutex)
+                                                  nil new-owner)))
+               (when prev
+                 (bug "Old owner in free mutex: ~S" prev))
+               t))
+            (waitp
+             (bug "Failed to acquire lock with WAITP."))))))
 
 (defun release-mutex (mutex)
   #!+sb-doc
   "Release MUTEX by setting it to NIL. Wake up threads waiting for
-this mutex."
+this mutex.
+
+RELEASE-MUTEX is not interrupt safe: interrupts should be disabled
+around calls to it.
+
+Signals a WARNING is current thread is not the current owner of the
+mutex."
   (declare (type mutex mutex))
-  (/show0 "Entering RELEASE-MUTEX")
-  (setf (mutex-value mutex) nil)
+  ;; Order matters: set owner to NIL before releasing state.
+  (let* ((self *current-thread*)
+         (old-owner (sb!ext:compare-and-swap (mutex-%owner mutex) self nil)))
+    (unless  (eql self old-owner)
+      (warn "Releasing ~S, owned by another thread: ~S" mutex old-owner)
+      (setf (mutex-%owner mutex) nil)))
   #!+sb-thread
   (progn
     #!+sb-lutex
     (with-lutex-address (lutex (mutex-lutex mutex))
       (%lutex-unlock lutex))
     #!-sb-lutex
-    (futex-wake (mutex-value-address mutex) 1)))
+    (let ((old (sb!ext:compare-and-swap (mutex-state mutex)
+                                        +lock-taken+ +lock-free+)))
+      (when (eql old +lock-contested+)
+        (sb!ext:compare-and-swap (mutex-state mutex)
+                                 +lock-contested+ +lock-free+)
+        (with-pinned-objects (mutex)
+          (futex-wake (mutex-state-address mutex) 1))))
+    nil))
 
 ;;;; waitqueues/condition variables
 
@@ -326,16 +374,16 @@ time we reacquire MUTEX and return to the caller."
   (assert mutex)
   #!-sb-thread (error "Not supported in unithread builds.")
   #!+sb-thread
-  (let ((value (mutex-value mutex)))
+  (let ((owner (mutex-%owner mutex)))
     (/show0 "CONDITION-WAITing")
     #!+sb-lutex
     (progn
       ;; FIXME: This doesn't look interrupt safe!
-      (setf (mutex-value mutex) nil)
+      (setf (mutex-%owner mutex) nil)
       (with-lutex-address (queue-lutex-address (waitqueue-lutex queue))
         (with-lutex-address (mutex-lutex-address (mutex-lutex mutex))
           (%lutex-wait queue-lutex-address mutex-lutex-address)))
-      (setf (mutex-value mutex) value))
+      (setf (mutex-%owner mutex) owner))
     #!-sb-lutex
     (unwind-protect
          (let ((me *current-thread*))
@@ -360,7 +408,7 @@ time we reacquire MUTEX and return to the caller."
       ;; before returning.  Ideally, in the case of an unhandled signal,
       ;; we should do them before entering the debugger, but this is
       ;; better than nothing.
-      (get-mutex mutex value))))
+      (get-mutex mutex owner))))
 
 (defun condition-notify (queue &optional (n 1))
   #!+sb-doc
index 01bbac7..1d751ac 100644 (file)
   #!+sb-doc
   "Mutex type."
   (name nil :type (or null simple-string))
-  (value nil)
+  (%owner nil :type (or null thread))
+  #!+(and (not sb-lutex) sb-thread)
+  (state 0 :type fixnum)
   #!+(and sb-lutex sb-thread)
   (lutex (make-lutex)))
 
+;;; FIXME: We probably want to rename the accessor MUTEX-OWNER.
+(defun mutex-value (mutex)
+  "Current owner of the mutex, NIL if the mutex is free."
+  (mutex-%owner mutex))
+
+(defsetf mutex-value set-mutex-value)
+
+(declaim (inline set-mutex-value))
+(defun set-mutex-value (mutex value)
+  (declare (ignore mutex value))
+  (error "~S is no longer supported." '(setf mutex-value)))
+
+(define-compiler-macro set-mutex-value (&whole form mutex value)
+  (declare (ignore mutex value))
+  (warn "~S is no longer supported, and will signal an error at runtime."
+        '(setf mutex-value))
+  form)
+
 (def!struct spinlock
   #!+sb-doc
   "Spinlock type."
@@ -203,7 +223,7 @@ provided the default value is used for the mutex."
 
   (defun call-with-recursive-lock (function mutex)
     (declare (function function))
-    (dx-let ((inner-lock-p (eq (mutex-value mutex) *current-thread*))
+    (dx-let ((inner-lock-p (eq (mutex-%owner mutex) *current-thread*))
              (got-it nil))
       (without-interrupts
         (unwind-protect
index 8f1c3d1..cb40691 100644 (file)
@@ -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.10.28"
+"1.0.10.29"