Clean up mutex issues.

    - Final fix for mutex bug.
    - Document weak-consistency decisions in mutex code.
    - Prevent aggressive optimizations around lwsync/isync instrs.
    - Fix minor bug in futex_wait system call.
    - Optimize futex path with likely/unlikely hints.

Change-Id: I26b54dee7e45bcb42195f730474b350b44f53cfc
Reviewed-on: http://gfw160.austin.ibm.com:8080/gerrit/233
Tested-by: Jenkins Server
Reviewed-by: A. Patrick Williams III <iawillia@us.ibm.com>

5 files changed, 48 insertions, 46 deletions

diff --git a/src/include/arch/ppc.H b/src/include/arch/ppc.H
index 5280c82e4..405a2c297 100644
--- a/src/include/arch/ppc.H
+++ b/src/include/arch/ppc.H
@@ -64,7 +64,6 @@ inline void setHSRR1(uint64_t _hsrr1)
     asm volatile("mtspr 315, %0" : : "r" (hsrr1));
 }
 
-
 ALWAYS_INLINE
 inline uint64_t getPVR()
 {
@@ -146,25 +145,25 @@ inline void setDEC(uint64_t _dec)
 ALWAYS_INLINE
 inline void sync()
 {
-    asm volatile("sync");
+    asm volatile("sync" ::: "memory");
 }
 
 ALWAYS_INLINE
 inline void lwsync()
 {
-    asm volatile("lwsync");
+    asm volatile("lwsync" ::: "memory");
 }
 
 ALWAYS_INLINE
 inline void isync()
 {
-    asm volatile("isync");
+    asm volatile("isync" ::: "memory");
 }
 
 ALWAYS_INLINE
 inline void eieio()
 {
-    asm volatile("eieio");
+    asm volatile("eieio" ::: "memory");
 }
 
 ALWAYS_INLINE
diff --git a/src/kernel/futexmgr.C b/src/kernel/futexmgr.C
index 6271471aa..37ad54586 100644
--- a/src/kernel/futexmgr.C
+++ b/src/kernel/futexmgr.C
@@ -33,7 +33,7 @@ uint64_t FutexManager::_wait(task_t* i_task, uint64_t * i_addr, uint64_t i_val)
 
     iv_lock.lock();
 
-    if(*i_addr != i_val)
+    if(unlikely(*i_addr != i_val))
     {
         // some other thread has modified the futex
         // bail-out retry required.
diff --git a/src/kernel/syscall.C b/src/kernel/syscall.C
index f96f8d372..1001ef361 100644
--- a/src/kernel/syscall.C
+++ b/src/kernel/syscall.C
@@ -356,11 +356,18 @@ namespace Systemcalls
         uint64_t * uaddr = (uint64_t *) TASK_GETARG0(t);
         uint64_t val   = (uint64_t) TASK_GETARG1(t);
 
+        // Set RC to success initially.
+        TASK_SETRTN(t,0);
+
         //TODO translate uaddr from user space to kernel space
         //     Right now they are the same.
 
         uint64_t rc = FutexManager::wait(t,uaddr,val);
-        TASK_SETRTN(t,rc);
+        if (rc != 0) // Can only set rc if we still have control of the task,
+                     // which is only (for certain) on error rc's.
+        {
+            TASK_SETRTN(t,rc);
+        }
     }
 
     /**
diff --git a/src/lib/sync.C b/src/lib/sync.C
index d0d15aeb4..be20903d7 100644
--- a/src/lib/sync.C
+++ b/src/lib/sync.C
@@ -85,50 +85,31 @@ void mutex_destroy(mutex_t *& i_mutex)
 
 void mutex_lock(mutex_t * i_mutex)
 {
+    // Weak consistency notes:
+    //     Since this is a lock, we do not need to ensure that all writes
+    //     are globally visible prior to execution (lwsync) but we do need
+    //     to ensure that all instructions finish completion prior to
+    //     leaving (isync).  Both __sync_val_compare_and_swap and
+    //     __sync_lock_test_and_set have an implied isync.
 
     uint64_t l_count = __sync_val_compare_and_swap(&(i_mutex->iv_val),0,1);
 
-    if(l_count != 0)
+    if(unlikely(l_count != 0))
     {
-        if (l_count != 2)
+        if (likely(l_count != 2))
         {
-            lwsync();
             l_count = __sync_lock_test_and_set(&(i_mutex->iv_val), 2);
         }
 
         while( l_count != 0 )
         {
             futex_wait( &(i_mutex->iv_val), 2);
-            lwsync();
             l_count = __sync_lock_test_and_set(&(i_mutex->iv_val),2);
             // if more than one thread gets out - one continues while
             // the rest get blocked again.
         }
     }
 
-#ifdef __IDEA_2
-    // Idea # 2
-    while(1)
-    {
-        uint64_t l_count = __sync_val_compare_and_swap(&(i_mutex->iv_val),0,1);
-        if( 0 == l_count ) // uncontended lock
-        {
-            break;
-        }
-        else if ( 2 == l_count ) // already contended
-        {
-            futex_wait( &(i_mutex->iv_val), l_count);
-        }
-        else if ( 1 == l_count ) // lock now contended
-        {
-            if (__sync_bool_compare_and_swap(&(i_mutex->iv_val),1,2))
-            {
-                futex_wait( &(i_mutex->iv_val), 2);
-            }
-        }
-    }
-#endif
-
     return;
 }
 
@@ -136,21 +117,21 @@ void mutex_lock(mutex_t * i_mutex)
 
 void mutex_unlock(mutex_t * i_mutex)
 {
-#ifdef __IDEA_1
-    if (__sync_fetch_and_sub(&(i_mutex->iv_val), 1) != 1)
-    {
-        i_mutex->iv_val = 0; // the thread that is started will set this back to 2
-        futex_wake(&(i_mutex->iv_val), 1); // wake one thread
-    }
-#endif
-    // #idea 2
-    lwsync();
-    uint64_t l_count = __sync_lock_test_and_set(&(i_mutex->iv_val),0);
-    if(2 == l_count)
+    // Weak consistency notes:
+    //     Since this is an unlock we need to ensure that all writes are
+    //     globally visible prior to execution (lwsync).  The
+    //     __sync_fetch_and_sub has an implied lwsync.  If we need to
+    //     release a task, due to a contended mutex, the write to iv_val
+    //     and futex_wake pair will appear globally ordered due to the
+    //     context synchronizing nature of the 'sc' instruction.
+
+    uint64_t l_count = __sync_fetch_and_sub(&(i_mutex->iv_val),1);
+    if(unlikely(2 <= l_count))
     {
+        i_mutex->iv_val = 0;
         futex_wake(&(i_mutex->iv_val), 1); // wake one thread
     }
- 
+
     return;
 }
 
diff --git a/src/usr/testcore/lib/synctest.H b/src/usr/testcore/lib/synctest.H
index 5a53d2e0a..8e7619d11 100644
--- a/src/usr/testcore/lib/synctest.H
+++ b/src/usr/testcore/lib/synctest.H
@@ -9,6 +9,7 @@
 #include <cxxtest/TestSuite.H>
 #include <sys/sync.h>
 #include <sys/task.h>
+#include <sys/time.h>
 
 class SyncTest: public CxxTest::TestSuite
 {
@@ -30,6 +31,20 @@ class SyncTest: public CxxTest::TestSuite
             TS_TRACE("ALL THREADS ENDED");
         }
 
+        void testMutexDoubleWait()
+        {
+            mutex_init(&mutex);
+            barrier_init(&barrier, 3);
+
+            mutex_lock(&mutex);
+            task_create(func2, this);
+            task_create(func2, this);
+            nanosleep(1,0);
+            mutex_unlock(&mutex);
+            barrier_wait(&barrier);
+            TS_TRACE("ALL THREADS ENDED");
+        }
+
         void testBarrier()
         {
             barrier_t barrier;