once: Fix race causing pa_once to sometimes run twice

There was a race in the existing code that could cause the pa_once code
to be run twice, see:
http://lists.freedesktop.org/archives/pulseaudio-discuss/2012-April/013354.html

Therefore the existing implementation was rewritten to instead look like
the reference implementation here:
http://www.hpl.hp.com/research/linux/atomic_ops/example.php4

Signed-off-by: David Henningsson <david.henningsson@canonical.com>

diff --git a/src/pulsecore/once.c b/src/pulsecore/once.c
index 4e509e0cca0fc71af5e85b990bad87ada5178e8b..30b35a6c36bcfc871558dba15c7183d28adb4153 100644 (file)
--- a/src/pulsecore/once.c
+++ b/src/pulsecore/once.c
@@ -24,46 +24,33 @@
 #endif
 
 #include <pulsecore/macro.h>
-#include <pulsecore/mutex.h>
 
 #include "once.h"
 
+/* See http://www.hpl.hp.com/research/linux/atomic_ops/example.php4 for the
+ * reference algorithm used here. */
+
 pa_bool_t pa_once_begin(pa_once *control) {
+    pa_mutex *m;
+
     pa_assert(control);
 
     if (pa_atomic_load(&control->done))
         return FALSE;
 
-    pa_atomic_inc(&control->ref);
-
     /* Caveat: We have to make sure that the once func has completed
      * before returning, even if the once func is not actually
      * executed by us. Hence the awkward locking. */
 
-    for (;;) {
-        pa_mutex *m;
-
-        if ((m = pa_atomic_ptr_load(&control->mutex))) {
-
-            /* The mutex is stored in locked state, hence let's just
-             * wait until it is unlocked */
-            pa_mutex_lock(m);
-
-            pa_assert(pa_atomic_load(&control->done));
-
-            pa_once_end(control);
-            return FALSE;
-        }
-
-        pa_assert_se(m = pa_mutex_new(FALSE, FALSE));
-        pa_mutex_lock(m);
-
-        if (pa_atomic_ptr_cmpxchg(&control->mutex, NULL, m))
-            return TRUE;
+    m = pa_static_mutex_get(&control->mutex, FALSE, FALSE);
+    pa_mutex_lock(m);
 
+    if (pa_atomic_load(&control->done)) {
         pa_mutex_unlock(m);
-        pa_mutex_free(m);
+        return FALSE;
     }
+
+    return TRUE;
 }
 
 void pa_once_end(pa_once *control) {
@@ -71,15 +58,11 @@ void pa_once_end(pa_once *control) {
 
     pa_assert(control);
 
+    pa_assert(!pa_atomic_load(&control->done));
     pa_atomic_store(&control->done, 1);
 
-    pa_assert_se(m = pa_atomic_ptr_load(&control->mutex));
+    m = pa_static_mutex_get(&control->mutex, FALSE, FALSE);
     pa_mutex_unlock(m);
-
-    if (pa_atomic_dec(&control->ref) <= 1) {
-        pa_assert_se(pa_atomic_ptr_cmpxchg(&control->mutex, m, NULL));
-        pa_mutex_free(m);
-    }
 }
 
 /* Not reentrant -- how could it be? */

diff --git a/src/pulsecore/once.h b/src/pulsecore/once.h
index edc818818692af495462caae810ad597295de03a..a478d1ff26d295de88cd5805ebf3ad9255298eef 100644 (file)
--- a/src/pulsecore/once.h
+++ b/src/pulsecore/once.h
@@ -23,16 +23,16 @@
 ***/
 
 #include <pulsecore/atomic.h>
+#include <pulsecore/mutex.h>
 
 typedef struct pa_once {
-    pa_atomic_ptr_t mutex;
-    pa_atomic_t ref, done;
+    pa_static_mutex mutex;
+    pa_atomic_t done;
 } pa_once;
 
 #define PA_ONCE_INIT                                                    \
     {                                                                   \
-        .mutex = PA_ATOMIC_PTR_INIT(NULL),                              \
-        .ref = PA_ATOMIC_INIT(0),                                       \
+        .mutex = PA_STATIC_MUTEX_INIT,                                  \
         .done = PA_ATOMIC_INIT(0)                                       \
     }