[PATCH 3/4] score: Add Watchdog_Iterator
Sebastian Huber
sebastian.huber at embedded-brains.de
Fri Apr 17 08:55:11 UTC 2015
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
More information about the devel
mailing list