Re: Clock.cxx: some bug fixes

[Jonathan Larmour <jifl at eCosCentric dot com>]

Nick Garnett wrote:
> 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.

Perhaps the thing to do is write a test (or expand an existing one) that 
pokes it in the right way to cause the problems it's trying to fix, i.e. 
tweaking the alarms from within an alarm handler.

> If I hear nothing in a few days, I'll go ahead and commit it.

I had already applied Thomas' patch (in a very slightly different way) at 
almost exactly the same time you sent this mail, sorry! The only 
difference (as I indicated in my mail) was to lock the scheduler _after_ 
the assign to alarm_list_ptr in the single list case. See the attached 
patch FAOD.

I can't help but think that the extra checking for changes i.e. next_next, 
next_prev and head, can be a config option given this is a well-trodden 
piece of code.

Well you did imply you wanted comments ;).

Jifl
--
eCosCentric    http://www.eCosCentric.com/    The eCos and RedBoot experts
--[ "You can complain because roses have thorns, or you ]--
--[  can rejoice because thorns have roses." -Lincoln   ]-- Opinions==mine

Index: src/common/clock.cxx
===================================================================
RCS file: /cvs/ecos/ecos/packages/kernel/current/src/common/clock.cxx,v
retrieving revision 1.15
retrieving revision 1.16
diff -u -5 -p -r1.15 -r1.16
--- src/common/clock.cxx	1 Oct 2002 19:10:21 -0000	1.15
+++ src/common/clock.cxx	25 Jun 2003 17:44:28 -0000	1.16
@@ -169,11 +169,11 @@ void Cyg_Counter::tick( cyg_uint32 ticks
 #else
 #error "No CYGIMP_KERNEL_COUNTERS_x_LIST config"
 #endif
 
         // Now that we have the list pointer, we can use common code for
-        // both list oragnizations.
+        // both list organizations.
 
 #ifdef CYGIMP_KERNEL_COUNTERS_SORT_LIST
 
         // With a sorted alarm list, we can simply pick alarms off the
         // front of the list until we find one that is in the future.
@@ -398,25 +398,26 @@ void Cyg_Counter::rem_alarm( Cyg_Alarm *
     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
 #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 );