I'm not a big fan of this "watchdog_remove_it" terminology. Can I suggest the state be called something else like WATCHDOG_REMOVING or maybe WATCHDOG_TRANSIENT, and perhaps the actual removal function be renamed _Watchdog_Remove_internal() or something similar? Also, given the refactoring here and that this _Watchdog_Remove_it() is only called by Watchdog_Remove/Tickle, perhaps it makes sense to put all three functions into a single source file and make the Remove_it() function static in that file.
Gedare On Fri, Apr 17, 2015 at 4:55 AM, Sebastian Huber <sebastian.hu...@embedded-brains.de> wrote: > Rewrite the _Watchdog_Insert(), _Watchdog_Remove() and > _Watchdog_Tickle() functions to use interator 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 | 130 > ++++++++---------------- > testsuites/sptests/spsize/size.c | 4 +- > testsuites/sptests/spwatchdog/init.c | 103 +++++++++++++++++++ > 8 files changed, 317 insertions(+), 169 deletions(-) > > diff --git a/cpukit/rtems/src/timercreate.c b/cpukit/rtems/src/timercreate.c > index 390c965..ef217e9 100644 > --- a/cpukit/rtems/src/timercreate.c > +++ b/cpukit/rtems/src/timercreate.c > @@ -28,6 +28,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: > @@ -44,6 +48,8 @@ void _Timer_Cancel( Timer_Control *the_timer ) > default: > 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 f52b55d..304392b 100644 > --- a/cpukit/score/include/rtems/score/watchdogimpl.h > +++ b/cpukit/score/include/rtems/score/watchdogimpl.h > @@ -46,6 +46,28 @@ extern "C" { > } > > /** > + * @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. > */ > typedef struct { > @@ -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. > @@ -139,6 +153,16 @@ Watchdog_States _Watchdog_Remove ( > ); > > /** > + * @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 0db60ef..11d3cf2 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 0ad59ff..6d2df82 100644 > --- a/cpukit/score/src/watchdoginsert.c > +++ b/cpukit/score/src/watchdoginsert.c > @@ -19,76 +19,92 @@ > #endif > > #include <rtems/score/watchdogimpl.h> > -#include <rtems/score/isrlevel.h> > -#include <rtems/score/percpu.h> > > -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 c765ac5..c896fbb 100644 > --- a/cpukit/score/src/watchdogremove.c > +++ b/cpukit/score/src/watchdogremove.c > @@ -18,9 +18,58 @@ > #include "config.h" > #endif > > -#include <rtems/system.h> > -#include <rtems/score/isr.h> > #include <rtems/score/watchdogimpl.h> > +#include <rtems/score/assert.h> > + > +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 2092010..2ae4f94 100644 > --- a/cpukit/score/src/watchdogtickle.c > +++ b/cpukit/score/src/watchdogtickle.c > @@ -19,99 +19,59 @@ > #endif > > #include <rtems/score/watchdogimpl.h> > -#include <rtems/score/isrlevel.h> > > 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; > + > + run = first->state == WATCHDOG_ACTIVE; > + _Watchdog_Remove_it( header, first ); > + _Watchdog_Release( header, &lock_context ); > + > + if ( run ) { > + (*first->routine)( first->id, first->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 f00da0d..58bcc89 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 1d3cb2f..54bd5c3 100644 > --- a/testsuites/sptests/spwatchdog/init.c > +++ b/testsuites/sptests/spwatchdog/init.c > @@ -34,6 +34,108 @@ 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 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 ) ); > + > + 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 ); > + > + 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 ); > +} > + > static void test_watchdog_static_init( void ) > { > #if defined(RTEMS_USE_16_BIT_OBJECT) > @@ -70,6 +172,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 ); > > -- > 1.8.4.5 > > _______________________________________________ > devel mailing list > devel@rtems.org > http://lists.rtems.org/mailman/listinfo/devel _______________________________________________ devel mailing list devel@rtems.org http://lists.rtems.org/mailman/listinfo/devel