Deprecated clients-per-query in favor of max-clients-per-query

Remove the complicated mechanism that would slowly ramp-up the number of allowed clients per query up to max-clients-per-query. The server can withstand much higher load without any problems now.
2023-02-01 09:22:40 +01:00
parent d6c3d9e04a
commit 1fe3e2b06c
7 changed files with 33 additions and 158 deletions
@@ -149,7 +149,6 @@ options {\n\
 	check-names secondary warn;\n\
 	check-spf warn;\n\
 	check-svcb yes;\n\
-	clients-per-query 10;\n\
 	dnssec-accept-expired no;\n\
 	dnssec-validation " VALIDATION_DEFAULT "; \n"
 #ifdef HAVE_DNSTAP
@@ -4100,7 +4100,6 @@ configure_view(dns_view_t *view, dns_viewlist_t *viewlist, cfg_obj_t *config,
 	uint32_t maxbits;
 	unsigned int resopts = 0;
 	dns_zone_t *zone = NULL;
-	uint32_t max_clients_per_query;
 	bool empty_zones_enable;
 	const cfg_obj_t *disablelist = NULL;
 	isc_stats_t *resstats = NULL;
@@ -5502,13 +5501,7 @@ configure_view(dns_view_t *view, dns_viewlist_t *viewlist, cfg_obj_t *config,
 	obj = NULL;
 	result = named_config_get(maps, "max-clients-per-query", &obj);
 	INSIST(result == ISC_R_SUCCESS);
-	max_clients_per_query = cfg_obj_asuint32(obj);
-
-	obj = NULL;
-	result = named_config_get(maps, "clients-per-query", &obj);
-	INSIST(result == ISC_R_SUCCESS);
-	dns_resolver_setclientsperquery(view->resolver, cfg_obj_asuint32(obj),
-					max_clients_per_query);
+	dns_resolver_setclientsperquery(view->resolver, cfg_obj_asuint32(obj));

 	obj = NULL;
 	result = named_config_get(maps, "max-recursion-depth", &obj);
@@ -3666,16 +3666,9 @@ system.
   server accepts. The default is ``150``.

 .. namedconf:statement:: clients-per-query
-   :tags: server
-   :short: Sets the initial minimum number of simultaneous recursive clients accepted by the server for any given query before the server drops additional clients.
+   :tags: deprecated

-   This sets the initial value (minimum) number of simultaneous recursive clients
-   for any given query (<qname,qtype,qclass>) that the server accepts before
-   dropping additional clents. :iscman:`named` attempts to self-tune this
-   value and changes are logged. The default value is 10.
-
-   The chosen value should reflect how many queries come in for a given name
-   in the time it takes to resolve that name.
+   This option is deprecated and no longer has any effect, see :any:`max-clients-per-query`.

 .. namedconf:statement:: max-clients-per-query
   :tags: server
@@ -3685,16 +3678,15 @@ system.
   given query (<qname,qtype,qclass>) that the server accepts before
   dropping additional clients.

-   If the number of queries exceeds :any:`clients-per-query`, :iscman:`named`
+   If the number of queries exceeds :any:`max-clients-per-query`, :iscman:`named`
   assumes that it is dealing with a non-responsive zone and drops additional
-   queries. If it gets a response after dropping queries, it raises the estimate,
-   up to a limit of :any:`max-clients-per-query`. The estimate is then lowered
-   after 20 minutes if it has remained unchanged.
+   queries.

   If :any:`max-clients-per-query` is set to zero, there is no upper bound, other
-   than that imposed by :any:`recursive-clients`. If :any:`clients-per-query` is
-   set to zero, :any:`max-clients-per-query` no longer applies and there is no
-   upper bound, other than that imposed by :any:`recursive-clients`.
+   than that imposed by :any:`recursive-clients`.
+
+   The chosen value should reflect how many queries come in for a given name
+   in the time it takes to resolve that name.  The default is 100.

 .. namedconf:statement:: fetches-per-zone
   :tags: server, query
@@ -7824,7 +7816,7 @@ Name Server Statistics Counters
    This indicates the number of queries which the server attempted to recurse but for which it discovered an existing query with the same IP address, port, query ID, name, type, and class already being processed. This corresponds to the ``duplicate`` counter of previous versions of BIND 9.

 ``QryDropped``
-    This indicates the number of recursive queries for which the server discovered an excessive number of existing recursive queries for the same name, type, and class, and which were subsequently dropped. This is the number of dropped queries due to the reason explained with the :any:`clients-per-query` and :any:`max-clients-per-query` options. This corresponds to the ``dropped`` counter of previous versions of BIND 9.
+    This indicates the number of recursive queries for which the server discovered an excessive number of existing recursive queries for the same name, type, and class, and which were subsequently dropped. This is the number of dropped queries due to the reason explained with the :any:`max-clients-per-query` option. This corresponds to the ``dropped`` counter of previous versions of BIND 9.

 ``QryFailure``
    This indicates the number of query failures. This corresponds to the ``failure`` counter of previous versions of BIND 9. Note: this counter is provided mainly for backward compatibility with previous versions; normally, more fine-grained counters such as ``AuthQryRej`` and ``RecQryRej`` that would also fall into this counter are provided, so this counter is not of much interest in practice.
@@ -90,7 +90,7 @@ options {
 	check-srv-cname ( fail | warn | ignore );
 	check-svcb <boolean>;
 	check-wildcard <boolean>;
-	clients-per-query <integer>;
+	clients-per-query <integer>; // deprecated
 	cookie-algorithm ( aes | siphash24 );
 	cookie-secret <string>; // may occur multiple times
 	deny-answer-addresses { <address_match_element>; ... } [ except-from { <string>; ... } ];
@@ -392,7 +392,7 @@ view <string> [ <class> ] {
 	check-srv-cname ( fail | warn | ignore );
 	check-svcb <boolean>;
 	check-wildcard <boolean>;
-	clients-per-query <integer>;
+	clients-per-query <integer>; // deprecated
 	deny-answer-addresses { <address_match_element>; ... } [ except-from { <string>; ... } ];
 	deny-answer-aliases { <string>; ... } [ except-from { <string>; ... } ];
 	dialup ( notify | notify-passive | passive | refresh | <boolean> );
@@ -533,8 +533,8 @@ dns_resolver_gettimeout(dns_resolver_t *resolver);
 */

 void
-dns_resolver_setclientsperquery(dns_resolver_t *resolver, uint32_t min,
-				uint32_t max);
+dns_resolver_setclientsperquery(dns_resolver_t *resolver,
+				uint_fast32_t	clients);
 void
 dns_resolver_setfetchesperzone(dns_resolver_t *resolver, uint32_t clients);

@@ -542,8 +542,8 @@ uint32_t
 dns_resolver_getfetchesperzone(dns_resolver_t *resolver);

 void
-dns_resolver_getclientsperquery(dns_resolver_t *resolver, uint32_t *cur,
-				uint32_t *min, uint32_t *max);
+dns_resolver_getclientsperquery(dns_resolver_t *resolver,
+				uint_fast32_t  *clients);

 bool
 dns_resolver_getzeronosoattl(dns_resolver_t *resolver);
@@ -567,9 +567,6 @@ struct dns_resolver {
 	dns_rbt_t *algorithms;
 	dns_rbt_t *digests;
 	dns_rbt_t *mustbesecure;
-	unsigned int spillatmax;
-	unsigned int spillatmin;
-	isc_timer_t *spillattimer;
 	bool zero_no_soa_ttl;
 	unsigned int query_timeout;
 	unsigned int maxdepth;
@@ -589,7 +586,7 @@ struct dns_resolver {
 	atomic_bool priming;

 	/* Locked by lock. */
-	unsigned int spillat; /* clients-per-query */
+	atomic_uint_fast32_t spillat; /* clients-per-query */

 	dns_badcache_t *badcache; /* Bad cache. */

@@ -1714,19 +1711,11 @@ fcount_decr(fetchctx_t *fctx) {
 	}
 }

-static void
-spillattimer_countdown(void *arg);
-
 static void
 fctx_sendevents(fetchctx_t *fctx, isc_result_t result) {
 	dns_fetchevent_t *event, *next_event;
 	isc_task_t *task;
-	unsigned int count = 0;
-	bool logit = false;
 	isc_time_t now;
-	unsigned int old_spillat;
-	unsigned int new_spillat = 0; /* initialized to silence
-				       * compiler warnings */

 	/*
 	 * Caller must be holding the fctx lock.
@@ -1784,51 +1773,8 @@ fctx_sendevents(fetchctx_t *fctx, isc_result_t result) {

 		FCTXTRACE("event");
 		isc_task_sendanddetach(&task, ISC_EVENT_PTR(&event));
-		count++;
 	}
 	UNLOCK(&fctx->lock);
-
-	if (HAVE_ANSWER(fctx) && fctx->spilled &&
-	    (count < fctx->res->spillatmax || fctx->res->spillatmax == 0))
-	{
-		LOCK(&fctx->res->lock);
-		if (count == fctx->res->spillat &&
-		    !atomic_load_acquire(&fctx->res->exiting))
-		{
-			old_spillat = fctx->res->spillat;
-			fctx->res->spillat += 5;
-			if (fctx->res->spillat > fctx->res->spillatmax &&
-			    fctx->res->spillatmax != 0)
-			{
-				fctx->res->spillat = fctx->res->spillatmax;
-			}
-			new_spillat = fctx->res->spillat;
-			if (new_spillat != old_spillat) {
-				logit = true;
-			}
-
-			/* Timer not running */
-			if (fctx->res->spillattimer == NULL) {
-				isc_interval_t i;
-
-				isc_timer_create(
-					isc_loop_current(fctx->res->loopmgr),
-					spillattimer_countdown, fctx->res,
-					&fctx->res->spillattimer);
-
-				isc_interval_set(&i, 20 * 60, 0);
-				isc_timer_start(fctx->res->spillattimer,
-						isc_timertype_ticker, &i);
-			}
-		}
-		UNLOCK(&fctx->res->lock);
-		if (logit) {
-			isc_log_write(dns_lctx, DNS_LOGCATEGORY_RESOLVER,
-				      DNS_LOGMODULE_RESOLVER, ISC_LOG_NOTICE,
-				      "clients-per-query increased to %u",
-				      new_spillat);
-		}
-	}
 }

 static bool
@@ -10091,34 +10037,6 @@ dns_resolver__destroy(dns_resolver_t *res) {
 	isc_mem_putanddetach(&res->mctx, res, sizeof(*res));
 }

-static void
-spillattimer_countdown(void *arg) {
-	dns_resolver_t *res = (dns_resolver_t *)arg;
-	unsigned int spillat = 0;
-
-	REQUIRE(VALID_RESOLVER(res));
-
-	if (atomic_load(&res->exiting)) {
-		isc_timer_destroy(&res->spillattimer);
-		return;
-	}
-
-	LOCK(&res->lock);
-	INSIST(!atomic_load_acquire(&res->exiting));
-	if (res->spillat > res->spillatmin) {
-		spillat = --res->spillat;
-	}
-	if (res->spillat <= res->spillatmin) {
-		isc_timer_destroy(&res->spillattimer);
-	}
-	UNLOCK(&res->lock);
-	if (spillat > 0) {
-		isc_log_write(dns_lctx, DNS_LOGCATEGORY_RESOLVER,
-			      DNS_LOGMODULE_RESOLVER, ISC_LOG_NOTICE,
-			      "clients-per-query decreased to %u", spillat);
-	}
-}
-
 isc_result_t
 dns_resolver_create(dns_view_t *view, isc_loopmgr_t *loopmgr,
 		    isc_taskmgr_t *taskmgr, unsigned int ndisp, isc_nm_t *nm,
@@ -10151,9 +10069,6 @@ dns_resolver_create(dns_view_t *view, isc_loopmgr_t *loopmgr,
 		.dispatchmgr = dispatchmgr,
 		.options = options,
 		.tlsctx_cache = tlsctx_cache,
-		.spillatmin = 10,
-		.spillat = 10,
-		.spillatmax = 100,
 		.retryinterval = 10000,
 		.nonbackofftries = 3,
 		.query_timeout = DEFAULT_QUERY_TIMEOUT,
@@ -10163,6 +10078,8 @@ dns_resolver_create(dns_view_t *view, isc_loopmgr_t *loopmgr,
 		.alternates = ISC_LIST_INITIALIZER,
 	};

+	isc_refcount_init(&res->spillat, 100);
+
 	dns_view_weakattach(view, &res->view);
 	isc_mem_attach(view->mctx, &res->mctx);

@@ -10381,12 +10298,6 @@ dns_resolver_shutdown(dns_resolver_t *res) {
 		}
 		isc_hashmap_iter_destroy(&it);
 		UNLOCK(&res->fctxs_lock);
-
-		LOCK(&res->lock);
-		if (res->spillattimer != NULL) {
-			isc_timer_async_destroy(&res->spillattimer);
-		}
-		UNLOCK(&res->lock);
 	}
 }

@@ -10600,8 +10511,6 @@ dns_resolver_createfetch(dns_resolver_t *res, const dns_name_t *name,
 	isc_result_t result = ISC_R_SUCCESS;
 	bool new_fctx = false;
 	unsigned int count = 0;
-	unsigned int spillat;
-	unsigned int spillatmin;

 	UNUSED(forwarders);

@@ -10637,11 +10546,6 @@ dns_resolver_createfetch(dns_resolver_t *res, const dns_name_t *name,
 		 * we also would never match it again.
 		 */

-		LOCK(&res->lock);
-		spillat = res->spillat;
-		spillatmin = res->spillatmin;
-		UNLOCK(&res->lock);
-
 		result = get_attached_fctx(res, name, type, domain, nameservers,
 					   client, options, depth, qc, &fctx,
 					   &new_fctx);
@@ -10672,15 +10576,13 @@ dns_resolver_createfetch(dns_resolver_t *res, const dns_name_t *name,
 				count++;
 			}
 		}
-		if (count >= spillatmin && spillatmin != 0) {
+
+		uint_fast32_t spillat = atomic_load_relaxed(&res->spillat);
+		if (spillat != 0 && count >= spillat) {
 			INSIST(fctx != NULL);
-			if (count >= spillat) {
-				fctx->spilled = true;
-			}
-			if (fctx->spilled) {
-				result = DNS_R_DROP;
-				goto unlock;
-			}
+			fctx->spilled = true;
+			result = DNS_R_DROP;
+			goto unlock;
 		}
 	} else {
 		result = fctx_create(res, name, type, domain, nameservers,
@@ -11257,32 +11159,21 @@ unlock:
 }

 void
-dns_resolver_getclientsperquery(dns_resolver_t *resolver, uint32_t *cur,
-				uint32_t *min, uint32_t *max) {
+dns_resolver_getclientsperquery(dns_resolver_t *resolver,
+				uint_fast32_t *clients) {
 	REQUIRE(VALID_RESOLVER(resolver));

-	LOCK(&resolver->lock);
-	if (cur != NULL) {
-		*cur = resolver->spillat;
+	if (clients != NULL) {
+		*clients = atomic_load_relaxed(&resolver->spillat);
 	}
-	if (min != NULL) {
-		*min = resolver->spillatmin;
-	}
-	if (max != NULL) {
-		*max = resolver->spillatmax;
-	}
-	UNLOCK(&resolver->lock);
 }

 void
-dns_resolver_setclientsperquery(dns_resolver_t *resolver, uint32_t min,
-				uint32_t max) {
+dns_resolver_setclientsperquery(dns_resolver_t *resolver,
+				uint_fast32_t clients) {
 	REQUIRE(VALID_RESOLVER(resolver));

-	LOCK(&resolver->lock);
-	resolver->spillatmin = resolver->spillat = min;
-	resolver->spillatmax = max;
-	UNLOCK(&resolver->lock);
+	atomic_store_relaxed(&resolver->spillat, clients);
 }

 void
@@ -2040,7 +2040,7 @@ static cfg_clausedef_t view_clauses[] = {
 	{ "catalog-zones", &cfg_type_catz, 0 },
 	{ "check-names", &cfg_type_checknames, CFG_CLAUSEFLAG_MULTI },
 	{ "cleaning-interval", NULL, CFG_CLAUSEFLAG_ANCIENT },
-	{ "clients-per-query", &cfg_type_uint32, 0 },
+	{ "clients-per-query", &cfg_type_uint32, CFG_CLAUSEFLAG_DEPRECATED },
 	{ "deny-answer-addresses", &cfg_type_denyaddresses, 0 },
 	{ "deny-answer-aliases", &cfg_type_denyaliases, 0 },
 	{ "disable-algorithms", &cfg_type_disablealgorithm,