Start locking up the active and dead threads lists. The active threads

list is protected by a spinlock_t, but the dead list uses a pthread_mutex
because it is necessary to synchronize other threads with the garbage
collector thread. Lock/Unlock macros are used so it's easier to make
changes to the locks in the future.

The 'dead thread list' lock is intended to replace the gc mutex.
This doesn't have any practical ramifications. It simply makes it
clearer what the purpose of the lock is. The gc will use this lock,
instead of the gc mutex, to synchronize access to the dead list with
other threads.

Modify _pthread_exit() to use these two new locks instead of GIANT_LOCK,
and also to properly lock and protect thread state changes,
especially with respect to a joining thread.

The gc thread was also re-arranged to be more organized and less nested.

_pthread_join() was also modified to use the thread list locks. However,
locking and unlocking here needs special care because a thread could find
itself in a position where it's joining an exiting thread that is
waiting on the dead list lock, which this thread (joiner) holds. If the
joiner doesn't take care to lock *and* unlock in the same order they
(the joiner and the joinee) could deadlock against each other.

Approved by:	re/blanket libthr

7 files changed, 135 insertions, 127 deletions

diff --git a/lib/libthr/thread/thr_exit.c b/lib/libthr/thread/thr_exit.c
index 118876f..8c4a060 100644
--- a/lib/libthr/thread/thr_exit.c
+++ b/lib/libthr/thread/thr_exit.c
@@ -95,6 +95,7 @@ void
 _pthread_exit(void *status)
 {
 	pthread_t pthread;
+	int exitNow = 0;
 
 	/* Check if this thread is already in the process of exiting: */
 	if ((curthread->flags & PTHREAD_EXITING) != 0) {
@@ -121,37 +122,14 @@ _pthread_exit(void *status)
 		_thread_cleanupspecific();
 	}
 
-	/*
-	 * Lock the garbage collector mutex to ensure that the garbage
-	 * collector is not using the dead thread list.
-	 */
-	if (pthread_mutex_lock(&_gc_mutex) != 0)
-		PANIC("Cannot lock gc mutex");
-
-	/* Add this thread to the list of dead threads. */
-	TAILQ_INSERT_HEAD(&_dead_list, curthread, dle);
-
-	/*
-	 * Signal the garbage collector thread that there is something
-	 * to clean up.
-	 */
-	if (pthread_cond_signal(&_gc_cond) != 0)
-		PANIC("Cannot signal gc cond");
-
-	/*
-	 * Avoid a race condition where a scheduling signal can occur
-	 * causing the garbage collector thread to run.  If this happens,
-	 * the current thread can be cleaned out from under us.
-	 */
-	GIANT_LOCK(curthread);
-
-	/* Unlock the garbage collector mutex. */
-	if (pthread_mutex_unlock(&_gc_mutex) != 0)
-		PANIC("Cannot unlock gc mutex");
+	/* Lock the dead list first to maintain correct lock order */
+	DEAD_LIST_LOCK;
+	_thread_critical_enter(curthread);
 
 	/* Check if there is a thread joining this one: */
 	if (curthread->joiner != NULL) {
 		pthread = curthread->joiner;
+		_SPINLOCK(&pthread->lock);
 		curthread->joiner = NULL;
 
 		/* Make the joining thread runnable: */
@@ -161,6 +139,7 @@ _pthread_exit(void *status)
 		pthread->join_status.ret = curthread->ret;
 		pthread->join_status.error = 0;
 		pthread->join_status.thread = NULL;
+		_SPINUNLOCK(&pthread->lock);
 
 		/* Make this thread collectable by the garbage collector. */
 		PTHREAD_ASSERT(((curthread->attr.flags & PTHREAD_DETACHED) ==
@@ -168,14 +147,31 @@ _pthread_exit(void *status)
 		curthread->attr.flags |= PTHREAD_DETACHED;
 	}
 
-	/* Remove this thread from the thread list: */
+	/*
+	 * Add this thread to the list of dead threads, and
+	 * also remove it from the active threads list.
+	 */
+	THREAD_LIST_LOCK;
+	TAILQ_INSERT_HEAD(&_dead_list, curthread, dle);
 	TAILQ_REMOVE(&_thread_list, curthread, tle);
-
 	PTHREAD_SET_STATE(curthread, PS_DEAD);
-	GIANT_UNLOCK(curthread);
+	_thread_critical_exit(curthread);
+	
+	/*
+	 * Signal the garbage collector thread that there is something
+	 * to clean up.
+	 */
+	if (pthread_cond_signal(&_gc_cond) != 0)
+		PANIC("Cannot signal gc cond");
 
 	/* If we're the last thread, call it quits */
 	if (TAILQ_EMPTY(&_thread_list))
+		exitNow = 1;
+
+	THREAD_LIST_UNLOCK;
+	DEAD_LIST_UNLOCK;
+
+	if (exitNow)
 		exit(0);
 
 	/*
diff --git a/lib/libthr/thread/thr_gc.c b/lib/libthr/thread/thr_gc.c
index e62c50b..d491bff 100644
--- a/lib/libthr/thread/thr_gc.c
+++ b/lib/libthr/thread/thr_gc.c
@@ -75,19 +75,17 @@ _thread_gc(pthread_addr_t arg)
 			_thread_dump_info();
 
 		/*
-		 * Lock the garbage collector mutex which ensures that
+		 * Lock the list of dead threads which ensures that
 		 * this thread sees another thread exit:
 		 */
-		if (pthread_mutex_lock(&_gc_mutex) != 0)
-			PANIC("Cannot lock gc mutex");
+		DEAD_LIST_LOCK;
 
 		/* No stack or thread structure to free yet. */
 		p_stack = NULL;
 		pthread_cln = NULL;
 
-		GIANT_LOCK(curthread);
-
 		/* Check if this is the last running thread. */
+		THREAD_LIST_LOCK;
 		if (TAILQ_FIRST(&_thread_list) == curthread &&
 		    TAILQ_NEXT(curthread, tle) == NULL)
 			/*
@@ -95,6 +93,7 @@ _thread_gc(pthread_addr_t arg)
 			 * now.
 			 */
 			f_done = 1;
+		THREAD_LIST_UNLOCK;
 
 		/*
 		 * Enter a loop to search for the first dead thread that
@@ -106,57 +105,43 @@ _thread_gc(pthread_addr_t arg)
 			/* Don't destroy the initial thread. */
 			if (pthread == _thread_initial) 
 				continue;
+
+			_SPINLOCK(&pthread->lock);
+
 			/*
-			 * Check if this thread has detached:
+			 * Check if the stack was not specified by
+			 * the caller to pthread_create() and has not
+			 * been destroyed yet: 
 			 */
-			if ((pthread->attr.flags &
-			    PTHREAD_DETACHED) != 0) {
-				/* Remove this thread from the dead list: */
-				TAILQ_REMOVE(&_dead_list, pthread, dle);
-
-				/*
-				 * Check if the stack was not specified by
-				 * the caller to pthread_create() and has not
-				 * been destroyed yet: 
-				 */
-				if (pthread->attr.stackaddr_attr == NULL &&
-				    pthread->stack != NULL) {
-					_thread_stack_free(pthread->stack,
-					    pthread->attr.stacksize_attr,
-					    pthread->attr.guardsize_attr);
-				}
-
-				/*
-				 * Point to the thread structure that must
-				 * be freed outside the locks:
-				 */
-				pthread_cln = pthread;
-
-			} else {
-				/*
-				 * This thread has not detached, so do
-				 * not destroy it.
-				 *
-				 * Check if the stack was not specified by
-				 * the caller to pthread_create() and has not
-				 * been destroyed yet: 
-				 */
-				if (pthread->attr.stackaddr_attr == NULL &&
-				    pthread->stack != NULL) {
-					_thread_stack_free(pthread->stack,
-					    pthread->attr.stacksize_attr,
-					    pthread->attr.guardsize_attr);
-
-					/*
-					 * NULL the stack pointer now that the
-					 * memory has been freed:
-					 */
-					pthread->stack = NULL;
-				}
+			if (pthread->attr.stackaddr_attr == NULL &&
+			    pthread->stack != NULL) {
+				_thread_stack_free(pthread->stack,
+				    pthread->attr.stacksize_attr,
+				    pthread->attr.guardsize_attr);
+				pthread->stack = NULL;
 			}
+
+			/*
+			 * If the thread has not been detached, leave
+			 * it on the dead thread list.
+			 */
+			if ((pthread->attr.flags & PTHREAD_DETACHED) == 0) {
+				_SPINUNLOCK(&pthread->lock);
+				continue;
+			}
+
+			/* Remove this thread from the dead list: */
+			TAILQ_REMOVE(&_dead_list, pthread, dle);
+
+			/*
+			 * Point to the thread structure that must
+			 * be freed outside the locks:
+			 */
+			pthread_cln = pthread;
+
+			_SPINUNLOCK(&pthread->lock);
 		}
 
-		GIANT_UNLOCK(curthread);
 		/*
 		 * Check if this is not the last thread and there is no
 		 * memory to free this time around.
@@ -177,15 +162,14 @@ _thread_gc(pthread_addr_t arg)
 			 * timeout (for a backup poll).
 			 */
 			if ((ret = pthread_cond_timedwait(&_gc_cond,
-			    &_gc_mutex, &abstime)) != 0 && ret != ETIMEDOUT) {
+			    &dead_list_lock, &abstime)) != 0 && ret != ETIMEDOUT) {
 				_thread_printf(STDERR_FILENO, "ret = %d", ret);
 				PANIC("gc cannot wait for a signal");
 			}
 		}
 
 		/* Unlock the garbage collector mutex: */
