> 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.
Agreed. I think we're a long ways from mutex performance being an issue for us. Much lower hanging fruit bouncing around. I think the patch is fine as is. Just a minor comment. Personally I'd prefer we change ovs_mutex_init() to not take a mutex type as an argument and simply use the error checking mutex always. I think it's going to be a long time before we actually need to configure this on a per mutex basis, and I don't think we know what abstraction we'll need at that point today. Acked-by: Ethan Jackson <[email protected]> > > Signed-off-by: Ben Pfaff <[email protected]> > --- > 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 > [email protected] > http://openvswitch.org/mailman/listinfo/dev _______________________________________________ dev mailing list [email protected] http://openvswitch.org/mailman/listinfo/dev
