[ovs-dev] [PATCH] Use "error-checking" mutexes in place of other kinds wherever possible.
Ethan Jackson
ethan at nicira.com
Sat Aug 17 04:47:30 UTC 2013
> 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 <ethan at nicira.com>
>
> Signed-off-by: Ben Pfaff <blp at 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 at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
More information about the dev
mailing list