Re: Bug in alarm handling fixed

[Jonathan Larmour <jifl at eCosCentric dot com>]

Christoph Csebits wrote:
> Hi,
>
> there is a bug in the sorted alarm list handling.
>
> If CYGIMP_KERNEL_COUNTERS_SORT_LIST is enabled
> the add_alarm is not working (at all).
>
> The problem is that alarms with a greater trigger
> value than the greatest trigger value in the list
> are not inserted (i.e. adding alarms to the end
> of the list is not working, except for a empty list).
You are of course right. That's a very bad bug to have been missed, or to 
last so long, oh well.

I've thought about your patch, and decided it could be done a slightly 
better way, with the assumption that the order for calling alarm functions 
registered for identical times is unspecified. I think that's reasonable, 
and I've documented it.

Please check my patch is fine for you although I'm checking it in now 
anyway so you can just update your sources to see.

> this patch solves also the problem described in
> http://sources.redhat.com/ml/ecos-discuss/2002-08/msg00065.html
I've CC'd the people in the thread in case they have any comments.

Jifl
--
--[ "You can complain because roses have thorns, or you ]--
--[  can rejoice because thorns have roses." -Lincoln   ]-- Opinions==mine
Index: ChangeLog
===================================================================
RCS file: /cvs/ecos/ecos/packages/kernel/current/ChangeLog,v
retrieving revision 1.80
diff -u -5 -p -r1.80 ChangeLog
--- ChangeLog	9 Aug 2002 17:13:28 -0000	1.80
+++ ChangeLog	1 Oct 2002 01:43:52 -0000
@@ -1,5 +1,16 @@
+2002-09-30  Jonathan Larmour  <jifl@eCosCentric.com>
+
+	* src/common/clock.cxx (add_alarm): Fix bug resulting in alarms
+	not being added at all if a lower triggered alarm already exists.
+	Reported by Christoph Csebits.
+	Also fix bug when alarm is entered for the same time as tail.
+	These bugs only apply for CYGIMP_KERNEL_COUNTERS_SORT_LIST enabled.
+
+	* doc/kernel.sgml: document that order of callback for alarms at
+	identical times is unspecified.
+
 2002-08-08  Nick Garnett  <nickg@calivar.demon.co.uk>
 
 	* src/sched/sched.cxx (unlock_inner): Removed initial
 	assertion. This has served its purpose and with the introduction
 	of routines such as unlock_reschedule() is prone to firing in
Index: doc/kernel.sgml
===================================================================
RCS file: /cvs/ecos/ecos/packages/kernel/current/doc/kernel.sgml,v
retrieving revision 1.3
diff -u -5 -p -r1.3 kernel.sgml
--- doc/kernel.sgml	15 Sep 2002 22:12:37 -0000	1.3
+++ doc/kernel.sgml	1 Oct 2002 01:43:56 -0000
@@ -2483,10 +2483,14 @@ and will happen in the same context. If 
 the system's real-time clock then this will be DSR context, following
 a clock interrupt. If the alarm is associated with some other
 application-specific counter then the details will depend on how that
 counter is updated.
       </para>
+      <para>
+If two or more alarms are registered for precisely the same counter tick,
+the order of execution of the alarm functions is unspecified.
+      </para>
     </refsect1>
 
     <refsect1 id="kernel-alarms-context"><title>Valid contexts</title>
       <para>
 <function>cyg_alarm_create</function>
Index: src/common/clock.cxx
===================================================================
RCS file: /cvs/ecos/ecos/packages/kernel/current/src/common/clock.cxx,v
retrieving revision 1.14
diff -u -5 -p -r1.14 clock.cxx
--- src/common/clock.cxx	23 May 2002 23:06:52 -0000	1.14
+++ src/common/clock.cxx	1 Oct 2002 01:43:56 -0000
@@ -7,10 +7,11 @@
 //==========================================================================
 //####ECOSGPLCOPYRIGHTBEGIN####
 // -------------------------------------------
 // This file is part of eCos, the Embedded Configurable Operating System.
 // Copyright (C) 1998, 1999, 2000, 2001, 2002 Red Hat, Inc.
+// Copyright (C) 2002 Jonathan Larmour
 //
 // 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.
 //
@@ -344,36 +345,39 @@ void Cyg_Counter::add_alarm( Cyg_Alarm *
 #endif
 
 #ifdef CYGIMP_KERNEL_COUNTERS_SORT_LIST
         
     // Now that we have the list pointer, we can use common code for
-    // both list oragnizations.
+    // both list organizations.
 
     Cyg_Alarm *list_alarm = alarm_list_ptr->get_head();
 
     if( list_alarm != NULL )
+    {
         do
         {
             CYG_ASSERTCLASS(list_alarm, "Bad alarm in counter list" );
 
             // The alarms are in ascending trigger order. When we
-            // find an alarm that is later than us, we go in front of
-            // it.
+            // find an alarm that is later or the same time as us, we go
+            // in front of it.
         
-            if( list_alarm->trigger > alarm->trigger )
+            if( list_alarm->trigger >= alarm->trigger )
             {
                 alarm_list_ptr->insert( list_alarm, alarm );
-                break;
+                goto add_alarm_unlock_return;
             }
 
             list_alarm = list_alarm->get_next();
             
         } while( list_alarm != alarm_list_ptr->get_head() );
+        // a lower or equal alarm time was not found, so drop through
+        // so it is added to the list tail
+    }
+    alarm_list_ptr->add_tail( alarm );
 
-    else
-        alarm_list_ptr->add_tail( alarm );
-
+ add_alarm_unlock_return:
 #else    
     
     alarm_list_ptr->add_tail( alarm );
         
 #endif