From ee0e41355d1faa5f74085effc4f57c3a7fc1f884 Mon Sep 17 00:00:00 2001 From: Sebastian Huber Date: Thu, 4 Aug 2016 10:20:29 +0200 Subject: score: Fix a release/cancel job race condition Split up the potential thread priority change in the scheduler release/cancel job operation. Protect the rate monotonic period state with a dedicated SMP lock. This avoids a race condition during _Rate_monotonic_Timeout() while _Rate_monotonic_Cancel() is called on another processor. --- cpukit/rtems/include/rtems/rtems/ratemon.h | 5 +++++ cpukit/rtems/include/rtems/rtems/ratemonimpl.h | 12 ++++++------ cpukit/rtems/src/ratemoncancel.c | 13 ++++++------- cpukit/rtems/src/ratemoncreate.c | 2 ++ cpukit/rtems/src/ratemongetstatistics.c | 6 ++---- cpukit/rtems/src/ratemongetstatus.c | 8 +++----- cpukit/rtems/src/ratemonperiod.c | 17 ++++++++--------- cpukit/rtems/src/ratemonresetstatistics.c | 6 ++---- cpukit/rtems/src/ratemontimeout.c | 4 ++-- cpukit/score/include/rtems/score/scheduler.h | 12 ++++++++---- cpukit/score/include/rtems/score/schedulercbs.h | 15 +-------------- cpukit/score/include/rtems/score/scheduleredf.h | 17 ++--------------- cpukit/score/include/rtems/score/schedulerimpl.h | 16 ++++++++++++---- cpukit/score/src/schedulercbsreleasejob.c | 6 +++--- cpukit/score/src/schedulerdefaultreleasejob.c | 8 ++++++-- cpukit/score/src/scheduleredfreleasejob.c | 8 ++++---- 16 files changed, 72 insertions(+), 83 deletions(-) diff --git a/cpukit/rtems/include/rtems/rtems/ratemon.h b/cpukit/rtems/include/rtems/rtems/ratemon.h index 3203eab394..a2df13f025 100644 --- a/cpukit/rtems/include/rtems/rtems/ratemon.h +++ b/cpukit/rtems/include/rtems/rtems/ratemon.h @@ -194,6 +194,11 @@ typedef struct { /** This field is the object management portion of a Period instance. */ Objects_Control Object; + /** + * @brief Protects the rate monotonic period state. + */ + ISR_LOCK_MEMBER( Lock ) + /** This is the timer used to provide the unblocking mechanism. */ Watchdog_Control Timer; diff --git a/cpukit/rtems/include/rtems/rtems/ratemonimpl.h b/cpukit/rtems/include/rtems/rtems/ratemonimpl.h index 9963cab612..b6b3ffd404 100644 --- a/cpukit/rtems/include/rtems/rtems/ratemonimpl.h +++ b/cpukit/rtems/include/rtems/rtems/ratemonimpl.h @@ -69,19 +69,19 @@ RTEMS_INLINE_ROUTINE Rate_monotonic_Control *_Rate_monotonic_Allocate( void ) } RTEMS_INLINE_ROUTINE void _Rate_monotonic_Acquire_critical( - Thread_Control *the_thread, - ISR_lock_Context *lock_context + Rate_monotonic_Control *the_period, + ISR_lock_Context *lock_context ) { - _Thread_Wait_acquire_default_critical( the_thread, lock_context ); + _ISR_lock_Acquire( &the_period->Lock, lock_context ); } RTEMS_INLINE_ROUTINE void _Rate_monotonic_Release( - Thread_Control *the_thread, - ISR_lock_Context *lock_context + Rate_monotonic_Control *the_period, + ISR_lock_Context *lock_context ) { - _Thread_Wait_release_default( the_thread, lock_context ); + _ISR_lock_Release_and_ISR_enable( &the_period->Lock, lock_context ); } RTEMS_INLINE_ROUTINE Rate_monotonic_Control *_Rate_monotonic_Get( diff --git a/cpukit/rtems/src/ratemoncancel.c b/cpukit/rtems/src/ratemoncancel.c index 41ba48856f..b4e899d296 100644 --- a/cpukit/rtems/src/ratemoncancel.c +++ b/cpukit/rtems/src/ratemoncancel.c @@ -28,18 +28,17 @@ void _Rate_monotonic_Cancel( ) { Per_CPU_Control *cpu_self; + Thread_Control *update_priority; - _Watchdog_Per_CPU_remove_relative( &the_period->Timer ); + _Rate_monotonic_Acquire_critical( the_period, lock_context ); - owner = the_period->owner; - _Rate_monotonic_Acquire_critical( owner, lock_context ); + _Watchdog_Per_CPU_remove_relative( &the_period->Timer ); the_period->state = RATE_MONOTONIC_INACTIVE; + update_priority = _Scheduler_Cancel_job( the_period->owner ); cpu_self = _Thread_Dispatch_disable_critical( lock_context ); - _Rate_monotonic_Release( owner, lock_context ); - - _Scheduler_Cancel_job( owner ); - + _Rate_monotonic_Release( the_period, lock_context ); + _Thread_Update_priority( update_priority ); _Thread_Dispatch_enable( cpu_self ); } diff --git a/cpukit/rtems/src/ratemoncreate.c b/cpukit/rtems/src/ratemoncreate.c index 1a5c9b2615..a86c6a1eeb 100644 --- a/cpukit/rtems/src/ratemoncreate.c +++ b/cpukit/rtems/src/ratemoncreate.c @@ -62,6 +62,8 @@ rtems_status_code rtems_rate_monotonic_create( return RTEMS_TOO_MANY; } + _ISR_lock_Initialize( &the_period->Lock, "Rate Monotonic Period" ); + the_period->owner = _Thread_Get_executing(); the_period->state = RATE_MONOTONIC_INACTIVE; diff --git a/cpukit/rtems/src/ratemongetstatistics.c b/cpukit/rtems/src/ratemongetstatistics.c index a6a0525fb0..7ffd2c5f5e 100644 --- a/cpukit/rtems/src/ratemongetstatistics.c +++ b/cpukit/rtems/src/ratemongetstatistics.c @@ -28,7 +28,6 @@ rtems_status_code rtems_rate_monotonic_get_statistics( { Rate_monotonic_Control *the_period; ISR_lock_Context lock_context; - Thread_Control *owner; const Rate_monotonic_Statistics *src; if ( dst == NULL ) { @@ -40,8 +39,7 @@ rtems_status_code rtems_rate_monotonic_get_statistics( return RTEMS_INVALID_ID; } - owner = the_period->owner; - _Rate_monotonic_Acquire_critical( owner, &lock_context ); + _Rate_monotonic_Acquire_critical( the_period, &lock_context ); src = &the_period->Statistics; dst->count = src->count; @@ -53,6 +51,6 @@ rtems_status_code rtems_rate_monotonic_get_statistics( _Timestamp_To_timespec( &src->max_wall_time, &dst->max_wall_time ); _Timestamp_To_timespec( &src->total_wall_time, &dst->total_wall_time ); - _Rate_monotonic_Release( owner, &lock_context ); + _Rate_monotonic_Release( the_period, &lock_context ); return RTEMS_SUCCESSFUL; } diff --git a/cpukit/rtems/src/ratemongetstatus.c b/cpukit/rtems/src/ratemongetstatus.c index 82a950ec6a..403c6ed097 100644 --- a/cpukit/rtems/src/ratemongetstatus.c +++ b/cpukit/rtems/src/ratemongetstatus.c @@ -28,7 +28,6 @@ rtems_status_code rtems_rate_monotonic_get_status( { Rate_monotonic_Control *the_period; ISR_lock_Context lock_context; - Thread_Control *owner; rtems_status_code status; if ( period_status == NULL ) { @@ -40,10 +39,9 @@ rtems_status_code rtems_rate_monotonic_get_status( return RTEMS_INVALID_ID; } - owner = the_period->owner; - _Rate_monotonic_Acquire_critical( owner, &lock_context ); + _Rate_monotonic_Acquire_critical( the_period, &lock_context ); - period_status->owner = owner->Object.id; + period_status->owner = the_period->owner->Object.id; period_status->state = the_period->state; if ( the_period->state == RATE_MONOTONIC_INACTIVE ) { @@ -81,6 +79,6 @@ rtems_status_code rtems_rate_monotonic_get_status( } } - _Rate_monotonic_Release( owner, &lock_context ); + _Rate_monotonic_Release( the_period, &lock_context ); return status; } diff --git a/cpukit/rtems/src/ratemonperiod.c b/cpukit/rtems/src/ratemonperiod.c index 303fe174e5..75a80d8088 100644 --- a/cpukit/rtems/src/ratemonperiod.c +++ b/cpukit/rtems/src/ratemonperiod.c @@ -71,21 +71,20 @@ static void _Rate_monotonic_Release_job( ) { Per_CPU_Control *cpu_self; - uint64_t deadline; + Thread_Control *update_priority; + uint64_t deadline; cpu_self = _Thread_Dispatch_disable_critical( lock_context ); - _Rate_monotonic_Release( owner, lock_context ); - _ISR_lock_ISR_disable( lock_context ); deadline = _Watchdog_Per_CPU_insert_relative( &the_period->Timer, cpu_self, next_length ); - _ISR_lock_ISR_enable( lock_context ); - - _Scheduler_Release_job( owner, deadline ); + update_priority = _Scheduler_Release_job( owner, deadline ); + _Rate_monotonic_Release( the_period, lock_context ); + _Thread_Update_priority( update_priority ); _Thread_Dispatch_enable( cpu_self ); } @@ -217,7 +216,7 @@ static rtems_status_code _Rate_monotonic_Block_while_active( _Thread_Wait_flags_set( executing, RATE_MONOTONIC_INTEND_TO_BLOCK ); cpu_self = _Thread_Dispatch_disable_critical( lock_context ); - _Rate_monotonic_Release( executing, lock_context ); + _Rate_monotonic_Release( the_period, lock_context ); _Thread_Set_state( executing, STATES_WAITING_FOR_PERIOD ); @@ -278,13 +277,13 @@ rtems_status_code rtems_rate_monotonic_period( return RTEMS_NOT_OWNER_OF_RESOURCE; } - _Rate_monotonic_Acquire_critical( executing, &lock_context ); + _Rate_monotonic_Acquire_critical( the_period, &lock_context ); state = the_period->state; if ( length == RTEMS_PERIOD_STATUS ) { status = _Rate_monotonic_Get_status_for_state( state ); - _Rate_monotonic_Release( executing, &lock_context ); + _Rate_monotonic_Release( the_period, &lock_context ); } else { switch ( state ) { case RATE_MONOTONIC_ACTIVE: diff --git a/cpukit/rtems/src/ratemonresetstatistics.c b/cpukit/rtems/src/ratemonresetstatistics.c index b146e6acb2..0b0e6c6369 100644 --- a/cpukit/rtems/src/ratemonresetstatistics.c +++ b/cpukit/rtems/src/ratemonresetstatistics.c @@ -26,16 +26,14 @@ rtems_status_code rtems_rate_monotonic_reset_statistics( { Rate_monotonic_Control *the_period; ISR_lock_Context lock_context; - Thread_Control *owner; the_period = _Rate_monotonic_Get( id, &lock_context ); if ( the_period == NULL ) { return RTEMS_INVALID_ID; } - owner = the_period->owner; - _Rate_monotonic_Acquire_critical( owner, &lock_context ); + _Rate_monotonic_Acquire_critical( the_period, &lock_context ); _Rate_monotonic_Reset_statistics( the_period ); - _Rate_monotonic_Release( owner, &lock_context ); + _Rate_monotonic_Release( the_period, &lock_context ); return RTEMS_SUCCESSFUL; } diff --git a/cpukit/rtems/src/ratemontimeout.c b/cpukit/rtems/src/ratemontimeout.c index bd38153b82..e514a314b3 100644 --- a/cpukit/rtems/src/ratemontimeout.c +++ b/cpukit/rtems/src/ratemontimeout.c @@ -31,7 +31,7 @@ void _Rate_monotonic_Timeout( Watchdog_Control *the_watchdog ) owner = the_period->owner; _ISR_lock_ISR_disable( &lock_context ); - _Rate_monotonic_Acquire_critical( owner, &lock_context ); + _Rate_monotonic_Acquire_critical( the_period, &lock_context ); wait_flags = _Thread_Wait_flags_get( owner ); if ( @@ -63,6 +63,6 @@ void _Rate_monotonic_Timeout( Watchdog_Control *the_watchdog ) } } else { the_period->state = RATE_MONOTONIC_EXPIRED; - _Rate_monotonic_Release( owner, &lock_context ); + _Rate_monotonic_Release( the_period, &lock_context ); } } diff --git a/cpukit/score/include/rtems/score/scheduler.h b/cpukit/score/include/rtems/score/scheduler.h index c8a7f87541..0a6d68208a 100644 --- a/cpukit/score/include/rtems/score/scheduler.h +++ b/cpukit/score/include/rtems/score/scheduler.h @@ -139,14 +139,14 @@ typedef struct { void ( *node_destroy )( const Scheduler_Control *, Scheduler_Node * ); /** @see _Scheduler_Release_job() */ - void ( *release_job ) ( + Thread_Control *( *release_job ) ( const Scheduler_Control *, Thread_Control *, uint64_t ); /** @see _Scheduler_Cancel_job() */ - void ( *cancel_job ) ( + Thread_Control *( *cancel_job ) ( const Scheduler_Control *, Thread_Control * ); @@ -533,8 +533,10 @@ void _Scheduler_default_Node_destroy( * @param[in] scheduler Unused. * @param[in] the_thread Unused. * @param[in] deadline Unused. + * + * @retval NULL Always. */ -void _Scheduler_default_Release_job( +Thread_Control *_Scheduler_default_Release_job( const Scheduler_Control *scheduler, Thread_Control *the_thread, uint64_t deadline @@ -545,8 +547,10 @@ void _Scheduler_default_Release_job( * * @param[in] scheduler Unused. * @param[in] the_thread Unused. + * + * @retval NULL Always. */ -void _Scheduler_default_Cancel_job( +Thread_Control *_Scheduler_default_Cancel_job( const Scheduler_Control *scheduler, Thread_Control *the_thread ); diff --git a/cpukit/score/include/rtems/score/schedulercbs.h b/cpukit/score/include/rtems/score/schedulercbs.h index bfad633987..c230e08d24 100644 --- a/cpukit/score/include/rtems/score/schedulercbs.h +++ b/cpukit/score/include/rtems/score/schedulercbs.h @@ -163,20 +163,7 @@ Scheduler_Void_or_thread _Scheduler_CBS_Unblock( Thread_Control *the_thread ); -/** - * @brief Called when a new job of task is released. - * - * This routine is called when a new job of task is released. - * It is called only from Rate Monotonic manager in the beginning - * of new period. Deadline has to be shifted and budget replenished. - * - * @param[in] scheduler The scheduler instance. - * @param[in] the_thread is the owner of the job. - * @param[in] length of the new job from now. If equal to 0, - * the job was cancelled or deleted. - */ - -void _Scheduler_CBS_Release_job ( +Thread_Control *_Scheduler_CBS_Release_job( const Scheduler_Control *scheduler, Thread_Control *the_thread, uint64_t length diff --git a/cpukit/score/include/rtems/score/scheduleredf.h b/cpukit/score/include/rtems/score/scheduleredf.h index 5d9b4359c4..81b245e391 100644 --- a/cpukit/score/include/rtems/score/scheduleredf.h +++ b/cpukit/score/include/rtems/score/scheduleredf.h @@ -215,26 +215,13 @@ Scheduler_Void_or_thread _Scheduler_EDF_Yield( Thread_Control *the_thread ); -/** - * @brief Called when a new job of task is released. - * - * This routine is called when a new job of task is released. - * It is called only from Rate Monotonic manager in the beginning - * of new period. - * - * @param[in] scheduler The scheduler instance. - * @param[in] the_thread is the owner of the job. - * @param[in] deadline of the new job from now. If equal to 0, - * the job was cancelled or deleted, thus a running task - * has to be suspended. - */ -void _Scheduler_EDF_Release_job( +Thread_Control *_Scheduler_EDF_Release_job( const Scheduler_Control *scheduler, Thread_Control *the_thread, uint64_t deadline ); -void _Scheduler_EDF_Cancel_job( +Thread_Control *_Scheduler_EDF_Cancel_job( const Scheduler_Control *scheduler, Thread_Control *the_thread ); diff --git a/cpukit/score/include/rtems/score/schedulerimpl.h b/cpukit/score/include/rtems/score/schedulerimpl.h index 91fa178d20..1c5a697619 100644 --- a/cpukit/score/include/rtems/score/schedulerimpl.h +++ b/cpukit/score/include/rtems/score/schedulerimpl.h @@ -496,29 +496,37 @@ RTEMS_INLINE_ROUTINE void _Scheduler_Node_destroy( * * @param[in] the_thread The thread. * @param[in] deadline The deadline in watchdog ticks since boot. + * + * @return The thread to hand over to _Thread_Update_priority(). */ -RTEMS_INLINE_ROUTINE void _Scheduler_Release_job( +RTEMS_INLINE_ROUTINE Thread_Control *_Scheduler_Release_job( Thread_Control *the_thread, uint64_t deadline ) { const Scheduler_Control *scheduler = _Scheduler_Get( the_thread ); - ( *scheduler->Operations.release_job )( scheduler, the_thread, deadline ); + return ( *scheduler->Operations.release_job )( + scheduler, + the_thread, + deadline + ); } /** * @brief Cancels a job of a thread with respect to the scheduler. * * @param[in] the_thread The thread. + * + * @return The thread to hand over to _Thread_Update_priority(). */ -RTEMS_INLINE_ROUTINE void _Scheduler_Cancel_job( +RTEMS_INLINE_ROUTINE Thread_Control *_Scheduler_Cancel_job( Thread_Control *the_thread ) { const Scheduler_Control *scheduler = _Scheduler_Get( the_thread ); - ( *scheduler->Operations.cancel_job )( scheduler, the_thread ); + return ( *scheduler->Operations.cancel_job )( scheduler, the_thread ); } /** diff --git a/cpukit/score/src/schedulercbsreleasejob.c b/cpukit/score/src/schedulercbsreleasejob.c index 124c02bac0..d2169af899 100644 --- a/cpukit/score/src/schedulercbsreleasejob.c +++ b/cpukit/score/src/schedulercbsreleasejob.c @@ -21,7 +21,7 @@ #include -void _Scheduler_CBS_Release_job( +Thread_Control *_Scheduler_CBS_Release_job( const Scheduler_Control *scheduler, Thread_Control *the_thread, uint64_t deadline @@ -30,8 +30,6 @@ void _Scheduler_CBS_Release_job( Scheduler_CBS_Node *node; Scheduler_CBS_Server *serv_info; - _Scheduler_EDF_Release_job( scheduler, the_thread, deadline ); - node = _Scheduler_CBS_Thread_get_node( the_thread ); serv_info = node->cbs_server; @@ -39,4 +37,6 @@ void _Scheduler_CBS_Release_job( if ( serv_info != NULL ) { the_thread->cpu_time_budget = serv_info->parameters.budget; } + + return _Scheduler_EDF_Release_job( scheduler, the_thread, deadline ); } diff --git a/cpukit/score/src/schedulerdefaultreleasejob.c b/cpukit/score/src/schedulerdefaultreleasejob.c index 439f6b2649..7272fc1946 100644 --- a/cpukit/score/src/schedulerdefaultreleasejob.c +++ b/cpukit/score/src/schedulerdefaultreleasejob.c @@ -21,7 +21,7 @@ #include -void _Scheduler_default_Release_job( +Thread_Control *_Scheduler_default_Release_job( const Scheduler_Control *scheduler, Thread_Control *the_thread, uint64_t deadline @@ -30,13 +30,17 @@ void _Scheduler_default_Release_job( (void) scheduler; (void) the_thread; (void) deadline; + + return NULL; } -void _Scheduler_default_Cancel_job( +Thread_Control *_Scheduler_default_Cancel_job( const Scheduler_Control *scheduler, Thread_Control *the_thread ) { (void) scheduler; (void) the_thread; + + return NULL; } diff --git a/cpukit/score/src/scheduleredfreleasejob.c b/cpukit/score/src/scheduleredfreleasejob.c index c10943499a..0fbf0f0f61 100644 --- a/cpukit/score/src/scheduleredfreleasejob.c +++ b/cpukit/score/src/scheduleredfreleasejob.c @@ -42,13 +42,13 @@ static bool _Scheduler_EDF_Release_job_filter( || !_Thread_Owns_resources( the_thread ); } -void _Scheduler_EDF_Release_job( +Thread_Control *_Scheduler_EDF_Release_job( const Scheduler_Control *scheduler, Thread_Control *the_thread, uint64_t deadline ) { - _Thread_Change_priority( + return _Thread_Apply_priority( the_thread, deadline, NULL, @@ -79,12 +79,12 @@ static bool _Scheduler_EDF_Cancel_job_filter( || !_Thread_Owns_resources( the_thread ); } -void _Scheduler_EDF_Cancel_job( +Thread_Control *_Scheduler_EDF_Cancel_job( const Scheduler_Control *scheduler, Thread_Control *the_thread ) { - _Thread_Change_priority( + return _Thread_Apply_priority( the_thread, 0, NULL, -- cgit v1.2.3