This is the mail archive of the
ecos-patches@sources.redhat.com
mailing list for the eCos project.
Re: Clock.cxx: some bug fixes
- From: Nick Garnett <nickg at ecoscentric dot com>
- To: ecos-patches at sources dot redhat dot com
- Date: 01 Jul 2003 18:37:53 +0100
- Subject: Re: Clock.cxx: some bug fixes
- References: <m3smpy2h7l.fsf@miso.calivar.com>
Nick Garnett <nickg@ecoscentric.com> writes:
> 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.
>
I have now committed this, together with a test program as suggested
by Jifl. The committed patch follows:
Index: ChangeLog
===================================================================
RCS file: /cvs/ecos/ecos/packages/kernel/current/ChangeLog,v
retrieving revision 1.104
diff -u -5 -r1.104 ChangeLog
--- ChangeLog 25 Jun 2003 17:44:28 -0000 1.104
+++ ChangeLog 1 Jul 2003 17:33:40 -0000
@@ -1,5 +1,34 @@
+2003-07-01 Nick Garnett <nickg@balti.calivar.com>
+
+ * include/clock.hxx: Made Cyg_Counter::add_alarm() and
+ Cyg_Counter::rem_alarm() private functions. They no longer lock
+ the scheduler, so should not be called directly, only via the
+ Cyg_Alarm functions.
+
+ * include/clock.inl: Removed inline version of
+ Cyg_Alarm::disable(). It's no longer a one-liner and is thus
+ better as a proper function.
+
+ * 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.
+ (Cyg_Alarm::initialize): Added scheduler locking to protect the
+ enabled flag.
+ (Cyg_Alarm::enable): Ditto.
+ (Cyg_Alarm::disable): Ditto. Moved here from clock.inl.
+ (Cyg_Alarm::add_alarm): Removed scheduler locking, it is now
+ always called from functions that have already locked it. Added an
+ assertion to double-check this.
+ (Cyg_Alarm::rem_alarm): Ditto.
+
+ * cdl/kernel.cdl:
+ * tests/kalarm0.c:
+ Added new test to test that alarms can be added and removed in
+ alarm functions safely.
+
2003-06-25 Thomas Binder <Thomas.Binder@frequentis.com>
* src/common/clock.cxx (Cyg_Counter::rem_alarm): Bugfix: call
Cyg_Scheduler::lock() before calculation of index into alarm_list
array to avoid race condition with multi list counters.
Index: cdl/kernel.cdl
===================================================================
RCS file: /cvs/ecos/ecos/packages/kernel/current/cdl/kernel.cdl,v
retrieving revision 1.19
diff -u -5 -r1.19 kernel.cdl
--- cdl/kernel.cdl 23 Jun 2003 18:10:00 -0000 1.19
+++ cdl/kernel.cdl 1 Jul 2003 17:33:40 -0000
@@ -321,11 +321,11 @@
display "Kernel tests"
flavor data
no_define
calculated {
"tests/bin_sem0 tests/bin_sem1 tests/bin_sem2 tests/bin_sem3 tests/clock0 tests/clock1 tests/clockcnv tests/clocktruth tests/cnt_sem0 tests/cnt_sem1 tests/except1 tests/flag0 tests/flag1 tests/intr0 tests/kill tests/mbox1 tests/mqueue1 tests/mutex0 tests/mutex1 tests/mutex2 tests/mutex3 tests/release tests/sched1 tests/sync2 tests/sync3 tests/thread0 tests/thread1 tests/thread2"
- . ((CYGFUN_KERNEL_API_C) ? " tests/kclock0 tests/kclock1 tests/kexcept1 tests/kflag0 tests/kflag1 tests/kintr0 tests/klock tests/kmbox1 tests/kmutex0 tests/kmutex1 tests/kmutex3 tests/kmutex4 tests/ksched1 tests/ksem0 tests/ksem1 tests/kthread0 tests/kthread1 tests/stress_threads tests/thread_gdb tests/timeslice tests/tm_basic tests/fptest" : "")
+ . ((CYGFUN_KERNEL_API_C) ? " tests/kclock0 tests/kclock1 tests/kexcept1 tests/kflag0 tests/kflag1 tests/kintr0 tests/klock tests/kmbox1 tests/kmutex0 tests/kmutex1 tests/kmutex3 tests/kmutex4 tests/ksched1 tests/ksem0 tests/ksem1 tests/kthread0 tests/kthread1 tests/stress_threads tests/thread_gdb tests/timeslice tests/tm_basic tests/fptest tests/kalarm0" : "")
. ((!CYGPKG_INFRA_DEBUG && !CYGPKG_KERNEL_INSTRUMENT && CYGFUN_KERNEL_API_C) ? " tests/dhrystone" : "")
. ((CYGPKG_KERNEL_SMP_SUPPORT && CYGFUN_KERNEL_API_C) ? " tests/smp" : "")
. ((!CYGINT_HAL_TESTS_NO_CACHES && CYGFUN_KERNEL_API_C) ? " tests/kcache1 tests/kcache2" : "")
}
description "
Index: include/clock.hxx
===================================================================
RCS file: /cvs/ecos/ecos/packages/kernel/current/include/clock.hxx,v
retrieving revision 1.9
diff -u -5 -r1.9 clock.hxx
--- include/clock.hxx 23 May 2002 23:06:46 -0000 1.9
+++ include/clock.hxx 1 Jul 2003 17:33:40 -0000
@@ -89,10 +89,16 @@
#endif
volatile cyg_tick_count counter; // counter value
cyg_uint32 increment; // increment per tick
+
+ // Add an alarm to this counter
+ void add_alarm( Cyg_Alarm *alarm );
+
+ // Remove an alarm from this counter
+ void rem_alarm( Cyg_Alarm *alarm );
public:
CYGDBG_DEFINE_CHECK_THIS
@@ -113,16 +119,10 @@
// Set the counter's current value
void set_value( cyg_tick_count new_value);
// Advance counter by some number of ticks
void tick( cyg_uint32 ticks = 1);
-
- // Add an alarm to this counter
- void add_alarm( Cyg_Alarm *alarm );
-
- // Remove an alarm from this counter
- void rem_alarm( Cyg_Alarm *alarm );
};
// -------------------------------------------------------------------------
// Clock class. This is derived from a Counter and defines extra
Index: include/clock.inl
===================================================================
RCS file: /cvs/ecos/ecos/packages/kernel/current/include/clock.inl,v
retrieving revision 1.6
diff -u -5 -r1.6 clock.inl
--- include/clock.inl 23 May 2002 23:06:46 -0000 1.6
+++ include/clock.inl 1 Jul 2003 17:33:40 -0000
@@ -119,18 +119,7 @@
// for smaller arguments.
return (cyg_tick_count)t;
}
// -------------------------------------------------------------------------
-// -------------------------------------------------------------------------
-// Alarm inlines
-
-// Ensure alarm disabled
-inline void Cyg_Alarm::disable()
-{
- if( enabled ) counter->rem_alarm(this);
-}
-
-
-// -------------------------------------------------------------------------
#endif // ifndef CYGONCE_KERNEL_CLOCK_INL
// EOF clock.inl
Index: src/common/clock.cxx
===================================================================
RCS file: /cvs/ecos/ecos/packages/kernel/current/src/common/clock.cxx,v
retrieving revision 1.16
diff -u -5 -r1.16 clock.cxx
--- src/common/clock.cxx 25 Jun 2003 17:44:28 -0000 1.16
+++ src/common/clock.cxx 1 Jul 2003 17:33:41 -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();
}
@@ -276,13 +298,12 @@
{
CYG_REPORT_FUNCTION();
CYG_ASSERTCLASS( this, "Bad counter object" );
CYG_ASSERTCLASS( alarm, "Bad alarm passed" );
+ CYG_ASSERT( Cyg_Scheduler::get_sched_lock() > 0, "Scheduler not locked");
- Cyg_Scheduler::lock();
-
// set this now to allow an immediate handler call to manipulate
// this alarm sensibly.
alarm->enabled = true;
// Check here for an alarm that triggers now or in the past and
@@ -312,11 +333,10 @@
else
{
// The alarm is all done with, disable it
// unlock and return.
alarm->enabled = false;
- Cyg_Scheduler::unlock();
return;
}
}
CYG_INSTRUMENT_ALARM( ADD, this, alarm );
@@ -380,11 +400,10 @@
alarm_list_ptr->add_tail( alarm );
#endif
- Cyg_Scheduler::unlock();
}
// -------------------------------------------------------------------------
// Remove an alarm from this counter
@@ -392,22 +411,20 @@
{
CYG_REPORT_FUNCTION();
CYG_ASSERTCLASS( this, "Bad counter object" );
CYG_ASSERTCLASS( alarm, "Bad alarm passed" );
+ CYG_ASSERT( Cyg_Scheduler::get_sched_lock() > 0, "Scheduler not locked");
Cyg_Alarm_List *alarm_list_ptr; // pointer to list
#if defined(CYGIMP_KERNEL_COUNTERS_SINGLE_LIST)
alarm_list_ptr = &alarm_list;
- Cyg_Scheduler::lock();
#elif defined(CYGIMP_KERNEL_COUNTERS_MULTI_LIST)
- Cyg_Scheduler::lock();
-
alarm_list_ptr = &(alarm_list[
((alarm->trigger+increment-1)/increment) %
CYGNUM_KERNEL_COUNTERS_MULTI_LIST_SIZE ] );
#else
@@ -421,11 +438,10 @@
alarm_list_ptr->remove( alarm );
alarm->enabled = false;
- Cyg_Scheduler::unlock();
}
//==========================================================================
// Constructor for clock object
@@ -707,10 +723,12 @@
cyg_tick_count i // Relative retrigger interval
)
{
CYG_REPORT_FUNCTION();
+ Cyg_Scheduler::lock();
+
// If already enabled, remove from counter
if( enabled ) counter->rem_alarm(this);
CYG_INSTRUMENT_ALARM( INIT, this, 0 );
@@ -723,10 +741,12 @@
trigger = t;
interval = i;
counter->add_alarm(this);
+
+ Cyg_Scheduler::unlock();
}
// -------------------------------------------------------------------------
// Synchronize with a past alarm stream that had been disabled,
// bring past times into synch, and the like.
@@ -757,17 +777,33 @@
// -------------------------------------------------------------------------
// Ensure alarm enabled
void Cyg_Alarm::enable()
{
+ Cyg_Scheduler::lock();
+
if( !enabled )
{
// ensure the alarm time is in our future:
synchronize();
enabled = true;
counter->add_alarm(this);
}
+
+ Cyg_Scheduler::unlock();
+}
+
+// -------------------------------------------------------------------------
+// Ensure alarm disabled
+
+void Cyg_Alarm::disable()
+{
+ Cyg_Scheduler::lock();
+
+ if( enabled ) counter->rem_alarm(this);
+
+ Cyg_Scheduler::unlock();
}
// -------------------------------------------------------------------------
// Get the current time values from the alarm
Index: tests/kalarm0.c
===================================================================
RCS file: tests/kalarm0.c
diff -N tests/kalarm0.c
--- /dev/null 1 Jan 1970 00:00:00 -0000
+++ tests/kalarm0.c 1 Jul 2003 17:33:41 -0000
@@ -0,0 +1,191 @@
+//==========================================================================
+//
+// kalarm0
+//
+// Alarm functionality test
+//
+//==========================================================================
+//####ECOSGPLCOPYRIGHTBEGIN####
+// -------------------------------------------
+// This file is part of eCos, the Embedded Configurable Operating System.
+// Copyright (C) 2003 Nick Garnett <nickg@calivar.com>
+//
+// eCos is free software; you can redistribute it and/or modify it under
+// the terms of the GNU General Public License as published by the Free
+// Software Foundation; either version 2 or (at your option) any later version.
+//
+// eCos is distributed in the hope that it will be useful, but WITHOUT ANY
+// WARRANTY; without even the implied warranty of MERCHANTABILITY or
+// FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
+// for more details.
+//
+// You should have received a copy of the GNU General Public License along
+// with eCos; if not, write to the Free Software Foundation, Inc.,
+// 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA.
+//
+// As a special exception, if other files instantiate templates or use macros
+// or inline functions from this file, or you compile this file and link it
+// with other works to produce a work based on this file, this file does not
+// by itself cause the resulting work to be covered by the GNU General Public
+// License. However the source code for this file must still be made available
+// in accordance with section (3) of the GNU General Public License.
+//
+// This exception does not invalidate any other reasons why a work based on
+// this file might be covered by the GNU General Public License.
+//
+// Alternative licenses for eCos may be arranged by contacting the
+// copyright holders.
+// -------------------------------------------
+//####ECOSGPLCOPYRIGHTEND####
+//==========================================================================
+//#####DESCRIPTIONBEGIN####
+//
+// Author(s): nickg
+// Contributors: nickg
+// Date: 2003-06-25
+// Description: Tests the ability of alarms to be added and removed by the
+// alarm functions of other alarms.
+//
+//####DESCRIPTIONEND####
+//==========================================================================
+
+#include <cyg/kernel/kapi.h>
+
+#include <cyg/infra/testcase.h>
+
+//==========================================================================
+
+#if defined(CYGVAR_KERNEL_COUNTERS_CLOCK) && \
+ defined(CYGFUN_KERNEL_API_C)
+
+#include "testaux.h"
+
+//==========================================================================
+
+//#define db_printf diag_printf
+#define db_printf( fmt, ... )
+
+//==========================================================================
+
+static cyg_counter counter_obj;
+static cyg_handle_t counter;
+static cyg_alarm alarm_obj[3];
+static cyg_handle_t alarm[3];
+
+static cyg_uint32 alarmfn_called[3];
+
+//==========================================================================
+
+void alarmfn0(cyg_handle_t alarmh, cyg_addrword_t data)
+{
+ db_printf("%s: %d\n",__PRETTY_FUNCTION__,cyg_counter_current_value( counter ));
+
+ // alarmfn0 just counts how many times it has been called
+
+ alarmfn_called[0]++;
+}
+
+//==========================================================================
+
+void alarmfn1(cyg_handle_t alarmh, cyg_addrword_t data)
+{
+ db_printf("%s: %d\n",__PRETTY_FUNCTION__,cyg_counter_current_value( counter ));
+
+ alarmfn_called[1]++;
+
+ // Reschedule alarm[0] to run every tick until alarm[2] next runs.
+
+ cyg_alarm_initialize( alarm[0], cyg_counter_current_value( counter )+1, 1 );
+
+}
+
+//==========================================================================
+
+void alarmfn2(cyg_handle_t alarmh, cyg_addrword_t data)
+{
+ db_printf("%s: %d\n",__PRETTY_FUNCTION__,cyg_counter_current_value( counter ));
+
+ alarmfn_called[2]++;
+
+ // Reschedule alarm[0] to run every 2 ticks until alarm[1] next runs.
+
+ cyg_alarm_initialize( alarm[0], cyg_counter_current_value( counter )+1, 2 );
+
+ // Reschedule alarm[1] to run every 3 ticks starting in 6 ticks time.
+
+ cyg_alarm_initialize( alarm[1], cyg_counter_current_value( counter )+6, 3 );
+}
+
+//==========================================================================
+
+void alarm0_main(void)
+{
+ int i;
+
+ CYG_TEST_INIT();
+
+ // Create the counter
+ cyg_counter_create( &counter, &counter_obj );
+
+ // Create the alarms
+ cyg_alarm_create( counter,
+ alarmfn0,
+ 0,
+ &alarm[0],
+ &alarm_obj[0]);
+
+
+ cyg_alarm_create( counter,
+ alarmfn1,
+ 1,
+ &alarm[1],
+ &alarm_obj[1]);
+
+ cyg_alarm_create( counter,
+ alarmfn2,
+ 2,
+ &alarm[2],
+ &alarm_obj[2]);
+
+
+ // Kick it all off by starting alarm[2]
+ cyg_alarm_initialize( alarm[2], 0, 10 );
+
+ // Run the whole thing for 10000 ticks
+ for( i = 0; i < 10000; i++ )
+ cyg_counter_tick( counter );
+
+ db_printf("alarmfn_called: %d %d %d\n",
+ alarmfn_called[0],alarmfn_called[1],alarmfn_called[2]);
+
+ CYG_TEST_CHECK( alarmfn_called[0]==5000, "alarmfn0 not called 5000 times\n");
+ CYG_TEST_CHECK( alarmfn_called[1]==2000, "alarmfn1 not called 2000 times\n");
+ CYG_TEST_CHECK( alarmfn_called[2]==1001, "alarmfn2 not called 1001 times\n");
+
+ CYG_TEST_PASS_FINISH("KAlarm0");
+}
+
+//==========================================================================
+
+externC void
+cyg_start( void )
+{
+ alarm0_main();
+}
+
+//==========================================================================
+
+#else
+
+externC void
+cyg_start( void )
+{
+ CYG_TEST_INIT();
+ CYG_TEST_NA( "This test needs CYGVAR_KERNEL_COUNTERS_CLOCK "
+ "and CYGFUN_KERNEL_API_C" );
+}
+
+#endif
+
+//==========================================================================
+// End of kalarm0.c
--
Nick Garnett eCos Kernel Architect
http://www.ecoscentric.com/ The eCos and RedBoot experts