-		if (pthread_mutex_unlock(&_gc_mutex) != 0)
-			PANIC("Cannot unlock gc mutex");
+		DEAD_LIST_UNLOCK;
 
 		/*
 		 * If there is memory to free, do it now. The call to
diff --git a/lib/libthr/thread/thr_info.c b/lib/libthr/thread/thr_info.c
index b76d8e5..b0fae83 100644
--- a/lib/libthr/thread/thr_info.c
+++ b/lib/libthr/thread/thr_info.c
@@ -109,6 +109,7 @@ _thread_dump_info(void)
 		}
 
 		/* Check if there are no dead threads: */
+		DEAD_LIST_LOCK;
 		if (TAILQ_FIRST(&_dead_list) == NULL) {
 			/* Output a record: */
 			strcpy(s, "\n\nTHERE ARE NO DEAD THREADS\n");
@@ -126,6 +127,7 @@ _thread_dump_info(void)
 				dump_thread(fd, pthread, /*long_version*/ 0);
 			}
 		}
+		DEAD_LIST_UNLOCK;
 
 		/* Close the dump file: */
 		__sys_close(fd);
diff --git a/lib/libthr/thread/thr_init.c b/lib/libthr/thread/thr_init.c
index 3b59da4..98b0eef 100644
--- a/lib/libthr/thread/thr_init.c
+++ b/lib/libthr/thread/thr_init.c
@@ -326,7 +326,7 @@ _thread_init(void)
 		    clockinfo.tick : CLOCK_RES_USEC_MIN;
 
 	/* Initialise the garbage collector mutex and condition variable. */
