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 <b...@nicira.com>
---
include/sparse/pthread.h | 3 ---
lib/dpif-linux.c | 2 +-
lib/netdev-bsd.c | 6 +++---
lib/netdev-dummy.c | 2 +-
lib/netdev-linux.c | 2 +-
lib/netdev-vport.c | 2 +-
lib/netlink-socket.c | 2 +-
lib/ovs-atomic-gcc4+.c | 2 +-
lib/ovs-thread.h | 35 ++++-------------------------------
lib/seq.c | 2 +-
lib/uuid.c | 2 +-
lib/vlog.c | 2 +-
lib/vlog.h | 2 +-
ofproto/ofproto-dpif-upcall.c | 8 ++++----
ofproto/ofproto-dpif.c | 8 ++++----
ofproto/ofproto.c | 2 +-
vswitchd/system-stats.c | 2 +-
17 files changed, 27 insertions(+), 57 deletions(-)
diff --git a/include/sparse/pthread.h b/include/sparse/pthread.h
index 40c5ca3..e5b2a08 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 1b97410..180899c 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, PTHREAD_MUTEX_ERRORCHECK);
dpif->epoll_fd = -1;
dpif_init(&dpif->dpif, &dpif_linux_class, dp->name,
diff --git a/lib/netdev-bsd.c b/lib/netdev-bsd.c
index 180ce7f..4f5eda3 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, PTHREAD_MUTEX_ERRORCHECK);
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, PTHREAD_MUTEX_ERRORCHECK);
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 5c31210..526c03c 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, PTHREAD_MUTEX_ERRORCHECK);
netdev->hwaddr[0] = 0xaa;
netdev->hwaddr[1] = 0x55;
netdev->hwaddr[2] = n >> 24;
diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c
index 2db56ac..2ebc688 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, PTHREAD_MUTEX_ERRORCHECK);
netdev->change_seq = 1;
}
diff --git a/lib/netdev-vport.c b/lib/netdev-vport.c
index 76aa148..e7a5aca 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, PTHREAD_MUTEX_ERRORCHECK);
netdev->change_seq = 1;
eth_addr_random(netdev->etheraddr);
diff --git a/lib/netlink-socket.c b/lib/netlink-socket.c
index 99bd4cc..9562d38 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 1693848..d6a68ae 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.h b/lib/ovs-thread.h
index b7bc5d1..a24f14e 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.
@@ -463,7 +436,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 ed205a1..7a34244 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/uuid.c b/lib/uuid.c
index 18748ea..315c851 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 ac229b4..061250a 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 87a9654..d2c6679 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-upcall.c b/ofproto/ofproto-dpif-upcall.c
index 2420865..c48c1e5 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, PTHREAD_MUTEX_ERRORCHECK);
+ ovs_mutex_init(&udpif->upcall_mutex, PTHREAD_MUTEX_ERRORCHECK);
+ ovs_mutex_init(&udpif->fmb_mutex, PTHREAD_MUTEX_ERRORCHECK);
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, PTHREAD_MUTEX_ERRORCHECK);
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 4690215..36cc349 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, PTHREAD_MUTEX_ERRORCHECK);
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, PTHREAD_MUTEX_ERRORCHECK);
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, PTHREAD_MUTEX_ERRORCHECK);
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, PTHREAD_MUTEX_ERRORCHECK);
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 c8edb2d..3315817 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -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, PTHREAD_MUTEX_ERRORCHECK);
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 ae523f3..e8a8512 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;
--
1.7.10.4
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
