[ovs-dev] [PATCH v2] Use "error-checking" mutexes in place of other kinds wherever possible.
Ben Pfaff
blp at nicira.com
Tue Aug 20 18:24:16 UTC 2013
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 at nicira.com>
---
v1->v2: Instead of passing a 'type' parameter to ovs_mutex_init(),
break off a separate function for recursive mutexes.
include/sparse/pthread.h | 3 ---
lib/dpif-linux.c | 2 +-
lib/fatal-signal.c | 2 +-
lib/lacp.c | 4 ++--
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.c | 16 ++++++++++++++--
lib/ovs-thread.h | 38 ++++++--------------------------------
lib/seq.c | 2 +-
lib/stp.c | 4 ++--
lib/uuid.c | 2 +-
lib/vlog.c | 2 +-
lib/vlog.h | 2 +-
ofproto/ofproto-dpif-sflow.c | 2 +-
ofproto/ofproto-dpif-upcall.c | 8 ++++----
ofproto/ofproto-dpif.c | 8 ++++----
ofproto/ofproto.c | 4 ++--
vswitchd/system-stats.c | 2 +-
22 files changed, 50 insertions(+), 67 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..a95ece1 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 b4c5ee6..e980f4b 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 8269874..5421e2a 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 180ce7f..7c19483 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 e17ef9d..ac26564 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 2db56ac..80fa39b 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 76aa148..0f5dd7a 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 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.c b/lib/ovs-thread.c
index c156070..cf6254c 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;
@@ -150,6 +150,18 @@ ovs_mutex_init(const struct ovs_mutex *l_, int type)
}
void
+ovs_mutex_init(const struct ovs_mutex *l)
+{
+ ovs_mutex_init__(l, PTHREAD_MUTEX_ERRORCHECK);
+}
+
+void
+ovs_mutex_init_recursive(const struct ovs_mutex *l)
+{
+ ovs_mutex_init__(l, PTHREAD_MUTEX_RECURSIVE);
+}
+
+void
ovs_rwlock_init(const struct ovs_rwlock *l_)
{
struct ovs_rwlock *l = CONST_CAST(struct ovs_rwlock *, l_);
diff --git a/lib/ovs-thread.h b/lib/ovs-thread.h
index b7bc5d1..f10bc28 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 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/stp.c b/lib/stp.c
index d570be9..6e1efd0 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 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-sflow.c b/ofproto/ofproto-dpif-sflow.c
index b387b94..3e6b987 100644
--- a/ofproto/ofproto-dpif-sflow.c
+++ b/ofproto/ofproto-dpif-sflow.c
@@ -317,7 +317,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 2420865..111b779 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 4690215..d4fc8ec 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 c8edb2d..1e09c56 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 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
More information about the dev
mailing list