-	if (_pthread_mutex_init(&_gc_mutex,NULL) != 0 ||
+	if (_pthread_mutex_init(&dead_list_lock,NULL) != 0 ||
 	    _pthread_cond_init(&_gc_cond,NULL) != 0)
 		PANIC("Failed to initialise garbage collector mutex or condvar");
 }
diff --git a/lib/libthr/thread/thr_join.c b/lib/libthr/thread/thr_join.c
index 35b52bc..061058a 100644
--- a/lib/libthr/thread/thr_join.c
+++ b/lib/libthr/thread/thr_join.c
@@ -60,38 +60,36 @@ _pthread_join(pthread_t pthread, void **thread_return)
 	}
 
 	/*
-	 * Lock the garbage collector mutex to ensure that the garbage
-	 * collector is not using the dead thread list.
-	 */
-	if (pthread_mutex_lock(&_gc_mutex) != 0)
-		PANIC("Cannot lock gc mutex");
-
-	GIANT_LOCK(curthread);
-
-	/*
 	 * Search for the specified thread in the list of active threads.  This
 	 * is done manually here rather than calling _find_thread() because
 	 * the searches in _thread_list and _dead_list (as well as setting up
 	 * join/detach state) have to be done atomically.
 	 */
+	DEAD_LIST_LOCK;
+	THREAD_LIST_LOCK;
 	TAILQ_FOREACH(thread, &_thread_list, tle)
-		if (thread == pthread)
+		if (thread == pthread) {
+			_SPINLOCK(&pthread->lock);
 			break;
+		}
 
 	if (thread == NULL)
 		/*
 		 * Search for the specified thread in the list of dead threads:
 		 */
 		TAILQ_FOREACH(thread, &_dead_list, dle)
