Skip to content

Commit

Permalink
Fix nasa#673 nasa#677, improve global lock on POSIX
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
jphickey committed Dec 10, 2020
1 parent 9407cdf commit a41f748
Show file tree
Hide file tree
Showing 7 changed files with 208 additions and 36 deletions.
137 changes: 105 additions & 32 deletions src/os/posix/src/os-impl-idmap.c
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand All @@ -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 */

Expand All @@ -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)
{
Expand All @@ -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
Expand All @@ -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
{
Expand All @@ -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)
Expand All @@ -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)
Expand All @@ -200,24 +262,35 @@ 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));
return_code = OS_ERROR;
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);
Expand Down
26 changes: 26 additions & 0 deletions src/os/rtems/src/os-impl-idmap.c
Original file line number Diff line number Diff line change
Expand Up @@ -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
***************************************************************************************/
Expand Down
16 changes: 16 additions & 0 deletions src/os/shared/inc/os-shared-common.h
Original file line number Diff line number Diff line change
Expand Up @@ -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_ */
13 changes: 13 additions & 0 deletions src/os/shared/inc/os-shared-idmap.h
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
42 changes: 39 additions & 3 deletions src/os/shared/src/osapi-idmap.c
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

/*
Expand Down Expand Up @@ -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
Expand Down
5 changes: 4 additions & 1 deletion src/unit-test-coverage/shared/src/coveragetest-idmap.c
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
5 changes: 5 additions & 0 deletions src/unit-test-coverage/ut-stubs/src/osapi-common-impl-stubs.c
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

0 comments on commit a41f748

Please sign in to comment.