Merge branch '3167-remove-isc__nmsocket_t-ah_handles' into 'main'

Remove active handles tracking from isc__nmsocket_t

Closes #3147 and #3167

See merge request isc-projects/bind9!5878

lib/isc/netmgr/netmgr-int.h

@@ -250,7 +250,6 @@ struct isc_nmhandle {
 	 * the socket.
 	 */
 	isc_nmsocket_t *sock;
-	size_t ah_pos; /* Position in the socket's 'active handles' array */
 
 	isc_nm_http_session_t *httpsession;
 
@@ -1014,6 +1013,7 @@ struct isc_nmsocket {
 	atomic_bool connected;
 	atomic_bool accepting;
 	atomic_bool reading;
+	atomic_bool timedout;
 	isc_refcount_t references;
 
 	/*%
@@ -1086,30 +1086,9 @@ struct isc_nmsocket {
 	isc_result_t result;
 
 	/*%
-	 * List of active handles.
+	 * Current number of active handles.
-	 * ah - current position in 'ah_frees'; this represents the
-	 *	current number of active handles;
-	 * ah_size - size of the 'ah_frees' and 'ah_handles' arrays
-	 * ah_handles - array pointers to active handles
-	 *
-	 * Adding a handle
-	 *  - if ah == ah_size, reallocate
-	 *  - x = ah_frees[ah]
-	 *  - ah_frees[ah++] = 0;
-	 *  - ah_handles[x] = handle
-	 *  - x must be stored with the handle!
-	 * Removing a handle:
-	 *  - ah_frees[--ah] = x
-	 *  - ah_handles[x] = NULL;
-	 *
-	 * XXX: for now this is locked with socket->lock, but we
-	 * might want to change it to something lockless in the
-	 * future.
 	 */
 	atomic_int_fast32_t ah;
-	size_t ah_size;
-	size_t *ah_frees;
-	isc_nmhandle_t **ah_handles;
 
 	/*% Buffer for TCPDNS processing */
 	size_t buf_size;

lib/isc/netmgr/netmgr.c

@@ -1279,10 +1279,6 @@ nmsocket_cleanup(isc_nmsocket_t *sock, bool dofree FLARG) {
 	isc_astack_destroy(sock->inactivereqs);
 	sock->magic = 0;
 
-	isc_mem_put(sock->mgr->mctx, sock->ah_frees,
-		    sock->ah_size * sizeof(sock->ah_frees[0]));
-	isc_mem_put(sock->mgr->mctx, sock->ah_handles,
-		    sock->ah_size * sizeof(sock->ah_handles[0]));
 	isc_condition_destroy(&sock->scond);
 	isc_condition_destroy(&sock->cond);
 	isc_mutex_destroy(&sock->lock);
@@ -1473,7 +1469,6 @@ isc___nmsocket_init(isc_nmsocket_t *sock, isc_nm_t *mgr, isc_nmsocket_type type,
 
 	*sock = (isc_nmsocket_t){ .type = type,
 				  .fd = -1,
-				  .ah_size = 32,
 				  .inactivehandles = isc_astack_new(
 					  mgr->mctx, ISC_NM_HANDLES_STACK_SIZE),
 				  .inactivereqs = isc_astack_new(
@@ -1498,15 +1493,7 @@ isc___nmsocket_init(isc_nmsocket_t *sock, isc_nm_t *mgr, isc_nmsocket_type type,
 	isc_nm_attach(mgr, &sock->mgr);
 	sock->uv_handle.handle.data = sock;
 
-	sock->ah_frees = isc_mem_get(mgr->mctx,
-				     sock->ah_size * sizeof(sock->ah_frees[0]));
-	sock->ah_handles = isc_mem_get(
-		mgr->mctx, sock->ah_size * sizeof(sock->ah_handles[0]));
 	ISC_LINK_INIT(&sock->quotacb, link);
-	for (size_t i = 0; i < 32; i++) {
-		sock->ah_frees[i] = i;
-		sock->ah_handles[i] = NULL;
-	}
 
 	switch (type) {
 	case isc_nm_udpsocket:
@@ -1577,6 +1564,7 @@ isc___nmsocket_init(isc_nmsocket_t *sock, isc_nm_t *mgr, isc_nmsocket_type type,
 	atomic_init(&sock->connecting, false);
 	atomic_init(&sock->keepalive, false);
 	atomic_init(&sock->connected, false);
+	atomic_init(&sock->timedout, false);
 
 	atomic_init(&sock->active_child_connections, 0);
 
@@ -1633,8 +1621,6 @@ isc_nmhandle_t *
 isc___nmhandle_get(isc_nmsocket_t *sock, isc_sockaddr_t *peer,
 		   isc_sockaddr_t *local FLARG) {
 	isc_nmhandle_t *handle = NULL;
-	size_t handlenum;
-	int pos;
 
 	REQUIRE(VALID_NMSOCK(sock));
 
@@ -1669,36 +1655,13 @@ isc___nmhandle_get(isc_nmsocket_t *sock, isc_sockaddr_t *peer,
 		handle->local = sock->iface;
 	}
 
-	LOCK(&sock->lock);
+	(void)atomic_fetch_add(&sock->ah, 1);
-	/* We need to add this handle to the list of active handles */
-	if ((size_t)atomic_load(&sock->ah) == sock->ah_size) {
-		sock->ah_frees = isc_mem_reget(
-			sock->mgr->mctx, sock->ah_frees,
-			sock->ah_size * sizeof(sock->ah_frees[0]),
-			sock->ah_size * 2 * sizeof(sock->ah_frees[0]));
-		sock->ah_handles = isc_mem_reget(
-			sock->mgr->mctx, sock->ah_handles,
-			sock->ah_size * sizeof(sock->ah_handles[0]),
-			sock->ah_size * 2 * sizeof(sock->ah_handles[0]));
 
-		for (size_t i = sock->ah_size; i < sock->ah_size * 2; i++) {
-			sock->ah_frees[i] = i;
-			sock->ah_handles[i] = NULL;
-		}
-
-		sock->ah_size *= 2;
-	}
-
-	handlenum = atomic_fetch_add(&sock->ah, 1);
-	pos = sock->ah_frees[handlenum];
-
-	INSIST(sock->ah_handles[pos] == NULL);
-	sock->ah_handles[pos] = handle;
-	handle->ah_pos = pos;
 #ifdef NETMGR_TRACE
+	LOCK(&sock->lock);
 	ISC_LIST_APPEND(sock->active_handles, handle, active_link);
-#endif
 	UNLOCK(&sock->lock);
+#endif
 
 	switch (sock->type) {
 	case isc_nm_udpsocket:
@@ -1775,7 +1738,6 @@ nmhandle_free(isc_nmsocket_t *sock, isc_nmhandle_t *handle) {
 
 static void
 nmhandle_deactivate(isc_nmsocket_t *sock, isc_nmhandle_t *handle) {
-	size_t handlenum;
 	bool reuse = false;
 
 	/*
@@ -1785,18 +1747,12 @@ nmhandle_deactivate(isc_nmsocket_t *sock, isc_nmhandle_t *handle) {
 	 */
 	LOCK(&sock->lock);
 
-	INSIST(sock->ah_handles[handle->ah_pos] == handle);
-	INSIST(sock->ah_size > handle->ah_pos);
-	INSIST(atomic_load(&sock->ah) > 0);
-
 #ifdef NETMGR_TRACE
 	ISC_LIST_UNLINK(sock->active_handles, handle, active_link);
 #endif
 
-	sock->ah_handles[handle->ah_pos] = NULL;
+	INSIST(atomic_fetch_sub(&sock->ah, 1) > 0);
-	handlenum = atomic_fetch_sub(&sock->ah, 1) - 1;
+
-	sock->ah_frees[handlenum] = handle->ah_pos;
-	handle->ah_pos = 0;
 	if (atomic_load(&sock->active)) {
 		reuse = isc_astack_trypush(sock->inactivehandles, handle);
 	}
@@ -2033,18 +1989,14 @@ isc__nmsocket_connecttimeout_cb(uv_timer_t *timer) {
 		sock->tls.pending_req = NULL;
 	}
 
-	/* Call the connect callback directly */
+	/*
+	 * Mark the connection as timed out and shutdown the socket.
+	 */
 
-	req->cb.connect(req->handle, ISC_R_TIMEDOUT, req->cbarg);
+	INSIST(atomic_compare_exchange_strong(&sock->timedout, &(bool){ false },
-
+					      true));
-	/* Timer is not running, cleanup and shutdown everything */
+	isc__nmsocket_clearcb(sock);
-	if (!isc__nmsocket_timer_running(sock)) {
+	isc__nmsocket_shutdown(sock);
-		INSIST(atomic_compare_exchange_strong(&sock->connecting,
-						      &(bool){ true }, false));
-		isc__nm_uvreq_put(&req, sock);
-		isc__nmsocket_clearcb(sock);
-		isc__nmsocket_shutdown(sock);
-	}
 }
 
 void

lib/isc/netmgr/tcp.c

@@ -239,15 +239,16 @@ tcp_connect_cb(uv_connect_t *uvreq, int status) {
 	isc__nmsocket_timer_stop(sock);
 	uv_handle_set_data((uv_handle_t *)&sock->read_timer, sock);
 
-	if (!atomic_load(&sock->connecting)) {
-		return;
-	}
-
 	req = uv_handle_get_data((uv_handle_t *)uvreq);
 
 	REQUIRE(VALID_UVREQ(req));
 	REQUIRE(VALID_NMHANDLE(req->handle));
 
+	if (atomic_load(&sock->timedout)) {
+		result = ISC_R_TIMEDOUT;
+		goto error;
+	}
+
 	if (!atomic_load(&sock->connecting)) {
 		/*
 		 * The connect was cancelled from timeout; just clean up

lib/isc/netmgr/tcpdns.c

@@ -206,15 +206,16 @@ tcpdns_connect_cb(uv_connect_t *uvreq, int status) {
 	isc__nmsocket_timer_stop(sock);
 	uv_handle_set_data((uv_handle_t *)&sock->read_timer, sock);
 
-	if (!atomic_load(&sock->connecting)) {
-		return;
-	}
-
 	req = uv_handle_get_data((uv_handle_t *)uvreq);
 
 	REQUIRE(VALID_UVREQ(req));
 	REQUIRE(VALID_NMHANDLE(req->handle));
 
+	if (atomic_load(&sock->timedout)) {
+		result = ISC_R_TIMEDOUT;
+		goto error;
+	}
+
 	if (isc__nm_closing(sock)) {
 		/* Network manager shutting down */
 		result = ISC_R_SHUTTINGDOWN;
@@ -225,7 +226,7 @@ tcpdns_connect_cb(uv_connect_t *uvreq, int status) {
 		goto error;
 	} else if (status == UV_ETIMEDOUT) {
 		/* Timeout status code here indicates hard error */
-		result = ISC_R_CANCELED;
+		result = ISC_R_TIMEDOUT;
 		goto error;
 	} else if (status != 0) {
 		result = isc__nm_uverr2result(status);

lib/isc/netmgr/tlsdns.c

@@ -221,15 +221,16 @@ tlsdns_connect_cb(uv_connect_t *uvreq, int status) {
 	REQUIRE(VALID_NMSOCK(sock));
 	REQUIRE(sock->tid == isc_nm_tid());
 
-	if (!atomic_load(&sock->connecting)) {
-		return;
-	}
-
 	req = uv_handle_get_data((uv_handle_t *)uvreq);
 
 	REQUIRE(VALID_UVREQ(req));
 	REQUIRE(VALID_NMHANDLE(req->handle));
 
+	if (atomic_load(&sock->timedout)) {
+		result = ISC_R_TIMEDOUT;
+		goto error;
+	}
+
 	if (isc__nm_closing(sock)) {
 		/* Network manager shutting down */
 		result = ISC_R_SHUTTINGDOWN;
@@ -240,7 +241,7 @@ tlsdns_connect_cb(uv_connect_t *uvreq, int status) {
 		goto error;
 	} else if (status == UV_ETIMEDOUT) {
 		/* Timeout status code here indicates hard error */
-		result = ISC_R_CANCELED;
+		result = ISC_R_TIMEDOUT;
 		goto error;
 	} else if (status != 0) {
 		result = isc__nm_uverr2result(status);