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>

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 {}

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,

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++) {

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);
     }
 

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) {

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;

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;
 }
 

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);
 

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

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                                                            \

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_)
 {

lib/ovs-thread.h

@@ -30,46 +30,20 @@ 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.
  *
  * 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)

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;
 

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);
     }
 

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();

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);

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. */

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);
     }
 

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);

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;

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;

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;