[PATCH 1/2] score: Implement scheduler helping protocol
Sebastian Huber
sebastian.huber at embedded-brains.de
Tue Jul 8 18:20:01 UTC 2014
Hello Gedare,
thanks for reviewing this huge patch.
On 07/08/2014 05:22 PM, Gedare Bloom wrote:
>> diff --git a/cpukit/score/include/rtems/score/thread.h b/cpukit/score/include/rtems/score/thread.h
>> >index a9a3f9f..4d758fb 100644
>> >--- a/cpukit/score/include/rtems/score/thread.h
>> >+++ b/cpukit/score/include/rtems/score/thread.h
>> >@@ -461,19 +461,72 @@ typedef struct {
>> > Thread_Control *terminator;
>> > } Thread_Life_control;
>> >
>> >+#if defined(RTEMS_SMP)
>> >+/**
>> >+ * @brief The thread state with respect to the scheduler.
>> >+ */
>> >+typedef enum {
>> >+ /**
>> >+ * @brief This thread is blocked with respect to the scheduler.
>> >+ *
>> >+ * This thread uses no scheduler nodes.
>> >+ */
>> >+ THREAD_SCHEDULER_BLOCKED,
>> >+
>> >+ /**
>> >+ * @brief This thread is scheduled with respect to the scheduler.
>> >+ *
>> >+ * This thread executes using one of its scheduler nodes. This could be its
>> >+ * own scheduler node or in case it owns resources taking part in the
>> >+ * scheduler helping protocol a scheduler node of another thread.
>> >+ */
>> >+ THREAD_SCHEDULER_SCHEDULED,
>> >+
>> >+ /**
>> >+ * @brief This thread is ready with respect to the scheduler.
>> >+ *
>> >+ * None of the scheduler nodes of this thread is scheduled.
>> >+ */
>> >+ THREAD_SCHEDULER_READY
>> >+} Thread_Scheduler_state;
>> >+#endif
>> >+
>> > /**
>> > * @brief Thread scheduler control.
>> > */
>> > typedef struct {
>> > #if defined(RTEMS_SMP)
>> > /**
>> >+ * @brief The current scheduler state of this thread.
>> >+ */
>> >+ Thread_Scheduler_state state;
> Should be named "State" for an embedded structure.
Its an enum, so it should be lower case.
>
>> >+
>> >+ /**
>> >+ * @brief The own scheduler control of this thread.
>> >+ *
>> >+ * This field is constant after initialization.
>> >+ */
>> >+ const struct Scheduler_Control *own_control;
>> >+
>> >+ /**
>> > * @brief The current scheduler control of this thread.
>> >+ *
>> >+ * The scheduler helping protocol may change this field.
>> > */
>> > const struct Scheduler_Control *control;
> Perhaps current_control is better now that there are two of these pointers?
Hm, I would rather remove the "current" from the @brief. This "current"
is probably a bit misleading.
>
>> >+
>> >+ /**
>> >+ * @brief The own scheduler node of this thread.
>> >+ *
>> >+ * This field is constant after initialization.
>> >+ */
>> >+ struct Scheduler_Node *own_node;
>> > #endif
>> >
>> > /**
>> > * @brief The current scheduler node of this thread.
>> >+ *
>> >+ * The scheduler helping protocol may change this field.
> but only for SMP schedulers? There is no helping protocol for UP right?
Ok, good catch. I will clarify, that this helping protocol is only used
in SMP configurations.
>
>> > */
>> > struct Scheduler_Node *node;
> Again current_node might be better now.
>
>> >
>> >diff --git a/cpukit/score/include/rtems/score/threadimpl.h b/cpukit/score/include/rtems/score/threadimpl.h
>> >index 4971e9d..cb7d5fe 100644
>> >--- a/cpukit/score/include/rtems/score/threadimpl.h
>> >+++ b/cpukit/score/include/rtems/score/threadimpl.h
>> >@@ -828,6 +828,16 @@ RTEMS_INLINE_ROUTINE bool _Thread_Owns_resources(
>> > return owns_resources;
>> > }
>> >
>> >+#if defined(RTEMS_SMP)
>> >+RTEMS_INLINE_ROUTINE Thread_Control *_Thread_Resource_node_to_thread(
>> >+ Resource_Node *node
>> >+)
>> >+{
>> >+ return (Thread_Control *)
>> >+ ( (char *) node - offsetof( Thread_Control, Resource_node ) );
>> >+}
> We should include some generic container_of function in rtems instead
> of reproducing it multiple places.
In <sys/cdefs.h> we have:
/*
* Given the pointer x to the member m of the struct s, return
* a pointer to the containing structure. When using GCC, we first
* assign pointer x to a local variable, to check that its type is
* compatible with member m.
*/
#if __GNUC_PREREQ__(3, 1)
#define __containerof(x, s, m) ({ \
const volatile __typeof__(((s *)0)->m) *__x = (x); \
__DEQUALIFY(s *, (const volatile char *)__x - __offsetof(s, m));\
})
#else
#define __containerof(x, s, m) \
__DEQUALIFY(s *, (const volatile char *)(x) - __offsetof(s, m))
#endif
What about adding a similar
_Container_of()
or
rtems_container_of()
to <rtems/score/basedefs.h>?
>
>> >+#endif
>> >+
>> > RTEMS_INLINE_ROUTINE void _Thread_Debug_set_real_processor(
>> > Thread_Control *the_thread,
>> > Per_CPU_Control *cpu
>> >diff --git a/cpukit/score/src/schedulerchangeroot.c b/cpukit/score/src/schedulerchangeroot.c
>> >new file mode 100644
>> >index 0000000..bdb7b30
>> >--- /dev/null
>> >+++ b/cpukit/score/src/schedulerchangeroot.c
>> >@@ -0,0 +1,85 @@
>> >+/*
>> >+ * Copyright (c) 2014 embedded brains GmbH. All rights reserved.
>> >+ *
>> >+ * embedded brains GmbH
>> >+ * Dornierstr. 4
>> >+ * 82178 Puchheim
>> >+ * Germany
>> >+ *<rtems at embedded-brains.de>
>> >+ *
>> >+ * The license and distribution terms for this file may be
>> >+ * found in the file LICENSE in this distribution or at
>> >+ *http://www.rtems.org/license/LICENSE.
>> >+ */
>> >+
>> >+#if HAVE_CONFIG_H
>> >+ #include "config.h"
>> >+#endif
>> >+
>> >+#include <rtems/score/schedulerimpl.h>
>> >+
>> >+typedef struct {
>> >+ Thread_Control *root;
>> >+ Thread_Control *needs_help;
>> >+} Scheduler_Set_root_context;
>> >+
>> >+RTEMS_INLINE_ROUTINE bool _Scheduler_Set_root_visitor(
>> >+ Resource_Node *resource_node,
>> >+ void *arg
>> >+)
>> >+{
>> >+ Scheduler_Set_root_context *ctx = arg;
>> >+ Thread_Control *root = ctx->root;
>> >+ Thread_Control *needs_help = root;
>> >+ Thread_Control *offers_help =
>> >+ _Thread_Resource_node_to_thread( resource_node );
>> >+ const Scheduler_Control *scheduler = _Scheduler_Get_own( offers_help );
>> >+ Thread_Control *needs_also_help;
> Minor nit: Better English grammar should not split the verb, and also
> is often used before the verb. Equivalentely, "too" can be used, and
> it usually comes after the verb. So it would be better to use either:
> "also_needs_help", "needs_help_also", or "needs_help_too". I like the
> "needs_help_too" form, it matches variable names a little nicer.
Ok, I change it to "needs_help_too".
>
>
>> >+
>> >+ _Resource_Node_set_root( resource_node, &root->Resource_node );
>> >+
>> >+ needs_also_help = ( *scheduler->Operations.ask_for_help )(
>> >+ scheduler,
>> >+ offers_help,
>> >+ needs_help
>> >+ );
>> >+
>> >+ if ( needs_also_help != needs_help && needs_also_help != NULL ) {
>> >+ _Assert( ctx->needs_help == NULL );
>> >+ ctx->needs_help = needs_also_help;
>> >+ }
>> >+
>> >+ return false;
>> >+}
>> >+
>> >+void _Scheduler_Thread_change_resource_root(
>> >+ Thread_Control *top,
>> >+ Thread_Control *root
>> >+)
>> >+{
>> >+ Scheduler_Set_root_context ctx = { root, NULL };
>> >+ Thread_Control *offers_help = top;
>> >+ Scheduler_Node *offers_help_node;
>> >+ Thread_Control *offers_also_help;
>> >+ ISR_Level level;
>> >+
>> >+ _ISR_Disable( level );
>> >+
>> >+ offers_help_node = _Scheduler_Thread_get_node( offers_help );
>> >+ offers_also_help = _Scheduler_Node_get_owner( offers_help_node );
>> >+
>> >+ if ( offers_help != offers_also_help ) {
>> >+ _Scheduler_Set_root_visitor( &offers_also_help->Resource_node, &ctx );
>> >+ _Assert( ctx.needs_help == offers_help );
>> >+ ctx.needs_help = NULL;
>> >+ }
>> >+
>> >+ _Scheduler_Set_root_visitor( &top->Resource_node, &ctx );
>> >+ _Resource_Iterate( &top->Resource_node, _Scheduler_Set_root_visitor, &ctx );
>> >+
> Does this iterate() with disabled interrupts have bad implications for
> schedulability / worst-case latency?
>
Yes, the worst-case latency depends now on the resource tree size. I
don't think its easy to avoid this. You have at least the following
options.
1. Use one lock or a hierarchy of locks to freeze the tree state and
thus enable a safe iteration.
2. Partially lock the tree and somehow provide safe iteration. How? Is
this possible with fine grained locking at all?
3. Organize the tree so that the interesting elements are the min/max
nodes. I don't know how this can be done. Each scheduler state change
may result in updates of all resource trees in the system.
This implementation was done with fine grained locking in mind. So I
did choose 1. We can use the tree to get a partial order of
per-resource locks necessary to avoid deadlocks.
--
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