[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