From 68ebd3f6e2a438a58e0647d5a53509aa2aa8e6ea Mon Sep 17 00:00:00 2001 From: Robert Collins Date: Sun, 6 May 2001 22:23:43 +0000 Subject: [PATCH] Sun May 6 17:05:00 2001 Robert Collins * thread.h (pthread_cond): New element cond_access to allow atomic broadcasts. * thread.cc (pthread_cond::pthread_cond): Initialise cond_access. (pthread_cond::~pthread_cond): Destroy cond_access. (pthread_cond::Broadcast): Use cond_access. (pthread_cond::Signal): Use cond_access. (pthread_cond_wait): Use cond_access. (pthread_cond_timedwait): Use cond_access. --- winsup/cygwin/ChangeLog | 9 ++++++ winsup/cygwin/thread.cc | 68 +++++++++++++++++++++++++++++++++++------ winsup/cygwin/thread.h | 2 ++ 3 files changed, 69 insertions(+), 10 deletions(-) diff --git a/winsup/cygwin/ChangeLog b/winsup/cygwin/ChangeLog index a303213f4..c72be5fe1 100644 --- a/winsup/cygwin/ChangeLog +++ b/winsup/cygwin/ChangeLog @@ -1,3 +1,12 @@ +Sun May 6 17:05:00 2001 Robert Collins + * thread.h (pthread_cond): New element cond_access to allow atomic broadcasts. + * thread.cc (pthread_cond::pthread_cond): Initialise cond_access. + (pthread_cond::~pthread_cond): Destroy cond_access. + (pthread_cond::Broadcast): Use cond_access. + (pthread_cond::Signal): Use cond_access. + (pthread_cond_wait): Use cond_access. + (pthread_cond_timedwait): Use cond_access. + Sun May 6 11:55:40 2001 Christopher Faylor * string.h (cygwin_strchr): Make 'static inline' so that things will diff --git a/winsup/cygwin/thread.cc b/winsup/cygwin/thread.cc index deedcae42..ad2ad00c4 100644 --- a/winsup/cygwin/thread.cc +++ b/winsup/cygwin/thread.cc @@ -386,6 +386,7 @@ pthread_condattr::~pthread_condattr () pthread_cond::pthread_cond (pthread_condattr * attr):verifyable_object (PTHREAD_COND_MAGIC) { + int temperr; this->shared = attr ? attr->shared : PTHREAD_PROCESS_PRIVATE; this->mutex = NULL; this->waiting = 0; @@ -393,6 +394,14 @@ pthread_cond::pthread_cond (pthread_condattr * attr):verifyable_object (PTHREAD_ this->win32_obj_id =::CreateEvent (&sec_none_nih, false, /* auto signal reset - which I think is pthreads like ? */ false, /* start non signaled */ NULL /* no name */); + /* TODO: make a shared mem mutex if out attributes request shared mem cond */ + cond_access=NULL; + if ((temperr = pthread_mutex_init (&this->cond_access, NULL))) + { + system_printf ("couldn't init mutex, this %0p errno=%d\n", this, temperr); + /* we need the mutex for correct behaviour */ + magic = 0; + } if (!this->win32_obj_id) magic = 0; @@ -402,27 +411,38 @@ pthread_cond::~pthread_cond () { if (win32_obj_id) CloseHandle (win32_obj_id); + pthread_mutex_destroy (&cond_access); } void pthread_cond::BroadCast () { - // This potentially has an unfairness bug. We should - // consider preventing the wakeups from resuming until we finish signalling. - if (!verifyable_object_isvalid (mutex, PTHREAD_MUTEX_MAGIC)) - return; - PulseEvent (win32_obj_id); - while (InterlockedDecrement (&waiting) != 0) + if (pthread_mutex_lock (&cond_access)) + system_printf ("Failed to lock condition variable access mutex, this %0p\n", this); + int count = waiting; + if (!verifyable_object_isvalid (mutex, PTHREAD_MUTEX_MAGIC)) + { + if (pthread_mutex_unlock (&cond_access)) + system_printf ("Failed to unlock condition variable access mutex, this %0p\n", this); + system_printf ("Broadcast called with invalid mutex\n"); + return; + } + while (count--) PulseEvent (win32_obj_id); - mutex = NULL; + if (pthread_mutex_unlock (&cond_access)) + system_printf ("Failed to unlock condition variable access mutex, this %0p\n", this); } void pthread_cond::Signal () { + if (pthread_mutex_lock (&cond_access)) + system_printf ("Failed to lock condition variable access mutex, this %0p\n", this); if (!verifyable_object_isvalid (mutex, PTHREAD_MUTEX_MAGIC)) return; PulseEvent (win32_obj_id); + if (pthread_mutex_unlock (&cond_access)) + system_printf ("Failed to unlock condition variable access mutex, this %0p\n", this); } int @@ -1608,6 +1628,8 @@ int __pthread_cond_timedwait (pthread_cond_t * cond, pthread_mutex_t * mutex, const struct timespec *abstime) { +// and yes cond_access here is still open to a race. (we increment, context swap, +// broadcast occurs - we miss the broadcast. the functions aren't split properly. int rv; if (!abstime) return EINVAL; @@ -1623,18 +1645,31 @@ __pthread_cond_timedwait (pthread_cond_t * cond, pthread_mutex_t * mutex, if (!verifyable_object_isvalid (*cond, PTHREAD_COND_MAGIC)) return EINVAL; + if (pthread_mutex_lock (&(*cond)->cond_access)) + system_printf ("Failed to lock condition variable access mutex, this %0p\n", *cond); + if ((*cond)->waiting) if ((*cond)->mutex && ((*cond)->mutex != (*themutex))) - return EINVAL; + { + if (pthread_mutex_unlock (&(*cond)->cond_access)) + system_printf ("Failed to unlock condition variable access mutex, this %0p\n", *cond); + return EINVAL; + } InterlockedIncrement (&((*cond)->waiting)); (*cond)->mutex = (*themutex); InterlockedIncrement (&((*themutex)->condwaits)); + if (pthread_mutex_unlock (&(*cond)->cond_access)) + system_printf ("Failed to unlock condition variable access mutex, this %0p\n", *cond); rv = (*cond)->TimedWait (abstime->tv_sec * 1000); + if (pthread_mutex_lock (&(*cond)->cond_access)) + system_printf ("Failed to lock condition variable access mutex, this %0p\n", *cond); (*cond)->mutex->Lock (); if (InterlockedDecrement (&((*cond)->waiting)) == 0) (*cond)->mutex = NULL; InterlockedDecrement (&((*themutex)->condwaits)); + if (pthread_mutex_unlock (&(*cond)->cond_access)) + system_printf ("Failed to unlock condition variable access mutex, this %0p\n", *cond); return rv; } @@ -1642,6 +1677,7 @@ __pthread_cond_timedwait (pthread_cond_t * cond, pthread_mutex_t * mutex, int __pthread_cond_wait (pthread_cond_t * cond, pthread_mutex_t * mutex) { +// see cond_timedwait for notes int rv; pthread_mutex_t *themutex = mutex; if (*mutex == PTHREAD_MUTEX_INITIALIZER) @@ -1654,19 +1690,31 @@ __pthread_cond_wait (pthread_cond_t * cond, pthread_mutex_t * mutex) if (!verifyable_object_isvalid (*cond, PTHREAD_COND_MAGIC)) return EINVAL; + if (pthread_mutex_lock (&(*cond)->cond_access)) + system_printf ("Failed to lock condition variable access mutex, this %0p\n", *cond); + if ((*cond)->waiting) if ((*cond)->mutex && ((*cond)->mutex != (*themutex))) - return EINVAL; + { + if (pthread_mutex_unlock (&(*cond)->cond_access)) + system_printf ("Failed to unlock condition variable access mutex, this %0p\n", *cond); + return EINVAL; + } InterlockedIncrement (&((*cond)->waiting)); (*cond)->mutex = (*themutex); InterlockedIncrement (&((*themutex)->condwaits)); + if (pthread_mutex_unlock (&(*cond)->cond_access)) + system_printf ("Failed to unlock condition variable access mutex, this %0p\n", *cond); rv = (*cond)->TimedWait (INFINITE); + if (pthread_mutex_lock (&(*cond)->cond_access)) + system_printf ("Failed to lock condition variable access mutex, this %0p\n", *cond); (*cond)->mutex->Lock (); if (InterlockedDecrement (&((*cond)->waiting)) == 0) (*cond)->mutex = NULL; InterlockedDecrement (&((*themutex)->condwaits)); - + if (pthread_mutex_unlock (&(*cond)->cond_access)) + system_printf ("Failed to unlock condition variable access mutex, this %0p\n", *cond); return rv; } diff --git a/winsup/cygwin/thread.h b/winsup/cygwin/thread.h index a43d9feb5..98c4a0b53 100644 --- a/winsup/cygwin/thread.h +++ b/winsup/cygwin/thread.h @@ -290,6 +290,8 @@ public: int shared; LONG waiting; pthread_mutex *mutex; + /* to allow atomic behaviour for cond_broadcast */ + pthread_mutex_t cond_access; HANDLE win32_obj_id; int TimedWait (DWORD dwMilliseconds); void BroadCast (); -- 2.43.5