From 814bfa23b208b0fd35e47047f0f012de8d6d599e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Witold=20Kr=C4=99cicki?= Date: Mon, 21 Jan 2019 09:32:36 +0100 Subject: [PATCH 1/4] Add atomic_store_relaxed and atomic_load_relaxed macros to isc/atomic.h, fix issues in isc/stdatomic.h --- lib/isc/include/isc/atomic.h | 9 +++++++++ lib/isc/include/isc/stdatomic.h | 10 ++++++---- 2 files changed, 15 insertions(+), 4 deletions(-) diff --git a/lib/isc/include/isc/atomic.h b/lib/isc/include/isc/atomic.h index d6f95de111..893f5526fd 100644 --- a/lib/isc/include/isc/atomic.h +++ b/lib/isc/include/isc/atomic.h @@ -16,3 +16,12 @@ #else #include #endif + +/* + * We define a few additional macros to make things easier + */ + +#define atomic_store_relaxed(o, v) atomic_store_explicit((o), \ + (v), \ + memory_order_relaxed) +#define atomic_load_relaxed(o) atomic_load_explicit((o), memory_order_relaxed) diff --git a/lib/isc/include/isc/stdatomic.h b/lib/isc/include/isc/stdatomic.h index 723ed1e5a8..e85027855e 100644 --- a/lib/isc/include/isc/stdatomic.h +++ b/lib/isc/include/isc/stdatomic.h @@ -11,6 +11,8 @@ #pragma once +#include + #if !defined(__has_feature) #define __has_feature(x) 0 #endif @@ -134,11 +136,11 @@ typedef uint_fast64_t atomic_uint_fast64_t; #define atomic_load(obj) \ atomic_load_explicit(obj, memory_order_seq_cst) -#define atomic_store(obj) \ - atomic_store_explicit(obj, memory_order_seq_cst) -#define atomic_fetch_add(obj) \ +#define atomic_store(obj, arg) \ + atomic_store_explicit(obj, arg, memory_order_seq_cst) +#define atomic_fetch_add(obj, arg) \ atomic_fetch_add_explicit(obj, arg, memory_order_seq_cst) -#define atomic_fetch_sub(obj) \ +#define atomic_fetch_sub(obj, arg) \ atomic_fetch_sub_explicit(obj, arg, memory_order_seq_cst) #define atomic_compare_exchange_strong(obj, expected, desired) \ atomic_compare_exchange_strong_explicit(obj, expected, desired, memory_order_seq_cst, memory_order_seq_cst) From 0af500a2c5c2bdc70f06766ed21dde5264d62395 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Witold=20Kr=C4=99cicki?= Date: Thu, 24 Jan 2019 12:01:11 +0100 Subject: [PATCH 2/4] - Make isc_quota use atomics instead of locks - Use getters for isc_quota parameters, make fields private - Fix a potential data race with recursion clients limits logging --- bin/named/server.c | 24 +++++++------ lib/isc/include/isc/quota.h | 33 ++++++++++++++---- lib/isc/quota.c | 69 +++++++++++++++++++++---------------- lib/isc/win32/libisc.def.in | 3 ++ lib/ns/query.c | 43 +++++++++++------------ 5 files changed, 103 insertions(+), 69 deletions(-) diff --git a/bin/named/server.c b/bin/named/server.c index ce048544d3..1bd098a228 100644 --- a/bin/named/server.c +++ b/bin/named/server.c @@ -8022,6 +8022,7 @@ load_configuration(const char *filename, named_server_t *server, ns_altsecretlist_t altsecrets, tmpaltsecrets; unsigned int maxsocks; uint32_t softquota = 0; + uint32_t max; unsigned int initial, idle, keepalive, advertised; dns_aclenv_t *env = ns_interfacemgr_getaclenv(named_g_server->interfacemgr); @@ -8214,20 +8215,20 @@ load_configuration(const char *filename, named_server_t *server, configure_server_quota(maps, "recursive-clients", &server->sctx->recursionquota); - if (server->sctx->recursionquota.max > 1000) { - int margin = ISC_MAX(100, named_g_cpus + 1); - if (margin > server->sctx->recursionquota.max - 100) { + max = isc_quota_getmax(&server->sctx->recursionquota); + if (max > 1000) { + unsigned margin = ISC_MAX(100, named_g_cpus + 1); + if (margin + 100 > max) { isc_log_write(named_g_lctx, NAMED_LOGCATEGORY_GENERAL, NAMED_LOGMODULE_SERVER, ISC_LOG_ERROR, "'recursive-clients %d' too low when " "running with %d worker threads", - server->sctx->recursionquota.max, - named_g_cpus); + max, named_g_cpus); CHECK(ISC_R_RANGE); } - softquota = server->sctx->recursionquota.max - margin; + softquota = max - margin; } else { - softquota = (server->sctx->recursionquota.max * 90) / 100; + softquota = (max * 90) / 100; } isc_quota_soft(&server->sctx->recursionquota, softquota); @@ -11452,13 +11453,14 @@ named_server_status(named_server_t *server, isc_buffer_t **text) { CHECK(putstr(text, line)); snprintf(line, sizeof(line), "recursive clients: %d/%d/%d\n", - server->sctx->recursionquota.used, - server->sctx->recursionquota.soft, - server->sctx->recursionquota.max); + isc_quota_getused(&server->sctx->recursionquota), + isc_quota_getsoft(&server->sctx->recursionquota), + isc_quota_getmax(&server->sctx->recursionquota)); CHECK(putstr(text, line)); snprintf(line, sizeof(line), "tcp clients: %d/%d\n", - server->sctx->tcpquota.used, server->sctx->tcpquota.max); + isc_quota_getused(&server->sctx->tcpquota), + isc_quota_getmax(&server->sctx->tcpquota)); CHECK(putstr(text, line)); if (server->reload_in_progress) { diff --git a/lib/isc/include/isc/quota.h b/lib/isc/include/isc/quota.h index e61e0e8834..4addbb8b23 100644 --- a/lib/isc/include/isc/quota.h +++ b/lib/isc/include/isc/quota.h @@ -30,6 +30,7 @@ *** Imports. ***/ +#include #include #include #include @@ -42,14 +43,14 @@ ISC_LANG_BEGINDECLS /*% isc_quota structure */ struct isc_quota { - isc_mutex_t lock; /*%< Locked by lock. */ - int max; - int used; - int soft; + atomic_uint_fast32_t __max; + atomic_uint_fast32_t __used; + atomic_uint_fast32_t __soft; }; + void -isc_quota_init(isc_quota_t *quota, int max); +isc_quota_init(isc_quota_t *quota, unsigned int max); /*%< * Initialize a quota object. */ @@ -61,17 +62,35 @@ isc_quota_destroy(isc_quota_t *quota); */ void -isc_quota_soft(isc_quota_t *quota, int soft); +isc_quota_soft(isc_quota_t *quota, unsigned int soft); /*%< * Set a soft quota. */ void -isc_quota_max(isc_quota_t *quota, int max); +isc_quota_max(isc_quota_t *quota, unsigned int max); /*%< * Re-set a maximum quota. */ +unsigned int +isc_quota_getmax(isc_quota_t *quota); +/*%< + * Get the maximum quota. + */ + +unsigned int +isc_quota_getsoft(isc_quota_t *quota); +/*%< + * Get the soft quota. + */ + +unsigned int +isc_quota_getused(isc_quota_t *quota); +/*%< + * Get the current usage of quota. + */ + isc_result_t isc_quota_reserve(isc_quota_t *quota); /*%< diff --git a/lib/isc/quota.c b/lib/isc/quota.c index 803bd4594c..edc9899ecb 100644 --- a/lib/isc/quota.c +++ b/lib/isc/quota.c @@ -16,62 +16,73 @@ #include +#include #include #include + void -isc_quota_init(isc_quota_t *quota, int max) { - quota->max = max; - quota->used = 0; - quota->soft = 0; - isc_mutex_init("a->lock); +isc_quota_init(isc_quota_t *quota, unsigned int max) { + atomic_store("a->__max, max); + atomic_store("a->__used, 0); + atomic_store("a->__soft, 0); } void isc_quota_destroy(isc_quota_t *quota) { - INSIST(quota->used == 0); - quota->max = 0; - quota->used = 0; - quota->soft = 0; - isc_mutex_destroy("a->lock); + INSIST(atomic_load("a->__used) == 0); + atomic_store("a->__max, 0); + atomic_store("a->__used, 0); + atomic_store("a->__soft, 0); } void -isc_quota_soft(isc_quota_t *quota, int soft) { - LOCK("a->lock); - quota->soft = soft; - UNLOCK("a->lock); +isc_quota_soft(isc_quota_t *quota, unsigned int soft) { + atomic_store("a->__soft, soft); } void -isc_quota_max(isc_quota_t *quota, int max) { - LOCK("a->lock); - quota->max = max; - UNLOCK("a->lock); +isc_quota_max(isc_quota_t *quota, unsigned int max) { + atomic_store("a->__max, max); +} + +unsigned int +isc_quota_getmax(isc_quota_t *quota) { + return (atomic_load_relaxed("a->__max)); +} + +unsigned int +isc_quota_getsoft(isc_quota_t *quota) { + return (atomic_load_relaxed("a->__soft)); +} + +unsigned int +isc_quota_getused(isc_quota_t *quota) { + return (atomic_load_relaxed("a->__used)); } isc_result_t isc_quota_reserve(isc_quota_t *quota) { isc_result_t result; - LOCK("a->lock); - if (quota->max == 0 || quota->used < quota->max) { - if (quota->soft == 0 || quota->used < quota->soft) + uint32_t max = atomic_load("a->__max); + uint32_t soft = atomic_load("a->__soft); + uint32_t used = atomic_fetch_add("a->__used, 1); + if (max == 0 || used < max) { + if (soft == 0 || used < soft) { result = ISC_R_SUCCESS; - else + } else { result = ISC_R_SOFTQUOTA; - quota->used++; - } else + } + } else { + INSIST(atomic_fetch_sub("a->__used, 1) > 0); result = ISC_R_QUOTA; - UNLOCK("a->lock); + } return (result); } void isc_quota_release(isc_quota_t *quota) { - LOCK("a->lock); - INSIST(quota->used > 0); - quota->used--; - UNLOCK("a->lock); + INSIST(atomic_fetch_sub("a->__used, 1) > 0); } isc_result_t diff --git a/lib/isc/win32/libisc.def.in b/lib/isc/win32/libisc.def.in index 5f78edae65..4cec81fca1 100644 --- a/lib/isc/win32/libisc.def.in +++ b/lib/isc/win32/libisc.def.in @@ -449,6 +449,9 @@ isc_portset_removerange isc_quota_attach isc_quota_destroy isc_quota_detach +isc_quota_getmax +isc_quota_getsoft +isc_quota_getused isc_quota_init isc_quota_max isc_quota_release diff --git a/lib/ns/query.c b/lib/ns/query.c index 1d02bd49c8..0798e2c817 100644 --- a/lib/ns/query.c +++ b/lib/ns/query.c @@ -5589,38 +5589,37 @@ ns_query_recurse(ns_client_t *client, dns_rdatatype_t qtype, dns_name_t *qname, ns_statscounter_recursclients); if (result == ISC_R_SOFTQUOTA) { - static isc_stdtime_t last = 0; + static atomic_uint_fast32_t last = 0; isc_stdtime_t now; isc_stdtime_get(&now); - if (now != last) { - last = now; + if (now != atomic_load_relaxed(&last)) { + atomic_store_relaxed(&last, now); ns_client_log(client, NS_LOGCATEGORY_CLIENT, - NS_LOGMODULE_QUERY, - ISC_LOG_WARNING, - "recursive-clients soft limit " - "exceeded (%d/%d/%d), " - "aborting oldest query", - client->recursionquota->used, - client->recursionquota->soft, - client->recursionquota->max); + NS_LOGMODULE_QUERY, ISC_LOG_WARNING, + "recursive-clients soft limit " + "exceeded (%u/%u/%u), " + "aborting oldest query", + isc_quota_getused(client->recursionquota), + isc_quota_getsoft(client->recursionquota), + isc_quota_getmax(client->recursionquota)); } ns_client_killoldestquery(client); result = ISC_R_SUCCESS; } else if (result == ISC_R_QUOTA) { - static isc_stdtime_t last = 0; + static atomic_uint_fast32_t last = 0; isc_stdtime_t now; isc_stdtime_get(&now); - if (now != last) { - last = now; + if (now != atomic_load_relaxed(&last)) { + ns_server_t *sctx = client->sctx; + atomic_store_relaxed(&last, now); ns_client_log(client, NS_LOGCATEGORY_CLIENT, - NS_LOGMODULE_QUERY, - ISC_LOG_WARNING, - "no more recursive clients " - "(%d/%d/%d): %s", - client->sctx->recursionquota.used, - client->sctx->recursionquota.soft, - client->sctx->recursionquota.max, - isc_result_totext(result)); + NS_LOGMODULE_QUERY, ISC_LOG_WARNING, + "no more recursive clients " + "(%u/%u/%u): %s", + isc_quota_getused(&sctx->recursionquota), + isc_quota_getsoft(&sctx->recursionquota), + isc_quota_getmax(&sctx->recursionquota), + isc_result_totext(result)); } ns_client_killoldestquery(client); } From 42d9a536a735c6e12a02ecb682e83ad310e34d93 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Witold=20Kr=C4=99cicki?= Date: Thu, 24 Jan 2019 12:05:29 +0100 Subject: [PATCH 3/4] Make isc_counter use atomics instead of locks --- lib/isc/counter.c | 49 ++++++++++++++++++----------------------------- 1 file changed, 19 insertions(+), 30 deletions(-) diff --git a/lib/isc/counter.c b/lib/isc/counter.c index 64defaaf83..5dbf93e05d 100644 --- a/lib/isc/counter.c +++ b/lib/isc/counter.c @@ -16,6 +16,7 @@ #include #include +#include #include #include #include @@ -27,10 +28,9 @@ struct isc_counter { unsigned int magic; isc_mem_t *mctx; - isc_mutex_t lock; - unsigned int references; - unsigned int limit; - unsigned int used; + atomic_uint_fast32_t references; + atomic_uint_fast32_t limit; + atomic_uint_fast32_t used; }; isc_result_t @@ -43,14 +43,12 @@ isc_counter_create(isc_mem_t *mctx, int limit, isc_counter_t **counterp) { if (counter == NULL) return (ISC_R_NOMEMORY); - isc_mutex_init(&counter->lock); - counter->mctx = NULL; isc_mem_attach(mctx, &counter->mctx); - counter->references = 1; - counter->limit = limit; - counter->used = 0; + atomic_store(&counter->references, 1); + atomic_store(&counter->limit, limit); + atomic_store(&counter->used, 0); counter->magic = COUNTER_MAGIC; *counterp = counter; @@ -61,11 +59,11 @@ isc_result_t isc_counter_increment(isc_counter_t *counter) { isc_result_t result = ISC_R_SUCCESS; - LOCK(&counter->lock); - counter->used++; - if (counter->limit != 0 && counter->used >= counter->limit) + uint32_t used = atomic_fetch_add(&counter->used, 1) + 1; + if (atomic_load(&counter->limit) != 0 && + used >= atomic_load(&counter->limit)) { result = ISC_R_QUOTA; - UNLOCK(&counter->lock); + } return (result); } @@ -74,16 +72,14 @@ unsigned int isc_counter_used(isc_counter_t *counter) { REQUIRE(VALID_COUNTER(counter)); - return (counter->used); + return (atomic_load(&counter->used)); } void isc_counter_setlimit(isc_counter_t *counter, int limit) { REQUIRE(VALID_COUNTER(counter)); - LOCK(&counter->lock); - counter->limit = limit; - UNLOCK(&counter->lock); + atomic_store(&counter->limit, limit); } void @@ -91,10 +87,7 @@ isc_counter_attach(isc_counter_t *source, isc_counter_t **targetp) { REQUIRE(VALID_COUNTER(source)); REQUIRE(targetp != NULL && *targetp == NULL); - LOCK(&source->lock); - source->references++; - INSIST(source->references > 0); - UNLOCK(&source->lock); + INSIST(atomic_fetch_add(&source->references, 1) > 0); *targetp = source; } @@ -102,14 +95,13 @@ isc_counter_attach(isc_counter_t *source, isc_counter_t **targetp) { static void destroy(isc_counter_t *counter) { counter->magic = 0; - isc_mutex_destroy(&counter->lock); isc_mem_putanddetach(&counter->mctx, counter, sizeof(*counter)); } void isc_counter_detach(isc_counter_t **counterp) { isc_counter_t *counter; - bool want_destroy = false; + uint32_t oldrefs; REQUIRE(counterp != NULL && *counterp != NULL); counter = *counterp; @@ -117,13 +109,10 @@ isc_counter_detach(isc_counter_t **counterp) { *counterp = NULL; - LOCK(&counter->lock); - INSIST(counter->references > 0); - counter->references--; - if (counter->references == 0) - want_destroy = true; - UNLOCK(&counter->lock); + oldrefs = atomic_fetch_sub(&counter->references, 1); + INSIST(oldrefs > 0); - if (want_destroy) + if (oldrefs == 1) { destroy(counter); + } } From d314e45cc3a68c754a5d4bd853db47b32a1148d5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Witold=20Kr=C4=99cicki?= Date: Fri, 25 Jan 2019 09:35:13 +0100 Subject: [PATCH 4/4] CHANGES entry --- CHANGES | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHANGES b/CHANGES index 2e1f56c1f3..3981dcb03d 100644 --- a/CHANGES +++ b/CHANGES @@ -1,3 +1,6 @@ +5145. [func] Use atomics instead of locked variables for isc_quota + and isc_counter. [GL !1389] + 5144. [bug] dig now returns a non-zero exit code when a TCP connection is prematurely closed by a peer more than once for the same lookup. [GL #820]