score: Fix and simplify thread wait locks

There was a subtile race condition in _Thread_queue_Do_extract_locked().
It must first update the thread wait flags and then restore the default
thread wait state.  In the previous implementation this could lead under
rare timing conditions to an ineffective _Thread_Wait_tranquilize()
resulting to a corrupt system state.

Update #2556.

4 files changed, 93 insertions, 194 deletions

diff --git a/cpukit/score/include/rtems/score/thread.h b/cpukit/score/include/rtems/score/thread.h
index d03f0c25e5..44080f3a82 100644
--- a/cpukit/score/include/rtems/score/thread.h
+++ b/cpukit/score/include/rtems/score/thread.h
@@ -326,6 +326,17 @@ typedef struct {
      * case the thread is enqueued on a thread queue.
      */
     Chain_Control Pending_requests;
+
+    /**
+     * @brief Tranquilizer gate used by _Thread_Wait_tranquilize().
+     *
+     * This gate is closed by _Thread_Wait_claim().  In case there are no
+     * pending requests during a _Thread_Wait_restore_default(), then this gate
+     * is opened immediately, otherwise it is placed on the pending request
+     * chain and opened by _Thread_Wait_remove_request_locked() as the last
+     * gate on the chain to signal overall request completion.
+     */
+    Thread_queue_Gate Tranquilizer;
   } Lock;
 
   /**
diff --git a/cpukit/score/include/rtems/score/threadimpl.h b/cpukit/score/include/rtems/score/threadimpl.h
index ea1c61fa1e..965b2d190c 100644
--- a/cpukit/score/include/rtems/score/threadimpl.h
+++ b/cpukit/score/include/rtems/score/threadimpl.h
@@ -1036,39 +1036,36 @@ RTEMS_INLINE_ROUTINE void _Thread_Wait_remove_request_locked(
   Thread_queue_Context *queue_context
 )
 {
-  _Chain_Extract_unprotected( &queue_context->Wait.Gate.Node );
-
-  if ( !_Chain_Is_empty( &the_thread->Wait.Lock.Pending_requests ) ) {
-    Thread_queue_Context *first;
+  Chain_Node *first;
 
-    first = THREAD_QUEUE_CONTEXT_OF_REQUEST(
-      _Chain_First( &the_thread->Wait.Lock.Pending_requests )
-    );
+  _Chain_Extract_unprotected( &queue_context->Wait.Gate.Node );
+  first = _Chain_First( &the_thread->Wait.Lock.Pending_requests );
 
-    _Thread_queue_Gate_open( &first->Wait.Gate );
+  if ( first != _Chain_Tail( &the_thread->Wait.Lock.Pending_requests ) ) {
+    _Thread_queue_Gate_open( (Thread_queue_Gate *) first );
   }
 }
 
 RTEMS_INLINE_ROUTINE void _Thread_Wait_acquire_queue_critical(
-  SMP_ticket_lock_Control *queue_lock,
-  Thread_queue_Context    *queue_context
+  Thread_queue_Queue   *queue,
+  Thread_queue_Context *queue_context
 )
 {
-  _SMP_ticket_lock_Acquire(
-    queue_lock,
+  _Thread_queue_Queue_acquire_critical(
+    queue,
     &_Thread_Executing->Potpourri_stats,
-    &queue_context->Lock_context.Lock_context.Stats_context
+    &queue_context->Lock_context
   );
 }
 
 RTEMS_INLINE_ROUTINE void _Thread_Wait_release_queue_critical(
-  SMP_ticket_lock_Control *queue_lock,
-  Thread_queue_Context    *queue_context
+  Thread_queue_Queue   *queue,
+  Thread_queue_Context *queue_context
 )
 {
-  _SMP_ticket_lock_Release(
-    queue_lock,
-    &queue_context->Lock_context.Lock_context.Stats_context
+  _Thread_queue_Queue_release_critical(
+    queue,
+    &queue_context->Lock_context
   );
 }
 #endif
@@ -1098,22 +1095,27 @@ RTEMS_INLINE_ROUTINE void _Thread_Wait_acquire_critical(
 
   queue = the_thread->Wait.queue;
   queue_context->Wait.queue = queue;
-  queue_context->Wait.operations = the_thread->Wait.operations;
 
   if ( queue != NULL ) {
-    queue_context->Wait.queue_lock = &queue->Lock;
-    _Chain_Initialize_node( &queue_context->Wait.Gate.Node );
-    _Chain_Append_unprotected(
+    _Thread_queue_Gate_add(
       &the_thread->Wait.Lock.Pending_requests,
-      &queue_context->Wait.Gate.Node
+      &queue_context->Wait.Gate
     );
     _Thread_Wait_release_default_critical(
       the_thread,
       &queue_context->Lock_context
     );
-    _Thread_Wait_acquire_queue_critical( &queue->Lock, queue_context );
-  } else {
-    queue_context->Wait.queue_lock = NULL;
+    _Thread_Wait_acquire_queue_critical( queue, queue_context );
+
+    if ( queue_context->Wait.queue == NULL ) {
+      _Thread_Wait_release_queue_critical( queue, queue_context );
+      _Thread_Wait_acquire_default_critical(
+        the_thread,
+        &queue_context->Lock_context
+      );
+      _Thread_Wait_remove_request_locked( the_thread, queue_context );
+      _Assert( the_thread->Wait.queue == NULL );
+    }
   }
 #else
   (void) the_thread;
@@ -1154,12 +1156,12 @@ RTEMS_INLINE_ROUTINE void _Thread_Wait_release_critical(
 )
 {
 #if defined(RTEMS_SMP)
-  SMP_ticket_lock_Control *queue_lock;
+  Thread_queue_Queue *queue;
 
-  queue_lock = queue_context->Wait.queue_lock;
+  queue = queue_context->Wait.queue;
 
-  if ( queue_lock != NULL ) {
-    _Thread_Wait_release_queue_critical( queue_lock, queue_context );
+  if ( queue != NULL ) {
+    _Thread_Wait_release_queue_critical( queue, queue_context );
     _Thread_Wait_acquire_default_critical(
       the_thread,
       &queue_context->Lock_context
@@ -1218,6 +1220,13 @@ RTEMS_INLINE_ROUTINE void _Thread_Wait_claim(
   _Thread_Wait_acquire_default_critical( the_thread, &lock_context );
 
   _Assert( the_thread->Wait.queue == NULL );
+
+#if defined(RTEMS_SMP)
+  _Chain_Initialize_empty( &the_thread->Wait.Lock.Pending_requests );
+  _Chain_Initialize_node( &the_thread->Wait.Lock.Tranquilizer.Node );
+  _Thread_queue_Gate_close( &the_thread->Wait.Lock.Tranquilizer );
+#endif
+
   the_thread->Wait.queue = queue;
   the_thread->Wait.operations = operations;
 
@@ -1273,22 +1282,27 @@ RTEMS_INLINE_ROUTINE void _Thread_Wait_restore_default(
   Chain_Node       *node;
   const Chain_Node *tail;
 
-  _Thread_Wait_acquire_default_critical(
-    the_thread,
-    &lock_context
-  );
+  _Thread_Wait_acquire_default_critical( the_thread, &lock_context );
 
   node = _Chain_First( &the_thread->Wait.Lock.Pending_requests );
   tail = _Chain_Immutable_tail( &the_thread->Wait.Lock.Pending_requests );
 
-  while ( node != tail ) {
-    Thread_queue_Context *queue_context;
+  if ( node != tail ) {
+    do {
+      Thread_queue_Context *queue_context;
 
-    queue_context = THREAD_QUEUE_CONTEXT_OF_REQUEST( node );
-    queue_context->Wait.queue = NULL;
-    queue_context->Wait.operations = &_Thread_queue_Operations_stale_queue;
+      queue_context = THREAD_QUEUE_CONTEXT_OF_REQUEST( node );
+      queue_context->Wait.queue = NULL;
 
-    node = _Chain_Next( node );
+      node = _Chain_Next( node );
+    } while ( node != tail );
+
+    _Thread_queue_Gate_add(
+      &the_thread->Wait.Lock.Pending_requests,
+      &the_thread->Wait.Lock.Tranquilizer
+    );
+  } else {
+    _Thread_queue_Gate_open( &the_thread->Wait.Lock.Tranquilizer );
   }
 #endif
 
@@ -1296,15 +1310,12 @@ RTEMS_INLINE_ROUTINE void _Thread_Wait_restore_default(
   the_thread->Wait.operations = &_Thread_queue_Operations_default;
 
 #if defined(RTEMS_SMP)
-  _Thread_Wait_release_default_critical(
-    the_thread,
-    &lock_context
-  );
+  _Thread_Wait_release_default_critical( the_thread, &lock_context );
 #endif
 }
 
 /**
- * @brief Tranquilizes the thread after a wait a thread queue.
+ * @brief Tranquilizes the thread after a wait on a thread queue.
  *
  * After the violent blocking procedure this function makes the thread calm and
  * peaceful again so that it can carry out its normal work.
@@ -1314,6 +1325,11 @@ RTEMS_INLINE_ROUTINE void _Thread_Wait_restore_default(
  *
  * On other configurations, this function does nothing.
  *
+ * It must be called after a _Thread_Wait_claim() exactly once
+ *  - after the corresponding thread queue lock was released, and
+ *  - the default wait state is restored or some other processor is about to do
+ *    this.
+ *
  * @param[in] the_thread The thread.
  */
 RTEMS_INLINE_ROUTINE void _Thread_Wait_tranquilize(
@@ -1321,22 +1337,7 @@ RTEMS_INLINE_ROUTINE void _Thread_Wait_tranquilize(
 )
 {
 #if defined(RTEMS_SMP)
-  Thread_queue_Context queue_context;
-
-  _Thread_queue_Context_initialize( &queue_context );
-  _Thread_Wait_acquire_default( the_thread, &queue_context.Lock_context );
-
-  if ( !_Chain_Is_empty( &the_thread->Wait.Lock.Pending_requests ) ) {
-    _Thread_queue_Gate_add(
-      &the_thread->Wait.Lock.Pending_requests,
-      &queue_context.Wait.Gate
-    );
-    _Thread_Wait_release_default( the_thread, &queue_context.Lock_context );
-    _Thread_queue_Gate_wait( &queue_context.Wait.Gate );
-    _Thread_Wait_remove_request( the_thread, &queue_context );
-  } else {
-    _Thread_Wait_release_default( the_thread, &queue_context.Lock_context );
-  }
+  _Thread_queue_Gate_wait( &the_thread->Wait.Lock.Tranquilizer );
 #else
   (void) the_thread;
 #endif
@@ -1354,13 +1355,21 @@ RTEMS_INLINE_ROUTINE void _Thread_Wait_cancel(
   Thread_queue_Context *queue_context
 )
 {
-  _Thread_queue_Context_extract( queue_context, the_thread );
+  Thread_queue_Queue *queue;
+
+  queue = the_thread->Wait.queue;
 
 #if defined(RTEMS_SMP)
-  if ( queue_context->Wait.queue != NULL ) {
+  if ( queue != NULL ) {
+    _Assert( queue_context->Wait.queue == queue );
 #endif
+
+    ( *the_thread->Wait.operations->extract )( queue, the_thread );
     _Thread_Wait_restore_default( the_thread );
+
 #if defined(RTEMS_SMP)
+    _Assert( queue_context->Wait.queue == NULL );
+    queue_context->Wait.queue = queue;
   }
 #endif
 }
diff --git a/cpukit/score/include/rtems/score/threadq.h b/cpukit/score/include/rtems/score/threadq.h
index a39a031905..3c689ac974 100644
--- a/cpukit/score/include/rtems/score/threadq.h
+++ b/cpukit/score/include/rtems/score/threadq.h
@@ -164,27 +164,9 @@ typedef struct {
     Thread_queue_Gate Gate;
 
     /**
-     * @brief The thread queue lock in case the thread is blocked on a thread
-     * queue at thread wait lock acquire time.
-     */
-    SMP_ticket_lock_Control *queue_lock;
-
-    /**
-     * @brief The thread queue after thread wait lock acquire.
-     *
-     * In case the thread queue is NULL and the thread queue lock is non-NULL
-     * in this context, then we have a stale thread queue.  This happens in
-     * case the thread wait default is restored while we wait on the thread
-     * queue lock, e.g. during a mutex ownership transfer.
-     *
-     * @see _Thread_Wait_restore_default().
+     * @brief The thread queue in case the thread is blocked on a thread queue.
      */
     Thread_queue_Queue *queue;
-
-    /**
-     * @brief The thread queue operations after thread wait lock acquire.
-     */
-    const Thread_queue_Operations *operations;
   } Wait;
 #endif
 } Thread_queue_Context;
@@ -349,19 +331,16 @@ struct Thread_queue_Queue {
 /**
  * @brief Thread queue priority change operation.
  *
+ * @param[in] queue The actual thread queue.
  * @param[in] the_thread The thread.
  * @param[in] new_priority The new priority value.
- * @param[in] prepend_it In case this is true, then the thread is prepended to
- *   its priority group in its scheduler instance, otherwise it is appended.
- * @param[in] queue The actual thread queue.
  *
  * @see Thread_queue_Operations.
  */
 typedef void ( *Thread_queue_Priority_change_operation )(
+  Thread_queue_Queue *queue,
   Thread_Control     *the_thread,
-  Priority_Control    new_priority,
-  bool                prepend_it,
-  Thread_queue_Queue *queue
+  Priority_Control    new_priority
 );
 
 /**
diff --git a/cpukit/score/include/rtems/score/threadqimpl.h b/cpukit/score/include/rtems/score/threadqimpl.h
index f0ca614ba1..75ef4dd87e 100644
--- a/cpukit/score/include/rtems/score/threadqimpl.h
+++ b/cpukit/score/include/rtems/score/threadqimpl.h
@@ -234,115 +234,19 @@ RTEMS_INLINE_ROUTINE void _Thread_queue_Context_set_MP_callout(
   } while ( 0 )
 #endif
 
-/**
- * @brief Gets the thread wait queue of the thread queue context.
- *
- * On SMP configurations, the value is stored in the thread queue context,
- * otherwise in the thread itself.
- *
- * @param queue_context The thread queue context.
- * @param the_thread The thread.
- */
-#if defined(RTEMS_SMP)
-#define _Thread_queue_Context_get_queue( queue_context, the_thread ) \
-  ( queue_context )->Wait.queue
-#else
-#define _Thread_queue_Context_get_queue( queue_context, the_thread ) \
-  ( the_thread )->Wait.queue
-#endif
-
-/**
- * @brief Gets the thread wait operations of the thread queue context.
- *
- * On SMP configurations, the value is stored in the thread queue context,
- * otherwise in the thread itself.
- *
- * @param queue_context The thread queue context.
- * @param the_thread The thread.
- */
-#if defined(RTEMS_SMP)
-#define _Thread_queue_Context_get_operations( queue_context, the_thread ) \
-  ( queue_context )->Wait.operations
-#else
-#define _Thread_queue_Context_get_operations( queue_context, the_thread ) \
-  ( the_thread )->Wait.operations
-#endif
-
-/**
- * @brief Thread priority change by means of the thread queue context.
- *
- * On SMP configurations, the used data is stored in the thread queue context,
- * otherwise in the thread itself.
- *
- * @param queue_context The thread queue context.
- * @param the_thread The thread.
- * @param new_priority The new thread priority.
- * @param prepend_it Prepend it to its priority group or not.
- */
-#if defined(RTEMS_SMP)
-#define _Thread_queue_Context_priority_change( \
-    queue_context, \
-    the_thread, \
-    new_priority, \
-    prepend_it \
-  ) \
-    ( *( queue_context )->Wait.operations->priority_change )( \
-      the_thread, \
-      new_priority, \
-      prepend_it, \
-      ( queue_context )->Wait.queue \
-    )
-#else
-#define _Thread_queue_Context_priority_change( \
-    queue_context, \
-    the_thread, \
-    new_priority, \
-    prepend_it \
-  ) \
-    ( *( the_thread )->Wait.operations->priority_change )( \
-      the_thread, \
-      new_priority, \
-      prepend_it, \
-      ( the_thread )->Wait.queue \
-    )
-#endif
-
-/**
- * @brief Thread queue extract by means of the thread queue context.
- *
- * On SMP configurations, the used data is stored in the thread queue context,
- * otherwise in the thread itself.
- *
- * @param queue_context The thread queue context.
- * @param the_thread The thread.
- */
 #if defined(RTEMS_SMP)
-#define _Thread_queue_Context_extract( \
-    queue_context, \
-    the_thread \
-  ) \
-    ( *( queue_context )->Wait.operations->extract )( \
-      ( queue_context )->Wait.queue, \
-      the_thread \
-    )
-#else
-#define _Thread_queue_Context_extract( \
-    queue_context, \
-    the_thread \
-  ) \
-    ( *( the_thread )->Wait.operations->extract )( \
-      ( the_thread )->Wait.queue, \
-      the_thread \
-    )
-#endif
+RTEMS_INLINE_ROUTINE void _Thread_queue_Gate_close(
+  Thread_queue_Gate *gate
+)
+{
+  _Atomic_Store_uint( &gate->go_ahead, 0, ATOMIC_ORDER_RELAXED );
+}
 
-#if defined(RTEMS_SMP)
 RTEMS_INLINE_ROUTINE void _Thread_queue_Gate_add(
   Chain_Control     *chain,
   Thread_queue_Gate *gate
 )
 {
-  _Atomic_Store_uint( &gate->go_ahead, 0, ATOMIC_ORDER_RELAXED );
   _Chain_Append_unprotected( chain, &gate->Node );
 }
 
@@ -1087,10 +991,6 @@ extern const Thread_queue_Operations _Thread_queue_Operations_priority;
 
 extern const Thread_queue_Operations _Thread_queue_Operations_priority_inherit;
 
-#if defined(RTEMS_SMP)
-extern const Thread_queue_Operations _Thread_queue_Operations_stale_queue;
-#endif
-
 /**@}*/
 
 #ifdef __cplusplus