Date: Fri, 24 Sep 2010 19:43:09 -0400 From: Jung-uk Kim <jkim@FreeBSD.org> To: John Baldwin <jhb@freebsd.org> Cc: Daniel Eischen <deischen@freebsd.org>, freebsd-threads@freebsd.org Subject: Re: threads/150889: PTHREAD_MUTEX_INITIALIZER + pthread_mutex_destroy () == EINVAL Message-ID: <201009241943.10401.jkim@FreeBSD.org> In-Reply-To: <201009241724.10223.jhb@freebsd.org> References: <201009232220.o8NMK3fX011639@freefall.freebsd.org> <201009241137.56764.jkim@FreeBSD.org> <201009241724.10223.jhb@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
--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--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201009241943.10401.jkim>