[PATCH 8/8] score: Rework thread priority management

Sebastian Huber sebastian.huber at embedded-brains.de
Thu Sep 8 11:13:37 UTC 2016


Hello Gedare,

thanks for your quick review.

On 07/09/16 20:52, Gedare Bloom wrote:
> On Tue, Sep 6, 2016 at 8:40 AM, Sebastian Huber
> <sebastian.huber at embedded-brains.de> wrote:
>> Add priority nodes which contribute to the overall thread priority.
>>
>> The actual priority of a thread is now an aggregation of priority nodes.
>> The thread priority aggregation for the home scheduler instance of a
>> thread consists of at least one priority node, which is normally the
>> real priority of the thread.  The locking protocols (e.g. priority
>> ceiling and priority inheritance), rate-monotonic period objects and the
>> POSIX sporadic server add, change and remove priority nodes.
>>
>> A thread changes its priority now immediately, e.g. priority changes are
>> not deferred until the thread releases its last resource.
>>
>> Replace the _Thread_Change_priority() function with
>>
>>   * _Thread_Priority_perform_actions(),
>>   * _Thread_Priority_add(),
>>   * _Thread_Priority_remove(),
>>   * _Thread_Priority_change(), and
>>   * _Thread_Priority_update().
>>
>> Update #2412.
>> Update #2556.
>> [...]
>> +   */
>> +  RBTree_Control Contributors;
>> +
> Is it the case that a priority node is only ever on one of these
> "Contributors" trees at a time?

Yes, a priority node can be added to at most one tree at a time. This 
priority aggregation is a container for priority nodes and is itself a 
priority node.

