Merge branch 'wpk-atomic-counters-quota' into 'master'

Atomic counters and quota

See merge request isc-projects/bind9!1389

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]

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) {

lib/isc/counter.c

@@ -16,6 +16,7 @@
 #include <stdbool.h>
 #include <stddef.h>
 
+#include <isc/atomic.h>
 #include <isc/counter.h>
 #include <isc/magic.h>
 #include <isc/mem.h>
@@ -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);
+	}
 }

lib/isc/include/isc/atomic.h

@@ -16,3 +16,12 @@
 #else
 #include <isc/stdatomic.h>
 #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)

lib/isc/include/isc/quota.h

@@ -30,6 +30,7 @@
  *** Imports.
  ***/
 
+#include <isc/atomic.h>
 #include <isc/lang.h>
 #include <isc/mutex.h>
 #include <isc/types.h>
@@ -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);
 /*%<

lib/isc/include/isc/stdatomic.h

@@ -11,6 +11,8 @@
 
 #pragma once
 
+#include <inttypes.h>
+
 #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)

lib/isc/quota.c

@@ -16,62 +16,73 @@
 
 #include <stddef.h>
 
+#include <isc/atomic.h>
 #include <isc/quota.h>
 #include <isc/util.h>
 
+
 void
-isc_quota_init(isc_quota_t *quota, int max) {
-	quota->max = max;
-	quota->used = 0;
-	quota->soft = 0;
-	isc_mutex_init(&quota->lock);
+isc_quota_init(isc_quota_t *quota, unsigned int max) {
+	atomic_store(&quota->__max, max);
+	atomic_store(&quota->__used, 0);
+	atomic_store(&quota->__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(&quota->lock);
+	INSIST(atomic_load(&quota->__used) == 0);
+	atomic_store(&quota->__max, 0);
+	atomic_store(&quota->__used, 0);
+	atomic_store(&quota->__soft, 0);
 }
 
 void
-isc_quota_soft(isc_quota_t *quota, int soft) {
-	LOCK(&quota->lock);
-	quota->soft = soft;
-	UNLOCK(&quota->lock);
+isc_quota_soft(isc_quota_t *quota, unsigned int soft) {
+	atomic_store(&quota->__soft, soft);
 }
 
 void
-isc_quota_max(isc_quota_t *quota, int max) {
-	LOCK(&quota->lock);
-	quota->max = max;
-	UNLOCK(&quota->lock);
+isc_quota_max(isc_quota_t *quota, unsigned int max) {
+	atomic_store(&quota->__max, max);
+}
+
+unsigned int
+isc_quota_getmax(isc_quota_t *quota) {
+	return (atomic_load_relaxed(&quota->__max));
+}
+
+unsigned int
+isc_quota_getsoft(isc_quota_t *quota) {
+	return (atomic_load_relaxed(&quota->__soft));
+}
+
+unsigned int
+isc_quota_getused(isc_quota_t *quota) {
+	return (atomic_load_relaxed(&quota->__used));
 }
 
 isc_result_t
 isc_quota_reserve(isc_quota_t *quota) {
 	isc_result_t result;
-	LOCK(&quota->lock);
-	if (quota->max == 0 || quota->used < quota->max) {
-		if (quota->soft == 0 || quota->used < quota->soft)
+	uint32_t max = atomic_load(&quota->__max);
+	uint32_t soft = atomic_load(&quota->__soft);
+	uint32_t used = atomic_fetch_add(&quota->__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(&quota->__used, 1) > 0);
 		result = ISC_R_QUOTA;
-	UNLOCK(&quota->lock);
+	}
 	return (result);
 }
 
 void
 isc_quota_release(isc_quota_t *quota) {
-	LOCK(&quota->lock);
-	INSIST(quota->used > 0);
-	quota->used--;
-	UNLOCK(&quota->lock);
+	INSIST(atomic_fetch_sub(&quota->__used, 1) > 0);
 }
 
 isc_result_t

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

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);
 		}