Date: Sat, 20 Nov 2004 04:24:42 GMT From: David Xu <davidxu@FreeBSD.org> To: Perforce Change Reviews <perforce@freebsd.org> Subject: PERFORCE change 65510 for review Message-ID: <200411200424.iAK4OgJC005824@repoman.freebsd.org>
next in thread | raw e-mail | index | archive | help
http://perforce.freebsd.org/chv.cgi?CH=65510 Change 65510 by davidxu@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; } /*
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200411200424.iAK4OgJC005824>