p_mutex_lock / p_mutex_trylock: race proofing.

Change up locking logic such that race conditions are
less of a problem.

in p_mutex_lock: spin until a lock is gotten *and* it
equals 1 with two nested loops and atomic operations only.

in p_mutex_trylock: fail early, but an atomic branch will
cause a small check to see if a race happened *and* the thread
asking for the mutex actually got the lock. No loops, only
returns.

Signed-off-by: Morgan Gangwere <[email protected]>

src/base/p_mutex_lock.c

@@ -22,11 +22,27 @@
  */
 int p_mutex_lock(p_mutex_t *mp)
 {
-	if(mp == NULL || *mp == NULL) {
-		return EINVAL;
-	}
-    // Spin while we wait for the lock to become 0.
-    while(**mp != 0) { ;; }
-    **mp = 1;
+    if(mp == NULL || *mp == NULL) {
+        return EINVAL;
+    }
+    /* We've got to avoid race conditions. Our solution to this is to spin
+     * until we can increment the mutex and make it 1, not 2.
+     * This works because our definition of "locked" is non-zero
+     * and our unlock is set to 0.
+     * 
+     * When we lose the race, our mutex will be a value other than 1.
+     *
+     * When we enter the loop, we spin until **mp == 0
+     * When **mp == 0, we check if ++(**mp) == 1
+     * If that condition is true, we are safe: We won the race.
+     * If that condition is false, we lost the race: keep trying.
+     */
+    do {
+        /* Spin while the lock is taken. */
+        while(**mp != 0) { ;; }
+        /* attempt to take the lock. If we lost the race
+         * we'll keep trying.
+         */
+    } while( ++(**mp) != 1);
     return (0);
 }

src/base/p_mutex_trylock.c

@@ -17,11 +17,15 @@ int p_mutex_trylock(p_mutex_t *mp)
 	if( mp == NULL || *mp == NULL) {
 		return EINVAL;
 	}
-	else if(**mp == 0) {
-        **mp = 1;
-        return 0;
-	}
-    else {
-        return EBUSY;
-	}
+    if(**mp !=0 ) { return EBUSY; }
+    /* Try to take the mutex. If we won, **mp will be 1. If we lost,
+     * it'll be something different.
+     * 
+     * There's one small caveat to this: If, by some magical means, you have
+     * the ability to make UINT_MAX attempts *simultaneously*
+     * you run the slight risk of overflowing.
+     * 
+     * That's a lot of attempts, though.
+     */
+    return ( ++(**mp) == 1 ? 0 : EBUSY );    
 }