From: Witold Kręcicki <wpk@isc.org>
Date: Thu, 24 Jan 2019 11:01:11 +0000 (+0100)
Subject: - Make isc_quota use atomics instead of locks
X-Git-Tag: v9.13.6~35^2~2
X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=0af500a2c5c2bdc70f06766ed21dde5264d62395;p=thirdparty%2Fbind9.git

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

diff --git a/bin/named/server.c b/bin/named/server.c
index ce048544d3b..1bd098a2281 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 e61e0e88343..4addbb8b238 100644
--- a/lib/isc/include/isc/quota.h
+++ b/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);
 /*%<
diff --git a/lib/isc/quota.c b/lib/isc/quota.c
index 803bd4594ce..edc9899ecb3 100644
--- a/lib/isc/quota.c
+++ b/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
diff --git a/lib/isc/win32/libisc.def.in b/lib/isc/win32/libisc.def.in
index 5f78edae652..4cec81fca15 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 1d02bd49c81..0798e2c817b 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);
 		}