DROPME: Set recursive-clients to UINT32_MAX

When max-client-per-query value is reached, drop the first event, not
the last one.

bin/named/config.c

@@ -99,7 +99,7 @@ options {\n\
 			    "\
 	prefetch 2 9;\n\
 	recursing-file \"named.recursing\";\n\
-	recursive-clients 1000;\n\
+	recursive-clients 100000;\n\
 	request-nsid false;\n\
 	reserved-sockets 512;\n\
 	resolver-query-timeout 10;\n\
@@ -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

bin/named/server.c

@@ -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);
@@ -8593,22 +8586,11 @@ load_configuration(const char *filename, named_server_t *server,
 	configure_server_quota(maps, "update-quota", &server->sctx->updquota);
 
 	max = isc_quota_getmax(&server->sctx->recursionquota);
-	if (max > 1000) {
-		unsigned int 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",
-				      max, named_g_cpus);
-			result = ISC_R_RANGE;
-
-			goto cleanup_bindkeys_parser;
-		}
-		softquota = max - margin;
-	} else {
-		softquota = (max * 90) / 100;
+	if (max < UINT32_MAX) {
+		isc_quota_max(&server->sctx->recursionquota, UINT32_MAX);
+		max = UINT32_MAX;
 	}
+	softquota = (max * 90) / 100;
 
 	isc_quota_soft(&server->sctx->recursionquota, softquota);
 

doc/arm/reference.rst

@@ -3641,9 +3641,12 @@ system.
    :tags: query
    :short: Specifies the maximum number of concurrent recursive queries the server can perform.
 
-   This sets the maximum number (a "hard quota") of simultaneous recursive lookups
-   the server performs on behalf of clients. The default is
-   ``1000``. Because each recursing client uses a fair bit of memory (on
+   This sets the maximum number (a "hard quota") of simultaneous recursive
+   lookups the server performs on behalf of clients. The default is ``1000000``
+   (one million).  The minimum is ``1000``. Values below the minimum are
+   adjusted to the minimum value.
+
+   Because each recursing client uses a fair bit of memory (on
    the order of 20 kilobytes), the value of the :any:`recursive-clients`
    option may have to be decreased on hosts with limited memory.
 
@@ -3654,9 +3657,7 @@ system.
 
    A "soft quota" is also set. When this lower quota is exceeded,
    incoming requests are accepted, but for each one, a pending request
-   is dropped. If :any:`recursive-clients` is greater than 1000, the
-   soft quota is set to :any:`recursive-clients` minus 100; otherwise it is
-   set to 90% of :any:`recursive-clients`.
+   is dropped. The soft quota is set to 90% of :any:`recursive-clients`.
 
 .. namedconf:statement:: tcp-clients
    :tags: server
@@ -3666,16 +3667,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 +3679,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 +7817,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.

doc/misc/options

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

lib/dns/include/dns/resolver.h

@@ -533,18 +533,14 @@ 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);
 
 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);
-
 bool
 dns_resolver_getzeronosoattl(dns_resolver_t *resolver);
 

lib/dns/resolver.c

@@ -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. */
 
@@ -1715,18 +1712,51 @@ fcount_decr(fetchctx_t *fctx) {
 }
 
 static void
-spillattimer_countdown(void *arg);
+fctx_sendevent(fetchctx_t *fctx, dns_fetchevent_t *event, isc_result_t result) {
+	isc_task_t *task;
+
+	ISC_LIST_UNLINK(fctx->events, event, ev_link);
+	if (event->ev_type == DNS_EVENT_TRYSTALE) {
+		/*
+		 * Not applicable to TRY STALE events, this function is
+		 * called when the fetch has either completed or timed
+		 * out due to resolver-query-timeout being reached.
+		 */
+		isc_task_detach((isc_task_t **)&event->ev_sender);
+		isc_event_free((isc_event_t **)&event);
+		return;
+	}
+	task = event->ev_sender;
+	event->ev_sender = fctx;
+	event->vresult = fctx->vresult;
+	if (!HAVE_ANSWER(fctx)) {
+		event->result = result;
+	}
+
+	INSIST(event->result != ISC_R_SUCCESS ||
+	       dns_rdataset_isassociated(event->rdataset) ||
+	       fctx->type == dns_rdatatype_any ||
+	       fctx->type == dns_rdatatype_rrsig ||
+	       fctx->type == dns_rdatatype_sig);
+
+	/*
+	 * Negative results must be indicated in event->result.
+	 */
+	if (dns_rdataset_isassociated(event->rdataset) &&
+	    NEGATIVE(event->rdataset))
+	{
+		INSIST(event->result == DNS_R_NCACHENXDOMAIN ||
+		       event->result == DNS_R_NCACHENXRRSET);
+	}
+
+	FCTXTRACE("event");
+	isc_task_sendanddetach(&task, ISC_EVENT_PTR(&event));
+}
 
 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.