> [...]
> +  struct {
> +    Priority_Actions Actions;
> +
> +    /**
> +     * @brief Count of threads to update the priority via
> +     * _Thread_Priority_update().
> +     */
> +    size_t update_count;
> I'd prefer uint32_t for unsigned counters since usually size_t denotes
> an object's (byte) size.

It is related to the update table, so I think size_t is the right type.

>
>> +
>> +    /**
>> +     * @brief Threads to update the priority via _Thread_Priority_update().
>> +     */
>> +    Thread_Control *update[ 2 ];
> Why 2?
>
>> +  } Priority;
>> +
>>     /**
>>      * @brief Invoked in case of a detected deadlock.
>>      *
>> @@ -237,7 +274,7 @@ typedef struct {
>>     /**
>>      * @brief The actual thread priority queue.
>>      */
>> -  RBTree_Control Queue;
>> +  Priority_Aggregation Queue;
>>   } Thread_queue_Priority_queue;
>>
>>   /**
>> @@ -289,6 +326,11 @@ typedef struct _Thread_queue_Heads {
>>
>>   #if defined(RTEMS_SMP)
>>     /**
>> +   * @brief Boost priority.
> This note could be expanded (e.g. boosted by xxx). Thanks for
> improving the doxy along the way!

The boosting stuff will go away soon. The main goal of OMIP is to get 
rid of priority boosting.

>
>> +   */
>> +  Priority_Node Boost_priority;
>> +
>> +  /**
>>      * @brief One priority queue per scheduler instance.
>>      */
>>     Thread_queue_Priority_queue Priority[ RTEMS_ZERO_LENGTH_ARRAY ];
>> @@ -337,34 +379,33 @@ struct Thread_queue_Queue {
>>   };
>>
>>   /**
>> - * @brief Thread queue priority change operation.
>> + * @brief Thread queue action operation.
>>    *
>>    * @param[in] queue The actual thread queue.
>>    * @param[in] the_thread The thread.
>> - * @param[in] new_priority The new priority value.
>> - *
>> - * @see Thread_queue_Operations.
>> + * @param[in] queue_context The thread queue context providing the thread queue
>> + *   action set to perform.  Returns the thread queue action set to perform on
>> + *   the thread queue owner or the empty set in case there is nothing to do.
>>    */
>> -typedef void ( *Thread_queue_Priority_change_operation )(
>> -  Thread_queue_Queue *queue,
>> -  Thread_Control     *the_thread,
>> -  Priority_Control    new_priority
>> +typedef void ( *Thread_queue_Priority_actions_operation )(
>> +  Thread_queue_Queue   *queue,
>> +  Priority_Actions     *priority_actions
>>   );
>>
>>   /**
>>    * @brief Thread queue enqueue operation.
>>    *
>>    * A potential thread to update the priority due to priority inheritance is
>> - * returned via the thread queue path.  This thread is handed over to
>> - * _Thread_Update_priority().
>> + * returned via the thread queue context.  This thread is handed over to
>> + * _Thread_Priority_update().
>>    *
>>    * @param[in] queue The actual thread queue.
>>    * @param[in] the_thread The thread to enqueue on the queue.
>>    */
>>   typedef void ( *Thread_queue_Enqueue_operation )(
>> -  Thread_queue_Queue *queue,
>> -  Thread_Control     *the_thread,
>> -  Thread_queue_Path  *path
>> +  Thread_queue_Queue   *queue,
>> +  Thread_Control       *the_thread,
>> +  Thread_queue_Context *queue_context
>>   );
>>
>>   /**
>> @@ -374,8 +415,9 @@ typedef void ( *Thread_queue_Enqueue_operation )(
>>    * @param[in] the_thread The thread to extract from the thread queue.
>>    */
>>   typedef void ( *Thread_queue_Extract_operation )(
>> -  Thread_queue_Queue *queue,
>> -  Thread_Control     *the_thread
>> +  Thread_queue_Queue   *queue,
>> +  Thread_Control       *the_thread,
>> +  Thread_queue_Context *queue_context
>>   );
>>
>>   /**
>> @@ -390,9 +432,10 @@ typedef void ( *Thread_queue_Extract_operation )(
>>    * @return The previous first thread on the queue.
>>    */
>>   typedef Thread_Control *( *Thread_queue_Surrender_operation )(
>> -  Thread_queue_Queue *queue,
>> -  Thread_queue_Heads *heads,
>> -  Thread_Control     *previous_owner
>> +  Thread_queue_Queue   *queue,
>> +  Thread_queue_Heads   *heads,
>> +  Thread_Control       *previous_owner,
>> +  Thread_queue_Context *queue_context
>>   );
>>
>>   /**
>> @@ -415,16 +458,9 @@ typedef Thread_Control *( *Thread_queue_First_operation )(
>>    */
>>   struct Thread_queue_Operations {
>>     /**
>> -   * @brief Thread queue priority change operation.
>> -   *
>> -   * Called by _Thread_Change_priority() to notify a thread about a priority
>> -   * change.  In case this thread waits currently for a resource the handler
>> -   * may adjust its data structures according to the new priority value.  This
>> -   * handler must not be NULL, instead the default handler
>> -   * _Thread_Do_nothing_priority_change() should be used in case nothing needs
>> -   * to be done during a priority change.
>> -   */
>> -  Thread_queue_Priority_change_operation priority_change;
>> +   * @brief Thread queue priority actions operation.
> This note also should be expanded a bit.

The details are in the function pointer type documentation.

>
>> +   */
>> +  Thread_queue_Priority_actions_operation priority_actions;
>>
>>     /**
>>      * @brief Thread queue enqueue operation.
>> diff --git a/cpukit/score/include/rtems/score/threadqimpl.h b/cpukit/score/include/rtems/score/threadqimpl.h
>> index 977b0ce..3555b7f 100644
>> --- a/cpukit/score/include/rtems/score/threadqimpl.h
>> +++ b/cpukit/score/include/rtems/score/threadqimpl.h
>> @@ -21,7 +21,7 @@
>>
>>   #include <rtems/score/threadq.h>
>>   #include <rtems/score/chainimpl.h>
>> -#include <rtems/score/rbtreeimpl.h>
>> +#include <rtems/score/priorityimpl.h>
>>   #include <rtems/score/scheduler.h>
>>   #include <rtems/score/smp.h>
>>   #include <rtems/score/thread.h>
>> @@ -39,38 +39,8 @@ extern "C" {
>>    */
>>   /**@{*/
>>
>> -/**
>> - * @brief Representation of a thread queue path from a start thread queue to
>> - * the terminal thread queue.
>> - *
>> - * The start thread queue is determined by the object on which a thread intends
>> - * to block.  The terminal thread queue is the thread queue reachable via
>> - * thread queue links those owner is not blocked on a thread queue.  The thread
>> - * queue links are determined by the thread queue owner and thread wait queue
>> - * relationships.
>> - */
>> -struct Thread_queue_Path {
>> -#if defined(RTEMS_SMP)
>> -  /**
>> -   * @brief The chain of thread queue links defining the thread queue path.
>> -   */
>> -  Chain_Control Links;
>> -
>> -  /**
>> -   * @brief The start of a thread queue path.
>> -   */
>> -  Thread_queue_Link Start;
>> -#endif
>> -
>> -  /**
>> -   * @brief A potential thread to update the priority via
>> -   * _Thread_Update_priority().
>> -   *
>> -   * This thread is determined by thread queues which support priority
>> -   * inheritance.
>> -   */
>> -  Thread_Control *update_priority;
>> -};
>> +#define THREAD_QUEUE_LINK_OF_PATH_NODE( node ) \
>> +  RTEMS_CONTAINER_OF( node, Thread_queue_Link, Path_node );
>>
>>   /**
>>    * @brief Thread queue with a layout compatible to struct _Thread_queue_Queue
>> @@ -210,6 +180,42 @@ RTEMS_INLINE_ROUTINE void _Thread_queue_Context_set_deadlock_callout(
>>     queue_context->deadlock_callout = deadlock_callout;
>>   }
>>
>> +RTEMS_INLINE_ROUTINE void _Thread_queue_Context_clear_priority_updates(
>> +  Thread_queue_Context *queue_context
>> +)
>> +{
>> +  queue_context->Priority.update_count = 0;
>> +}
>> +
>> +RTEMS_INLINE_ROUTINE size_t _Thread_queue_Context_get_priority_updates(
>> +  Thread_queue_Context *queue_context
>> +)
>> +{
>> +  return queue_context->Priority.update_count;
>> +}
>> +
>> +RTEMS_INLINE_ROUTINE void _Thread_queue_Context_set_priority_updates(
>> +  Thread_queue_Context *queue_context,
>> +  size_t                update_count
>> +)
>> +{
>> +  queue_context->Priority.update_count = update_count;
>> +}
> Are there some rules for what can/should set the priority updates? Is
> this a monotonic counter until clear()?

I renamed these functions to

_Thread_queue_Context_save_priority_updates()
_Thread_queue_Context_restore_priority_updates()

The only user is _Thread_Priority_perform_actions(). I added a comment 
why we do a save/restore.

> [...]
>
> -void _Thread_Restore_priority( Thread_Control *the_thread )
> +void _Thread_Priority_update( Thread_queue_Context *queue_context )
>   {
> -  _Thread_Change_priority(
> -    the_thread,
> -    0,
> -    NULL,
> -    _Thread_Restore_priority_filter,
> -    true
> -  );
> +  size_t i;
> +  size_t n;
> +
> +  n = queue_context->Priority.update_count;
> +
> +  for ( i = 0; i < n ; ++i ) {
> +    Thread_Control   *the_thread;
> +    ISR_lock_Context  lock_context;
> +
> +    the_thread = queue_context->Priority.update[ i ];
> +    _Thread_State_acquire( the_thread, &lock_context );
> +    _Scheduler_Update_priority( the_thread );
> +    _Thread_State_release( the_thread, &lock_context );
> +  }
> Why doesn't this decrement the update_count after completing the
> update? I'm missing some important detail about the "lifetime" of
> these update counters.

I added a comment.

-- 
Sebastian Huber, embedded brains GmbH

Address : Dornierstr. 4, D-82178 Puchheim, Germany
Phone   : +49 89 189 47 41-16
Fax     : +49 89 189 47 41-09
E-Mail  : sebastian.huber at embedded-brains.de
PGP     : Public key available on request.

Diese Nachricht ist keine geschäftliche Mitteilung im Sinne des EHUG.




More information about the devel mailing list