From a41f748e460eadd279370d69907d2c302efe798c Mon Sep 17 00:00:00 2001 From: Joseph Hickey Date: Wed, 9 Dec 2020 09:07:14 -0500 Subject: [PATCH] Fix #673 #677, improve global lock on POSIX Removes the signal mask updates from the POSIX global lock (not needed). Adds a condition variable to the structure, which can be used to directly wake up a waiting task rather than requiring that task to poll the global state. --- src/os/posix/src/os-impl-idmap.c | 137 ++++++++++++++---- src/os/rtems/src/os-impl-idmap.c | 26 ++++ src/os/shared/inc/os-shared-common.h | 16 ++ src/os/shared/inc/os-shared-idmap.h | 13 ++ src/os/shared/src/osapi-idmap.c | 42 +++++- .../shared/src/coveragetest-idmap.c | 5 +- .../ut-stubs/src/osapi-common-impl-stubs.c | 5 + 7 files changed, 208 insertions(+), 36 deletions(-) diff --git a/src/os/posix/src/os-impl-idmap.c b/src/os/posix/src/os-impl-idmap.c index 64b29aeef..348bb75ad 100644 --- a/src/os/posix/src/os-impl-idmap.c +++ b/src/os/posix/src/os-impl-idmap.c @@ -38,7 +38,7 @@ typedef struct { pthread_mutex_t mutex; - sigset_t sigmask; + pthread_cond_t cond; } POSIX_GlobalLock_t; static POSIX_GlobalLock_t OS_global_task_table_mut; @@ -75,6 +75,15 @@ enum MUTEX_TABLE_SIZE = (sizeof(MUTEX_TABLE) / sizeof(MUTEX_TABLE[0])) }; +/*--------------------------------------------------------------------------------------- + * Helper function for releasing the mutex in case the thread + * executing pthread_condwait() is canceled. + ----------------------------------------------------------------------------------------*/ +void OS_Posix_ReleaseTableMutex(void *mut) +{ + pthread_mutex_unlock(mut); +} + /*---------------------------------------------------------------- * * Function: OS_Lock_Global_Impl @@ -86,28 +95,27 @@ enum int32 OS_Lock_Global_Impl(osal_objtype_t idtype) { POSIX_GlobalLock_t *mut; - sigset_t previous; - - mut = MUTEX_TABLE[idtype]; + int ret; - if (mut == NULL) + if (idtype < MUTEX_TABLE_SIZE) { - return OS_ERROR; + mut = MUTEX_TABLE[idtype]; } - - if (pthread_sigmask(SIG_SETMASK, &POSIX_GlobalVars.MaximumSigMask, &previous) != 0) + else { - return OS_ERROR; + mut = NULL; } - if (pthread_mutex_lock(&mut->mutex) != 0) + if (mut != NULL) { - return OS_ERROR; + ret = pthread_mutex_lock(&mut->mutex); + if (ret != 0) + { + OS_DEBUG("pthread_mutex_lock(&mut->mutex): %s", strerror(ret)); + return OS_ERROR; + } } - /* Only set values inside the GlobalLock _after_ it is locked */ - mut->sigmask = previous; - return OS_SUCCESS; } /* end OS_Lock_Global_Impl */ @@ -122,7 +130,7 @@ int32 OS_Lock_Global_Impl(osal_objtype_t idtype) int32 OS_Unlock_Global_Impl(osal_objtype_t idtype) { POSIX_GlobalLock_t *mut; - sigset_t previous; + int ret; if (idtype < MUTEX_TABLE_SIZE) { @@ -133,23 +141,74 @@ int32 OS_Unlock_Global_Impl(osal_objtype_t idtype) mut = NULL; } - if (mut == NULL) + if (mut != NULL) { - return OS_ERROR; + /* Notify any waiting threads that the state _may_ have changed */ + ret = pthread_cond_broadcast(&mut->cond); + if (ret != 0) + { + OS_DEBUG("pthread_cond_broadcast(&mut->cond): %s", strerror(ret)); + /* unexpected but keep going (not critical) */ + } + + ret = pthread_mutex_unlock(&mut->mutex); + if (ret != 0) + { + OS_DEBUG("pthread_mutex_unlock(&mut->mutex): %s", strerror(ret)); + return OS_ERROR; + } } - /* Only get values inside the GlobalLock _before_ it is unlocked */ - previous = mut->sigmask; + return OS_SUCCESS; +} /* end OS_Unlock_Global_Impl */ + +/*---------------------------------------------------------------- + * + * Function: OS_WaitForStateChange_Impl + * + * Purpose: Implemented per internal OSAL API + * See prototype for argument/return detail + * + *-----------------------------------------------------------------*/ +void OS_WaitForStateChange_Impl(osal_objtype_t idtype, uint32 attempts) +{ + POSIX_GlobalLock_t *impl; + struct timespec ts; + + impl = MUTEX_TABLE[idtype]; - if (pthread_mutex_unlock(&mut->mutex) != 0) + if (impl != NULL) { - return OS_ERROR; - } + /* + * because pthread_cond_timedwait() is also a cancellation point, + * this pushes a cleanup handler to ensure that if canceled during this call, + * the mutex will be released. + */ + pthread_cleanup_push(OS_Posix_ReleaseTableMutex, &impl->mutex); - pthread_sigmask(SIG_SETMASK, &previous, NULL); + clock_gettime(CLOCK_REALTIME, &ts); - return OS_SUCCESS; -} /* end OS_Unlock_Global_Impl */ + if (attempts <= 10) + { + /* Wait an increasing amount of time, starting at 10ms */ + ts.tv_nsec += attempts * attempts * 10000000; + if (ts.tv_nsec >= 1000000000) + { + ts.tv_nsec -= 1000000000; + ++ts.tv_sec; + } + } + else + { + /* wait 1 second (max for polling) */ + ++ts.tv_sec; + } + + pthread_cond_timedwait(&impl->cond, &impl->mutex, &ts); + + pthread_cleanup_pop(false); + } +} /*--------------------------------------------------------------------------------------- Name: OS_Posix_TableMutex_Init @@ -163,6 +222,7 @@ int32 OS_Posix_TableMutex_Init(osal_objtype_t idtype) int ret; int32 return_code = OS_SUCCESS; pthread_mutexattr_t mutex_attr; + POSIX_GlobalLock_t *impl; do { @@ -171,14 +231,16 @@ int32 OS_Posix_TableMutex_Init(osal_objtype_t idtype) break; } + impl = MUTEX_TABLE[idtype]; + /* Initialize the table mutex for the given idtype */ - if (MUTEX_TABLE[idtype] == NULL) + if (impl == NULL) { break; } /* - ** initialize the pthread mutex attribute structure with default values + * initialize the pthread mutex attribute structure with default values */ ret = pthread_mutexattr_init(&mutex_attr); if (ret != 0) @@ -189,7 +251,7 @@ int32 OS_Posix_TableMutex_Init(osal_objtype_t idtype) } /* - ** Allow the mutex to use priority inheritance + * Allow the mutex to use priority inheritance */ ret = pthread_mutexattr_setprotocol(&mutex_attr, PTHREAD_PRIO_INHERIT); if (ret != 0) @@ -200,10 +262,10 @@ int32 OS_Posix_TableMutex_Init(osal_objtype_t idtype) } /* - ** Set the mutex type to RECURSIVE so a thread can do nested locks - ** TBD - not sure if this is really desired, but keep it for now. + * Use normal (faster/non-recursive) mutex implementation + * There should not be any instances of OSAL locking its own table more than once. */ - ret = pthread_mutexattr_settype(&mutex_attr, PTHREAD_MUTEX_RECURSIVE); + ret = pthread_mutexattr_settype(&mutex_attr, PTHREAD_MUTEX_NORMAL); if (ret != 0) { OS_DEBUG("Error: pthread_mutexattr_settype failed: %s\n", strerror(ret)); @@ -211,13 +273,24 @@ int32 OS_Posix_TableMutex_Init(osal_objtype_t idtype) break; } - ret = pthread_mutex_init(&MUTEX_TABLE[idtype]->mutex, &mutex_attr); + ret = pthread_mutex_init(&impl->mutex, &mutex_attr); if (ret != 0) { OS_DEBUG("Error: pthread_mutex_init failed: %s\n", strerror(ret)); return_code = OS_ERROR; break; } + + /* create a condition variable with default attributes. + * This will be broadcast every time the object table changes */ + ret = pthread_cond_init(&impl->cond, NULL); + if (ret != 0) + { + OS_DEBUG("Error: pthread_cond_init failed: %s\n", strerror(ret)); + return_code = OS_ERROR; + break; + } + } while (0); return (return_code); diff --git a/src/os/rtems/src/os-impl-idmap.c b/src/os/rtems/src/os-impl-idmap.c index 7e08b6986..5036e97b1 100644 --- a/src/os/rtems/src/os-impl-idmap.c +++ b/src/os/rtems/src/os-impl-idmap.c @@ -146,6 +146,32 @@ int32 OS_Unlock_Global_Impl(osal_objtype_t idtype) return OS_SUCCESS; } /* end OS_Unlock_Global_Impl */ +/*---------------------------------------------------------------- + * + * Function: OS_WaitForStateChange_Impl + * + * Purpose: Implemented per internal OSAL API + * See prototype for argument/return detail + * + *-----------------------------------------------------------------*/ +void OS_WaitForStateChange_Impl(osal_objtype_t idtype, uint32 attempts) +{ + uint32 wait_ms; + + if (attempts <= 10) + { + wait_ms = attempts * attempts * 10; + } + else + { + wait_ms = 1000; + } + + OS_Unlock_Global_Impl(idtype); + OS_TaskDelay(wait_ms); + OS_Lock_Global_Impl(idtype); +} + /**************************************************************************************** INITIALIZATION FUNCTION ***************************************************************************************/ diff --git a/src/os/shared/inc/os-shared-common.h b/src/os/shared/inc/os-shared-common.h index 7ad513e24..0cc8a07db 100644 --- a/src/os/shared/inc/os-shared-common.h +++ b/src/os/shared/inc/os-shared-common.h @@ -127,4 +127,20 @@ void OS_IdleLoop_Impl(void); ------------------------------------------------------------------*/ void OS_ApplicationShutdown_Impl(void); +/*---------------------------------------------------------------- + + Function: OS_WaitForStateChange_Impl + + Purpose: Block the caller until some sort of change event + has occurred for the given object type, such as a record changing + state i.e. the acquisition or release of a lock/refcount from + another thread. + + It is not guaranteed what, if any, state change has actually + occured when this function returns. This may be implement as + a simple OS_TaskDelay(). + + ------------------------------------------------------------------*/ +void OS_WaitForStateChange_Impl(osal_objtype_t objtype, uint32 attempts); + #endif /* INCLUDE_OS_SHARED_COMMON_H_ */ diff --git a/src/os/shared/inc/os-shared-idmap.h b/src/os/shared/inc/os-shared-idmap.h index a63d2eea6..a9e244137 100644 --- a/src/os/shared/inc/os-shared-idmap.h +++ b/src/os/shared/inc/os-shared-idmap.h @@ -176,6 +176,19 @@ void OS_Unlock_Global(osal_objtype_t idtype); ------------------------------------------------------------------*/ int32 OS_Unlock_Global_Impl(osal_objtype_t idtype); +/*---------------------------------------------------------------- + + Function: OS_WaitForStateChange + + Purpose: Waits for a change in the global table identified by "idtype" + + NOTE: The table must be already "owned" (via OS_Lock_Global) by the calling + at the time this function is invoked. The lock is released and re-acquired + before returning from this function. + + -----------------------------------------------------------------*/ +void OS_WaitForStateChange(osal_objtype_t idtype, uint32 attempts); + /* Function prototypes for routines implemented in common layers but private to OSAL diff --git a/src/os/shared/src/osapi-idmap.c b/src/os/shared/src/osapi-idmap.c index 0ccaf16f0..c4d944ea6 100644 --- a/src/os/shared/src/osapi-idmap.c +++ b/src/os/shared/src/osapi-idmap.c @@ -433,9 +433,10 @@ int32 OS_ObjectIdConvertToken(OS_object_token_t *token) break; } - OS_Unlock_Global(token->obj_type); - OS_TaskDelay_Impl(attempts); - OS_Lock_Global(token->obj_type); + /* + * Call the impl layer to wait for some sort of change to occur. + */ + OS_WaitForStateChange(token->obj_type, attempts); } /* @@ -730,6 +731,41 @@ void OS_Unlock_Global(osal_objtype_t idtype) } } +/*---------------------------------------------------------------- + * + * Function: OS_WaitForStateChange + * + * Purpose: Local helper routine, not part of OSAL API. + * Waits for a change in the global table identified by "idtype" + * + *-----------------------------------------------------------------*/ +void OS_WaitForStateChange(osal_objtype_t idtype, uint32 attempts) +{ + osal_id_t saved_owner_id; + OS_objtype_state_t *objtype; + + if (idtype < OS_OBJECT_TYPE_USER) + { + objtype = &OS_objtype_state[idtype]; + saved_owner_id = objtype->table_owner; + + /* temporarily release the table */ + objtype->table_owner = OS_OBJECT_ID_UNDEFINED; + + /* + * The implementation layer takes care of the actual unlock + wait. + * This permits use of condition variables where these two actions + * are done atomically. + */ + OS_WaitForStateChange_Impl(idtype, attempts); + + /* + * After return, this task owns the table again + */ + objtype->table_owner = saved_owner_id; + } +} + /*---------------------------------------------------------------- * * Function: OS_ObjectIdFinalizeNew diff --git a/src/unit-test-coverage/shared/src/coveragetest-idmap.c b/src/unit-test-coverage/shared/src/coveragetest-idmap.c index b5bd75288..178f69801 100644 --- a/src/unit-test-coverage/shared/src/coveragetest-idmap.c +++ b/src/unit-test-coverage/shared/src/coveragetest-idmap.c @@ -187,7 +187,10 @@ void Test_OS_ObjectIdConvertToken(void) (long)actual, (long)expected); /* should have delayed 4 times, on the 5th try it returns error */ - UtAssert_STUB_COUNT(OS_TaskDelay_Impl, 4); + UtAssert_STUB_COUNT(OS_WaitForStateChange_Impl, 4); + + /* It should also have preserved the original ID */ + UtAssert_True(OS_ObjectIdEqual(record->active_id, objid), "OS_ObjectIdConvertLock(EXCLUSIVE) objid restored"); /* * Use mode OS_LOCK_MODE_EXCLUSIVE with matching ID and no other refs. diff --git a/src/unit-test-coverage/ut-stubs/src/osapi-common-impl-stubs.c b/src/unit-test-coverage/ut-stubs/src/osapi-common-impl-stubs.c index 5f3ee077a..07edfa887 100644 --- a/src/unit-test-coverage/ut-stubs/src/osapi-common-impl-stubs.c +++ b/src/unit-test-coverage/ut-stubs/src/osapi-common-impl-stubs.c @@ -45,3 +45,8 @@ void OS_ApplicationShutdown_Impl(void) { UT_DEFAULT_IMPL(OS_ApplicationShutdown_Impl); } + +void OS_WaitForStateChange_Impl(osal_objtype_t objtype, uint32 attempts) +{ + UT_DEFAULT_IMPL(OS_WaitForStateChange_Impl); +}