Merge pull request #678 from jphickey/fix-673-677-global-locks

Fix #673 and #677, update global locks

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,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);

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
  ***************************************************************************************/

src/os/shared/inc/os-shared-common.h

@@ -128,4 +128,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  /* OS_SHARED_COMMON_H  */

src/os/shared/inc/os-shared-idmap.h

@@ -177,6 +177,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
 

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

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.

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);
+}