Use "error-checking" mutexes in place of other kinds wherever possible.

We've seen a number of deadlocks in the tree since thread safety was
introduced.  So far, all of these are self-deadlocks, that is, a single
thread acquiring a lock and then attempting to re-acquire the same lock
recursively.  When this has happened, the process simply hung, and it was
somewhat difficult to find the cause.

POSIX "error-checking" mutexes check for this specific problem (and
others).  This commit switches from other types of mutexes to
error-checking mutexes everywhere that we can, that is, everywhere that
we're not using recursive mutexes.  This ought to help find problems more
quickly in the future.

There might be performance advantages to other kinds of mutexes in some
cases.  However, the existing mutex type choices were just guesses, so I'd
rather go for easy detection of errors until we know that other mutex
types actually perform better in specific cases.  Also, I did a quick
microbenchmark of glibc mutex types on my host and found that the
error checking mutexes weren't any slower than the other types, at least
when the mutex is uncontended.

Signed-off-by: Ben Pfaff <blp@nicira.com>
Acked-by: Ethan Jackson <ethan@nicira.com>

22 files changed, 52 insertions, 67 deletions

diff --git a/include/sparse/pthread.h b/include/sparse/pthread.h
index 40c5ca3a5..e5b2a08d0 100644
--- a/include/sparse/pthread.h
+++ b/include/sparse/pthread.h
@@ -30,8 +30,5 @@
 #undef PTHREAD_RWLOCK_INITIALIZER
 #define PTHREAD_RWLOCK_INITIALIZER {}
 
-#undef PTHREAD_ADAPTIVE_MUTEX_INITIALIZER_NP
-#define PTHREAD_ADAPTIVE_MUTEX_INITIALIZER_NP {}
-
 #undef PTHREAD_ERRORCHECK_MUTEX_INITIALIZER_NP
 #define PTHREAD_ERRORCHECK_MUTEX_INITIALIZER_NP {}
diff --git a/lib/dpif-linux.c b/lib/dpif-linux.c
index 1b97410e7..a95ece16d 100644
--- a/lib/dpif-linux.c
+++ b/lib/dpif-linux.c
@@ -248,7 +248,7 @@ open_dpif(const struct dpif_linux_dp *dp, struct dpif **dpifp)
 
     dpif = xzalloc(sizeof *dpif);
     dpif->port_notifier = NULL;
