PERFORCE change 65510 for review
David Xu
davidxu at FreeBSD.org
Fri Nov 19 20:24:44 PST 2004
http://perforce.freebsd.org/chv.cgi?CH=65510
Change 65510 by davidxu at davidxu_alona on 2004/11/20 04:24:16
1. introduce init_static function to init statically initialized cond var.
2. fix famous race condition in cond var.
3. fix signal backout code bug.
Affected files ...
.. //depot/projects/davidxu_thread/src/lib/libthread/thread/thr_cond.c#2 edit
Differences ...
==== //depot/projects/davidxu_thread/src/lib/libthread/thread/thr_cond.c#2 (text+ko) ====
@@ -44,9 +44,13 @@
/*
* Prototypes
*/
-static inline struct pthread *cond_queue_deq(pthread_cond_t);
static inline void cond_queue_remove(pthread_cond_t, pthread_t);
static inline void cond_queue_enq(pthread_cond_t, pthread_t);
+static void cond_wait_backout(void *);
+static inline void check_continuation(struct pthread *,
+ struct pthread_cond *, pthread_mutex_t *);
+static int init_static(struct pthread *thread,
+ pthread_cond_t *cond);
/*
* Double underscore versions are cancellation points. Single underscore
@@ -106,8 +110,7 @@
if ((pcond = (pthread_cond_t)
malloc(sizeof(struct pthread_cond))) == NULL) {
rval = ENOMEM;
- } else if (_lock_init(&pcond->c_lock, LCK_ADAPTIVE,
- _thr_lock_wait, _thr_lock_wakeup) != 0) {
+ } else if (_lock_init(&pcond->c_lock) != 0) {
free(pcond);
rval = ENOMEM;
} else {
@@ -128,6 +131,21 @@
return (rval);
}
+static int
+init_static(struct pthread *thread, pthread_cond_t *cond)
+{
+ int ret;
+
+ THR_LOCK_ACQUIRE(thread, &_cond_static_lock);
+ if (*cond == NULL)
+ ret = pthread_cond_init(cond, NULL);
+ else
+ ret = 0;
+ THR_LOCK_RELEASE(thread, &_cond_static_lock);
+
+ return (ret);
+}
+
int
_pthread_cond_destroy(pthread_cond_t *cond)
{
@@ -171,8 +189,7 @@
struct pthread *curthread = _get_curthread();
int rval = 0;
int done = 0;
- int interrupted = 0;
- int unlock_mutex = 1;
+ int mutex_locked = 1;
int seqno;
if (cond == NULL)
@@ -183,12 +200,9 @@
* perform the dynamic initialization:
*/
if (*cond == NULL &&
- (rval = pthread_cond_init(cond, NULL)) != 0)
+ (rval = init_static(curthread, cond)) != 0)
return (rval);
- if (!_kse_isthreaded())
- _kse_setthreaded(1);
-
/*
* Enter a loop waiting for a condition signal or broadcast
* to wake up this thread. A loop is needed in case the waiting
@@ -198,10 +212,11 @@
* and backed out of the waiting queue prior to executing the
* signal handler.
*/
+
+ /* Lock the condition variable structure: */
+ THR_LOCK_ACQUIRE(curthread, &(*cond)->c_lock);
+ seqno = (*cond)->c_seqno;
do {
- /* Lock the condition variable structure: */
- THR_LOCK_ACQUIRE(curthread, &(*cond)->c_lock);
-
/*
* If the condvar was statically allocated, properly
* initialize the tail queue.
@@ -217,9 +232,6 @@
case COND_TYPE_FAST:
if ((mutex == NULL) || (((*cond)->c_mutex != NULL) &&
((*cond)->c_mutex != *mutex))) {
- /* Unlock the condition variable structure: */
- THR_LOCK_RELEASE(curthread, &(*cond)->c_lock);
-
/* Return invalid argument error: */
rval = EINVAL;
} else {
@@ -233,15 +245,15 @@
*/
cond_queue_enq(*cond, curthread);
- /* Remember the mutex and sequence number: */
+ /* Remember the mutex: */
(*cond)->c_mutex = *mutex;
- seqno = (*cond)->c_seqno;
+ curthread->sigbackout = cond_wait_backout;
/* Wait forever: */
curthread->wakeup_time.tv_sec = -1;
/* Unlock the mutex: */
- if ((unlock_mutex != 0) &&
+ if (mutex_locked &&
((rval = _mutex_cv_unlock(mutex)) != 0)) {
/*
* Cannot unlock the mutex, so remove
@@ -249,13 +261,11 @@
* variable queue:
*/
cond_queue_remove(*cond, curthread);
+ curthread->sigbackout = NULL;
/* Check for no more waiters: */
if (TAILQ_FIRST(&(*cond)->c_queue) == NULL)
(*cond)->c_mutex = NULL;
-
- /* Unlock the condition variable structure: */
- THR_LOCK_RELEASE(curthread, &(*cond)->c_lock);
}
else {
/*
@@ -264,7 +274,7 @@
* thread has to be requeued after
* handling a signal).
*/
- unlock_mutex = 0;
+ mutex_locked = 0;
/*
* This thread is active and is in a
@@ -272,21 +282,18 @@
* lock); we should be able to safely
* set the state.
*/
- THR_SCHED_LOCK(curthread, curthread);
+ THR_LOCK_SWITCH(curthread);
THR_SET_STATE(curthread, PS_COND_WAIT);
/* Remember the CV: */
curthread->data.cond = *cond;
- THR_SCHED_UNLOCK(curthread, curthread);
/* Unlock the CV structure: */
THR_LOCK_RELEASE(curthread,
&(*cond)->c_lock);
/* Schedule the next thread: */
- _thr_sched_switch(curthread);
-
- curthread->data.cond = NULL;
+ _thr_sched_switch_unlocked(curthread);
/*
* XXX - This really isn't a good check
@@ -299,41 +306,39 @@
* should be sent "as soon as possible".
*/
done = (seqno != (*cond)->c_seqno);
-
- if (THR_IN_SYNCQ(curthread)) {
+ if (done && !THR_IN_CONDQ(curthread)) {
/*
- * Lock the condition variable
- * while removing the thread.
+ * The thread is dequeued, so
+ * it is safe to clear this.
*/
- THR_LOCK_ACQUIRE(curthread,
- &(*cond)->c_lock);
+ curthread->data.cond = NULL;
+ curthread->sigbackout = NULL;
+ check_continuation(curthread,
+ NULL, mutex);
+ return (_mutex_cv_lock(mutex));
+ }
+
+ /* Relock the CV structure: */
+ THR_LOCK_ACQUIRE(curthread,
+ &(*cond)->c_lock);
+
+ /*
+ * Clear these after taking the lock to
+ * prevent a race condition where a
+ * signal can arrive before dequeueing
+ * the thread.
+ */
+ curthread->data.cond = NULL;
+ curthread->sigbackout = NULL;
+ done = (seqno != (*cond)->c_seqno);
+ if (THR_IN_CONDQ(curthread)) {
cond_queue_remove(*cond,
curthread);
/* Check for no more waiters: */
if (TAILQ_FIRST(&(*cond)->c_queue) == NULL)
(*cond)->c_mutex = NULL;
-
- THR_LOCK_RELEASE(curthread,
- &(*cond)->c_lock);
- }
-
- /*
- * Save the interrupted flag; locking
- * the mutex may destroy it.
- */
- interrupted = curthread->interrupted;
-
- /*
- * Note that even though this thread may
- * have been canceled, POSIX requires
- * that the mutex be reaquired prior to
- * cancellation.
- */
- if (done || interrupted) {
- rval = _mutex_cv_lock(mutex);
- unlock_mutex = 1;
}
}
}
@@ -341,18 +346,21 @@
/* Trap invalid condition variable types: */
default:
- /* Unlock the condition variable structure: */
- THR_LOCK_RELEASE(curthread, &(*cond)->c_lock);
-
/* Return an invalid argument error: */
rval = EINVAL;
break;
}
- if ((interrupted != 0) && (curthread->continuation != NULL))
- curthread->continuation((void *) curthread);
+ check_continuation(curthread, *cond,
+ mutex_locked ? NULL : mutex);
} while ((done == 0) && (rval == 0));
+ /* Unlock the condition variable structure: */
+ THR_LOCK_RELEASE(curthread, &(*cond)->c_lock);
+
+ if (mutex_locked == 0)
+ _mutex_cv_lock(mutex);
+
/* Return the completion status: */
return (rval);
}
@@ -378,8 +386,7 @@
struct pthread *curthread = _get_curthread();
int rval = 0;
int done = 0;
- int interrupted = 0;
- int unlock_mutex = 1;
+ int mutex_locked = 1;
int seqno;
THR_ASSERT(curthread->locklevel == 0,
@@ -392,12 +399,9 @@
* If the condition variable is statically initialized, perform dynamic
* initialization.
*/
- if (*cond == NULL && (rval = pthread_cond_init(cond, NULL)) != 0)
+ if (*cond == NULL && (rval = init_static(curthread, cond)) != 0)
return (rval);
- if (!_kse_isthreaded())
- _kse_setthreaded(1);
-
/*
* Enter a loop waiting for a condition signal or broadcast
* to wake up this thread. A loop is needed in case the waiting
@@ -407,10 +411,11 @@
* and backed out of the waiting queue prior to executing the
* signal handler.
*/
+
+ /* Lock the condition variable structure: */
+ THR_LOCK_ACQUIRE(curthread, &(*cond)->c_lock);
+ seqno = (*cond)->c_seqno;
do {
- /* Lock the condition variable structure: */
- THR_LOCK_ACQUIRE(curthread, &(*cond)->c_lock);
-
/*
* If the condvar was statically allocated, properly
* initialize the tail queue.
@@ -428,9 +433,6 @@
((*cond)->c_mutex != *mutex))) {
/* Return invalid argument error: */
rval = EINVAL;
-
- /* Unlock the condition variable structure: */
- THR_LOCK_RELEASE(curthread, &(*cond)->c_lock);
} else {
/* Set the wakeup time: */
curthread->wakeup_time.tv_sec = abstime->tv_sec;
@@ -449,10 +451,10 @@
/* Remember the mutex and sequence number: */
(*cond)->c_mutex = *mutex;
- seqno = (*cond)->c_seqno;
+ curthread->sigbackout = cond_wait_backout;
/* Unlock the mutex: */
- if ((unlock_mutex != 0) &&
+ if (mutex_locked &&
((rval = _mutex_cv_unlock(mutex)) != 0)) {
/*
* Cannot unlock the mutex; remove the
@@ -460,13 +462,11 @@
* variable queue:
*/
cond_queue_remove(*cond, curthread);
+ curthread->sigbackout = NULL;
/* Check for no more waiters: */
if (TAILQ_FIRST(&(*cond)->c_queue) == NULL)
(*cond)->c_mutex = NULL;
-
- /* Unlock the condition variable structure: */
- THR_LOCK_RELEASE(curthread, &(*cond)->c_lock);
} else {
/*
* Don't unlock the mutex the next
@@ -474,7 +474,7 @@
* thread has to be requeued after
* handling a signal).
*/
- unlock_mutex = 0;
+ mutex_locked = 0;
/*
* This thread is active and is in a
@@ -482,21 +482,18 @@
* lock); we should be able to safely
* set the state.
*/
- THR_SCHED_LOCK(curthread, curthread);
+ THR_LOCK_SWITCH(curthread);
THR_SET_STATE(curthread, PS_COND_WAIT);
/* Remember the CV: */
curthread->data.cond = *cond;
- THR_SCHED_UNLOCK(curthread, curthread);
/* Unlock the CV structure: */
THR_LOCK_RELEASE(curthread,
&(*cond)->c_lock);
/* Schedule the next thread: */
- _thr_sched_switch(curthread);
-
- curthread->data.cond = NULL;
+ _thr_sched_switch_unlocked(curthread);
/*
* XXX - This really isn't a good check
@@ -509,38 +506,45 @@
* should be sent "as soon as possible".
*/
done = (seqno != (*cond)->c_seqno);
-
- if (THR_IN_CONDQ(curthread)) {
+ if (done && !THR_IN_CONDQ(curthread)) {
/*
- * Lock the condition variable
- * while removing the thread.
+ * The thread is dequeued, so
+ * it is safe to clear this.
*/
- THR_LOCK_ACQUIRE(curthread,
- &(*cond)->c_lock);
+ curthread->data.cond = NULL;
+ curthread->sigbackout = NULL;
+ check_continuation(curthread,
+ NULL, mutex);
+ return (_mutex_cv_lock(mutex));
+ }
+
+ /* Relock the CV structure: */
+ THR_LOCK_ACQUIRE(curthread,
+ &(*cond)->c_lock);
+
+ /*
+ * Clear these after taking the lock to
+ * prevent a race condition where a
+ * signal can arrive before dequeueing
+ * the thread.
+ */
+ curthread->data.cond = NULL;
+ curthread->sigbackout = NULL;
+
+ done = (seqno != (*cond)->c_seqno);
+ if (THR_IN_CONDQ(curthread)) {
cond_queue_remove(*cond,
curthread);
/* Check for no more waiters: */
if (TAILQ_FIRST(&(*cond)->c_queue) == NULL)
(*cond)->c_mutex = NULL;
-
- THR_LOCK_RELEASE(curthread,
- &(*cond)->c_lock);
}
- /*
- * Save the interrupted flag; locking
- * the mutex may destroy it.
- */
- interrupted = curthread->interrupted;
if (curthread->timeout != 0) {
/* The wait timedout. */
rval = ETIMEDOUT;
- (void)_mutex_cv_lock(mutex);
- } else if (interrupted || done) {
- rval = _mutex_cv_lock(mutex);
- unlock_mutex = 1;
}
}
}
@@ -548,18 +552,21 @@
/* Trap invalid condition variable types: */
default:
- /* Unlock the condition variable structure: */
- THR_LOCK_RELEASE(curthread, &(*cond)->c_lock);
-
/* Return an invalid argument error: */
rval = EINVAL;
break;
}
- if ((interrupted != 0) && (curthread->continuation != NULL))
- curthread->continuation((void *)curthread);
+ check_continuation(curthread, *cond,
+ mutex_locked ? NULL : mutex);
} while ((done == 0) && (rval == 0));
+ /* Unlock the condition variable structure: */
+ THR_LOCK_RELEASE(curthread, &(*cond)->c_lock);
+
+ if (mutex_locked == 0)
+ _mutex_cv_lock(mutex);
+
/* Return the completion status: */
return (rval);
}
@@ -583,7 +590,7 @@
{
struct pthread *curthread = _get_curthread();
struct pthread *pthread;
- struct kse_mailbox *kmbx;
+ long tid = -1;
int rval = 0;
THR_ASSERT(curthread->locklevel == 0,
@@ -594,7 +601,7 @@
* If the condition variable is statically initialized, perform dynamic
* initialization.
*/
- else if (*cond != NULL || (rval = pthread_cond_init(cond, NULL)) == 0) {
+ else if (*cond != NULL || (rval = init_static(curthread, cond)) == 0) {
/* Lock the condition variable structure: */
THR_LOCK_ACQUIRE(curthread, &(*cond)->c_lock);
@@ -613,16 +620,11 @@
*/
if ((pthread = TAILQ_FIRST(&(*cond)->c_queue))
!= NULL) {
- THR_SCHED_LOCK(curthread, pthread);
+ THR_THREAD_LOCK(curthread, pthread);
cond_queue_remove(*cond, pthread);
- if ((pthread->kseg == curthread->kseg) &&
- (pthread->active_priority >
- curthread->active_priority))
- curthread->critical_yield = 1;
- kmbx = _thr_setrunnable_unlocked(pthread);
- THR_SCHED_UNLOCK(curthread, pthread);
- if (kmbx != NULL)
- kse_wakeup(kmbx);
+ pthread->sigbackout = NULL;
+ tid = _thr_setrunnable_unlocked(pthread);
+ THR_THREAD_UNLOCK(curthread, pthread);
}
/* Check for no more waiters: */
if (TAILQ_FIRST(&(*cond)->c_queue) == NULL)
@@ -638,6 +640,8 @@
/* Unlock the condition variable structure: */
THR_LOCK_RELEASE(curthread, &(*cond)->c_lock);
+ if (tid != -1)
+ thr_wake(tid);
}
/* Return the completion status: */
@@ -651,7 +655,7 @@
{
struct pthread *curthread = _get_curthread();
struct pthread *pthread;
- struct kse_mailbox *kmbx;
+ long tid = -1;
int rval = 0;
THR_ASSERT(curthread->locklevel == 0,
@@ -679,16 +683,13 @@
*/
while ((pthread = TAILQ_FIRST(&(*cond)->c_queue))
!= NULL) {
- THR_SCHED_LOCK(curthread, pthread);
+ THR_THREAD_LOCK(curthread, pthread);
cond_queue_remove(*cond, pthread);
- if ((pthread->kseg == curthread->kseg) &&
- (pthread->active_priority >
- curthread->active_priority))
- curthread->critical_yield = 1;
- kmbx = _thr_setrunnable_unlocked(pthread);
- THR_SCHED_UNLOCK(curthread, pthread);
- if (kmbx != NULL)
- kse_wakeup(kmbx);
+ pthread->sigbackout = NULL;
+ tid = _thr_setrunnable_unlocked(pthread);
+ THR_THREAD_UNLOCK(curthread, pthread);
+ if (tid != -1)
+ thr_wake(tid);
}
/* There are no more waiting threads: */
@@ -712,15 +713,38 @@
__strong_reference(_pthread_cond_broadcast, _thr_cond_broadcast);
-void
-_cond_wait_backout(struct pthread *curthread)
+static inline void
+check_continuation(struct pthread *curthread, struct pthread_cond *cond,
+ pthread_mutex_t *mutex)
+{
+ if ((curthread->interrupted != 0) &&
+ (curthread->continuation != NULL)) {
+ if (cond != NULL)
+ /* Unlock the condition variable structure: */
+ THR_LOCK_RELEASE(curthread, &cond->c_lock);
+ /*
+ * Note that even though this thread may have been
+ * canceled, POSIX requires that the mutex be
+ * reaquired prior to cancellation.
+ */
+ if (mutex != NULL)
+ _mutex_cv_lock(mutex);
+ curthread->continuation((void *) curthread);
+ PANIC("continuation returned in pthread_cond_wait.\n");
+ }
+}
+
+static void
+cond_wait_backout(void *arg)
{
+ struct pthread *curthread = (struct pthread *)arg;
pthread_cond_t cond;
cond = curthread->data.cond;
if (cond != NULL) {
/* Lock the condition variable structure: */
THR_LOCK_ACQUIRE(curthread, &cond->c_lock);
+ curthread->data.cond = NULL;
/* Process according to condition variable type: */
switch (cond->c_type) {
@@ -740,31 +764,8 @@
/* Unlock the condition variable structure: */
THR_LOCK_RELEASE(curthread, &cond->c_lock);
}
-}
-
-/*
- * Dequeue a waiting thread from the head of a condition queue in
- * descending priority order.
- */
-static inline struct pthread *
-cond_queue_deq(pthread_cond_t cond)
-{
- struct pthread *pthread;
-
- while ((pthread = TAILQ_FIRST(&cond->c_queue)) != NULL) {
- TAILQ_REMOVE(&cond->c_queue, pthread, sqe);
- THR_CONDQ_CLEAR(pthread);
- if ((pthread->timeout == 0) && (pthread->interrupted == 0))
- /*
- * Only exit the loop when we find a thread
- * that hasn't timed out or been canceled;
- * those threads are already running and don't
- * need their run state changed.
- */
- break;
- }
-
- return (pthread);
+ /* No need to call this again. */
+ curthread->sigbackout = NULL;
}
/*
More information about the p4-projects
mailing list