[PATCH] score: Add Thread_queue_Deadlock_status
Gedare Bloom
gedare at rtems.org
Fri Oct 1 04:11:13 UTC 2021
On Fri, Sep 24, 2021 at 12:22 PM Sebastian Huber
<sebastian.huber at embedded-brains.de> wrote:
>
> Replace the boolen return value with the new enum
> Thread_queue_Deadlock_status. This improves the code readability.
> Improve documentation. Shorten function names.
> ---
> cpukit/include/rtems/score/threadqimpl.h | 50 ++++++++++++++-----
> cpukit/score/src/threadchangepriority.c | 4 +-
> cpukit/score/src/threadqenqueue.c | 63 ++++++++++++++----------
> cpukit/score/src/threadqops.c | 4 +-
> 4 files changed, 80 insertions(+), 41 deletions(-)
>
> diff --git a/cpukit/include/rtems/score/threadqimpl.h b/cpukit/include/rtems/score/threadqimpl.h
> index 22e0c7f069..d4fa8b8316 100644
> --- a/cpukit/include/rtems/score/threadqimpl.h
> +++ b/cpukit/include/rtems/score/threadqimpl.h
> @@ -1339,28 +1339,56 @@ void _Thread_queue_Unblock_proxy(
> #endif
>
> /**
> - * @brief Acquires the thread queue path in a critical section.
> + * @brief This is a status code to indicate if a deadlock was detected or not.
> + */
> +typedef enum {
> + /**
> + * @brief The operation detected a deadlock.
> + */
> + THREAD_QUEUE_DEADLOCK_DETECTED,
> +
> + /**
> + * @brief The operation did not detect a deadlock.
> + */
> + THREAD_QUEUE_NO_DEADLOCK
> +} Thread_queue_Deadlock_status;
> +
A minor note, I might switch the order of the two enum values so that
THREAD_QUEUE_NO_DEADLOCK==0==false, as this is more consistent with
the previous usage of the bool variable statuses. However, this should
not matter since you replaced the only spots the boolean value was
being used with explicit checks against the enum type. So feel free
to ignore :)
> +#if defined(RTEMS_SMP)
> +/**
> + * @brief Acquires the thread queue path.
> *
> - * @param queue The thread queue queue.
> - * @param the_thread The thread for the operation.
> - * @param queue_context The thread queue context.
> + * The caller must own the thread queue lock.
> + *
> + * A acquired thread queue path must be released by calling
minor: A -> An
Rest of this looks good.
> + * _Thread_queue_Path_release() with the same thread queue context.
> + *
> + * @param queue is the thread queue queue.
> + *
> + * @param the_thread is the thread for the operation.
> + *
> + * @param queue_context is the thread queue context.
> *
> - * @retval true The operation was successful.
> - * @retval false The operation failed.
> + * @retval THREAD_QUEUE_NO_DEADLOCK No deadlock was detected.
> + *
> + * @retval THREAD_QUEUE_DEADLOCK_DETECTED A deadlock was detected while
> + * acquiring the thread queue path. The thread queue path must still be
> + * released by _Thread_queue_Path_release() in this case.
> */
> -#if defined(RTEMS_SMP)
> -bool _Thread_queue_Path_acquire_critical(
> +Thread_queue_Deadlock_status _Thread_queue_Path_acquire(
> Thread_queue_Queue *queue,
> Thread_Control *the_thread,
> Thread_queue_Context *queue_context
> );
>
> /**
> - * @brief Releases the thread queue path in a critical section.
> + * @brief Releases the thread queue path.
> *
> - * @param queue_context The thread queue context.
> + * The caller must have acquired the thread queue path with a corresponding
> + * _Thread_queue_Path_acquire().
> + *
> + * @param queue_context is the thread queue context.
> */
> -void _Thread_queue_Path_release_critical(
> +void _Thread_queue_Path_release(
> Thread_queue_Context *queue_context
> );
> #endif
> diff --git a/cpukit/score/src/threadchangepriority.c b/cpukit/score/src/threadchangepriority.c
> index 613d0cd7af..bd4fef279b 100644
> --- a/cpukit/score/src/threadchangepriority.c
> +++ b/cpukit/score/src/threadchangepriority.c
> @@ -278,11 +278,11 @@ static void _Thread_Priority_apply(
>
> if ( !_Priority_Actions_is_empty( &queue_context->Priority.Actions ) ) {
> #if defined(RTEMS_SMP)
> - _Thread_queue_Path_acquire_critical( queue, the_thread, queue_context );
> + (void) _Thread_queue_Path_acquire( queue, the_thread, queue_context );
> #endif
> _Thread_Priority_perform_actions( queue->owner, queue_context );
> #if defined(RTEMS_SMP)
> - _Thread_queue_Path_release_critical( queue_context );
> + _Thread_queue_Path_release( queue_context );
> #endif
> }
> }
> diff --git a/cpukit/score/src/threadqenqueue.c b/cpukit/score/src/threadqenqueue.c
> index 1b8b82eab9..ee5c6fff5e 100644
> --- a/cpukit/score/src/threadqenqueue.c
> +++ b/cpukit/score/src/threadqenqueue.c
> @@ -8,10 +8,9 @@
> * _Thread_queue_Do_dequeue(), _Thread_queue_Enqueue(),
> * _Thread_queue_Enqueue_do_nothing_extra(), _Thread_queue_Enqueue_sticky(),
> * _Thread_queue_Extract(), _Thread_queue_Extract_locked(),
> - * _Thread_queue_Path_acquire_critical(),
> - * _Thread_queue_Path_release_critical(),
> + * _Thread_queue_Path_acquire(), _Thread_queue_Path_release(),
> * _Thread_queue_Resume(),_Thread_queue_Surrender(),
> - * _Thread_queue_Surrender_sticky().
> + * _Thread_queue_Surrender_no_priority(), _Thread_queue_Surrender_sticky().
> */
>
> /*
> @@ -113,7 +112,7 @@ static Thread_queue_Link *_Thread_queue_Link_find(
> );
> }
>
> -static bool _Thread_queue_Link_add(
> +static Thread_queue_Deadlock_status _Thread_queue_Link_add(
> Thread_queue_Link *link,
> Thread_queue_Queue *source,
> Thread_queue_Queue *target
> @@ -144,7 +143,7 @@ static bool _Thread_queue_Link_add(
>
> if ( recursive_target == source ) {
> _ISR_lock_Release( &links->Lock, &lock_context );
> - return false;
> + return THREAD_QUEUE_DEADLOCK_DETECTED;
> }
> }
>
> @@ -156,7 +155,7 @@ static bool _Thread_queue_Link_add(
> );
>
> _ISR_lock_Release( &links->Lock, &lock_context );
> - return true;
> + return THREAD_QUEUE_NO_DEADLOCK;
> }
>
> static void _Thread_queue_Link_remove( Thread_queue_Link *link )
> @@ -175,7 +174,7 @@ static void _Thread_queue_Link_remove( Thread_queue_Link *link )
> #if !defined(RTEMS_SMP)
> static
> #endif
> -void _Thread_queue_Path_release_critical(
> +void _Thread_queue_Path_release(
> Thread_queue_Context *queue_context
> )
> {
> @@ -260,7 +259,7 @@ static void _Thread_queue_Path_append_deadlock_thread(
> #if !defined(RTEMS_SMP)
> static
> #endif
> -bool _Thread_queue_Path_acquire_critical(
> +Thread_queue_Deadlock_status _Thread_queue_Path_acquire(
> Thread_queue_Queue *queue,
> Thread_Control *the_thread,
> Thread_queue_Context *queue_context
> @@ -284,11 +283,11 @@ bool _Thread_queue_Path_acquire_critical(
> owner = queue->owner;
>
> if ( owner == NULL ) {
> - return true;
> + return THREAD_QUEUE_NO_DEADLOCK;
> }
>
> if ( owner == the_thread ) {
> - return false;
> + return THREAD_QUEUE_DEADLOCK_DETECTED;
> }
>
> _Chain_Initialize_node(
> @@ -311,7 +310,11 @@ bool _Thread_queue_Path_acquire_critical(
> link->Lock_context.Wait.queue = target;
>
> if ( target != NULL ) {
> - if ( _Thread_queue_Link_add( link, queue, target ) ) {
> + Thread_queue_Deadlock_status deadlock_status;
> +
> + deadlock_status = _Thread_queue_Link_add( link, queue, target );
> +
> + if ( deadlock_status == THREAD_QUEUE_NO_DEADLOCK ) {
> _Thread_queue_Gate_add(
> &owner->Wait.Lock.Pending_requests,
> &link->Lock_context.Wait.Gate
> @@ -331,15 +334,15 @@ bool _Thread_queue_Path_acquire_critical(
> );
> _Thread_Wait_remove_request_locked( owner, &link->Lock_context );
> _Assert( owner->Wait.queue == NULL );
> - return true;
> + return THREAD_QUEUE_NO_DEADLOCK;
> }
> } else {
> link->Lock_context.Wait.queue = NULL;
> _Thread_queue_Path_append_deadlock_thread( owner, queue_context );
> - return false;
> + return THREAD_QUEUE_DEADLOCK_DETECTED;
> }
> } else {
> - return true;
> + return THREAD_QUEUE_NO_DEADLOCK;
> }
>
> link = &owner->Wait.Link;
> @@ -351,18 +354,18 @@ bool _Thread_queue_Path_acquire_critical(
> owner = queue->owner;
>
> if ( owner == NULL ) {
> - return true;
> + return THREAD_QUEUE_NO_DEADLOCK;
> }
>
> if ( owner == the_thread ) {
> - return false;
> + return THREAD_QUEUE_DEADLOCK_DETECTED;
> }
>
> queue = owner->Wait.queue;
> } while ( queue != NULL );
> #endif
>
> - return true;
> + return THREAD_QUEUE_NO_DEADLOCK;
> }
>
> void _Thread_queue_Enqueue_do_nothing_extra(
> @@ -392,8 +395,9 @@ void _Thread_queue_Enqueue(
> Thread_queue_Context *queue_context
> )
> {
> - Per_CPU_Control *cpu_self;
> - bool success;
> + Thread_queue_Deadlock_status deadlock_status;
> + Per_CPU_Control *cpu_self;
> + bool success;
>
> _Assert( queue_context->enqueue_callout != NULL );
>
> @@ -405,8 +409,11 @@ void _Thread_queue_Enqueue(
>
> _Thread_Wait_claim( the_thread, queue );
>
> - if ( !_Thread_queue_Path_acquire_critical( queue, the_thread, queue_context ) ) {
> - _Thread_queue_Path_release_critical( queue_context );
> + deadlock_status =
> + _Thread_queue_Path_acquire( queue, the_thread, queue_context );
> +
> + if ( deadlock_status == THREAD_QUEUE_DEADLOCK_DETECTED ) {
> + _Thread_queue_Path_release( queue_context );
> _Thread_Wait_restore_default( the_thread );
> _Thread_queue_Queue_release( queue, &queue_context->Lock_context.Lock_context );
> _Thread_Wait_tranquilize( the_thread );
> @@ -419,7 +426,7 @@ void _Thread_queue_Enqueue(
> _Thread_Wait_claim_finalize( the_thread, operations );
> ( *operations->enqueue )( queue, the_thread, queue_context );
>
> - _Thread_queue_Path_release_critical( queue_context );
> + _Thread_queue_Path_release( queue_context );
>
> the_thread->Wait.return_code = STATUS_SUCCESSFUL;
> _Thread_Wait_flags_set( the_thread, THREAD_QUEUE_INTEND_TO_BLOCK );
> @@ -468,14 +475,18 @@ Status_Control _Thread_queue_Enqueue_sticky(
> Thread_queue_Context *queue_context
> )
> {
> - Per_CPU_Control *cpu_self;
> + Thread_queue_Deadlock_status deadlock_status;
> + Per_CPU_Control *cpu_self;
>
> _Assert( queue_context->enqueue_callout != NULL );
>
> _Thread_Wait_claim( the_thread, queue );
>
> - if ( !_Thread_queue_Path_acquire_critical( queue, the_thread, queue_context ) ) {
> - _Thread_queue_Path_release_critical( queue_context );
> + deadlock_status =
> + _Thread_queue_Path_acquire( queue, the_thread, queue_context );
> +
> + if ( deadlock_status == THREAD_QUEUE_DEADLOCK_DETECTED ) {
> + _Thread_queue_Path_release( queue_context );
> _Thread_Wait_restore_default( the_thread );
> _Thread_queue_Queue_release( queue, &queue_context->Lock_context.Lock_context );
> _Thread_Wait_tranquilize( the_thread );
> @@ -487,7 +498,7 @@ Status_Control _Thread_queue_Enqueue_sticky(
> _Thread_Wait_claim_finalize( the_thread, operations );
> ( *operations->enqueue )( queue, the_thread, queue_context );
>
> - _Thread_queue_Path_release_critical( queue_context );
> + _Thread_queue_Path_release( queue_context );
>
> the_thread->Wait.return_code = STATUS_SUCCESSFUL;
> _Thread_Wait_flags_set( the_thread, THREAD_QUEUE_INTEND_TO_BLOCK );
> diff --git a/cpukit/score/src/threadqops.c b/cpukit/score/src/threadqops.c
> index a42876cf09..f09cf77df1 100644
> --- a/cpukit/score/src/threadqops.c
> +++ b/cpukit/score/src/threadqops.c
> @@ -1197,7 +1197,7 @@ static void _Thread_queue_Priority_inherit_extract(
> * resolves the deadlock. Thread T1 and T2 can the complete their
> * operations.
> */
> - _Thread_queue_Path_acquire_critical( queue, the_thread, queue_context );
> + (void) _Thread_queue_Path_acquire( queue, the_thread, queue_context );
> #endif
>
> _Thread_queue_Queue_extract(
> @@ -1210,7 +1210,7 @@ static void _Thread_queue_Priority_inherit_extract(
> );
>
> #if defined(RTEMS_SMP)
> - _Thread_queue_Path_release_critical( queue_context );
> + _Thread_queue_Path_release( queue_context );
> #endif
> }
>
> --
> 2.31.1
>
> _______________________________________________
> devel mailing list
> devel at rtems.org
> http://lists.rtems.org/mailman/listinfo/devel
More information about the devel
mailing list