From 1ccbd052910ed16131c74b0d5595c8a94066942d Mon Sep 17 00:00:00 2001 From: Sebastian Huber Date: Wed, 15 Apr 2015 10:53:29 +0200 Subject: score: Add Watchdog_Iterator Rewrite the _Watchdog_Insert(), _Watchdog_Remove() and _Watchdog_Tickle() functions to use iterator items to synchronize concurrent operations. This makes it possible to get rid of the global variables _Watchdog_Sync_level and _Watchdog_Sync_count which are a blocking point for scalable SMP solutions. Update #2307. --- cpukit/rtems/src/timercreate.c | 6 + cpukit/score/include/rtems/score/watchdogimpl.h | 58 +++++++--- cpukit/score/src/watchdog.c | 2 - cpukit/score/src/watchdoginsert.c | 112 +++++++++++-------- cpukit/score/src/watchdogremove.c | 71 +++++++++--- cpukit/score/src/watchdogtickle.c | 139 +++++++++--------------- testsuites/sptests/spsize/size.c | 4 +- testsuites/sptests/spwatchdog/init.c | 114 +++++++++++++++++++ 8 files changed, 337 insertions(+), 169 deletions(-) diff --git a/cpukit/rtems/src/timercreate.c b/cpukit/rtems/src/timercreate.c index 2c6d251b8a..13a01feda9 100644 --- a/cpukit/rtems/src/timercreate.c +++ b/cpukit/rtems/src/timercreate.c @@ -29,6 +29,10 @@ void _Timer_Cancel( Timer_Control *the_timer ) { Timer_server_Control *timer_server; + ISR_Level level; + + /* The timer class must not change during the cancel operation */ + _ISR_Disable( level ); switch ( the_timer->the_class ) { case TIMER_INTERVAL: @@ -46,6 +50,8 @@ void _Timer_Cancel( Timer_Control *the_timer ) _Assert( the_timer->the_class == TIMER_DORMANT ); break; } + + _ISR_Enable( level ); } rtems_status_code rtems_timer_create( diff --git a/cpukit/score/include/rtems/score/watchdogimpl.h b/cpukit/score/include/rtems/score/watchdogimpl.h index f52b55d83c..304392b46c 100644 --- a/cpukit/score/include/rtems/score/watchdogimpl.h +++ b/cpukit/score/include/rtems/score/watchdogimpl.h @@ -45,6 +45,28 @@ extern "C" { ( routine ), ( id ), ( user_data ) \ } +/** + * @brief Iterator item to synchronize concurrent insert, remove and tickle + * operations. + */ +typedef struct { + /** + * @brief A node for a Watchdog_Header::Iterators chain. + */ + Chain_Node Node; + + /** + * @brief The current delta interval of the new watchdog to insert. + */ + Watchdog_Interval delta_interval; + + /** + * @brief The current watchdog of the chain on the way to insert the new + * watchdog. + */ + Chain_Node *current; +} Watchdog_Iterator; + /** * @brief Watchdog header. */ @@ -58,23 +80,15 @@ typedef struct { * @brief The chain of active or transient watchdogs. */ Chain_Control Watchdogs; -} Watchdog_Header; - -/** - * @brief Watchdog synchronization level. - * - * This used for synchronization purposes - * during an insert on a watchdog delta chain. - */ -SCORE_EXTERN volatile uint32_t _Watchdog_Sync_level; -/** - * @brief Watchdog synchronization count. - * - * This used for synchronization purposes - * during an insert on a watchdog delta chain. - */ -SCORE_EXTERN volatile uint32_t _Watchdog_Sync_count; + /** + * @brief Currently active iterators. + * + * The iterators are registered in _Watchdog_Insert() and updated in case the + * watchdog chain changes. + */ + Chain_Control Iterators; +} Watchdog_Header; /** * @brief Watchdog chain which is managed at ticks. @@ -138,6 +152,16 @@ Watchdog_States _Watchdog_Remove ( Watchdog_Control *the_watchdog ); +/** + * @brief Actually removes an WATCHDOG_ACTIVE or WATCHDOG_REMOVE_IT watchdog. + * + * @see _Watchdog_Remove() and _Watchdog_Tickle(). + */ +void _Watchdog_Remove_it( + Watchdog_Header *header, + Watchdog_Control *the_watchdog +); + /** * @brief Adjusts the header watchdog chain in the backward direction for * units ticks. @@ -437,7 +461,9 @@ RTEMS_INLINE_ROUTINE void _Watchdog_Header_initialize( Watchdog_Header *header ) { + _ISR_lock_Initialize( &header->Lock, "Watchdog" ); _Chain_Initialize_empty( &header->Watchdogs ); + _Chain_Initialize_empty( &header->Iterators ); } /** @} */ diff --git a/cpukit/score/src/watchdog.c b/cpukit/score/src/watchdog.c index 0db60efe6b..11d3cf289f 100644 --- a/cpukit/score/src/watchdog.c +++ b/cpukit/score/src/watchdog.c @@ -25,8 +25,6 @@ void _Watchdog_Handler_initialization( void ) { - _Watchdog_Sync_count = 0; - _Watchdog_Sync_level = 0; _Watchdog_Ticks_since_boot = 0; _Watchdog_Header_initialize( &_Watchdog_Ticks_header ); diff --git a/cpukit/score/src/watchdoginsert.c b/cpukit/score/src/watchdoginsert.c index 0ad59ff61a..6d2df8222f 100644 --- a/cpukit/score/src/watchdoginsert.c +++ b/cpukit/score/src/watchdoginsert.c @@ -19,76 +19,92 @@ #endif #include -#include -#include -void _Watchdog_Insert( - Watchdog_Header *header, - Watchdog_Control *the_watchdog +static void _Watchdog_Insert_fixup( + Watchdog_Header *header, + Watchdog_Control *next_watchdog, + Watchdog_Interval delta ) { - ISR_lock_Context lock_context; - Watchdog_Control *after; - uint32_t insert_isr_nest_level; - Watchdog_Interval delta_interval; + const Chain_Node *iterator_tail; + Chain_Node *iterator_node; + next_watchdog->delta_interval -= delta; - insert_isr_nest_level = _ISR_Nest_level; + iterator_node = _Chain_First( &header->Iterators ); + iterator_tail = _Chain_Immutable_tail( &header->Iterators ); - _Watchdog_Acquire( header, &lock_context ); + while ( iterator_node != iterator_tail ) { + Watchdog_Iterator *iterator; - /* - * Check to see if the watchdog has just been inserted by a - * higher priority interrupt. If so, abandon this insert. - */ + iterator = (Watchdog_Iterator *) iterator_node; - if ( the_watchdog->state != WATCHDOG_INACTIVE ) { - _Watchdog_Release( header, &lock_context ); - return; + if ( iterator->current == &next_watchdog->Node ) { + iterator->delta_interval -= delta; + } + + iterator_node = _Chain_Next( iterator_node ); } +} - the_watchdog->state = WATCHDOG_BEING_INSERTED; - _Watchdog_Sync_count++; +void _Watchdog_Insert( + Watchdog_Header *header, + Watchdog_Control *the_watchdog +) +{ + ISR_lock_Context lock_context; -restart: - delta_interval = the_watchdog->initial; + _Watchdog_Acquire( header, &lock_context ); - for ( after = _Watchdog_First( header ) ; - ; - after = _Watchdog_Next( after ) ) { + if ( the_watchdog->state == WATCHDOG_INACTIVE ) { + Watchdog_Iterator iterator; + Chain_Node *current; + Chain_Node *next; + Watchdog_Interval delta; - if ( delta_interval == 0 || !_Watchdog_Next( after ) ) - break; + the_watchdog->state = WATCHDOG_BEING_INSERTED; - if ( delta_interval < after->delta_interval ) { - after->delta_interval -= delta_interval; - break; - } + _Chain_Append_unprotected( &header->Iterators, &iterator.Node ); - delta_interval -= after->delta_interval; + delta = the_watchdog->initial; + current = _Chain_Head( &header->Watchdogs ); - _Watchdog_Flash( header, &lock_context ); + while ( + ( next = _Chain_Next( current ) ) != _Chain_Tail( &header->Watchdogs ) + ) { + Watchdog_Control *next_watchdog; + Watchdog_Interval delta_next; - if ( the_watchdog->state != WATCHDOG_BEING_INSERTED ) { - goto exit_insert; - } + next_watchdog = (Watchdog_Control *) next; + delta_next = next_watchdog->delta_interval; - if ( _Watchdog_Sync_level > insert_isr_nest_level ) { - _Watchdog_Sync_level = insert_isr_nest_level; - goto restart; - } - } + if ( delta < delta_next ) { + _Watchdog_Insert_fixup( header, next_watchdog, delta ); + break; + } + + iterator.delta_interval = delta - delta_next; + iterator.current = next; - _Watchdog_Activate( the_watchdog ); + _Watchdog_Flash( header, &lock_context ); - the_watchdog->delta_interval = delta_interval; + if ( the_watchdog->state != WATCHDOG_BEING_INSERTED ) { + goto abort_insert; + } - _Chain_Insert_unprotected( after->Node.previous, &the_watchdog->Node ); + delta = iterator.delta_interval; + current = iterator.current; + } - the_watchdog->start_time = _Watchdog_Ticks_since_boot; + the_watchdog->delta_interval = delta; + the_watchdog->start_time = _Watchdog_Ticks_since_boot; + _Watchdog_Activate( the_watchdog ); + _Chain_Insert_unprotected( current, &the_watchdog->Node ); + +abort_insert: + + _Chain_Extract_unprotected( &iterator.Node ); + } -exit_insert: - _Watchdog_Sync_level = insert_isr_nest_level; - _Watchdog_Sync_count--; _Watchdog_Release( header, &lock_context ); } diff --git a/cpukit/score/src/watchdogremove.c b/cpukit/score/src/watchdogremove.c index c765ac55c7..c896fbb00f 100644 --- a/cpukit/score/src/watchdogremove.c +++ b/cpukit/score/src/watchdogremove.c @@ -18,9 +18,58 @@ #include "config.h" #endif -#include -#include #include +#include + +void _Watchdog_Remove_it( + Watchdog_Header *header, + Watchdog_Control *the_watchdog +) +{ + Chain_Node *next; + Watchdog_Interval delta; + const Chain_Node *iterator_tail; + Chain_Node *iterator_node; + + _Assert( + the_watchdog->state == WATCHDOG_ACTIVE + || the_watchdog->state == WATCHDOG_REMOVE_IT + ); + + the_watchdog->state = WATCHDOG_INACTIVE; + the_watchdog->stop_time = _Watchdog_Ticks_since_boot; + + next = _Chain_Next( &the_watchdog->Node ); + delta = the_watchdog->delta_interval; + + if ( next != _Chain_Tail( &header->Watchdogs ) ) { + Watchdog_Control *next_watchdog; + + next_watchdog = (Watchdog_Control *) next; + next_watchdog->delta_interval += delta; + } + + _Chain_Extract_unprotected( &the_watchdog->Node ); + + iterator_node = _Chain_First( &header->Iterators ); + iterator_tail = _Chain_Immutable_tail( &header->Iterators ); + + while ( iterator_node != iterator_tail ) { + Watchdog_Iterator *iterator; + + iterator = (Watchdog_Iterator *) iterator_node; + + if ( iterator->current == next ) { + iterator->delta_interval += delta; + } + + if ( iterator->current == &the_watchdog->Node ) { + iterator->current = _Chain_Previous( &the_watchdog->Node ); + } + + iterator_node = _Chain_Next( iterator_node ); + } +} Watchdog_States _Watchdog_Remove( Watchdog_Header *header, @@ -29,7 +78,7 @@ Watchdog_States _Watchdog_Remove( { ISR_lock_Context lock_context; Watchdog_States previous_state; - Watchdog_Control *next_watchdog; + Watchdog_Interval now; _Watchdog_Acquire( header, &lock_context ); previous_state = the_watchdog->state; @@ -44,24 +93,16 @@ Watchdog_States _Watchdog_Remove( * the Insert operation we interrupted will be aborted. */ the_watchdog->state = WATCHDOG_INACTIVE; + now = _Watchdog_Ticks_since_boot; + the_watchdog->start_time = now; + the_watchdog->stop_time = now; break; case WATCHDOG_ACTIVE: case WATCHDOG_REMOVE_IT: - - the_watchdog->state = WATCHDOG_INACTIVE; - next_watchdog = _Watchdog_Next( the_watchdog ); - - if ( _Watchdog_Next(next_watchdog) ) - next_watchdog->delta_interval += the_watchdog->delta_interval; - - if ( _Watchdog_Sync_count ) - _Watchdog_Sync_level = _ISR_Nest_level; - - _Chain_Extract_unprotected( &the_watchdog->Node ); + _Watchdog_Remove_it( header, the_watchdog ); break; } - the_watchdog->stop_time = _Watchdog_Ticks_since_boot; _Watchdog_Release( header, &lock_context ); return( previous_state ); diff --git a/cpukit/score/src/watchdogtickle.c b/cpukit/score/src/watchdogtickle.c index 2092010dcc..7a80008c4d 100644 --- a/cpukit/score/src/watchdogtickle.c +++ b/cpukit/score/src/watchdogtickle.c @@ -19,99 +19,68 @@ #endif #include -#include void _Watchdog_Tickle( Watchdog_Header *header ) { - ISR_lock_Context lock_context; - Watchdog_Control *the_watchdog; - Watchdog_States watchdog_state; - - /* - * See the comment in watchdoginsert.c and watchdogadjust.c - * about why it's safe not to declare header a pointer to - * volatile data - till, 2003/7 - */ + ISR_lock_Context lock_context; _Watchdog_Acquire( header, &lock_context ); - if ( _Watchdog_Is_empty( header ) ) - goto leave; - - the_watchdog = _Watchdog_First( header ); - - /* - * For some reason, on rare occasions the_watchdog->delta_interval - * of the head of the watchdog chain is 0. Before this test was - * added, on these occasions an event (which usually was supposed - * to have a timeout of 1 tick would have a delta_interval of 0, which - * would be decremented to 0xFFFFFFFF by the unprotected - * "the_watchdog->delta_interval--;" operation. - * This would mean the event would not timeout, and also the chain would - * be blocked, because a timeout with a very high number would be at the - * head, rather than at the end. - * The test "if (the_watchdog->delta_interval != 0)" - * here prevents this from occuring. - * - * We were not able to categorically identify the situation that causes - * this, but proved it to be true empirically. So this check causes - * correct behaviour in this circumstance. - * - * The belief is that a race condition exists whereby an event at the head - * of the chain is removed (by a pending ISR or higher priority task) - * during the _ISR_Flash( level ); in _Watchdog_Insert, but the watchdog - * to be inserted has already had its delta_interval adjusted to 0, and - * so is added to the head of the chain with a delta_interval of 0. - * - * Steven Johnson - 12/2005 (gcc-3.2.3 -O3 on powerpc) - */ - if (the_watchdog->delta_interval != 0) { - the_watchdog->delta_interval--; - if ( the_watchdog->delta_interval != 0 ) - goto leave; + if ( !_Watchdog_Is_empty( header ) ) { + Watchdog_Control *first; + Watchdog_Interval delta; + + first = _Watchdog_First( header ); + delta = first->delta_interval; + + /* + * Although it is forbidden to insert watchdogs with a delta interval of + * zero it is possible to observe watchdogs with a delta interval of zero + * at this point. For example lets have a watchdog chain of one watchdog + * with a delta interval of one and insert a new one with an initial value + * of one. At the start of the insert procedure it will advance one step + * and reduce its delta interval by one yielding zero. Now a tick happens. + * This will remove the watchdog on the chain and update the insert + * iterator. Now the insert operation continues and will insert the new + * watchdog with a delta interval of zero. + */ + if ( delta > 0 ) { + --delta; + first->delta_interval = delta; + } + + while ( delta == 0 ) { + bool run; + Watchdog_Service_routine_entry routine; + Objects_Id id; + void *user_data; + + run = ( first->state == WATCHDOG_ACTIVE ); + + _Watchdog_Remove_it( header, first ); + + routine = first->routine; + id = first->id; + user_data = first->user_data; + + _Watchdog_Release( header, &lock_context ); + + if ( run ) { + (*routine)( id, user_data ); + } + + _Watchdog_Acquire( header, &lock_context ); + + if ( _Watchdog_Is_empty( header ) ) { + break; + } + + first = _Watchdog_First( header ); + delta = first->delta_interval; + } } - do { - watchdog_state = _Watchdog_Remove( header, the_watchdog ); - - _Watchdog_Release( header, &lock_context ); - - switch( watchdog_state ) { - case WATCHDOG_ACTIVE: - (*the_watchdog->routine)( - the_watchdog->id, - the_watchdog->user_data - ); - break; - - case WATCHDOG_INACTIVE: - /* - * This state indicates that the watchdog is not on any chain. - * Thus, it is NOT on a chain being tickled. This case should - * never occur. - */ - break; - - case WATCHDOG_BEING_INSERTED: - /* - * This state indicates that the watchdog is in the process of - * BEING inserted on the chain. Thus, it can NOT be on a chain - * being tickled. This case should never occur. - */ - break; - - case WATCHDOG_REMOVE_IT: - break; - } - - _Watchdog_Acquire( header, &lock_context ); - - the_watchdog = _Watchdog_First( header ); - } while ( !_Watchdog_Is_empty( header ) && - (the_watchdog->delta_interval == 0) ); - -leave: - _Watchdog_Release( header, &lock_context ); + _Watchdog_Release( header, &lock_context ); } diff --git a/testsuites/sptests/spsize/size.c b/testsuites/sptests/spsize/size.c index f00da0d465..58bcc89312 100644 --- a/testsuites/sptests/spsize/size.c +++ b/testsuites/sptests/spsize/size.c @@ -399,9 +399,7 @@ uninitialized = /*userext.h*/ (sizeof _User_extensions_List) + -/*watchdog.h*/ (sizeof _Watchdog_Sync_level) + - (sizeof _Watchdog_Sync_count) + - (sizeof _Watchdog_Ticks_since_boot) + +/*watchdog.h*/ (sizeof _Watchdog_Ticks_since_boot) + (sizeof _Watchdog_Ticks_header) + (sizeof _Watchdog_Seconds_header) + diff --git a/testsuites/sptests/spwatchdog/init.c b/testsuites/sptests/spwatchdog/init.c index 1d3cb2f9dd..283f4c87a0 100644 --- a/testsuites/sptests/spwatchdog/init.c +++ b/testsuites/sptests/spwatchdog/init.c @@ -34,6 +34,119 @@ static void test_watchdog_routine( Objects_Id id, void *arg ) rtems_test_assert( 0 ); } +static void init_watchdogs( + Watchdog_Header *header, + Watchdog_Control watchdogs[3] +) +{ + Watchdog_Control *a = &watchdogs[0]; + Watchdog_Control *b = &watchdogs[1]; + Watchdog_Control *c = &watchdogs[2]; + Watchdog_Control *d = &watchdogs[3]; + + _Watchdog_Header_initialize( header ); + rtems_test_assert( _Watchdog_Is_empty( header ) ); + rtems_test_assert( _Chain_Is_empty( &header->Iterators ) ); + + _Watchdog_Initialize( c, NULL, 0, NULL ); + c->initial = 6; + _Watchdog_Insert( header, c ); + rtems_test_assert( c->delta_interval == 6 ); + + rtems_test_assert( !_Watchdog_Is_empty( header ) ); + rtems_test_assert( _Chain_Is_empty( &header->Iterators ) ); + + _Watchdog_Initialize( a, NULL, 0, NULL ); + a->initial = 2; + _Watchdog_Insert( header, a ); + rtems_test_assert( a->delta_interval == 2 ); + rtems_test_assert( c->delta_interval == 4 ); + + _Watchdog_Initialize( b, NULL, 0, NULL ); + b->initial = 4; + _Watchdog_Insert( header, b ); + rtems_test_assert( a->delta_interval == 2 ); + rtems_test_assert( b->delta_interval == 2 ); + rtems_test_assert( c->delta_interval == 2 ); + + _Watchdog_Initialize( d, NULL, 0, NULL ); +} + +static void destroy_watchdogs( + Watchdog_Header *header +) +{ + _ISR_lock_Destroy( &header->Lock ); +} + +static void add_iterator( + Watchdog_Header *header, + Watchdog_Iterator *i, + Watchdog_Control *w +) +{ + _Chain_Append_unprotected( &header->Iterators, &i->Node ); + i->delta_interval = 2; + i->current = &w->Node; +} + +static void test_watchdog_insert_and_remove( void ) +{ + Watchdog_Header header; + Watchdog_Control watchdogs[4]; + Watchdog_Control *a = &watchdogs[0]; + Watchdog_Control *b = &watchdogs[1]; + Watchdog_Control *c = &watchdogs[2]; + Watchdog_Control *d = &watchdogs[3]; + Watchdog_Iterator i; + + init_watchdogs( &header, watchdogs ); + add_iterator( &header, &i, c ); + + /* Remove next watchdog of iterator */ + _Watchdog_Remove( &header, c ); + rtems_test_assert( i.delta_interval == 2 ); + rtems_test_assert( i.current == &b->Node ); + + /* Remove watchdog before the current watchdog of iterator */ + _Watchdog_Remove( &header, a ); + rtems_test_assert( i.delta_interval == 4 ); + rtems_test_assert( i.current == &b->Node ); + + /* Remove current (= last) watchdog of iterator */ + _Watchdog_Remove( &header, b ); + rtems_test_assert( i.delta_interval == 4 ); + rtems_test_assert( i.current == _Chain_Head( &header.Watchdogs ) ); + + /* Insert first watchdog */ + a->initial = 1; + _Watchdog_Insert( &header, a ); + rtems_test_assert( i.delta_interval == 4 ); + rtems_test_assert( i.current == _Chain_Head( &header.Watchdogs ) ); + + destroy_watchdogs( &header ); + init_watchdogs( &header, watchdogs ); + add_iterator( &header, &i, b ); + + /* Insert right before current watchdog of iterator */ + d->initial = 3; + _Watchdog_Insert( &header, d ); + rtems_test_assert( i.delta_interval == 1 ); + rtems_test_assert( i.current == &b->Node ); + + destroy_watchdogs( &header ); + init_watchdogs( &header, watchdogs ); + add_iterator( &header, &i, b ); + + /* Insert right after current watchdog of iterator */ + d->initial = 5; + _Watchdog_Insert( &header, d ); + rtems_test_assert( i.delta_interval == 2 ); + rtems_test_assert( i.current == &b->Node ); + + destroy_watchdogs( &header ); +} + static void test_watchdog_static_init( void ) { #if defined(RTEMS_USE_16_BIT_OBJECT) @@ -70,6 +183,7 @@ rtems_task Init( TEST_BEGIN(); test_watchdog_static_init(); + test_watchdog_insert_and_remove(); build_time( &time, 12, 31, 1988, 9, 0, 0, 0 ); -- cgit v1.2.3