From efedd0d21e4caf6edae6872f29c470a464e1917a Mon Sep 17 00:00:00 2001 From: Tobias Brunner Date: Fri, 11 Apr 2014 15:13:22 +0200 Subject: [PATCH 1/6] utils: Add ref_cur() to retrieve the current value of a reference counter On many architectures it is safe to read the value directly (those using cache coherency protocols, and with atomic loads for 32-bit values) but it is not if that's not the case or if we ever decide to make refcount_t 64-bit (load not atomic on x86). So make sure the operation is actually atomic and that users do not have to care about the size of refcount_t. --- src/libstrongswan/utils/utils.c | 14 +++++++++++++- src/libstrongswan/utils/utils.h | 13 +++++++++++-- 2 files changed, 24 insertions(+), 3 deletions(-) diff --git a/src/libstrongswan/utils/utils.c b/src/libstrongswan/utils/utils.c index fe80edb82..e4da6ddb9 100644 --- a/src/libstrongswan/utils/utils.c +++ b/src/libstrongswan/utils/utils.c @@ -528,7 +528,6 @@ refcount_t ref_get(refcount_t *ref) pthread_mutex_lock(&ref_mutex); current = ++(*ref); pthread_mutex_unlock(&ref_mutex); - return current; } @@ -545,6 +544,19 @@ bool ref_put(refcount_t *ref) return !more_refs; } +/** + * Current refcount + */ +refcount_t ref_cur(refcount_t *ref) +{ + refcount_t current; + + pthread_mutex_lock(&ref_mutex); + current = *ref; + pthread_mutex_unlock(&ref_mutex); + return current; +} + /** * Single mutex for all compare and swap operations. */ diff --git a/src/libstrongswan/utils/utils.h b/src/libstrongswan/utils/utils.h index a55e7d831..4b2990371 100644 --- a/src/libstrongswan/utils/utils.h +++ b/src/libstrongswan/utils/utils.h @@ -752,6 +752,7 @@ typedef u_int refcount_t; #define ref_get(ref) __sync_add_and_fetch(ref, 1) #define ref_put(ref) (!__sync_sub_and_fetch(ref, 1)) +#define ref_cur(ref) __sync_fetch_and_add(ref, 0) #define cas_bool(ptr, oldval, newval) \ (__sync_bool_compare_and_swap(ptr, oldval, newval)) @@ -763,7 +764,7 @@ typedef u_int refcount_t; /** * Get a new reference. * - * Increments the reference counter atomic. + * Increments the reference counter atomically. * * @param ref pointer to ref counter * @return new value of ref @@ -773,7 +774,7 @@ refcount_t ref_get(refcount_t *ref); /** * Put back a unused reference. * - * Decrements the reference counter atomic and + * Decrements the reference counter atomically and * says if more references available. * * @param ref pointer to ref counter @@ -781,6 +782,14 @@ refcount_t ref_get(refcount_t *ref); */ bool ref_put(refcount_t *ref); +/** + * Get the current value of the reference counter. + * + * @param ref pointer to ref counter + * @return current value of ref + */ +refcount_t ref_cur(refcount_t *ref); + /** * Atomically replace value of ptr with newval if it currently equals oldval. * From 0f603d425df132adcb60b9f25ac277b26e80c72a Mon Sep 17 00:00:00 2001 From: Tobias Brunner Date: Fri, 11 Apr 2014 16:07:32 +0200 Subject: [PATCH 2/6] utils: Use GCC's __atomic built-ins if available These are available since GCC 4.7 and will eventually replace the __sync operations. They support the memory model defined by C++11. For instance, by using __ATOMIC_RELAXED for some operations on the reference counters we can avoid memory barriers, which are required by __sync operations (whose memory model essentially is __ATOMIC_SEQ_CST). --- configure.ac | 32 +++++++++++++++++++++++++------- src/libstrongswan/utils/utils.c | 4 ++-- src/libstrongswan/utils/utils.h | 21 ++++++++++++++++++++- 3 files changed, 47 insertions(+), 10 deletions(-) diff --git a/configure.ac b/configure.ac index 6ae0314b0..699933644 100644 --- a/configure.ac +++ b/configure.ac @@ -667,21 +667,39 @@ AC_COMPILE_IFELSE( [AC_MSG_RESULT([no])] ) -AC_MSG_CHECKING([for gcc atomic operations]) +AC_MSG_CHECKING([for GCC __atomic operations]) AC_RUN_IFELSE([AC_LANG_SOURCE( [[ int main() { - volatile int ref = 1; - __sync_fetch_and_add (&ref, 1); - __sync_sub_and_fetch (&ref, 1); - /* Make sure test fails if operations are not supported */ - __sync_val_compare_and_swap(&ref, 1, 0); + int ref = 1, val; + __atomic_fetch_add(&ref, 1, __ATOMIC_RELAXED); + val = __atomic_sub_fetch(&ref, 1, __ATOMIC_RELAXED); + __atomic_compare_exchange_n(&ref, &val, 0, 0, __ATOMIC_RELAXED, + __ATOMIC_RELAXED); return ref; } ]])], [AC_MSG_RESULT([yes]); AC_DEFINE([HAVE_GCC_ATOMIC_OPERATIONS], [], - [have GCC __sync_* atomic operations])], + [have GCC __atomic_* operations])], + [AC_MSG_RESULT([no])], + [AC_MSG_RESULT([no])] +) + +AC_MSG_CHECKING([for GCC __sync operations]) +AC_RUN_IFELSE([AC_LANG_SOURCE( + [[ + int main() { + int ref = 1; + __sync_fetch_and_add (&ref, 1); + __sync_sub_and_fetch (&ref, 1); + __sync_val_compare_and_swap(&ref, 1, 0); + return ref; + } + ]])], + [AC_MSG_RESULT([yes]); + AC_DEFINE([HAVE_GCC_SYNC_OPERATIONS], [], + [have GCC __sync_* operations])], [AC_MSG_RESULT([no])], [AC_MSG_RESULT([no])] ) diff --git a/src/libstrongswan/utils/utils.c b/src/libstrongswan/utils/utils.c index e4da6ddb9..f2a4a065c 100644 --- a/src/libstrongswan/utils/utils.c +++ b/src/libstrongswan/utils/utils.c @@ -511,7 +511,7 @@ void nop() { } -#ifndef HAVE_GCC_ATOMIC_OPERATIONS +#if !defined(HAVE_GCC_ATOMIC_OPERATIONS) && !defined(HAVE_GCC_SYNC_OPERATIONS) /** * We use a single mutex for all refcount variables. @@ -578,7 +578,7 @@ bool cas_##name(type *ptr, type oldval, type newval) \ _cas_impl(bool, bool) _cas_impl(ptr, void*) -#endif /* HAVE_GCC_ATOMIC_OPERATIONS */ +#endif /* !HAVE_GCC_ATOMIC_OPERATIONS && !HAVE_GCC_SYNC_OPERATIONS */ #ifdef HAVE_FMEMOPEN_FALLBACK diff --git a/src/libstrongswan/utils/utils.h b/src/libstrongswan/utils/utils.h index 4b2990371..8f91e8431 100644 --- a/src/libstrongswan/utils/utils.h +++ b/src/libstrongswan/utils/utils.h @@ -750,6 +750,25 @@ typedef u_int refcount_t; #ifdef HAVE_GCC_ATOMIC_OPERATIONS +#define ref_get(ref) __atomic_add_fetch(ref, 1, __ATOMIC_RELAXED) +/* The relaxed memory model works fine for increments as these (usually) don't + * change the state of refcounted objects. But here we have to ensure that we + * free the right stuff if ref counted objects are mutable. So we have to sync + * with other threads that call ref_put(). It would be sufficient to use + * __ATOMIC_RELEASE here and then call __atomic_thread_fence() with + * __ATOMIC_ACQUIRE if we reach 0, but since we don't have control over the use + * of ref_put() we have to make sure. */ +#define ref_put(ref) (!__atomic_sub_fetch(ref, 1, __ATOMIC_ACQ_REL)) +#define ref_cur(ref) __atomic_load_n(ref, __ATOMIC_RELAXED) + +#define _cas_impl(ptr, oldval, newval) ({ typeof(oldval) _old = oldval; \ + __atomic_compare_exchange_n(ptr, &_old, newval, FALSE, \ + __ATOMIC_SEQ_CST, __ATOMIC_RELAXED); }) +#define cas_bool(ptr, oldval, newval) _cas_impl(ptr, oldval, newval) +#define cas_ptr(ptr, oldval, newval) _cas_impl(ptr, oldval, newval) + +#elif defined(HAVE_GCC_SYNC_OPERATIONS) + #define ref_get(ref) __sync_add_and_fetch(ref, 1) #define ref_put(ref) (!__sync_sub_and_fetch(ref, 1)) #define ref_cur(ref) __sync_fetch_and_add(ref, 0) @@ -759,7 +778,7 @@ typedef u_int refcount_t; #define cas_ptr(ptr, oldval, newval) \ (__sync_bool_compare_and_swap(ptr, oldval, newval)) -#else /* !HAVE_GCC_ATOMIC_OPERATIONS */ +#else /* !HAVE_GCC_ATOMIC_OPERATIONS && !HAVE_GCC_SYNC_OPERATIONS */ /** * Get a new reference. From a68454bd68221490e5504969b5378a1e86f204de Mon Sep 17 00:00:00 2001 From: Tobias Brunner Date: Fri, 11 Apr 2014 16:23:39 +0200 Subject: [PATCH 3/6] ike-sa-manager: Improve scalability of half-open IKE_SA checking This patch is based on one by Christoph Gouault. Currently, to count the total number of half_open IKE_SAs, get_half_open_count sums up the count of each segment in the SA hash table (acquiring a lock for each segment). This procedure does not scale well when the number of segments increases, as the method is called for each new negotiation. Instead, lets maintain a global atomic counter. This optimization allows the use of big values for charon.ikesa_table_size and charon.ikesa_table_segments. --- src/libcharon/sa/ike_sa_manager.c | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/src/libcharon/sa/ike_sa_manager.c b/src/libcharon/sa/ike_sa_manager.c index f2f81cf33..525117f3b 100644 --- a/src/libcharon/sa/ike_sa_manager.c +++ b/src/libcharon/sa/ike_sa_manager.c @@ -353,6 +353,11 @@ struct private_ike_sa_manager_t { */ shareable_segment_t *half_open_segments; + /** + * Total number of half-open IKE_SAs. + */ + refcount_t half_open_count; + /** * Hash table with connected_peers_t objects. */ @@ -764,6 +769,7 @@ static void put_half_open(private_ike_sa_manager_t *this, entry_t *entry) this->half_open_table[row] = item; } this->half_open_segments[segment].count++; + ref_get(&this->half_open_count); lock->unlock(lock); } @@ -803,6 +809,7 @@ static void remove_half_open(private_ike_sa_manager_t *this, entry_t *entry) free(item); } this->half_open_segments[segment].count--; + ignore_result(ref_put(&this->half_open_count)); break; } prev = item; @@ -1962,13 +1969,7 @@ METHOD(ike_sa_manager_t, get_half_open_count, u_int, } else { - for (segment = 0; segment < this->segment_count; segment++) - { - lock = this->half_open_segments[segment].lock; - lock->read_lock(lock); - count += this->half_open_segments[segment].count; - lock->unlock(lock); - } + count = (u_int)ref_cur(&this->half_open_count); } return count; } From 2cbaa632951dd662e26e63299d8049c0d2bed8b1 Mon Sep 17 00:00:00 2001 From: Christophe Gouault Date: Tue, 8 Apr 2014 17:11:13 +0200 Subject: [PATCH 4/6] load-tester: Fix race condition issuing same identity Due to an unprotected incrementation, two load-tester initiators occasionally use the same identifier under high load. The responder typically drops one of the connections. Use an atomic incrementation to avoid this race condition. Signed-off-by: Christophe Gouault --- src/libcharon/plugins/load_tester/load_tester_config.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libcharon/plugins/load_tester/load_tester_config.c b/src/libcharon/plugins/load_tester/load_tester_config.c index e133190b4..bc7c0ffbc 100644 --- a/src/libcharon/plugins/load_tester/load_tester_config.c +++ b/src/libcharon/plugins/load_tester/load_tester_config.c @@ -150,7 +150,7 @@ struct private_load_tester_config_t { /** * incremental numbering of generated configs */ - u_int num; + refcount_t num; /** * Dynamic source port, if used @@ -802,7 +802,7 @@ METHOD(backend_t, get_peer_cfg_by_name, peer_cfg_t*, { if (streq(name, "load-test")) { - return generate_config(this, this->num++); + return generate_config(this, (u_int)ref_get(&this->num)); } return NULL; } From 7b08063e70d916b16f1de2dba22110a92f7ff598 Mon Sep 17 00:00:00 2001 From: Christophe Gouault Date: Tue, 8 Apr 2014 17:11:14 +0200 Subject: [PATCH 5/6] load-tester: Fix race condition issuing same SPI Due to an unprotected incrementation, two load-tester initiators occasionally use the same SPI under high load, and hence generate 2 IPsec SAs with the same identifier. The responder IPsec stack will refuse to configure the second SA. Use an atomic incrementation to avoid this race condition. Signed-off-by: Christophe Gouault --- src/libcharon/plugins/load_tester/load_tester_ipsec.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libcharon/plugins/load_tester/load_tester_ipsec.c b/src/libcharon/plugins/load_tester/load_tester_ipsec.c index 02b1d4216..5edd3b82d 100644 --- a/src/libcharon/plugins/load_tester/load_tester_ipsec.c +++ b/src/libcharon/plugins/load_tester/load_tester_ipsec.c @@ -31,14 +31,14 @@ struct private_load_tester_ipsec_t { /** * faked SPI counter */ - u_int32_t spi; + refcount_t spi; }; METHOD(kernel_ipsec_t, get_spi, status_t, private_load_tester_ipsec_t *this, host_t *src, host_t *dst, u_int8_t protocol, u_int32_t reqid, u_int32_t *spi) { - *spi = ++this->spi; + *spi = (uint32_t)ref_get(&this->spi); return SUCCESS; } From 73c33ff42318809a002e10b6e117104b4e48d30d Mon Sep 17 00:00:00 2001 From: Tobias Brunner Date: Thu, 17 Apr 2014 10:47:32 +0200 Subject: [PATCH 6/6] bus: Add a fast-path if log messages don't have to be logged For some rwlock_t implementations acquiring the read lock could be quite expensive even if there are no writers (e.g. because the implementation requires acquiring a mutex to check for writers) particularly if the lock is highly contended, like it is for the vlog() method. --- src/libcharon/bus/bus.c | 60 ++++++++++++++++++++++++++++++++++++----- 1 file changed, 53 insertions(+), 7 deletions(-) diff --git a/src/libcharon/bus/bus.c b/src/libcharon/bus/bus.c index b46184809..bc080d1c0 100644 --- a/src/libcharon/bus/bus.c +++ b/src/libcharon/bus/bus.c @@ -1,5 +1,5 @@ /* - * Copyright (C) 2011-2012 Tobias Brunner + * Copyright (C) 2011-2014 Tobias Brunner * Copyright (C) 2006 Martin Willi * Hochschule fuer Technik Rapperswil * @@ -23,6 +23,31 @@ #include #include +/** + * These operations allow us to speed up the log level checks on some platforms. + * In particular if acquiring the read lock is expensive even in the absence of + * any writers. + * + * Note that while holding the read/write lock the read does not have to be + * atomic as the write lock must be held to set the level. + */ +#ifdef HAVE_GCC_ATOMIC_OPERATIONS + +#define skip_level(ptr, level) (__atomic_load_n(ptr, __ATOMIC_RELAXED) < level) +#define set_level(ptr, val) __atomic_store_n(ptr, val, __ATOMIC_RELAXED) + +#elif defined(HAVE_GCC_SYNC_OPERATIONS) + +#define skip_level(ptr, level) (__sync_fetch_and_add(ptr, 0) < level) +#define set_level(ptr, val) __sync_bool_compare_and_swap(ptr, *ptr, val) + +#else + +#define skip_level(ptr, level) FALSE +#define set_level(ptr, val) ({ *ptr = val; }) + +#endif + typedef struct private_bus_t private_bus_t; /** @@ -173,11 +198,12 @@ static inline void register_logger(private_bus_t *this, debug_t group, if (entry->logger->log) { - this->max_level[group] = max(this->max_level[group], level); + set_level(&this->max_level[group], max(this->max_level[group], level)); } if (entry->logger->vlog) { - this->max_vlevel[group] = max(this->max_vlevel[group], level); + set_level(&this->max_vlevel[group], + max(this->max_vlevel[group], level)); } } @@ -205,6 +231,7 @@ static inline void unregister_logger(private_bus_t *this, logger_t *logger) if (found) { + level_t level = LEVEL_SILENT, vlevel = LEVEL_SILENT; debug_t group; for (group = 0; group < DBG_MAX; group++) @@ -214,13 +241,19 @@ static inline void unregister_logger(private_bus_t *this, logger_t *logger) loggers = this->loggers[group]; loggers->remove(loggers, found, NULL); - this->max_level[group] = LEVEL_SILENT; - this->max_vlevel[group] = LEVEL_SILENT; if (loggers->get_first(loggers, (void**)&entry) == SUCCESS) { - this->max_level[group] = entry->levels[group]; - this->max_vlevel[group] = entry->levels[group]; + if (entry->logger->log) + { + level = entry->levels[group]; + } + if (entry->logger->vlog) + { + vlevel = entry->levels[group]; + } } + set_level(&this->max_level[group], level); + set_level(&this->max_vlevel[group], vlevel); } } free(found); @@ -324,6 +357,19 @@ METHOD(bus_t, vlog, void, linked_list_t *loggers; log_data_t data; + /* NOTE: This is not 100% thread-safe and done here only because it is + * performance critical. We therefore ignore the following two issues for + * this particular case: 1) We might miss some log messages if another + * thread concurrently increases the log level or registers a new logger. + * 2) We might have to acquire the read lock below even if it wouldn't be + * necessary anymore due to another thread concurrently unregistering a + * logger or reducing the level. */ + if (skip_level(&this->max_level[group], level) && + skip_level(&this->max_vlevel[group], level)) + { + return; + } + this->log_lock->read_lock(this->log_lock); loggers = this->loggers[group];