[PATCH v2] score: Fix _Thread_Change_priority()
Joel Sherrill
joel.sherrill at oarcorp.com
Wed Mar 18 20:31:19 UTC 2015
On 3/18/2015 3:20 PM, Sebastian Huber wrote:
> Atomically update the current priority of a thread and the wait queue.
> Serialize the scheduler update in a separate critical section with a
> generation number.
>
> New test sptests/spintrcritical23.
>
> Close #2310.
> ---
> cpukit/score/include/rtems/score/thread.h | 9 ++
> cpukit/score/src/threadchangepriority.c | 35 ++--
> cpukit/score/src/threadinitialize.c | 1 +
> testsuites/sptests/Makefile.am | 1 +
> testsuites/sptests/configure.ac | 1 +
> testsuites/sptests/spintrcritical23/Makefile.am | 22 +++
> testsuites/sptests/spintrcritical23/init.c | 177 +++++++++++++++++++++
> .../sptests/spintrcritical23/spintrcritical23.doc | 12 ++
> .../sptests/spintrcritical23/spintrcritical23.scn | 2 +
> 9 files changed, 246 insertions(+), 14 deletions(-)
> create mode 100644 testsuites/sptests/spintrcritical23/Makefile.am
> create mode 100644 testsuites/sptests/spintrcritical23/init.c
> create mode 100644 testsuites/sptests/spintrcritical23/spintrcritical23.doc
> create mode 100644 testsuites/sptests/spintrcritical23/spintrcritical23.scn
>
> diff --git a/cpukit/score/include/rtems/score/thread.h b/cpukit/score/include/rtems/score/thread.h
> index cea88f4..11f60f3 100644
> --- a/cpukit/score/include/rtems/score/thread.h
> +++ b/cpukit/score/include/rtems/score/thread.h
> @@ -582,6 +582,15 @@ struct Thread_Control_struct {
> Priority_Control current_priority;
> /** This field is the base priority of this thread. */
> Priority_Control real_priority;
> +
> + /**
> + * @brief Generation of the current priority value.
> + *
> + * It is used in _Thread_Change_priority() to serialize the update of
> + * priority related data structures.
> + */
> + uint32_t priority_generation;
> +
> /** This field is the number of mutexes currently held by this thread. */
> uint32_t resource_count;
> /** This field is the blocking information for this thread. */
> diff --git a/cpukit/score/src/threadchangepriority.c b/cpukit/score/src/threadchangepriority.c
> index d61dfb8..6ee65f5 100644
> --- a/cpukit/score/src/threadchangepriority.c
> +++ b/cpukit/score/src/threadchangepriority.c
> @@ -29,29 +29,36 @@ void _Thread_Change_priority(
> bool prepend_it
> )
> {
> + ISR_Level level;
> +
> + _ISR_Disable( level );
> +
> /*
> * Do not bother recomputing all the priority related information if
> * we are not REALLY changing priority.
> */
> if ( the_thread->current_priority != new_priority ) {
> - ISR_Level level;
> -
> - _ISR_Disable( level );
> + uint32_t my_generation = the_thread->priority_generation + 1;
>
> the_thread->current_priority = new_priority;
> + the_thread->priority_generation = my_generation;
>
> - if ( _States_Is_ready( the_thread->current_state ) ) {
> - _Scheduler_Change_priority(
> - the_thread,
> - new_priority,
> - prepend_it
> - );
> - } else {
> - _Scheduler_Update_priority( the_thread, new_priority );
> - }
> + _Thread_queue_Requeue( the_thread->Wait.queue, the_thread );
Can we get a comment above this call that _Thread_queue_Requeue
checks that the thread is actually blocked on the specified queue so
we don't need to check the thread state?
> - _ISR_Enable( level );
> + _ISR_Flash( level );
>
> - _Thread_queue_Requeue( the_thread->Wait.queue, the_thread );
> + if ( the_thread->priority_generation == my_generation ) {
> + if ( _States_Is_ready( the_thread->current_state ) ) {
> + _Scheduler_Change_priority(
> + the_thread,
> + new_priority,
> + prepend_it
> + );
> + } else {
> + _Scheduler_Update_priority( the_thread, new_priority );
> + }
> + }
> }
> +
> + _ISR_Enable( level );
> }
> diff --git a/cpukit/score/src/threadinitialize.c b/cpukit/score/src/threadinitialize.c
> index 934fea9..993d95f 100644
> --- a/cpukit/score/src/threadinitialize.c
> +++ b/cpukit/score/src/threadinitialize.c
> @@ -198,6 +198,7 @@ bool _Thread_Initialize(
> the_thread->Wait.queue = NULL;
> the_thread->resource_count = 0;
> the_thread->real_priority = priority;
> + the_thread->priority_generation = 0;
> the_thread->Start.initial_priority = priority;
>
> _Thread_Wait_flags_set( the_thread, THREAD_WAIT_FLAGS_INITIAL );
> diff --git a/testsuites/sptests/Makefile.am b/testsuites/sptests/Makefile.am
> index 0d1e687..9025ff3 100644
> --- a/testsuites/sptests/Makefile.am
> +++ b/testsuites/sptests/Makefile.am
> @@ -37,6 +37,7 @@ if HAS_SMP
> else
> _SUBDIRS += sp29
> endif
> +_SUBDIRS += spintrcritical23
> _SUBDIRS += spatomic01
> _SUBDIRS += spintrcritical22
> _SUBDIRS += spsem03
> diff --git a/testsuites/sptests/configure.ac b/testsuites/sptests/configure.ac
> index eef901b..ae3c763 100644
> --- a/testsuites/sptests/configure.ac
> +++ b/testsuites/sptests/configure.ac
> @@ -40,6 +40,7 @@ AM_CONDITIONAL(HAS_SMP,test "$rtems_cv_RTEMS_SMP" = "yes")
>
> # Explicitly list all Makefiles here
> AC_CONFIG_FILES([Makefile
> +spintrcritical23/Makefile
> spatomic01/Makefile
> spglobalcon01/Makefile
> spintrcritical22/Makefile
> diff --git a/testsuites/sptests/spintrcritical23/Makefile.am b/testsuites/sptests/spintrcritical23/Makefile.am
> new file mode 100644
> index 0000000..6baaf7b
> --- /dev/null
> +++ b/testsuites/sptests/spintrcritical23/Makefile.am
> @@ -0,0 +1,22 @@
> +rtems_tests_PROGRAMS = spintrcritical23
> +spintrcritical23_SOURCES = init.c
> +spintrcritical23_SOURCES += ../spintrcritical_support/intrcritical.h
> +spintrcritical23_SOURCES += ../spintrcritical_support/intrcritical.c
> +
> +dist_rtems_tests_DATA = spintrcritical23.scn spintrcritical23.doc
> +
> +include $(RTEMS_ROOT)/make/custom/@RTEMS_BSP at .cfg
> +include $(top_srcdir)/../automake/compile.am
> +include $(top_srcdir)/../automake/leaf.am
> +
> +AM_CPPFLAGS += -I$(top_srcdir)/../support/include
> +AM_CPPFLAGS += -I$(top_srcdir)/spintrcritical_support
> +
> +LINK_OBJS = $(spintrcritical23_OBJECTS)
> +LINK_LIBS = $(spintrcritical23_LDLIBS)
> +
> +spintrcritical23$(EXEEXT): $(spintrcritical23_OBJECTS) $(spintrcritical23_DEPENDENCIES)
> + @rm -f spintrcritical23$(EXEEXT)
> + $(make-exe)
> +
> +include $(top_srcdir)/../automake/local.am
> diff --git a/testsuites/sptests/spintrcritical23/init.c b/testsuites/sptests/spintrcritical23/init.c
> new file mode 100644
> index 0000000..01cb379
> --- /dev/null
> +++ b/testsuites/sptests/spintrcritical23/init.c
> @@ -0,0 +1,177 @@
> +/*
> + * Copyright (c) 2015 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.com/license/LICENSE.
> + */
> +
> +#ifdef HAVE_CONFIG_H
> + #include "config.h"
> +#endif
> +
> +#include <tmacros.h>
> +#include <intrcritical.h>
> +
> +#include <string.h>
> +
> +#include <rtems.h>
> +#include <rtems/score/schedulerpriority.h>
> +#include <rtems/score/threadimpl.h>
> +
> +const char rtems_test_name[] = "SPINTRCRITICAL 23";
> +
> +typedef struct {
> + RTEMS_INTERRUPT_LOCK_MEMBER(lock)
> + rtems_id task_id;
> + Thread_Control *tcb;
> + rtems_task_priority next_priority;
> + uint32_t priority_generation;
> + Scheduler_priority_Node scheduler_node;
> + bool done;
> +} test_context;
> +
> +static test_context ctx_instance;
> +
> +static Thread_Control *get_tcb(rtems_id id)
> +{
> + Objects_Locations location;
> + Thread_Control *tcb;
> +
> + tcb = _Thread_Get(id, &location);
> + _Objects_Put(&tcb->Object);
> +
> + rtems_test_assert(tcb != NULL && location == OBJECTS_LOCAL);
> +
> + return tcb;
> +}
> +
> +static bool scheduler_node_unchanged(const test_context *ctx)
> +{
> + return memcmp(
> + &ctx->scheduler_node,
> + ctx->tcb->Scheduler.node,
> + sizeof(ctx->scheduler_node)
> + ) == 0;
> +}
> +
> +static void change_priority(rtems_id timer, void *arg)
> +{
> + /* The arg is NULL */
> + test_context *ctx = &ctx_instance;
> + rtems_interrupt_lock_context lock_context;
> + rtems_task_priority my_priority;
> +
> + rtems_interrupt_lock_acquire(&ctx->lock, &lock_context);
> + if (
> + ctx->priority_generation != ctx->tcb->priority_generation
> + && scheduler_node_unchanged(ctx)
> + ) {
> + ctx->done = true;
> + }
> + my_priority = ctx->next_priority;
> + rtems_interrupt_lock_release(&ctx->lock, &lock_context);
> +
> + if (ctx->done) {
> + rtems_status_code sc;
> +
> + sc = rtems_task_set_priority(
> + ctx->task_id,
> + my_priority,
> + &my_priority
> + );
> + rtems_test_assert(sc == RTEMS_SUCCESSFUL);
> + }
> +}
> +
> +static bool test_body(void *arg)
> +{
> + test_context *ctx = arg;
> + rtems_status_code sc;
> + rtems_interrupt_lock_context lock_context;
> + rtems_task_priority my_priority;
> + rtems_task_priority last_priority;
> +
> + rtems_interrupt_lock_acquire(&ctx->lock, &lock_context);
> + my_priority = 1 + (ctx->next_priority + 1) % 3;
> + ctx->next_priority = 1 + (my_priority + 1) % 3;
> + ctx->priority_generation = ctx->tcb->priority_generation;
> + memcpy(
> + &ctx->scheduler_node,
> + ctx->tcb->Scheduler.node,
> + sizeof(ctx->scheduler_node)
> + );
> + rtems_interrupt_lock_release(&ctx->lock, &lock_context);
> +
> + sc = rtems_task_set_priority(
> + ctx->task_id,
> + my_priority,
> + &last_priority
> + );
> + rtems_test_assert(sc == RTEMS_SUCCESSFUL);
> +
> + if (ctx->done) {
> + rtems_task_priority next_priority;
> +
> + sc = rtems_task_set_priority(
> + ctx->task_id,
> + RTEMS_CURRENT_PRIORITY,
> + &next_priority
> + );
> + rtems_test_assert(sc == RTEMS_SUCCESSFUL);
> + rtems_test_assert(next_priority == ctx->next_priority);
> + rtems_test_assert(next_priority != my_priority);
> + }
> +
> + return ctx->done;
> +}
> +
> +static void Init(rtems_task_argument arg)
> +{
> + test_context *ctx = &ctx_instance;
> + rtems_status_code sc;
> +
> + TEST_BEGIN();
> +
> + sc = rtems_task_create(
> + rtems_build_name('T', 'E', 'S', 'T'),
> + 1,
> + RTEMS_MINIMUM_STACK_SIZE,
> + RTEMS_DEFAULT_MODES,
> + RTEMS_DEFAULT_ATTRIBUTES,
> + &ctx->task_id
> + );
> + rtems_test_assert(sc == RTEMS_SUCCESSFUL);
> +
> + rtems_interrupt_lock_initialize(&ctx->lock, "Test");
> + ctx->tcb = get_tcb(ctx->task_id);
> +
> + interrupt_critical_section_test(test_body, ctx, change_priority);
> + rtems_test_assert(ctx->done);
> +
> + TEST_END();
> + rtems_test_exit(0);
> +}
> +
> +#define CONFIGURE_APPLICATION_NEEDS_CLOCK_DRIVER
> +#define CONFIGURE_APPLICATION_NEEDS_CONSOLE_DRIVER
> +
> +#define CONFIGURE_MICROSECONDS_PER_TICK 1000
> +
> +#define CONFIGURE_MAXIMUM_TASKS 2
> +#define CONFIGURE_MAXIMUM_TIMERS 1
> +#define CONFIGURE_MAXIMUM_USER_EXTENSIONS 1
> +
> +#define CONFIGURE_INITIAL_EXTENSIONS RTEMS_TEST_INITIAL_EXTENSION
> +
> +#define CONFIGURE_RTEMS_INIT_TASKS_TABLE
> +
> +#define CONFIGURE_INIT
> +
> +#include <rtems/confdefs.h>
> diff --git a/testsuites/sptests/spintrcritical23/spintrcritical23.doc b/testsuites/sptests/spintrcritical23/spintrcritical23.doc
> new file mode 100644
> index 0000000..89e5281
> --- /dev/null
> +++ b/testsuites/sptests/spintrcritical23/spintrcritical23.doc
> @@ -0,0 +1,12 @@
> +This file describes the directives and concepts tested by this test set.
> +
> +test set name: spintrcritical23
> +
> +directives:
> +
> + - _Thread_Change_priority()
> +
> +concepts:
> +
> + - Ensure that priority updates work if carried out in interrupt context while
> + a priority change at task level is in progress.
> diff --git a/testsuites/sptests/spintrcritical23/spintrcritical23.scn b/testsuites/sptests/spintrcritical23/spintrcritical23.scn
> new file mode 100644
> index 0000000..cea3292
> --- /dev/null
> +++ b/testsuites/sptests/spintrcritical23/spintrcritical23.scn
> @@ -0,0 +1,2 @@
> +*** BEGIN OF TEST SPINTRCRITICAL 23 ***
> +*** END OF TEST SPINTRCRITICAL 23 ***
> --
> 2.1.4
> _______________________________________________
> devel mailing list
> devel at rtems.org
> http://lists.rtems.org/mailman/listinfo/devel
--
Joel Sherrill, Ph.D. Director of Research & Development
joel.sherrill at OARcorp.com On-Line Applications Research
Ask me about RTEMS: a free RTOS Huntsville AL 35805
Support Available (256) 722-9985
More information about the devel
mailing list