-			if (thread == pthread)
+			if (thread == pthread) {
+				_SPINLOCK(&pthread->lock);
 				break;
-
+			}
+	THREAD_LIST_UNLOCK;
 
 	/* Check if the thread was not found or has been detached: */
 	if (thread == NULL ||
 	    ((pthread->attr.flags & PTHREAD_DETACHED) != 0)) {
-		if (pthread_mutex_unlock(&_gc_mutex) != 0)
-			PANIC("Cannot lock gc mutex");
+		if (thread != NULL)
+			_SPINUNLOCK(&pthread->lock);
+		DEAD_LIST_UNLOCK;
 		ret = ESRCH;
 		goto out;
 
@@ -99,33 +97,32 @@ _pthread_join(pthread_t pthread, void **thread_return)
 	if (pthread->joiner != NULL) {
 		/* Multiple joiners are not supported. */
 		/* XXXTHR - support multiple joiners. */
-		if (pthread_mutex_unlock(&_gc_mutex) != 0)
-			PANIC("Cannot lock gc mutex");
+		_SPINUNLOCK(&pthread->lock);
+		DEAD_LIST_UNLOCK;
 		ret = ENOTSUP;
 		goto out;
 
 	}
-	/*
-	 * Unlock the garbage collector mutex, now that the garbage collector
-	 * can't be run:
-	 */
-	if (pthread_mutex_unlock(&_gc_mutex) != 0)
-		PANIC("Cannot lock gc mutex");
 
 	/* Check if the thread is not dead: */
 	if (pthread->state != PS_DEAD) {
+		_thread_critical_enter(curthread);
 		/* Set the running thread to be the joiner: */
 		pthread->joiner = curthread;
 
 		/* Keep track of which thread we're joining to: */
 		curthread->join_status.thread = pthread;
+		_SPINUNLOCK(&pthread->lock);
 
 		while (curthread->join_status.thread == pthread) {
 			PTHREAD_SET_STATE(curthread, PS_JOIN);
 			/* Wait for our signal to wake up. */
-			GIANT_UNLOCK(curthread);
+			_thread_critical_exit(curthread);
+			DEAD_LIST_UNLOCK;
 			_thread_suspend(curthread, NULL);
-			GIANT_LOCK(curthread);
+			/* XXX - For correctness reasons. */
+			DEAD_LIST_LOCK;
+			_thread_critical_enter(curthread);
 		}
 
 		/*
@@ -135,6 +132,15 @@ _pthread_join(pthread_t pthread, void **thread_return)
 		ret = curthread->join_status.error;
 		if ((ret == 0) && (thread_return != NULL))
 			*thread_return = curthread->join_status.ret;
+		_thread_critical_exit(curthread);
+		/*
+		 * XXX - Must unlock here, instead of doing it earlier,
+		 *	 because it could lead to a deadlock. If the thread
+		 *	 we are joining is waiting on this lock we would
+		 *	 deadlock if we released this lock before unlocking the
+		 *	 joined thread.
+		 */
+		DEAD_LIST_UNLOCK;
 	} else {
 		/*
 		 * The thread exited (is dead) without being detached, and no
@@ -149,12 +155,13 @@ _pthread_join(pthread_t pthread, void **thread_return)
 
 		/* Make the thread collectable by the garbage collector. */
 		pthread->attr.flags |= PTHREAD_DETACHED;
-
+		_SPINUNLOCK(&pthread->lock);
+		if (pthread_cond_signal(&_gc_cond) != 0)
+			PANIC("Cannot signal gc cond");
+		DEAD_LIST_UNLOCK;
 	}
 
 out:
-	GIANT_UNLOCK(curthread);
-
 	_thread_leave_cancellation_point();
 
 	/* Return the completion status: */
diff --git a/lib/libthr/thread/thr_private.h b/lib/libthr/thread/thr_private.h
index 6048bd0..3668a0e 100644
--- a/lib/libthr/thread/thr_private.h
+++ b/lib/libthr/thread/thr_private.h
@@ -594,6 +594,34 @@ SCLASS TAILQ_HEAD(, pthread) _dead_list
 ;
 #endif
 
+/*
+ * These two locks protect the global active threads list and
+ * the global dead threads list, respectively. Combining these
+ * into one lock for both lists doesn't seem wise, since it
+ * would likely increase contention during busy thread creation
+ * and destruction for very little savings in space.
+ *
+ * The lock for the "dead threads list" must be a pthread mutex
+ * because it is used with condition variables to synchronize
+ * the gc thread with active threads in the process of exiting or
+ * dead threads who have just been joined.
+ */
+SCLASS spinlock_t thread_list_lock
+#ifdef GLOBAL_PTHREAD_PRIVATE
+= _SPINLOCK_INITIALIZER
+#endif 
+;
+SCLASS pthread_mutex_t dead_list_lock
+#ifdef GLOBAL_PTHREAD_PRIVATE
+= NULL
+#endif
+;
+
+#define THREAD_LIST_LOCK	_SPINLOCK(&thread_list_lock)
+#define THREAD_LIST_UNLOCK	_SPINUNLOCK(&thread_list_lock)
+#define DEAD_LIST_LOCK		_pthread_mutex_lock(&dead_list_lock)
+#define DEAD_LIST_UNLOCK	_pthread_mutex_unlock(&dead_list_lock)
+
 /* Initial thread: */
 SCLASS struct pthread *_thread_initial
 #ifdef GLOBAL_PTHREAD_PRIVATE
@@ -644,12 +672,7 @@ SCLASS struct umtx _giant_mutex
 
 SCLASS int _giant_count;
 
-/* Garbage collector mutex and condition variable. */
-SCLASS	pthread_mutex_t _gc_mutex
-#ifdef GLOBAL_PTHREAD_PRIVATE
-= NULL
-#endif
-;
+/* Garbage collector condition variable. */
 SCLASS	pthread_cond_t  _gc_cond
 #ifdef GLOBAL_PTHREAD_PRIVATE
 = NULL
diff --git a/lib/libthr/thread/thr_stack.c b/lib/libthr/thread/thr_stack.c
index c75d6ee..28b396b 100644
--- a/lib/libthr/thread/thr_stack.c
+++ b/lib/libthr/thread/thr_stack.c
@@ -144,8 +144,7 @@ _thread_stack_alloc(size_t stacksize, size_t guardsize)
 		 * Use the garbage collector mutex for synchronization of the
 		 * spare stack list.
 		 */
-		if (pthread_mutex_lock(&_gc_mutex) != 0)
-			PANIC("Cannot lock gc mutex");
+		DEAD_LIST_LOCK;
 
 		if ((spare_stack = LIST_FIRST(&_dstackq)) != NULL) {
 				/* Use the spare stack. */
@@ -154,8 +153,7 @@ _thread_stack_alloc(size_t stacksize, size_t guardsize)
 		}
 
 		/* Unlock the garbage collector mutex. */
-		if (pthread_mutex_unlock(&_gc_mutex) != 0)
-			PANIC("Cannot unlock gc mutex");
+		DEAD_LIST_UNLOCK;
 	}
 	/*
 	 * The user specified a non-default stack and/or guard size, so try to
@@ -167,8 +165,7 @@ _thread_stack_alloc(size_t stacksize, size_t guardsize)
 		 * Use the garbage collector mutex for synchronization of the
 		 * spare stack list.
 		 */
-		if (pthread_mutex_lock(&_gc_mutex) != 0)
-			PANIC("Cannot lock gc mutex");
+		DEAD_LIST_LOCK;
 
 		LIST_FOREACH(spare_stack, &_mstackq, qe) {
 			if (spare_stack->stacksize == stack_size &&
@@ -180,8 +177,7 @@ _thread_stack_alloc(size_t stacksize, size_t guardsize)
 		}
 
 		/* Unlock the garbage collector mutex. */
-		if (pthread_mutex_unlock(&_gc_mutex) != 0)
-			PANIC("Cannot unlock gc mutex");
+		DEAD_LIST_UNLOCK;
 	}
 
 	/* Check if a stack was not allocated from a stack cache: */
@@ -212,7 +208,7 @@ _thread_stack_alloc(size_t stacksize, size_t guardsize)
 	return (stack);
 }
 
-/* This function must be called with _gc_mutex held. */
+/* This function must be called with the 'dead thread list' lock held. */
 void
 _thread_stack_free(void *stack, size_t stacksize, size_t guardsize)
 {