[ovs-dev] [PATCH v2 3/3] lib/ovs-thread: Avoid atomic read in ovsthread_once_start().

[Jarno Rajahalme]

We can use a normal bool and rely on the mutex_lock/unlock and an
atomic_thread_fence for synchronization.

Also flip the return value of ovsthread_once_start__() to match the
one of ovsthread_once_start().

Signed-off-by: Jarno Rajahalme <jrajaha...@nicira.com>
---
v3: Simplified.

 lib/ovs-thread.c |   14 ++++++++++----
 lib/ovs-thread.h |   22 ++++++++--------------
 2 files changed, 18 insertions(+), 18 deletions(-)

diff --git a/lib/ovs-thread.c b/lib/ovs-thread.c
index 955c1c1..e2c3971 100644
--- a/lib/ovs-thread.c
+++ b/lib/ovs-thread.c
@@ -367,17 +367,23 @@ bool
 ovsthread_once_start__(struct ovsthread_once *once)
 {
     ovs_mutex_lock(&once->mutex);
-    if (!ovsthread_once_is_done__(once)) {
-        return false;
+    /* Mutex synchronizes memory, so we get the current value of 'done'. */
+    if (!once->done) {
+        return true;
     }
     ovs_mutex_unlock(&once->mutex);
-    return true;
+    return false;
 }
 
 void
 ovsthread_once_done(struct ovsthread_once *once)
 {
-    atomic_store(&once->done, true);
+    /* We need release semantics here, so that the following store may not
+     * be moved ahead of any of the preceding initialization operations.
+     * A release atomic_thread_fence provides that prior memory accesses
+     * will not be reordered to take place after the following store. */
+    atomic_thread_fence(memory_order_release);
+    once->done = true;
     ovs_mutex_unlock(&once->mutex);
 }
 
diff --git a/lib/ovs-thread.h b/lib/ovs-thread.h
index 962e867..42808b9 100644
--- a/lib/ovs-thread.h
+++ b/lib/ovs-thread.h
@@ -533,13 +533,13 @@ void *ovsthread_getspecific(ovsthread_key_t);
  */
 
 struct ovsthread_once {
-    atomic_bool done;
+    bool done;               /* Non-atomic, false negatives possible. */
     struct ovs_mutex mutex;
 };
 
 #define OVSTHREAD_ONCE_INITIALIZER              \
     {                                           \
-        ATOMIC_VAR_INIT(false),                 \
+        false,                                  \
         OVS_MUTEX_INITIALIZER,                  \
     }
 
@@ -549,16 +549,7 @@ void ovsthread_once_done(struct ovsthread_once *once)
     OVS_RELEASES(once->mutex);
 
 bool ovsthread_once_start__(struct ovsthread_once *once)
-    OVS_TRY_LOCK(false, once->mutex);
-
-static inline bool
-ovsthread_once_is_done__(struct ovsthread_once *once)
-{
-    bool done;
-
-    atomic_read_relaxed(&once->done, &done);
-    return done;
-}
+    OVS_TRY_LOCK(true, once->mutex);
 
 /* Returns true if this is the first call to ovsthread_once_start() for
  * 'once'.  In this case, the caller should perform whatever initialization
@@ -570,8 +561,11 @@ ovsthread_once_is_done__(struct ovsthread_once *once)
 static inline bool
 ovsthread_once_start(struct ovsthread_once *once)
 {
-    return OVS_UNLIKELY(!ovsthread_once_is_done__(once)
-                        && !ovsthread_once_start__(once));
+    /* We may be reading 'done' at the same time as the first thread
+     * is writing on it, or we can be using a stale copy of it.  The
+     * worst that can happen is that we call ovsthread_once_start__()
+     * once when strictly not necessary. */
+    return OVS_UNLIKELY(!once->done && ovsthread_once_start__(once));
 }
 
 /* Thread ID.
-- 
1.7.10.4

_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev