[ECOS] Synchronization problem in IP stack

[ECOS] Synchronization problem in IP stack

[Wade Jensen]

[-- Attachment #1: Type: text/plain, Size: 879 bytes --]

Hello,

I was running into a rare problem when two threads attempted to send on the same
TCP socket.  There is a race condition in the sblock() routine that sometimes
causes one of the threads to never wake up.

I believe the following patch fixes this problem.  I was able to reproduce it
very quickly, so I am fairly confident that this fixes the problem.

What this patch fixes:

1.  I noticed that there are many places in the IP stack that use #ifdef __ECOS
or #ifdef __ECOS__, but the __ECOS__ label was not defined in the compiler
options.  Because of this, a call to sblock() was using the macro rather than
the function call.

2. The sblock() function was incorrectly assuming that sb_lock() unlocked the
scheduler.

3. There was an unnecessary assertion in cyg_tsleep().

Please let me know if this is incorrect.

Thanks,
Wade Jensen

(See attached file: patch.sblock)

[-- Attachment #2: patch.sblock --]
[-- Type: text/x-diff, Size: 2661 bytes --]

diff -r -w -u5 -p ecos_prev/packages/net/tcpip/current/cdl/net.cdl ecos/packages/net/tcpip/current/cdl/net.cdl
--- ecos_prev/packages/net/tcpip/current/cdl/net.cdl	Wed Sep 12 12:33:27 2001
+++ ecos/packages/net/tcpip/current/cdl/net.cdl	Wed Sep 12 12:04:38 2001
@@ -405,11 +405,11 @@ cdl_package CYGPKG_NET {
 
         cdl_option CYGPKG_NET_CFLAGS_ADD {
             display "Additional compiler flags"
             flavor  data
             no_define
-            default_value { "-D_KERNEL -D__ECOS" }
+            default_value { "-D_KERNEL -D__ECOS -D__ECOS__" }
             description   "
                 This option modifies the set of compiler flags for
                 building the networking package.
                 These flags are used in addition
                 to the set of global flags."
diff -r -w -u5 -p ecos_prev/packages/net/tcpip/current/src/ecos/synch.c ecos/packages/net/tcpip/current/src/ecos/synch.c
--- ecos_prev/packages/net/tcpip/current/src/ecos/synch.c	Wed Sep 12 12:33:33 2001
+++ ecos/packages/net/tcpip/current/src/ecos/synch.c	Wed Sep 12 13:04:10 2001
@@ -306,12 +306,11 @@ cyg_tsleep(void *chan, int pri, char *wm
             ev->chan = chan;
             break;
         }
     }
     CYG_ASSERT( i <  CYGPKG_NET_NUM_WAKEUP_EVENTS, "no sleep slots" );
-    CYG_ASSERT( 1 == cyg_scheduler_read_lock(),
-                "Tsleep - called with scheduler locked" );
+
     // Defensive:
     if ( i >= CYGPKG_NET_NUM_WAKEUP_EVENTS ) {
         cyg_scheduler_unlock();
         return ETIMEDOUT;
     }
diff -r -w -u5 -p ecos_prev/packages/net/tcpip/current/src/sys/kern/uipc_socket2.c ecos/packages/net/tcpip/current/src/sys/kern/uipc_socket2.c
--- ecos_prev/packages/net/tcpip/current/src/sys/kern/uipc_socket2.c	Wed Sep 12 12:33:34 2001
+++ ecos/packages/net/tcpip/current/src/sys/kern/uipc_socket2.c	Wed Sep 12 13:05:30 2001
@@ -376,26 +376,24 @@ sb_lock(sb)
  * Returns error without lock if sleep is interrupted.
  */
 int 
 sblock(struct sockbuf *sb, int wf)
 {
-    int flags, res;
-    cyg_scheduler_safe_lock();
+    int res;
+    cyg_scheduler_lock();
     if (sb->sb_flags & SB_LOCK) {
         // Already locked by another thread
         if (wf == M_WAITOK) {
             res = sb_lock(sb);
-            // Note: scheduler unlocked by 'sb_lock()'
         } else {
             res = EWOULDBLOCK;
-            cyg_scheduler_unlock();
         }
     } else {
         sb->sb_flags |= SB_LOCK;
         res = 0;
-        cyg_scheduler_unlock();
     }
+    cyg_scheduler_unlock();
     return res;
 }
 
 /* release lock on sockbuf sb */
 void

Re: [ECOS] Synchronization problem in IP stack

[Jonathan Larmour]

Wade Jensen wrote:
> 
> 1.  I noticed that there are many places in the IP stack that use #ifdef __ECOS
> or #ifdef __ECOS__, but the __ECOS__ label was not defined in the compiler
> options.  Because of this, a call to sblock() was using the macro rather than
> the function call.

True. I think the better fix is to fix the places using __ECOS__ to use
__ECOS. I'll make it so.
 
> 2. The sblock() function was incorrectly assuming that sb_lock() unlocked the
> scheduler.
> 
> 3. There was an unnecessary assertion in cyg_tsleep().

While I'm tempted to agree with you and just apply these bits of the patch,
I'll have to defer to Hugo, who has studied this part of the stack far more
than I.

Jifl
-- 
Red Hat, Rustat House, Clifton Road, Cambridge, UK. Tel: +44 (1223) 271062
Maybe this world is another planet's Hell -Aldous Huxley || Opinions==mine