This is the mail archive of the
ecos-patches@sources.redhat.com
mailing list for the eCos project.
Clock.cxx: some bug fixes
- From: Nick Garnett <nickg at ecoscentric dot com>
- To: ecos-patches at sources dot redhat dot com
- Date: 25 Jun 2003 18:35:26 +0100
- Subject: Clock.cxx: some bug fixes
The following patch fixes a couple of bugs that have popped up on the
discussion list recently. The rem_alarm() fix is straightforward.
The tick() fix is a total rewrite of a section of code. Before
committing it I would like some others to look it over and check that
it does what it is supposed to. Since CVS diff has made the new code
hard to read in the patch, I have appended a clean copy to the end of
this message.
If I hear nothing in a few days, I'll go ahead and commit it.
Index: ChangeLog
===================================================================
RCS file: /cvs/ecos/ecos/packages/kernel/current/ChangeLog,v
retrieving revision 1.103
diff -u -5 -r1.103 ChangeLog
--- ChangeLog 23 Jun 2003 18:09:59 -0000 1.103
+++ ChangeLog 25 Jun 2003 17:14:11 -0000
@@ -1,5 +1,17 @@
+2003-06-25 Nick Garnett <nickg@balti.calivar.com>
+
+ * src/common/clock.cxx (Cyg_Counter::tick): Rewrote the unsorted
+ list option. If the function of one alarm adds or removes other
+ alarms, then is was possible for the list to become corrupted. The
+ new implementation attempts to avoid this problem.
+
+ * src/common/clock.cxx (Cyg_Counter::rem_alarm): Call
+ Cyg_Scheduler::lock() before calculation of index into alarm_list
+ array to avoid race condition with multi list counters.
+ [Found and fixed by Thomas Binder <Thomas.Binder@frequentis.com>]
+
2003-06-06 David Brennan <eCos@brennanhome.com>
2003-06-23 Nick Garnett <nickg@balti.calivar.com>
* cdl/kernel.cdl: Added tests/bin_sem3 to list of kernel tests.
Index: src/common/clock.cxx
===================================================================
RCS file: /cvs/ecos/ecos/packages/kernel/current/src/common/clock.cxx,v
retrieving revision 1.15
diff -u -5 -r1.15 clock.cxx
--- src/common/clock.cxx 1 Oct 2002 19:10:21 -0000 1.15
+++ src/common/clock.cxx 25 Jun 2003 17:14:13 -0000
@@ -162,11 +162,11 @@
// With multiple lists, each one contains only the alarms
// that will expire at a given tick modulo the list number.
// So we only have a fraction of the alarms to check here.
alarm_list_ptr = &(alarm_list[
- (counter/increment) % CYGNUM_KERNEL_COUNTERS_MULTI_LIST_SIZE ] );
+ (counter/increment) % CYGNUM_KERNEL_COUNTERS_MULTI_LIST_SIZE ] );
#else
#error "No CYGIMP_KERNEL_COUNTERS_x_LIST config"
#endif
@@ -209,60 +209,82 @@
else break;
}
#else
- // With an unsorted list, we must scan the whole list for
- // candidates. We move the whole list to a temporary location
- // before doing this so that we are not disturbed by new
- // alarms being added to the list. As we consider and
- // eliminate alarms we put them onto the done_list and at the
- // end we then move it back to where it belongs.
+ // With unsorted lists we must scan the whole list for
+ // candidates. However, we must be careful here since it is
+ // possible for the function of one alarm to add or remove
+ // other alarms to/from this list. Having the list shift under
+ // our feet in this way could be disasterous. We solve this by
+ // detecting when the list has changed and restarting the scan
+ // from the beginning.
- Cyg_Alarm_List done_list;
+ Cyg_DNode_T<Cyg_Alarm> *node = alarm_list_ptr->get_head();
- Cyg_Alarm_List alarm_list;
-
- alarm_list.merge( *alarm_list_ptr );
-
- while( !alarm_list.empty() )
+ while( node != NULL )
{
- Cyg_Alarm *alarm = alarm_list.rem_head();
+ Cyg_Alarm *alarm = CYG_CLASSFROMBASE( Cyg_Alarm, Cyg_DNode, node );
+ Cyg_DNode_T<Cyg_Alarm> *next = alarm->get_next();
CYG_ASSERTCLASS(alarm, "Bad alarm in counter list" );
if( alarm->trigger <= counter )
{
+ Cyg_DNode *next_next, *next_prev;
+ Cyg_Alarm *head;
+
+ alarm_list_ptr->remove(alarm);
+
if( alarm->interval != 0 )
{
// The alarm has a retrigger interval.
// Reset the trigger time and requeue
// the alarm.
alarm->trigger += alarm->interval;
add_alarm( alarm );
}
else alarm->enabled = false;
+ // Save some details of the list state so we can
+ // detect if it has changed.
+ next_next = next->get_next();
+ next_prev = next->get_prev();
+ head = alarm_list_ptr->get_head();
+
CYG_INSTRUMENT_ALARM( CALL, this, alarm );
- // call alarm function
+ // Call alarm function
alarm->alarm(alarm, alarm->data);
- // all done, loop
+ // If that alarm function has added or removed an
+ // alarm on this list that might affect this scan,
+ // then start again from beginning. The main test here
+ // is whether the link fields of the next node have
+ // changed, implying that it may have been moved. The
+ // test against the list head detects the case where
+ // the next node was the only node in the list, when
+ // moving is would have left the links unchanged.
+
+ if( next_next != next->get_next() ||
+ next_prev != next->get_prev() ||
+ head != alarm_list_ptr->get_head() )
+ {
+ node = alarm_list_ptr->get_head();
+ continue;
+ }
}
+
+ // If the next node is the head of the list, then we have
+ // looped all the way around. The node == next test
+ // catches the case where we only had one element to start
+ // with.
+ if( next == alarm_list_ptr->get_head() || node == next )
+ node = NULL;
else
- {
- // add unused alarm to done list.
- done_list.add_tail(alarm);
- }
+ node = next;
}
-
- // Return done list to real list. If any alarms have been
- // added to the alarm list while we have been scanning then
- // the done list will be added behind them.
-
- alarm_list_ptr->merge( done_list );
#endif
Cyg_Scheduler::unlock();
}
@@ -395,10 +417,12 @@
CYG_ASSERTCLASS( this, "Bad counter object" );
CYG_ASSERTCLASS( alarm, "Bad alarm passed" );
Cyg_Alarm_List *alarm_list_ptr; // pointer to list
+ Cyg_Scheduler::lock();
+
#if defined(CYGIMP_KERNEL_COUNTERS_SINGLE_LIST)
alarm_list_ptr = &alarm_list;
#elif defined(CYGIMP_KERNEL_COUNTERS_MULTI_LIST)
@@ -411,12 +435,10 @@
#error "No CYGIMP_KERNEL_COUNTERS_x_LIST config"
#endif
// Now that we have the list pointer, we can use common code for
// both list organizations.
-
- Cyg_Scheduler::lock();
CYG_INSTRUMENT_ALARM( REM, this, alarm );
alarm_list_ptr->remove( alarm );
-------------------------------------------------------------------------
#else
// With unsorted lists we must scan the whole list for
// candidates. However, we must be careful here since it is
// possible for the function of one alarm to add or remove
// other alarms to/from this list. Having the list shift under
// our feet in this way could be disasterous. We solve this by
// detecting when the list has changed and restarting the scan
// from the beginning.
Cyg_DNode_T<Cyg_Alarm> *node = alarm_list_ptr->get_head();
while( node != NULL )
{
Cyg_Alarm *alarm = CYG_CLASSFROMBASE( Cyg_Alarm, Cyg_DNode, node );
Cyg_DNode_T<Cyg_Alarm> *next = alarm->get_next();
CYG_ASSERTCLASS(alarm, "Bad alarm in counter list" );
if( alarm->trigger <= counter )
{
Cyg_DNode *next_next, *next_prev;
Cyg_Alarm *head;
alarm_list_ptr->remove(alarm);
if( alarm->interval != 0 )
{
// The alarm has a retrigger interval.
// Reset the trigger time and requeue
// the alarm.
alarm->trigger += alarm->interval;
add_alarm( alarm );
}
else alarm->enabled = false;
// Save some details of the list state so we can
// detect if it has changed.
next_next = next->get_next();
next_prev = next->get_prev();
head = alarm_list_ptr->get_head();
CYG_INSTRUMENT_ALARM( CALL, this, alarm );
// Call alarm function
alarm->alarm(alarm, alarm->data);
// If that alarm function has added or removed an
// alarm on this list that might affect this scan,
// then start again from beginning. The main test here
// is whether the link fields of the next node have
// changed, implying that it may have been moved. The
// test against the list head detects the case where
// the next node was the only node in the list, when
// moving is would have left the links unchanged.
if( next_next != next->get_next() ||
next_prev != next->get_prev() ||
head != alarm_list_ptr->get_head() )
{
node = alarm_list_ptr->get_head();
continue;
}
}
// If the next node is the head of the list, then we have
// looped all the way around. The node == next test
// catches the case where we only had one element to start
// with.
if( next == alarm_list_ptr->get_head() || node == next )
node = NULL;
else
node = next;
}
#endif
--
Nick Garnett eCos Kernel Architect
http://www.ecoscentric.com/ The eCos and RedBoot experts