o Make the setting/checking of cancel state atomic with

  respect to other threads and signal handlers by moving to
  the _thread_critical_enter/exit functions.

o Introduce an static function, testcancel(), that is used by
  the other functions in this module. This allows it to make
  locking assumptions that the top-level functions can't.

o Rework the code flow a bit to reduce indentation levels.

Approved by:	markm/mentor, re/blanket libthr
Reviewed by:	jeff

1 files changed, 111 insertions, 87 deletions

diff --git a/lib/libthr/thread/thr_cancel.c b/lib/libthr/thread/thr_cancel.c
index ab4637c..2d6d765 100644
--- a/lib/libthr/thread/thr_cancel.c
+++ b/lib/libthr/thread/thr_cancel.c
@@ -6,6 +6,11 @@
 #include <pthread.h>
 #include "thr_private.h"
 
+/*
+ * Static prototypes
+ */
+static void	testcancel(void);
+
 __weak_reference(_pthread_cancel, pthread_cancel);
 __weak_reference(_pthread_setcancelstate, pthread_setcancelstate);
 __weak_reference(_pthread_setcanceltype, pthread_setcanceltype);
@@ -16,85 +21,102 @@ _pthread_cancel(pthread_t pthread)
 {
 	int ret;
 
-	if ((ret = _find_thread(pthread)) != 0) {
-		/* NOTHING */
-	} else if (pthread->state == PS_DEAD || pthread->state == PS_DEADLOCK
+	if ((ret = _find_thread(pthread)) != 0)
+		/* The thread is not on the list of active threads */
+		goto out;
+
+	_thread_critical_enter(pthread);
+
+	if (pthread->state == PS_DEAD || pthread->state == PS_DEADLOCK
 	    || (pthread->flags & PTHREAD_EXITING) != 0) {
+		/*
+		 * The thread is in the process of (or has already) exited
+		 * or is deadlocked.
+		 */
+		_thread_critical_exit(pthread);
 		ret = 0;
-	} else {
-		GIANT_LOCK(curthread);
+		goto out;
+	}
+
+	/*
+	 * The thread is on the active thread list and is not in the process
+	 * of exiting.
+	 */
+
+	if (((pthread->cancelflags & PTHREAD_CANCEL_DISABLE) != 0) ||
+	    (((pthread->cancelflags & PTHREAD_CANCEL_ASYNCHRONOUS) == 0) &&
+	    ((pthread->cancelflags & PTHREAD_AT_CANCEL_POINT) == 0)))
+		/* Just mark it for cancellation: */
+		pthread->cancelflags |= PTHREAD_CANCELLING;
+	else {
+		/*
+		 * Check if we need to kick it back into the
+		 * run queue:
+		 */
+		switch (pthread->state) {
+		case PS_RUNNING:
+			/* No need to resume: */
+			pthread->cancelflags |= PTHREAD_CANCELLING;
+			break;
 
-		if (((pthread->cancelflags & PTHREAD_CANCEL_DISABLE) != 0) ||
-		    (((pthread->cancelflags & PTHREAD_CANCEL_ASYNCHRONOUS) == 0) &&
-		    ((pthread->cancelflags & PTHREAD_AT_CANCEL_POINT) == 0)))
-			/* Just mark it for cancellation: */
+		case PS_SLEEP_WAIT:
+		case PS_WAIT_WAIT:
 			pthread->cancelflags |= PTHREAD_CANCELLING;
-		else {
+			PTHREAD_NEW_STATE(pthread, PS_RUNNING);
+			break;
+
+		case PS_JOIN:
 			/*
-			 * Check if we need to kick it back into the
-			 * run queue:
+			 * Disconnect the thread from the joinee:
 			 */
-			switch (pthread->state) {
-			case PS_RUNNING:
-				/* No need to resume: */
-				pthread->cancelflags |= PTHREAD_CANCELLING;
-				break;
-
-			case PS_SLEEP_WAIT:
-			case PS_WAIT_WAIT:
-				pthread->cancelflags |= PTHREAD_CANCELLING;
-				PTHREAD_NEW_STATE(pthread, PS_RUNNING);
-				break;
-
-			case PS_JOIN:
-				/*
-				 * Disconnect the thread from the joinee:
-				 */
-				if (pthread->join_status.thread != NULL) {
-					pthread->join_status.thread->joiner
-					    = NULL;
-					pthread->join_status.thread = NULL;
-				}
-				pthread->cancelflags |= PTHREAD_CANCELLING;
-				PTHREAD_NEW_STATE(pthread, PS_RUNNING);
-				break;
-
-			case PS_MUTEX_WAIT:
-			case PS_COND_WAIT:
-				/*
-				 * Threads in these states may be in queues.
-				 * In order to preserve queue integrity, the
-				 * cancelled thread must remove itself from the
-				 * queue.  When the thread resumes, it will
-				 * remove itself from the queue and call the
-				 * cancellation routine.
-				 */
-				pthread->cancelflags |= PTHREAD_CANCELLING;
-				PTHREAD_NEW_STATE(pthread, PS_RUNNING);
-				break;
-
-			case PS_DEAD:
-			case PS_DEADLOCK:
-			case PS_STATE_MAX:
-				/* Ignore - only here to silence -Wall: */
-				break;
+			if (pthread->join_status.thread != NULL) {
+				pthread->join_status.thread->joiner
+				    = NULL;
+				pthread->join_status.thread = NULL;
 			}
-		}
+			pthread->cancelflags |= PTHREAD_CANCELLING;
+			PTHREAD_NEW_STATE(pthread, PS_RUNNING);
+			break;
 
-		/* Unprotect the scheduling queues: */
-		GIANT_UNLOCK(curthread);
+		case PS_MUTEX_WAIT:
+		case PS_COND_WAIT:
+			/*
+			 * Threads in these states may be in queues.
+			 * In order to preserve queue integrity, the
+			 * cancelled thread must remove itself from the
+			 * queue.  When the thread resumes, it will
+			 * remove itself from the queue and call the
+			 * cancellation routine.
+			 */
+			pthread->cancelflags |= PTHREAD_CANCELLING;
+			PTHREAD_NEW_STATE(pthread, PS_RUNNING);
+			break;
 
-		ret = 0;
+		case PS_DEAD:
+		case PS_DEADLOCK:
+		case PS_STATE_MAX:
+			/* Ignore - only here to silence -Wall: */
+			break;
+		}
 	}
+
+	/* Unprotect the scheduling queues: */
+	_thread_critical_exit(pthread);
+
+	ret = 0;
+out:
 	return (ret);
 }
 
 int
 _pthread_setcancelstate(int state, int *oldstate)
 {
-	int ostate;
+	int ostate, ret;
+
+	ret = 0;
+
+	_thread_critical_enter(curthread);
 
-	GIANT_LOCK(curthread);
 	ostate = curthread->cancelflags & PTHREAD_CANCEL_DISABLE;
 
 	switch (state) {
@@ -102,23 +124,21 @@ _pthread_setcancelstate(int state, int *oldstate)
 		if (oldstate != NULL)
 			*oldstate = ostate;
 		curthread->cancelflags &= ~PTHREAD_CANCEL_DISABLE;
-		if ((curthread->cancelflags & PTHREAD_CANCEL_ASYNCHRONOUS) == 0) 
+		if ((curthread->cancelflags & PTHREAD_CANCEL_ASYNCHRONOUS) == 0)
 			break;
-		GIANT_UNLOCK(curthread);
-		pthread_testcancel();
+		testcancel();
 		break;
 	case PTHREAD_CANCEL_DISABLE:
 		if (oldstate != NULL)
 			*oldstate = ostate;
 		curthread->cancelflags |= PTHREAD_CANCEL_DISABLE;
-		GIANT_UNLOCK(curthread);
 		break;
 	default:
-		GIANT_UNLOCK(curthread);
-		return (EINVAL);
+		ret = EINVAL;
 	}
 
-	return (0);
+	_thread_critical_exit(curthread);
+	return (ret);
 }
 
 int
@@ -126,37 +146,43 @@ _pthread_setcanceltype(int type, int *oldtype)
 {
 	int otype;
 
-	GIANT_LOCK(curthread);
+	_thread_critical_enter(curthread);
 	otype = curthread->cancelflags & PTHREAD_CANCEL_ASYNCHRONOUS;
 	switch (type) {
 	case PTHREAD_CANCEL_ASYNCHRONOUS:
 		if (oldtype != NULL)
 			*oldtype = otype;
 		curthread->cancelflags |= PTHREAD_CANCEL_ASYNCHRONOUS;
-		GIANT_UNLOCK(curthread);
-		pthread_testcancel();
+		testcancel();
 		break;
 	case PTHREAD_CANCEL_DEFERRED:
 		if (oldtype != NULL)
 			*oldtype = otype;
 		curthread->cancelflags &= ~PTHREAD_CANCEL_ASYNCHRONOUS;
-		GIANT_UNLOCK(curthread);
 		break;
 	default:
-		GIANT_UNLOCK(curthread);
 		return (EINVAL);
 	}
 
+	_thread_critical_exit(curthread);
 	return (0);
 }
 
-/*
- * XXXTHR Make an internal locked version.
- */
 void
 _pthread_testcancel(void)
 {
-	GIANT_LOCK(curthread);
+	_thread_critical_enter(curthread);
+	testcancel();
+	_thread_critical_exit(curthread);
+}
+
+static void
+testcancel()
+{
+	/*
+	 * This pthread should already be locked by the caller.
+	 */
+
 	if (((curthread->cancelflags & PTHREAD_CANCEL_DISABLE) == 0) &&
 	    ((curthread->cancelflags & PTHREAD_CANCELLING) != 0) &&
 	    ((curthread->flags & PTHREAD_EXITING) == 0)) {
@@ -166,30 +192,28 @@ _pthread_testcancel(void)
 		 * to be cancelled again.
 		 */
 		curthread->cancelflags &= ~PTHREAD_CANCELLING;
-		GIANT_UNLOCK(curthread);
+		_thread_critical_exit(curthread);
 		_thread_exit_cleanup();
 		pthread_exit(PTHREAD_CANCELED);
 		PANIC("cancel");
 	}
-	GIANT_UNLOCK(curthread);
 }
 
 void
 _thread_enter_cancellation_point(void)
 {
-	pthread_testcancel();
-
-	GIANT_LOCK(curthread);
+	_thread_critical_enter(curthread);
+	testcancel();
 	curthread->cancelflags |= PTHREAD_AT_CANCEL_POINT;
-	GIANT_UNLOCK(curthread);
+	_thread_critical_exit(curthread);
 }
 
 void
 _thread_leave_cancellation_point(void)
 {
-	GIANT_LOCK(curthread);
+	_thread_critical_enter(curthread);
 	curthread->cancelflags &= ~PTHREAD_AT_CANCEL_POINT;
-	GIANT_UNLOCK(curthread);
+	testcancel();
+	_thread_critical_exit(curthread);
 
-	pthread_testcancel();
 }