-    ovs_mutex_init(&dpif->upcall_lock, PTHREAD_MUTEX_DEFAULT);
+    ovs_mutex_init(&dpif->upcall_lock);
     dpif->epoll_fd = -1;
 
     dpif_init(&dpif->dpif, &dpif_linux_class, dp->name,
diff --git a/lib/fatal-signal.c b/lib/fatal-signal.c
index b4c5ee6ac..e980f4be9 100644
--- a/lib/fatal-signal.c
+++ b/lib/fatal-signal.c
@@ -77,7 +77,7 @@ fatal_signal_init(void)
         assert_single_threaded();
         inited = true;
 
-        ovs_mutex_init(&mutex, PTHREAD_MUTEX_RECURSIVE);
+        ovs_mutex_init_recursive(&mutex);
         xpipe_nonblocking(signal_fds);
 
         for (i = 0; i < ARRAY_SIZE(fatal_signals); i++) {
diff --git a/lib/lacp.c b/lib/lacp.c
index 82698746d..5421e2ae3 100644
--- a/lib/lacp.c
+++ b/lib/lacp.c
@@ -1,4 +1,4 @@
-/* Copyright (c) 2011, 2012 Nicira, Inc.
+/* Copyright (c) 2011, 2012, 2013 Nicira, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -207,7 +207,7 @@ lacp_create(void) OVS_EXCLUDED(mutex)
     struct lacp *lacp;
 
     if (ovsthread_once_start(&once)) {
-        ovs_mutex_init(&mutex, PTHREAD_MUTEX_RECURSIVE);
+        ovs_mutex_init_recursive(&mutex);
         ovsthread_once_done(&once);
     }
 
diff --git a/lib/netdev-bsd.c b/lib/netdev-bsd.c
index 180ce7fa9..7c19483b1 100644
--- a/lib/netdev-bsd.c
+++ b/lib/netdev-bsd.c
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2011 Gaetano Catalli.
+ * Copyright (c) 2011, 2013 Gaetano Catalli.
  * Copyright (c) 2013 YAMAMOTO Takashi.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
@@ -293,7 +293,7 @@ netdev_bsd_construct_system(struct netdev *netdev_)
         return error;
     }
 
-    ovs_mutex_init(&netdev->mutex, PTHREAD_MUTEX_NORMAL);
+    ovs_mutex_init(&netdev->mutex);
     netdev->change_seq = 1;
     netdev->tap_fd = -1;
     netdev->kernel_name = xstrdup(netdev_->name);
@@ -327,7 +327,7 @@ netdev_bsd_construct_tap(struct netdev *netdev_)
 
     /* Create a tap device by opening /dev/tap.  The TAPGIFNAME ioctl is used
      * to retrieve the name of the tap device. */
-    ovs_mutex_init(&netdev->mutex, PTHREAD_MUTEX_NORMAL);
+    ovs_mutex_init(&netdev->mutex);
     netdev->tap_fd = open("/dev/tap", O_RDWR);
     netdev->change_seq = 1;
     if (netdev->tap_fd < 0) {
diff --git a/lib/netdev-dummy.c b/lib/netdev-dummy.c
index e17ef9ddb..ac2656490 100644
--- a/lib/netdev-dummy.c
+++ b/lib/netdev-dummy.c
@@ -271,7 +271,7 @@ netdev_dummy_construct(struct netdev *netdev_)
 
     atomic_add(&next_n, 1, &n);
 
-    ovs_mutex_init(&netdev->mutex, PTHREAD_MUTEX_NORMAL);
+    ovs_mutex_init(&netdev->mutex);
     ovs_mutex_lock(&netdev->mutex);
     netdev->hwaddr[0] = 0xaa;
     netdev->hwaddr[1] = 0x55;
diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c
index 2db56ac30..80fa39b47 100644
--- a/lib/netdev-linux.c
+++ b/lib/netdev-linux.c
@@ -619,7 +619,7 @@ netdev_linux_alloc(void)
 static void
 netdev_linux_common_construct(struct netdev_linux *netdev)
 {
-    ovs_mutex_init(&netdev->mutex, PTHREAD_MUTEX_NORMAL);
+    ovs_mutex_init(&netdev->mutex);
     netdev->change_seq = 1;
 }
 
diff --git a/lib/netdev-vport.c b/lib/netdev-vport.c
index 76aa148cc..0f5dd7ab3 100644
--- a/lib/netdev-vport.c
+++ b/lib/netdev-vport.c
@@ -171,7 +171,7 @@ netdev_vport_construct(struct netdev *netdev_)
 {
     struct netdev_vport *netdev = netdev_vport_cast(netdev_);
 
-    ovs_mutex_init(&netdev->mutex, PTHREAD_MUTEX_NORMAL);
+    ovs_mutex_init(&netdev->mutex);
     netdev->change_seq = 1;
     eth_addr_random(netdev->etheraddr);
 
diff --git a/lib/netlink-socket.c b/lib/netlink-socket.c
index 99bd4cc05..9562d38e5 100644
--- a/lib/netlink-socket.c
+++ b/lib/netlink-socket.c
@@ -1012,7 +1012,7 @@ struct nl_pool {
     int n;
 };
 
-static struct ovs_mutex pool_mutex = OVS_ADAPTIVE_MUTEX_INITIALIZER;
+static struct ovs_mutex pool_mutex = OVS_MUTEX_INITIALIZER;
 static struct nl_pool pools[MAX_LINKS] OVS_GUARDED_BY(pool_mutex);
 
 static int
diff --git a/lib/ovs-atomic-gcc4+.c b/lib/ovs-atomic-gcc4+.c
index 169384873..d6a68ae38 100644
--- a/lib/ovs-atomic-gcc4+.c
+++ b/lib/ovs-atomic-gcc4+.c
@@ -20,7 +20,7 @@
 #include "ovs-thread.h"
 
 #if OVS_ATOMIC_GCC4P_IMPL
-static struct ovs_mutex mutex = OVS_ADAPTIVE_MUTEX_INITIALIZER;
+static struct ovs_mutex mutex = OVS_MUTEX_INITIALIZER;
 
 #define DEFINE_LOCKED_OP(TYPE, NAME, OPERATOR)                          \
     TYPE##_t                                                            \
diff --git a/lib/ovs-thread.c b/lib/ovs-thread.c
index c1560703b..e9366e2de 100644
--- a/lib/ovs-thread.c
+++ b/lib/ovs-thread.c
@@ -132,8 +132,8 @@ typedef void destructor_func(void *);
 XPTHREAD_FUNC2(pthread_key_create, pthread_key_t *, destructor_func *);
 XPTHREAD_FUNC2(pthread_setspecific, pthread_key_t, const void *);
 
-void
-ovs_mutex_init(const struct ovs_mutex *l_, int type)
+static void
+ovs_mutex_init__(const struct ovs_mutex *l_, int type)
 {
     struct ovs_mutex *l = CONST_CAST(struct ovs_mutex *, l_);
     pthread_mutexattr_t attr;
@@ -149,6 +149,20 @@ ovs_mutex_init(const struct ovs_mutex *l_, int type)
     xpthread_mutexattr_destroy(&attr);
 }
 
+/* Initializes 'mutex' as a normal (non-recursive) mutex. */
+void
+ovs_mutex_init(const struct ovs_mutex *mutex)
+{
+    ovs_mutex_init__(mutex, PTHREAD_MUTEX_ERRORCHECK);
+}
+
+/* Initializes 'mutex' as a recursive mutex. */
+void
+ovs_mutex_init_recursive(const struct ovs_mutex *mutex)
+{
+    ovs_mutex_init__(mutex, PTHREAD_MUTEX_RECURSIVE);
+}
+
 void
 ovs_rwlock_init(const struct ovs_rwlock *l_)
 {
diff --git a/lib/ovs-thread.h b/lib/ovs-thread.h
index b7bc5d198..f10bc28bf 100644
--- a/lib/ovs-thread.h
+++ b/lib/ovs-thread.h
@@ -30,38 +30,11 @@ struct OVS_LOCKABLE ovs_mutex {
     const char *where;
 };
 
-/* "struct ovs_mutex" initializers:
- *
- *    - OVS_MUTEX_INITIALIZER: common case.
- *
- *    - OVS_ADAPTIVE_MUTEX_INITIALIZER for a mutex that spins briefly then goes
- *      to sleeps after some number of iterations.
- *
- *    - OVS_ERRORCHECK_MUTEX_INITIALIZER for a mutex that is used for
- *      error-checking. */
-#define OVS_MUTEX_INITIALIZER { PTHREAD_MUTEX_INITIALIZER, NULL }
-#ifdef PTHREAD_ADAPTIVE_MUTEX_INITIALIZER_NP
-#define OVS_ADAPTIVE_MUTEX_INITIALIZER \
-    { PTHREAD_ADAPTIVE_MUTEX_INITIALIZER_NP, NULL }
-#else
-#define OVS_ADAPTIVE_MUTEX_INITIALIZER OVS_MUTEX_INITIALIZER
-#endif
+/* "struct ovs_mutex" initializer. */
 #ifdef PTHREAD_ERRORCHECK_MUTEX_INITIALIZER_NP
-#define OVS_ERRORCHECK_MUTEX_INITIALIZER \
-    { PTHREAD_ERRORCHECK_MUTEX_INITIALIZER_NP, NULL }
+#define OVS_MUTEX_INITIALIZER { PTHREAD_ERRORCHECK_MUTEX_INITIALIZER_NP, NULL }
 #else
-#define OVS_ERRORCHECK_MUTEX_INITIALIZER OVS_MUTEX_INITIALIZER
-#endif
-
-/* Mutex types, suitable for use with pthread_mutexattr_settype().
- * There is only one nonstandard type:
- *
- *    - PTHREAD_MUTEX_ADAPTIVE_NP, the type used for
- *      OVS_ADAPTIVE_MUTEX_INITIALIZER. */
-#ifdef PTHREAD_ADAPTIVE_MUTEX_INITIALIZER_NP
-#define OVS_MUTEX_ADAPTIVE PTHREAD_MUTEX_ADAPTIVE_NP
-#else
-#define OVS_MUTEX_ADAPTIVE PTHREAD_MUTEX_NORMAL
+#define OVS_MUTEX_INITIALIZER { PTHREAD_MUTEX_INITIALIZER, NULL }
 #endif
 
 /* ovs_mutex functions analogous to pthread_mutex_*() functions.
@@ -69,7 +42,8 @@ struct OVS_LOCKABLE ovs_mutex {
  * Most of these functions abort the process with an error message on any
  * error.  ovs_mutex_trylock() is an exception: it passes through a 0 or EBUSY
  * return value to the caller and aborts on any other error. */
-void ovs_mutex_init(const struct ovs_mutex *, int type);
+void ovs_mutex_init(const struct ovs_mutex *);
+void ovs_mutex_init_recursive(const struct ovs_mutex *);
 void ovs_mutex_destroy(const struct ovs_mutex *);
 void ovs_mutex_unlock(const struct ovs_mutex *mutex) OVS_RELEASES(mutex);
 void ovs_mutex_lock_at(const struct ovs_mutex *mutex, const char *where)
@@ -463,7 +437,7 @@ struct ovsthread_once {
 #define OVSTHREAD_ONCE_INITIALIZER              \
     {                                           \
         ATOMIC_VAR_INIT(false),                 \
-        OVS_ADAPTIVE_MUTEX_INITIALIZER,         \
+        OVS_MUTEX_INITIALIZER,                  \
     }
 
 static inline bool ovsthread_once_start(struct ovsthread_once *once)
diff --git a/lib/seq.c b/lib/seq.c
index ed205a137..7a3424402 100644
--- a/lib/seq.c
+++ b/lib/seq.c
@@ -52,7 +52,7 @@ struct seq_thread {
     bool waiting OVS_GUARDED;        /* True if latch_wait() already called. */
 };
 
-static struct ovs_mutex seq_mutex = OVS_ADAPTIVE_MUTEX_INITIALIZER;
+static struct ovs_mutex seq_mutex = OVS_MUTEX_INITIALIZER;
 
 static uint64_t seq_next OVS_GUARDED_BY(seq_mutex) = 1;
 
diff --git a/lib/stp.c b/lib/stp.c
index d570be957..6e1efd07a 100644
--- a/lib/stp.c
+++ b/lib/stp.c
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2008, 2009, 2010, 2011, 2012 Nicira, Inc.
+ * Copyright (c) 2008, 2009, 2010, 2011, 2012, 2013 Nicira, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -264,7 +264,7 @@ stp_create(const char *name, stp_identifier bridge_id,
          * into the stp module through a patch port.  This happens
          * intentionally as part of the unit tests.  Ideally we'd ditch
          * the call back function, but for now this is what we have. */
-        ovs_mutex_init(&mutex,  PTHREAD_MUTEX_RECURSIVE);
+        ovs_mutex_init_recursive(&mutex);
         ovsthread_once_done(&once);
     }
 
diff --git a/lib/uuid.c b/lib/uuid.c
index 18748ea95..315c851c2 100644
--- a/lib/uuid.c
+++ b/lib/uuid.c
@@ -81,7 +81,7 @@ uuid_init(void)
 void
 uuid_generate(struct uuid *uuid)
 {
-    static struct ovs_mutex mutex = OVS_ADAPTIVE_MUTEX_INITIALIZER;
+    static struct ovs_mutex mutex = OVS_MUTEX_INITIALIZER;
     uint64_t copy[2];
 
     uuid_init();
diff --git a/lib/vlog.c b/lib/vlog.c
index ac229b4ee..061250a3e 100644
--- a/lib/vlog.c
+++ b/lib/vlog.c
@@ -106,7 +106,7 @@ DEFINE_STATIC_PER_THREAD_DATA(unsigned int, msg_num, 0);
  *
  * All of the following is protected by 'log_file_mutex', which nests inside
  * pattern_rwlock. */
-static struct ovs_mutex log_file_mutex = OVS_ADAPTIVE_MUTEX_INITIALIZER;
+static struct ovs_mutex log_file_mutex = OVS_MUTEX_INITIALIZER;
 static char *log_file_name OVS_GUARDED_BY(log_file_mutex);
 static int log_fd OVS_GUARDED_BY(log_file_mutex) = -1;
 static struct async_append *log_writer OVS_GUARDED_BY(log_file_mutex);
diff --git a/lib/vlog.h b/lib/vlog.h
index 87a96548d..d2c6679e2 100644
--- a/lib/vlog.h
+++ b/lib/vlog.h
@@ -119,7 +119,7 @@ struct vlog_rate_limit {
             0,                              /* first_dropped */         \
             0,                              /* last_dropped */          \
             0,                              /* n_dropped */             \
-            OVS_ADAPTIVE_MUTEX_INITIALIZER  /* mutex */                 \
+            OVS_MUTEX_INITIALIZER           /* mutex */                 \
         }
 
 /* Configuring how each module logs messages. */
diff --git a/ofproto/ofproto-dpif-sflow.c b/ofproto/ofproto-dpif-sflow.c
index 44ad92733..158887f03 100644
--- a/ofproto/ofproto-dpif-sflow.c
+++ b/ofproto/ofproto-dpif-sflow.c
@@ -318,7 +318,7 @@ dpif_sflow_create(void)
     struct dpif_sflow *ds;
 
     if (ovsthread_once_start(&once)) {
-        ovs_mutex_init(&mutex, PTHREAD_MUTEX_RECURSIVE);
+        ovs_mutex_init_recursive(&mutex);
         ovsthread_once_done(&once);
     }
 
diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
index 242086584..111b77979 100644
--- a/ofproto/ofproto-dpif-upcall.c
+++ b/ofproto/ofproto-dpif-upcall.c
@@ -123,9 +123,9 @@ udpif_create(struct dpif_backer *backer, struct dpif *dpif)
     list_init(&udpif->upcalls);
     list_init(&udpif->fmbs);
     atomic_init(&udpif->reval_seq, 0);
-    ovs_mutex_init(&udpif->drop_key_mutex, PTHREAD_MUTEX_NORMAL);
-    ovs_mutex_init(&udpif->upcall_mutex, PTHREAD_MUTEX_NORMAL);
-    ovs_mutex_init(&udpif->fmb_mutex, PTHREAD_MUTEX_NORMAL);
+    ovs_mutex_init(&udpif->drop_key_mutex);
+    ovs_mutex_init(&udpif->upcall_mutex);
+    ovs_mutex_init(&udpif->fmb_mutex);
 
     return udpif;
 }
@@ -219,7 +219,7 @@ udpif_recv_set(struct udpif *udpif, size_t n_handlers, bool enable)
             handler->udpif = udpif;
             list_init(&handler->upcalls);
             xpthread_cond_init(&handler->wake_cond, NULL);
-            ovs_mutex_init(&handler->mutex, PTHREAD_MUTEX_NORMAL);
+            ovs_mutex_init(&handler->mutex);
             xpthread_create(&handler->thread, NULL, udpif_miss_handler, handler);
         }
         xpthread_create(&udpif->dispatcher, NULL, udpif_dispatcher, udpif);
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 46902159b..d4fc8ec30 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -1268,20 +1268,20 @@ construct(struct ofproto *ofproto_)
     ofproto->ml = mac_learning_create(MAC_ENTRY_DEFAULT_IDLE_TIME);
     ofproto->mbridge = mbridge_create();
     ofproto->has_bonded_bundles = false;
-    ovs_mutex_init(&ofproto->vsp_mutex, PTHREAD_MUTEX_NORMAL);
+    ovs_mutex_init(&ofproto->vsp_mutex);
 
     classifier_init(&ofproto->facets);
     ofproto->consistency_rl = LLONG_MIN;
 
     list_init(&ofproto->completions);
 
-    ovs_mutex_init(&ofproto->flow_mod_mutex, PTHREAD_MUTEX_NORMAL);
+    ovs_mutex_init(&ofproto->flow_mod_mutex);
     ovs_mutex_lock(&ofproto->flow_mod_mutex);
     list_init(&ofproto->flow_mods);
     ofproto->n_flow_mods = 0;
     ovs_mutex_unlock(&ofproto->flow_mod_mutex);
 
-    ovs_mutex_init(&ofproto->pin_mutex, PTHREAD_MUTEX_NORMAL);
+    ovs_mutex_init(&ofproto->pin_mutex);
     ovs_mutex_lock(&ofproto->pin_mutex);
     list_init(&ofproto->pins);
     ofproto->n_pins = 0;
@@ -4866,7 +4866,7 @@ static enum ofperr
 rule_construct(struct rule *rule_)
 {
     struct rule_dpif *rule = rule_dpif_cast(rule_);
-    ovs_mutex_init(&rule->stats_mutex, PTHREAD_MUTEX_NORMAL);
+    ovs_mutex_init(&rule->stats_mutex);
     ovs_mutex_lock(&rule->stats_mutex);
     rule->packet_count = 0;
     rule->byte_count = 0;
diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index c8edb2d47..1e09c5662 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -437,7 +437,7 @@ ofproto_create(const char *datapath_name, const char *datapath_type,
     ofproto->n_tables = 0;
     hindex_init(&ofproto->cookies);
     list_init(&ofproto->expirable);
-    ovs_mutex_init(&ofproto->expirable_mutex, PTHREAD_MUTEX_RECURSIVE);
+    ovs_mutex_init_recursive(&ofproto->expirable_mutex);
     ofproto->connmgr = connmgr_create(ofproto, datapath_name, datapath_name);
     ofproto->state = S_OPENFLOW;
     list_init(&ofproto->pending);
@@ -3436,7 +3436,7 @@ add_flow(struct ofproto *ofproto, struct ofconn *ofconn,
     rule->flow_cookie = fm->new_cookie;
     rule->created = rule->modified = rule->used = time_msec();
 
-    ovs_mutex_init(&rule->timeout_mutex, OVS_MUTEX_ADAPTIVE);
+    ovs_mutex_init(&rule->timeout_mutex);
     ovs_mutex_lock(&rule->timeout_mutex);
     rule->idle_timeout = fm->idle_timeout;
     rule->hard_timeout = fm->hard_timeout;
diff --git a/vswitchd/system-stats.c b/vswitchd/system-stats.c
index ae523f34c..e8a851214 100644
--- a/vswitchd/system-stats.c
+++ b/vswitchd/system-stats.c
@@ -506,7 +506,7 @@ get_filesys_stats(struct smap *stats OVS_UNUSED)
 
 #define SYSTEM_STATS_INTERVAL (5 * 1000) /* In milliseconds. */
 
-static struct ovs_mutex mutex = OVS_ADAPTIVE_MUTEX_INITIALIZER;
+static struct ovs_mutex mutex = OVS_MUTEX_INITIALIZER;
 static pthread_cond_t cond = PTHREAD_COND_INITIALIZER;
 static struct latch latch OVS_GUARDED_BY(mutex);
 static bool enabled;