Re: timed condition variable patch

[Nick Garnett <nickg at calivar dot demon dot co dot uk>]

Jonathan Larmour <jifl@eCosCentric.com> writes:

> Wade Jensen wrote:
> > Hello,
> > There is a very rare condition in Cyg_Condition_Variable::wait_inner(
> > Cyg_Mutex *mx, cyg_tick_count timeout ) that causes the "Unnecessary call to
> > unlock_inner()" assertion to fail.
> 
> This looks familiar.
> 
> Nick, I seem to recall I discussed this with you IRL in the Red Hat
> office about this, or something similar - i.e. what happens about
> timers that trigger "now" or in the past, and I think it was also in
> the context of select(). Do you remember what the outcome was? I can't
> find any kernel change resulting certainly, but then I can't remember
> the problem :-).

Yes, I also recall such a conversation, and would have sworn I made
changes to fix it. Maybe I only did this in a branch. 

> 
> > Here is a patch for this problem.  It is based off of the 2.0 snapshot
> > source.
> 
> I'd probably prefer to include the if conditional in the "    if(
> self->get_wake_reason() == Cyg_Thread::NONE )" test further up. But I
> can change that later, depending on what Nick says.
> 

Yep, this is exactly the change I intended to make. Specifically the
following:

[I don't currently have write access to the repository, so if you
could do the honours, Jifl, I would appreciate it. And give me write
access while you are about it.]

Index: ChangeLog
===================================================================
RCS file: /cvs/ecos/ecos/packages/kernel/current/ChangeLog,v
retrieving revision 1.79
diff -u -r1.79 ChangeLog
--- ChangeLog	5 Aug 2002 21:52:58 -0000	1.79
+++ ChangeLog	7 Aug 2002 13:26:16 -0000
@@ -1,3 +1,10 @@
+2002-08-07  Nick Garnett  <nickg@calivar.demon.co.uk>
+
+	* src/sync/mutex.cxx (Cyg_Condition_Variable::wait_inner): Only
+	wait if the timeout has not already fired. This is a result of the
+	change in semantics of unlock_reschedule() from the code that was
+	there before.
+
 2002-08-05  Bart Veer  <bartv@tymora.demon.co.uk>
 
 	* cdl/kernel.cdl, include/kapidata.h, include/kapi.h:
Index: src/sync/mutex.cxx
===================================================================
RCS file: /cvs/ecos/ecos/packages/kernel/current/src/sync/mutex.cxx,v
retrieving revision 1.14
diff -u -r1.14 mutex.cxx
--- src/sync/mutex.cxx	23 May 2002 23:06:56 -0000	1.14
+++ src/sync/mutex.cxx	7 Aug 2002 13:26:20 -0000
@@ -802,18 +802,21 @@
     // Set the timer and sleep reason
     self->set_timer( timeout, Cyg_Thread::TIMEOUT );
 
-    // Only enqueue if the timeout has not already fired.
+    // Only wait if the timeout has not already fired.
     if( self->get_wake_reason() == Cyg_Thread::NONE )
+    {
         queue.enqueue( self );
 
-    // Avoid calling ASRs during the following unlock.
-    self->set_asr_inhibit();
+        // Avoid calling ASRs during the following unlock.
+        self->set_asr_inhibit();
         
-    // Unlock the scheduler and switch threads
-    Cyg_Scheduler::unlock_reschedule();
+        // Unlock the scheduler and switch threads
+        Cyg_Scheduler::unlock_reschedule();
 
-    // Allow ASRs again
-    self->clear_asr_inhibit();
+        // Allow ASRs again
+        self->clear_asr_inhibit();
+    }
+    else Cyg_Scheduler::unlock();
                 
     CYG_ASSERTCLASS( this, "Bad this pointer");
     CYG_ASSERTCLASS( mx, "Corrupt mutex");



-- 
Nick Garnett - eCos Kernel Architect