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.hu...@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.hu...@embedded-brains.de
PGP     : Public key available on request.

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

_______________________________________________
devel mailing list
devel@rtems.org
http://lists.rtems.org/mailman/listinfo/devel

Reply via email to