From owner-freebsd-threads@FreeBSD.ORG Fri Sep 24 23:43:18 2010 Return-Path: Delivered-To: freebsd-threads@freebsd.org Received: from [127.0.0.1] (freefall.freebsd.org [IPv6:2001:4f8:fff6::28]) by hub.freebsd.org (Postfix) with ESMTP id 833C3106566C; Fri, 24 Sep 2010 23:43:17 +0000 (UTC) (envelope-from jkim@FreeBSD.org) From: Jung-uk Kim To: John Baldwin Date: Fri, 24 Sep 2010 19:43:09 -0400 User-Agent: KMail/1.6.2 References: <201009232220.o8NMK3fX011639@freefall.freebsd.org> <201009241137.56764.jkim@FreeBSD.org> <201009241724.10223.jhb@freebsd.org> In-Reply-To: <201009241724.10223.jhb@freebsd.org> MIME-Version: 1.0 Content-Disposition: inline Content-Type: Multipart/Mixed; boundary="Boundary-00=_OeTnMm3Rs0n8fas" Message-Id: <201009241943.10401.jkim@FreeBSD.org> Cc: Daniel Eischen , freebsd-threads@freebsd.org Subject: Re: threads/150889: PTHREAD_MUTEX_INITIALIZER + pthread_mutex_destroy () == EINVAL X-BeenThere: freebsd-threads@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Threading on FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 24 Sep 2010 23:43:18 -0000 --Boundary-00=_OeTnMm3Rs0n8fas Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Content-Disposition: inline On Friday 24 September 2010 05:24 pm, John Baldwin wrote: > On Friday, September 24, 2010 11:37:53 am Jung-uk Kim wrote: > > On Friday 24 September 2010 09:26 am, John Baldwin wrote: > > > On Thursday, September 23, 2010 11:48:40 pm Jung-uk Kim wrote: > > > > On Thursday 23 September 2010 06:44 pm, Daniel Eischen wrote: > > > > > You shouldn't have to call pthread_mutex_init() on a mutex > > > > > initialized with PTHREAD_MUTEX_INITIALIZER. Our > > > > > implementation should auto initialize the mutex when it is > > > > > first used; if it doesn't, I think that is a bug. > > > > > > > > Ah, I see. I verified that libthr does it correctly. > > > > However, that's a hack and it is far from real static > > > > allocation although it should work pretty well in reality, > > > > IMHO. More over, it will have a side-effect, i.e., any > > > > destroyed mutex may be resurrected if it is used again. > > > > POSIX seems to say it should return EINVAL when it happens. > > > > :-( > > > > > > I think the fix there is that we should put a different value > > > ((void *)1 for example) into "destroyed" mutex objects than 0 > > > so that destroyed mutexes can be differentiated from statically > > > initialized mutexes. This would also allow us to properly > > > return EBUSY, etc. > > > > It would be nice if we had "uninitialized" as (void *)0 and > > "static initializer" as (void *)1. IMHO, it looks more natural, > > i.e., "uninitialized" or "destroyed" one gets zero, and > > "dynamically initialized" or "statically initialized" one gets > > non-zero. Can we break the ABI for 9, maybe? ;-) > > I actually find the (void *)1 more natural for a destroyed state > FWIW. One thing I would advocate is that regardless of the values > chosen, use constants for both the INITIALIZER and DESTROYED > values. That would make the code more obvious. In general I think > your patch in your followup is correct, but having explicitly > DESTROYED constants that you check against instead of NULL would > improve the code. Here goes more complicated patch. Not really tested, though. Jung-uk Kim --Boundary-00=_OeTnMm3Rs0n8fas Content-Type: text/plain; charset="iso-8859-1"; name="pthread2.diff" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="pthread2.diff" --- include/pthread.h 2010-09-24 16:49:50.000000000 -0400 +++ include/pthread.h 2010-09-24 17:42:00.000000000 -0400 @@ -97,10 +97,10 @@ /* * Static initialization values. */ -#define PTHREAD_MUTEX_INITIALIZER NULL -#define PTHREAD_ADAPTIVE_MUTEX_INITIALIZER_NP NULL -#define PTHREAD_COND_INITIALIZER NULL -#define PTHREAD_RWLOCK_INITIALIZER NULL +#define PTHREAD_MUTEX_INITIALIZER ((void *)1) +#define PTHREAD_ADAPTIVE_MUTEX_INITIALIZER_NP ((void *)1) +#define PTHREAD_COND_INITIALIZER ((void *)1) +#define PTHREAD_RWLOCK_INITIALIZER ((void *)1) /* * Default attribute arguments (draft 4, deprecated). --- lib/libthr/thread/thr_cond.c 2010-09-24 16:49:50.000000000 -0400 +++ lib/libthr/thread/thr_cond.c 2010-09-24 19:32:34.000000000 -0400 @@ -64,27 +64,28 @@ static int cond_init(pthread_cond_t *cond, const pthread_condattr_t *cond_attr) { pthread_cond_t pcond; - int rval = 0; - if ((pcond = (pthread_cond_t) - calloc(1, sizeof(struct pthread_cond))) == NULL) { - rval = ENOMEM; + if (__predict_false(cond == NULL)) + return (EINVAL); + + pcond = (pthread_cond_t)calloc(1, sizeof(struct pthread_cond)); + if (pcond == NULL) + return (ENOMEM); + + /* + * Initialise the condition variable structure: + */ + if (cond_attr == NULL || *cond_attr == NULL) { + pcond->c_pshared = 0; + pcond->c_clockid = CLOCK_REALTIME; } else { - /* - * Initialise the condition variable structure: - */ - if (cond_attr == NULL || *cond_attr == NULL) { - pcond->c_pshared = 0; - pcond->c_clockid = CLOCK_REALTIME; - } else { - pcond->c_pshared = (*cond_attr)->c_pshared; - pcond->c_clockid = (*cond_attr)->c_clockid; - } - _thr_umutex_init(&pcond->c_lock); - *cond = pcond; + pcond->c_pshared = (*cond_attr)->c_pshared; + pcond->c_clockid = (*cond_attr)->c_clockid; } - /* Return the completion status: */ - return (rval); + _thr_umutex_init(&pcond->c_lock); + *cond = pcond; + + return (0); } static int @@ -94,7 +95,7 @@ init_static(struct pthread *thread, pthr THR_LOCK_ACQUIRE(thread, &_cond_static_lock); - if (*cond == NULL) + if (*cond == THR_STATIC_INITIALIZER) ret = cond_init(cond, NULL); else ret = 0; @@ -108,7 +109,6 @@ int _pthread_cond_init(pthread_cond_t *cond, const pthread_condattr_t *cond_attr) { - *cond = NULL; return (cond_init(cond, cond_attr)); } @@ -117,10 +117,12 @@ _pthread_cond_destroy(pthread_cond_t *co { struct pthread *curthread = _get_curthread(); struct pthread_cond *cv; - int rval = 0; - if (*cond == NULL) - rval = EINVAL; + if (__predict_false(cond == NULL || *cond == THR_UNINITIALIZED)) + return (EINVAL); + + if (*cond == THR_STATIC_INITIALIZER) + *cond = THR_UNINITIALIZED; else { cv = *cond; THR_UMUTEX_LOCK(curthread, &cv->c_lock); @@ -128,7 +130,7 @@ _pthread_cond_destroy(pthread_cond_t *co * NULL the caller's pointer now that the condition * variable has been destroyed: */ - *cond = NULL; + *cond = THR_UNINITIALIZED; THR_UMUTEX_UNLOCK(curthread, &cv->c_lock); /* @@ -137,8 +139,8 @@ _pthread_cond_destroy(pthread_cond_t *co */ free(cv); } - /* Return the completion status: */ - return (rval); + + return (0); } struct cond_cancel_info @@ -184,7 +186,7 @@ cond_wait_common(pthread_cond_t *cond, p * If the condition variable is statically initialized, * perform the dynamic initialization: */ - if (__predict_false(*cond == NULL && + if (__predict_false(*cond == THR_STATIC_INITIALIZER && (ret = init_static(curthread, cond)) != 0)) return (ret); @@ -273,7 +275,7 @@ cond_signal_common(pthread_cond_t *cond, * If the condition variable is statically initialized, perform dynamic * initialization. */ - if (__predict_false(*cond == NULL && + if (__predict_false(*cond == THR_STATIC_INITIALIZER && (ret = init_static(curthread, cond)) != 0)) return (ret); --- /usr/src/lib/libthr/thread/thr_mutex.c 2010-09-24 16:49:50.000000000 -0400 +++ lib/libthr/thread/thr_mutex.c 2010-09-24 19:25:30.000000000 -0400 @@ -130,9 +130,12 @@ mutex_init(pthread_mutex_t *mutex, const struct pthread_mutex_attr *attr; struct pthread_mutex *pmutex; - if (mutex_attr == NULL) { + if (__predict_false(mutex == NULL)) + return (EINVAL); + + if (mutex_attr == NULL) attr = &_pthread_mutexattr_default; - } else { + else { attr = *mutex_attr; if (attr->m_type < PTHREAD_MUTEX_ERRORCHECK || attr->m_type >= PTHREAD_MUTEX_TYPE_MAX) @@ -141,8 +144,8 @@ mutex_init(pthread_mutex_t *mutex, attr->m_protocol > PTHREAD_PRIO_PROTECT) return (EINVAL); } - if ((pmutex = (pthread_mutex_t) - calloc_cb(1, sizeof(struct pthread_mutex))) == NULL) + pmutex = (pthread_mutex_t)calloc_cb(1, sizeof(struct pthread_mutex)); + if (pmutex == NULL) return (ENOMEM); pmutex->m_type = attr->m_type; @@ -184,7 +187,7 @@ init_static(struct pthread *thread, pthr THR_LOCK_ACQUIRE(thread, &_mutex_static_lock); - if (*mutex == NULL) + if (*mutex == THR_STATIC_INITIALIZER) ret = mutex_init(mutex, NULL, calloc); else ret = 0; @@ -259,48 +262,53 @@ _pthread_mutex_destroy(pthread_mutex_t * struct pthread *curthread = _get_curthread(); pthread_mutex_t m; uint32_t id; - int ret = 0; + int ret; - if (__predict_false(*mutex == NULL)) - ret = EINVAL; - else { - id = TID(curthread); + if (__predict_false(mutex == NULL || *mutex == THR_UNINITIALIZED)) + return (EINVAL); + if (*mutex == THR_STATIC_INITIALIZER) { + *mutex = THR_UNINITIALIZED; + return (0); + } + + id = TID(curthread); + + /* + * Try to lock the mutex structure, we only need to try once, + * if failed, the mutex is in used. + */ + ret = _thr_umutex_trylock(&(*mutex)->m_lock, id); + if (ret) + return (ret); + + /* + * Check mutex other fields to see if this mutex is in use. + * Mostly for prority mutex types, or there are condition + * variables referencing it. + */ + m = *mutex; + if (m->m_owner != NULL || m->m_refcount != 0) { + if (m->m_lock.m_flags & UMUTEX_PRIO_PROTECT) + set_inherited_priority(curthread, m); + _thr_umutex_unlock(&m->m_lock, id); + return (EBUSY); + } else { /* - * Try to lock the mutex structure, we only need to - * try once, if failed, the mutex is in used. - */ - ret = _thr_umutex_trylock(&(*mutex)->m_lock, id); - if (ret) - return (ret); - m = *mutex; - /* - * Check mutex other fields to see if this mutex is - * in use. Mostly for prority mutex types, or there - * are condition variables referencing it. + * Save a pointer to the mutex so it can be free'd + * and set the caller's pointer to NULL. */ - if (m->m_owner != NULL || m->m_refcount != 0) { - if (m->m_lock.m_flags & UMUTEX_PRIO_PROTECT) - set_inherited_priority(curthread, m); - _thr_umutex_unlock(&m->m_lock, id); - ret = EBUSY; - } else { - /* - * Save a pointer to the mutex so it can be free'd - * and set the caller's pointer to NULL. - */ - *mutex = NULL; + *mutex = THR_UNINITIALIZED; - if (m->m_lock.m_flags & UMUTEX_PRIO_PROTECT) - set_inherited_priority(curthread, m); - _thr_umutex_unlock(&m->m_lock, id); + if (m->m_lock.m_flags & UMUTEX_PRIO_PROTECT) + set_inherited_priority(curthread, m); + _thr_umutex_unlock(&m->m_lock, id); - MUTEX_ASSERT_NOT_OWNED(m); - free(m); - } + MUTEX_ASSERT_NOT_OWNED(m); + free(m); } - return (ret); + return (0); } #define ENQUEUE_MUTEX(curthread, m) \ @@ -342,11 +350,14 @@ __pthread_mutex_trylock(pthread_mutex_t struct pthread *curthread = _get_curthread(); int ret; + if (__predict_false(mutex == NULL)) + return (EINVAL); + /* * If the mutex is statically initialized, perform the dynamic * initialization: */ - if (__predict_false(*mutex == NULL)) { + if (__predict_false(*mutex == THR_STATIC_INITIALIZER)) { ret = init_static(curthread, mutex); if (__predict_false(ret)) return (ret); @@ -446,6 +457,9 @@ __pthread_mutex_lock(pthread_mutex_t *mu struct pthread_mutex *m; int ret; + if (__predict_false(mutex == NULL)) + return (EINVAL); + _thr_check_init(); curthread = _get_curthread(); @@ -454,7 +468,7 @@ __pthread_mutex_lock(pthread_mutex_t *mu * If the mutex is statically initialized, perform the dynamic * initialization: */ - if (__predict_false((m = *mutex) == NULL)) { + if (__predict_false((m = *mutex) == THR_STATIC_INITIALIZER)) { ret = init_static(curthread, mutex); if (__predict_false(ret)) return (ret); @@ -471,6 +485,9 @@ __pthread_mutex_timedlock(pthread_mutex_ struct pthread_mutex *m; int ret; + if (__predict_false(mutex == NULL)) + return (EINVAL); + _thr_check_init(); curthread = _get_curthread(); @@ -479,7 +496,7 @@ __pthread_mutex_timedlock(pthread_mutex_ * If the mutex is statically initialized, perform the dynamic * initialization: */ - if (__predict_false((m = *mutex) == NULL)) { + if (__predict_false((m = *mutex) == THR_STATIC_INITIALIZER)) { ret = init_static(curthread, mutex); if (__predict_false(ret)) return (ret); @@ -611,9 +628,12 @@ mutex_unlock_common(pthread_mutex_t *mut struct pthread_mutex *m; uint32_t id; - if (__predict_false((m = *mutex) == NULL)) + if (__predict_false(mutex == NULL || (m = *mutex) == THR_UNINITIALIZED)) return (EINVAL); + if (*mutex == THR_STATIC_INITIALIZER) + return (EPERM); + /* * Check if the running thread is not the owner of the mutex. */ @@ -649,7 +669,7 @@ _mutex_cv_unlock(pthread_mutex_t *mutex, struct pthread *curthread = _get_curthread(); struct pthread_mutex *m; - if (__predict_false((m = *mutex) == NULL)) + if (__predict_false((m = *mutex) == THR_UNINITIALIZED)) return (EINVAL); /* @@ -685,18 +705,14 @@ int _pthread_mutex_getprioceiling(pthread_mutex_t *mutex, int *prioceiling) { - int ret; - if (*mutex == NULL) - ret = EINVAL; - else if (((*mutex)->m_lock.m_flags & UMUTEX_PRIO_PROTECT) == 0) - ret = EINVAL; - else { - *prioceiling = (*mutex)->m_lock.m_ceilings[0]; - ret = 0; - } + if (mutex == NULL || *mutex == THR_UNINITIALIZED || + ((*mutex)->m_lock.m_flags & UMUTEX_PRIO_PROTECT) == 0) + return (EINVAL); + + *prioceiling = (*mutex)->m_lock.m_ceilings[0]; - return(ret); + return (0); } int @@ -707,10 +723,11 @@ _pthread_mutex_setprioceiling(pthread_mu struct pthread_mutex *m, *m1, *m2; int ret; - m = *mutex; - if (m == NULL || (m->m_lock.m_flags & UMUTEX_PRIO_PROTECT) == 0) + if (mutex == NULL || *mutex == THR_UNINITIALIZED || + ((*mutex)->m_lock.m_flags & UMUTEX_PRIO_PROTECT) == 0) return (EINVAL); + m = *mutex; ret = __thr_umutex_set_ceiling(&m->m_lock, ceiling, old_ceiling); if (ret != 0) return (ret); @@ -737,7 +754,8 @@ _pthread_mutex_setprioceiling(pthread_mu int _pthread_mutex_getspinloops_np(pthread_mutex_t *mutex, int *count) { - if (*mutex == NULL) + + if (mutex == NULL || *mutex == THR_UNINITIALIZED) return (EINVAL); *count = (*mutex)->m_spinloops; return (0); @@ -749,7 +767,7 @@ __pthread_mutex_setspinloops_np(pthread_ struct pthread *curthread = _get_curthread(); int ret; - if (__predict_false(*mutex == NULL)) { + if (__predict_false(*mutex == THR_STATIC_INITIALIZER)) { ret = init_static(curthread, mutex); if (__predict_false(ret)) return (ret); @@ -761,7 +779,8 @@ __pthread_mutex_setspinloops_np(pthread_ int _pthread_mutex_getyieldloops_np(pthread_mutex_t *mutex, int *count) { - if (*mutex == NULL) + + if (mutex == NULL || *mutex == THR_UNINITIALIZED) return (EINVAL); *count = (*mutex)->m_yieldloops; return (0); @@ -773,7 +792,9 @@ __pthread_mutex_setyieldloops_np(pthread struct pthread *curthread = _get_curthread(); int ret; - if (__predict_false(*mutex == NULL)) { + if (__predict_false(mutex == NULL)) + return (EINVAL); + if (__predict_false(*mutex == THR_STATIC_INITIALIZER)) { ret = init_static(curthread, mutex); if (__predict_false(ret)) return (ret); @@ -788,7 +809,9 @@ _pthread_mutex_isowned_np(pthread_mutex_ struct pthread *curthread = _get_curthread(); int ret; - if (__predict_false(*mutex == NULL)) { + if (__predict_false(mutex == NULL)) + return (EINVAL); + if (__predict_false(*mutex == THR_STATIC_INITIALIZER)) { ret = init_static(curthread, mutex); if (__predict_false(ret)) return (ret); --- lib/libthr/thread/thr_private.h 2010-09-24 16:49:50.000000000 -0400 +++ lib/libthr/thread/thr_private.h 2010-09-24 17:45:12.000000000 -0400 @@ -125,6 +125,9 @@ TAILQ_HEAD(mutex_queue, pthread_mutex); } \ } while (0) +#define THR_UNINITIALIZED ((void *)0) +#define THR_STATIC_INITIALIZER ((void *)1) + struct pthread_mutex { /* * Lock for accesses to this structure. --- lib/libthr/thread/thr_rwlock.c 2010-09-24 16:49:50.000000000 -0400 +++ lib/libthr/thread/thr_rwlock.c 2010-09-24 19:26:20.000000000 -0400 @@ -54,6 +54,9 @@ rwlock_init(pthread_rwlock_t *rwlock, co { pthread_rwlock_t prwlock; + if (__predict_false(rwlock == NULL)) + return (EINVAL); + prwlock = (pthread_rwlock_t)calloc(1, sizeof(struct pthread_rwlock)); if (prwlock == NULL) return (ENOMEM); @@ -64,20 +67,14 @@ rwlock_init(pthread_rwlock_t *rwlock, co int _pthread_rwlock_destroy (pthread_rwlock_t *rwlock) { - int ret; - if (rwlock == NULL) - ret = EINVAL; - else { - pthread_rwlock_t prwlock; - - prwlock = *rwlock; - *rwlock = NULL; + if (__predict_false(rwlock == NULL || *rwlock == THR_UNINITIALIZED)) + return (EINVAL); - free(prwlock); - ret = 0; - } - return (ret); + if (*rwlock != THR_STATIC_INITIALIZER) + free(*rwlock); + *rwlock = THR_UNINITIALIZED; + return (0); } static int @@ -87,7 +84,7 @@ init_static(struct pthread *thread, pthr THR_LOCK_ACQUIRE(thread, &_rwlock_static_lock); - if (*rwlock == NULL) + if (*rwlock == THR_STATIC_INITIALIZER) ret = rwlock_init(rwlock, NULL); else ret = 0; @@ -100,7 +97,7 @@ init_static(struct pthread *thread, pthr int _pthread_rwlock_init (pthread_rwlock_t *rwlock, const pthread_rwlockattr_t *attr) { - *rwlock = NULL; + return (rwlock_init(rwlock, attr)); } @@ -119,7 +116,7 @@ rwlock_rdlock_common(pthread_rwlock_t *r prwlock = *rwlock; /* check for static initialization */ - if (__predict_false(prwlock == NULL)) { + if (__predict_false(prwlock == THR_STATIC_INITIALIZER)) { if ((ret = init_static(curthread, rwlock)) != 0) return (ret); @@ -212,7 +209,7 @@ _pthread_rwlock_tryrdlock (pthread_rwloc prwlock = *rwlock; /* check for static initialization */ - if (__predict_false(prwlock == NULL)) { + if (__predict_false(prwlock == THR_STATIC_INITIALIZER)) { if ((ret = init_static(curthread, rwlock)) != 0) return (ret); @@ -256,7 +253,7 @@ _pthread_rwlock_trywrlock (pthread_rwloc prwlock = *rwlock; /* check for static initialization */ - if (__predict_false(prwlock == NULL)) { + if (__predict_false(prwlock == THR_STATIC_INITIALIZER)) { if ((ret = init_static(curthread, rwlock)) != 0) return (ret); @@ -283,7 +280,7 @@ rwlock_wrlock_common (pthread_rwlock_t * prwlock = *rwlock; /* check for static initialization */ - if (__predict_false(prwlock == NULL)) { + if (__predict_false(prwlock == THR_STATIC_INITIALIZER)) { if ((ret = init_static(curthread, rwlock)) != 0) return (ret); @@ -361,9 +358,12 @@ _pthread_rwlock_unlock (pthread_rwlock_t prwlock = *rwlock; - if (__predict_false(prwlock == NULL)) + if (__predict_false(prwlock == THR_UNINITIALIZED)) return (EINVAL); + if (prwlock == THR_STATIC_INITIALIZER) + return (EPERM); + state = prwlock->lock.rw_state; if (state & URWLOCK_WRITE_OWNER) { if (__predict_false(prwlock->owner != curthread)) --Boundary-00=_OeTnMm3Rs0n8fas--