From 16c1d1eb2ea99909351bb8b74773cbdf37ad35cc Mon Sep 17 00:00:00 2001 From: Artem Boldariev Date: Thu, 13 Jun 2024 14:34:20 +0300 Subject: [PATCH] Avoid indefinite send re-scheduling in TLS DNS When a peer is not reading the data we are sending it was for the TLS DNS code to end up in a situation when it would indefinitely reschedule send requests, effectively turning the 'uv_loop' into a busy loop that would consume CPU cycles in endless efforts to send outgoing data. The main reason for that was only one send buffer dedicated for sends: the code would re-queue sends until it is empty - that would never happen when the remote side is not reading data. That seems like an omission from the older day of the Network Manager as it is quiet simple to make the code use multiple buffers for sends. That ultimately breaks the cycle of futile send request rescheduling. As a side effect, this commit also gets rid of one memory copying on a hot path. --- lib/isc/netmgr/netmgr-int.h | 8 +++--- lib/isc/netmgr/tlsdns.c | 49 ++++++++++++++----------------------- 2 files changed, 23 insertions(+), 34 deletions(-) diff --git a/lib/isc/netmgr/netmgr-int.h b/lib/isc/netmgr/netmgr-int.h index 75b221e338..cc635e3f5f 100644 --- a/lib/isc/netmgr/netmgr-int.h +++ b/lib/isc/netmgr/netmgr-int.h @@ -378,9 +378,10 @@ struct isc__nm_uvreq { int magic; isc_nmsocket_t *sock; isc_nmhandle_t *handle; - char tcplen[2]; /* The TCP DNS message length */ - uv_buf_t uvbuf; /* translated isc_region_t, to be - * sent or received */ + char tcplen[2]; /* The TCP DNS message length */ + uv_buf_t uvbuf; /* translated isc_region_t, to be + * sent or received */ + isc_region_t userbuf; isc_sockaddr_t local; /* local address */ isc_sockaddr_t peer; /* peer address */ isc__nm_cb_t cb; /* callback */ @@ -995,7 +996,6 @@ struct isc_nmsocket { TLS_STATE_ERROR, TLS_STATE_CLOSING } state; - isc_region_t senddata; ISC_LIST(isc__nm_uvreq_t) sendreqs; bool cycle; isc_result_t pending_error; diff --git a/lib/isc/netmgr/tlsdns.c b/lib/isc/netmgr/tlsdns.c index 32e685724c..b41c353842 100644 --- a/lib/isc/netmgr/tlsdns.c +++ b/lib/isc/netmgr/tlsdns.c @@ -1277,17 +1277,17 @@ call_pending_send_callbacks(isc_nmsocket_t *sock, const isc_result_t result) { } static void -free_senddata(isc_nmsocket_t *sock, const isc_result_t result) { +free_senddata(isc_nmsocket_t *sock, isc__nm_uvreq_t *req, + const isc_result_t result) { REQUIRE(VALID_NMSOCK(sock)); - REQUIRE(sock->tls.senddata.base != NULL); - REQUIRE(sock->tls.senddata.length > 0); + REQUIRE(req != NULL && req->userbuf.base != NULL && + req->userbuf.length > 0); - isc_mem_put(sock->mgr->mctx, sock->tls.senddata.base, - sock->tls.senddata.length); - sock->tls.senddata.base = NULL; - sock->tls.senddata.length = 0; + isc_mem_put(sock->mgr->mctx, req->userbuf.base, req->userbuf.length); call_pending_send_callbacks(sock, result); + + isc__nm_uvreq_put(&req, sock); } static void @@ -1300,9 +1300,7 @@ tls_write_cb(uv_write_t *req, int status) { isc_nm_timer_stop(uvreq->timer); isc_nm_timer_detach(&uvreq->timer); - free_senddata(sock, result); - - isc__nm_uvreq_put(&uvreq, sock); + free_senddata(sock, uvreq, result); if (status != 0) { if (!sock->client && @@ -1339,23 +1337,18 @@ tls_cycle_output(isc_nmsocket_t *sock) { int rv; int r; - if (sock->tls.senddata.base != NULL || - sock->tls.senddata.length > 0) - { - break; - } - if (pending > (int)ISC_NETMGR_TCP_RECVBUF_SIZE) { pending = (int)ISC_NETMGR_TCP_RECVBUF_SIZE; } - sock->tls.senddata.base = isc_mem_get(sock->mgr->mctx, pending); - sock->tls.senddata.length = pending; - /* It's a bit misnomer here, but it does the right thing */ req = isc__nm_get_read_req(sock, NULL); - req->uvbuf.base = (char *)sock->tls.senddata.base; - req->uvbuf.len = sock->tls.senddata.length; + + req->userbuf.base = isc_mem_get(sock->mgr->mctx, pending); + req->userbuf.length = (size_t)pending; + + req->uvbuf.base = (char *)req->userbuf.base; + req->uvbuf.len = (size_t)req->userbuf.length; rv = BIO_read_ex(sock->tls.app_rbio, req->uvbuf.base, req->uvbuf.len, &bytes); @@ -1367,23 +1360,20 @@ tls_cycle_output(isc_nmsocket_t *sock) { if (r == pending) { /* Wrote everything, restart */ - isc__nm_uvreq_put(&req, sock); - free_senddata(sock, ISC_R_SUCCESS); + free_senddata(sock, req, ISC_R_SUCCESS); continue; } if (r > 0) { /* Partial write, send rest asynchronously */ - memmove(req->uvbuf.base, req->uvbuf.base + r, - req->uvbuf.len - r); - req->uvbuf.len = req->uvbuf.len - r; + req->uvbuf.base += r; + req->uvbuf.len -= r; } else if (r == UV_ENOSYS || r == UV_EAGAIN) { /* uv_try_write is not supported, send * asynchronously */ } else { result = isc__nm_uverr2result(r); - isc__nm_uvreq_put(&req, sock); - free_senddata(sock, result); + free_senddata(sock, req, result); break; } @@ -1391,8 +1381,7 @@ tls_cycle_output(isc_nmsocket_t *sock) { &req->uvbuf, 1, tls_write_cb); if (r < 0) { result = isc__nm_uverr2result(r); - isc__nm_uvreq_put(&req, sock); - free_senddata(sock, result); + free_senddata(sock, req, result); break; }