Reduce the number of clientmgr objects created

Previously, as a way of reducing the contention between threads a clientmgr object would be created for each interface/IP address. We tasks being more strictly bound to netmgr workers, this is no longer needed and we can just create clientmgr object per worker queue (ncpus). Each clientmgr object than would have a single task and single memory context.
2025-08-31 06:25:31 +00:00 · 2021-05-22 18:12:11 +02:00
parent aad7856b8e
commit 28b65d8256
24 changed files with 264 additions and 217 deletions
--- a/lib/ns/client.c
+++ b/lib/ns/client.c
@@ -132,8 +132,6 @@ ns_client_dumpmessage(ns_client_t *client, const char *reason);
 static void
 compute_cookie(ns_client_t *client, uint32_t when, uint32_t nonce,
 	       const unsigned char *secret, isc_buffer_t *buf);
-static void
-get_clienttask(ns_clientmgr_t *manager, isc_task_t **taskp);

 void
 ns_client_recursing(ns_client_t *client) {
@@ -321,10 +319,10 @@ client_sendpkg(ns_client_t *client, isc_buffer_t *buffer) {
 void
 ns_client_sendraw(ns_client_t *client, dns_message_t *message) {
 	isc_result_t result;
-	unsigned char *data;
+	unsigned char *data = NULL;
 	isc_buffer_t buffer;
 	isc_region_t r;
-	isc_region_t *mr;
+	isc_region_t *mr = NULL;

 	REQUIRE(NS_CLIENT_VALID(client));

@@ -387,7 +385,7 @@ done:
 void
 ns_client_send(ns_client_t *client) {
 	isc_result_t result;
-	unsigned char *data;
+	unsigned char *data = NULL;
 	isc_buffer_t buffer = { .magic = 0 };
 	isc_region_t r;
 	dns_compress_t cctx;
@@ -396,7 +394,7 @@ ns_client_send(ns_client_t *client) {
 	unsigned int preferred_glue;
 	bool opt_included = false;
 	size_t respsize;
-	dns_aclenv_t *env;
+	dns_aclenv_t *env = NULL;
 #ifdef HAVE_DNSTAP
 	unsigned char zone[DNS_NAME_MAXWIRE];
 	dns_dtmsgtype_t dtmsgtype;
@@ -414,7 +412,7 @@ ns_client_send(ns_client_t *client) {
 	 * Delay the response according to the -T delay option
 	 */

-	env = ns_interfacemgr_getaclenv(client->manager->interface->mgr);
+	env = client->manager->aclenv;

 	CTRACE("send");

@@ -876,24 +874,24 @@ isc_result_t
 ns_client_addopt(ns_client_t *client, dns_message_t *message,
 		 dns_rdataset_t **opt) {
 	unsigned char ecs[ECS_SIZE];
-	char nsid[BUFSIZ], *nsidp;
+	char nsid[BUFSIZ], *nsidp = NULL;
 	unsigned char cookie[COOKIE_SIZE];
 	isc_result_t result;
-	dns_view_t *view;
-	dns_resolver_t *resolver;
+	dns_view_t *view = NULL;
+	dns_resolver_t *resolver = NULL;
 	uint16_t udpsize;
 	dns_ednsopt_t ednsopts[DNS_EDNSOPTIONS];
 	int count = 0;
 	unsigned int flags;
 	unsigned char expire[4];
 	unsigned char advtimo[2];
-	dns_aclenv_t *env;
+	dns_aclenv_t *env = NULL;

 	REQUIRE(NS_CLIENT_VALID(client));
 	REQUIRE(opt != NULL && *opt == NULL);
 	REQUIRE(message != NULL);

-	env = ns_interfacemgr_getaclenv(client->manager->interface->mgr);
+	env = client->manager->aclenv;
 	view = client->view;
 	resolver = (view != NULL) ? view->resolver : NULL;
 	if (resolver != NULL) {
@@ -1569,10 +1567,6 @@ ns__client_put_cb(void *client0) {
 	client->magic = 0;
 	client->shuttingdown = true;

-	if (client->manager != NULL) {
-		clientmgr_detach(&client->manager);
-	}
-
 	isc_mem_put(client->mctx, client->sendbuf, NS_CLIENT_SEND_BUFFER_SIZE);
 	if (client->opt != NULL) {
 		INSIST(dns_rdataset_isassociated(client->opt));
@@ -1582,6 +1576,10 @@ ns__client_put_cb(void *client0) {

 	dns_message_detach(&client->message);

+	if (client->manager != NULL) {
+		clientmgr_detach(&client->manager);
+	}
+
 	/*
 	 * Detaching the task must be done after unlinking from
 	 * the manager's lists because the manager accesses
@@ -1601,9 +1599,7 @@ ns__client_put_cb(void *client0) {
 		ns_server_detach(&client->sctx);
 	}

-	if (client->mctx != NULL) {
-		isc_mem_detach(&client->mctx);
-	}
+	isc_mem_detach(&client->mctx);
 }

 /*
@@ -1628,6 +1624,7 @@ ns__client_request(isc_nmhandle_t *handle, isc_result_t eresult,
 	bool notimp;
 	size_t reqsize;
 	dns_aclenv_t *env = NULL;
+	isc_sockaddr_t sockaddr;
 #ifdef HAVE_DNSTAP
 	dns_dtmsgtype_t dtmsgtype;
 #endif /* ifdef HAVE_DNSTAP */
@@ -1639,12 +1636,14 @@ ns__client_request(isc_nmhandle_t *handle, isc_result_t eresult,
 	client = isc_nmhandle_getdata(handle);
 	if (client == NULL) {
 		ns_interface_t *ifp = (ns_interface_t *)arg;
+		ns_clientmgr_t *clientmgr =
+			ns_interfacemgr_getclientmgr(ifp->mgr);

-		INSIST(VALID_MANAGER(ifp->clientmgr));
+		INSIST(VALID_MANAGER(clientmgr));

 		client = isc_nmhandle_getextra(handle);

-		result = ns__client_setup(client, ifp->clientmgr, true);
+		result = ns__client_setup(client, clientmgr, true);
 		if (result != ISC_R_SUCCESS) {
 			return;
 		}
@@ -1705,7 +1704,7 @@ ns__client_request(isc_nmhandle_t *handle, isc_result_t eresult,
 	}
 #endif /* if NS_CLIENT_DROPPORT */

-	env = ns_interfacemgr_getaclenv(client->manager->interface->mgr);
+	env = client->manager->aclenv;
 	if (client->sctx->blackholeacl != NULL &&
 	    (dns_acl_match(&netaddr, NULL, client->sctx->blackholeacl, env,
 			   &match, NULL) == ISC_R_SUCCESS) &&
@@ -1921,26 +1920,8 @@ ns__client_request(isc_nmhandle_t *handle, isc_result_t eresult,
 		return;
 	}

-	/*
-	 * Determine the destination address.  If the receiving interface is
-	 * bound to a specific address, we simply use it regardless of the
-	 * address family.  All IPv4 queries should fall into this case.
-	 * Otherwise, if this is a TCP query, get the address from the
-	 * receiving socket (this needs a system call and can be heavy).
-	 * For IPv6 UDP queries, we get this from the pktinfo structure (if
-	 * supported).
-	 *
-	 * If all the attempts fail (this can happen due to memory shortage,
-	 * etc), we regard this as an error for safety.
-	 */
-	if ((client->manager->interface->flags & NS_INTERFACEFLAG_ANYADDR) == 0)
-	{
-		isc_netaddr_fromsockaddr(&client->destaddr,
-					 &client->manager->interface->addr);
-	} else {
-		isc_sockaddr_t sockaddr = isc_nmhandle_localaddr(handle);
-		isc_netaddr_fromsockaddr(&client->destaddr, &sockaddr);
-	}
+	sockaddr = isc_nmhandle_localaddr(handle);
+	isc_netaddr_fromsockaddr(&client->destaddr, &sockaddr);

 	isc_sockaddr_fromnetaddr(&client->destsockaddr, &client->destaddr, 0);

@@ -2202,17 +2183,6 @@ ns__client_tcpconn(isc_nmhandle_t *handle, isc_result_t result, void *arg) {
 	return (ISC_R_SUCCESS);
 }

-static void
-get_clienttask(ns_clientmgr_t *manager, isc_task_t **taskp) {
-	MTRACE("clienttask");
-
-	int tid = isc_nm_tid();
-	REQUIRE(tid >= 0);
-	REQUIRE(tid < manager->ncpus);
-
-	isc_task_attach(manager->taskpool[tid], taskp);
-}
-
 isc_result_t
 ns__client_setup(ns_client_t *client, ns_clientmgr_t *mgr, bool new) {
 	isc_result_t result;
@@ -2225,16 +2195,17 @@ ns__client_setup(ns_client_t *client, ns_clientmgr_t *mgr, bool new) {
 	 * The caller is responsible for that.
 	 */

-	REQUIRE(NS_CLIENT_VALID(client) || (new &&client != NULL));
-	REQUIRE(VALID_MANAGER(mgr) || !new);
-
 	if (new) {
-		*client = (ns_client_t){ .magic = 0 };
+		REQUIRE(VALID_MANAGER(mgr));
+		REQUIRE(client != NULL);
+		REQUIRE(mgr->tid == isc_nm_tid());
+
+		*client = (ns_client_t){ .magic = 0, .tid = mgr->tid };

 		isc_mem_attach(mgr->mctx, &client->mctx);
 		clientmgr_attach(mgr, &client->manager);
 		ns_server_attach(mgr->sctx, &client->sctx);
-		get_clienttask(mgr, &client->task);
+		isc_task_attach(mgr->task, &client->task);

 		dns_message_create(client->mctx, DNS_MESSAGE_INTENTPARSE,
 				   &client->message);
@@ -2251,6 +2222,9 @@ ns__client_setup(ns_client_t *client, ns_clientmgr_t *mgr, bool new) {
 			goto cleanup;
 		}
 	} else {
+		REQUIRE(NS_CLIENT_VALID(client));
+		REQUIRE(client->tid == isc_nm_tid());
+
 		ns_clientmgr_t *oldmgr = client->manager;
 		ns_server_t *sctx = client->sctx;
 		isc_task_t *task = client->task;
@@ -2258,6 +2232,7 @@ ns__client_setup(ns_client_t *client, ns_clientmgr_t *mgr, bool new) {
 		dns_message_t *message = client->message;
 		isc_mem_t *oldmctx = client->mctx;
 		ns_query_t query = client->query;
+		int tid = client->tid;

 		/*
 		 * Retain these values from the existing client, but
@@ -2270,7 +2245,8 @@ ns__client_setup(ns_client_t *client, ns_clientmgr_t *mgr, bool new) {
 					 .task = task,
 					 .sendbuf = sendbuf,
 					 .message = message,
-					 .query = query };
+					 .query = query,
+					 .tid = tid };
 	}

 	client->query.attributes &= ~NS_QUERYATTR_ANSWERED;
@@ -2308,9 +2284,7 @@ cleanup:
 	if (client->manager != NULL) {
 		clientmgr_detach(&client->manager);
 	}
-	if (client->mctx != NULL) {
-		isc_mem_detach(&client->mctx);
-	}
+	isc_mem_detach(&client->mctx);
 	if (client->sctx != NULL) {
 		ns_server_detach(&client->sctx);
 	}
@@ -2359,16 +2333,12 @@ clientmgr_detach(ns_clientmgr_t **mp) {

 static void
 clientmgr_destroy(ns_clientmgr_t *manager) {
-	int i;
-
 	MTRACE("clientmgr_destroy");

 	isc_refcount_destroy(&manager->references);
 	manager->magic = 0;

-	if (manager->interface != NULL) {
-		ns_interface_detach(&manager->interface);
-	}
+	dns_aclenv_detach(&manager->aclenv);

 	isc_mutex_destroy(&manager->lock);
 	isc_mutex_destroy(&manager->reclock);
@@ -2377,54 +2347,45 @@ clientmgr_destroy(ns_clientmgr_t *manager) {
 		isc_task_detach(&manager->excl);
 	}

-	for (i = 0; i < manager->ncpus; i++) {
-		if (manager->taskpool[i] != NULL) {
-			isc_task_detach(&manager->taskpool[i]);
-		}
-	}
-	isc_mem_put(manager->mctx, manager->taskpool,
-		    manager->ncpus * sizeof(manager->taskpool[0]));
+	isc_task_detach(&manager->task);
 	ns_server_detach(&manager->sctx);

-	isc_mem_put(manager->mctx, manager, sizeof(*manager));
+	isc_mem_putanddetach(&manager->mctx, manager, sizeof(*manager));
 }

 isc_result_t
-ns_clientmgr_create(isc_mem_t *mctx, ns_server_t *sctx, isc_taskmgr_t *taskmgr,
-		    isc_timermgr_t *timermgr, ns_interface_t *interface,
-		    int ncpus, ns_clientmgr_t **managerp) {
-	ns_clientmgr_t *manager;
+ns_clientmgr_create(ns_server_t *sctx, isc_taskmgr_t *taskmgr,
+		    isc_timermgr_t *timermgr, dns_aclenv_t *aclenv, int tid,
+		    ns_clientmgr_t **managerp) {
+	ns_clientmgr_t *manager = NULL;
+	isc_mem_t *mctx = NULL;
 	isc_result_t result;
-	int i;
+
+	isc_mem_create(&mctx);

 	manager = isc_mem_get(mctx, sizeof(*manager));
-	*manager = (ns_clientmgr_t){ .magic = 0 };
+	*manager = (ns_clientmgr_t){ .magic = 0, .mctx = mctx };
+
+	result = isc_taskmgr_excltask(taskmgr, &manager->excl);
+	if (result != ISC_R_SUCCESS) {
+		isc_mem_put(mctx, manager, sizeof(*manager));
+		return (result);
+	}

 	isc_mutex_init(&manager->lock);
 	isc_mutex_init(&manager->reclock);

-	manager->excl = NULL;
-	result = isc_taskmgr_excltask(taskmgr, &manager->excl);
-	if (result != ISC_R_SUCCESS) {
-		goto cleanup_reclock;
-	}
-
-	manager->mctx = mctx;
 	manager->taskmgr = taskmgr;
 	manager->timermgr = timermgr;
-	manager->ncpus = ncpus;
+	manager->tid = tid;

-	ns_interface_attach(interface, &manager->interface);
+	dns_aclenv_attach(aclenv, &manager->aclenv);

 	manager->exiting = false;
-	manager->taskpool = isc_mem_get(
-		mctx, manager->ncpus * sizeof(manager->taskpool[0]));
-	for (i = 0; i < manager->ncpus; i++) {
-		manager->taskpool[i] = NULL;
-		result = isc_task_create_bound(manager->taskmgr, 20,
-					       &manager->taskpool[i], i);
-		RUNTIME_CHECK(result == ISC_R_SUCCESS);
-	}
+	result = isc_task_create_bound(manager->taskmgr, 20, &manager->task,
+				       manager->tid);
+	RUNTIME_CHECK(result == ISC_R_SUCCESS);
+
 	isc_refcount_init(&manager->references, 1);
 	manager->sctx = NULL;
 	ns_server_attach(sctx, &manager->sctx);
@@ -2438,14 +2399,6 @@ ns_clientmgr_create(isc_mem_t *mctx, ns_server_t *sctx, isc_taskmgr_t *taskmgr,
 	*managerp = manager;

 	return (ISC_R_SUCCESS);
-
-cleanup_reclock:
-	isc_mutex_destroy(&manager->reclock);
-	isc_mutex_destroy(&manager->lock);
-
-	isc_mem_put(mctx, manager, sizeof(*manager));
-
-	return (result);
 }

 void
@@ -2496,8 +2449,7 @@ isc_result_t
 ns_client_checkaclsilent(ns_client_t *client, isc_netaddr_t *netaddr,
 			 dns_acl_t *acl, bool default_allow) {
 	isc_result_t result;
-	dns_aclenv_t *env =
-		ns_interfacemgr_getaclenv(client->manager->interface->mgr);
+	dns_aclenv_t *env = client->manager->aclenv;
 	isc_netaddr_t tmpnetaddr;
 	int match;