@@ -1748,87 +1778,10 @@ fctx_sendevents(fetchctx_t *fctx, isc_result_t result) {
 	     event = next_event)
 	{
 		next_event = ISC_LIST_NEXT(event, ev_link);
-		ISC_LIST_UNLINK(fctx->events, event, ev_link);
-		if (event->ev_type == DNS_EVENT_TRYSTALE) {
-			/*
-			 * Not applicable to TRY STALE events, this function is
-			 * called when the fetch has either completed or timed
-			 * out due to resolver-query-timeout being reached.
-			 */
-			isc_task_detach((isc_task_t **)&event->ev_sender);
-			isc_event_free((isc_event_t **)&event);
-			continue;
-		}
-		task = event->ev_sender;
-		event->ev_sender = fctx;
-		event->vresult = fctx->vresult;
-		if (!HAVE_ANSWER(fctx)) {
-			event->result = result;
-		}
 
-		INSIST(event->result != ISC_R_SUCCESS ||
-		       dns_rdataset_isassociated(event->rdataset) ||
-		       fctx->type == dns_rdatatype_any ||
-		       fctx->type == dns_rdatatype_rrsig ||
-		       fctx->type == dns_rdatatype_sig);
-
-		/*
-		 * Negative results must be indicated in event->result.
-		 */
-		if (dns_rdataset_isassociated(event->rdataset) &&
-		    NEGATIVE(event->rdataset))
-		{
-			INSIST(event->result == DNS_R_NCACHENXDOMAIN ||
-			       event->result == DNS_R_NCACHENXRRSET);
-		}
-
-		FCTXTRACE("event");
-		isc_task_sendanddetach(&task, ISC_EVENT_PTR(&event));
-		count++;
+		fctx_sendevent(fctx, event, result);
 	}
 	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 +10044,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 +10076,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 +10085,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 +10305,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 +10518,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 +10553,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 +10583,21 @@ dns_resolver_createfetch(dns_resolver_t *res, const dns_name_t *name,
 				count++;
 			}
 		}
-		if (count >= spillatmin && spillatmin != 0) {
-			INSIST(fctx != NULL);
-			if (count >= spillat) {
-				fctx->spilled = true;
+
+		uint_fast32_t spillat = atomic_load_relaxed(&res->spillat);
+		if (spillat != 0 && count >= spillat) {
+			dns_fetchevent_t *event = ISC_LIST_HEAD(fctx->events);
+			while (event != NULL && event->sigrdataset != NULL) {
+				event = ISC_LIST_NEXT(event, ev_link);
 			}
-			if (fctx->spilled) {
+
+			if (event != NULL) {
+				fctx_sendevent(fctx, event, DNS_R_DROP);
+			} else {
 				result = DNS_R_DROP;
 				goto unlock;
 			}
+			fctx->spilled = true;
 		}
 	} else {
 		result = fctx_create(res, name, type, domain, nameservers,
@@ -11257,32 +11174,11 @@ unlock:
 }
 
 void
-dns_resolver_getclientsperquery(dns_resolver_t *resolver, uint32_t *cur,
-				uint32_t *min, uint32_t *max) {
+dns_resolver_setclientsperquery(dns_resolver_t *resolver,
+				uint_fast32_t clients) {
 	REQUIRE(VALID_RESOLVER(resolver));
 
-	LOCK(&resolver->lock);
-	if (cur != NULL) {
-		*cur = 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) {
-	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

lib/isccfg/namedconf.c

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