From 0956fb9b9ec4d07f1b744a829dacfe7a1251a58e Mon Sep 17 00:00:00 2001 From: Artem Boldariev Date: Thu, 20 Feb 2025 22:08:01 +0200 Subject: [PATCH 1/5] DoH: Simplify http_do_bio() This commit significantly simplifies the code flow in the http_do_bio() function, which is responsible for processing incoming and outgoing HTTP/2 data. It seems that the way it was structured before was indirectly caused by the presence of the missing callback calls bug, fixed in 8b8f4d500d9c1d41d95d34a79c8935823978114c. The change introduced by this commit is known to remove a bottleneck and allows reproducible and measurable performance improvement for long runs (>= 1h) of "stress:long:rpz:doh+udp:linux:*" tests. Additionally, it fixes a similar issue with potentially missing send callback calls processing and hardens the code against use-after-free errors related to the session object (they can potentially occur). --- lib/isc/netmgr/http.c | 85 +++++++++++++++++++++++-------------------- 1 file changed, 45 insertions(+), 40 deletions(-) diff --git a/lib/isc/netmgr/http.c b/lib/isc/netmgr/http.c index cb8a77197f..21a52936dc 100644 --- a/lib/isc/netmgr/http.c +++ b/lib/isc/netmgr/http.c @@ -1293,7 +1293,10 @@ http_readcb(isc_nmhandle_t *handle ISC_ATTR_UNUSED, isc_result_t result, } isc_buffer_putmem(session->buf, region->base + readlen, unread_size); - isc_nm_read_stop(session->handle); + if (session->handle != NULL) { + INSIST(VALID_NMHANDLE(session->handle)); + isc_nm_read_stop(session->handle); + } http_do_bio_async(session); } else { /* We might have something to receive or send, do IO */ @@ -1599,10 +1602,12 @@ http_too_many_active_streams(isc_nm_http_session_t *session) { static void http_do_bio(isc_nm_http_session_t *session, isc_nmhandle_t *send_httphandle, isc_nm_cb_t send_cb, void *send_cbarg) { + isc__nm_uvreq_t *req = NULL; + size_t remaining = 0; REQUIRE(VALID_HTTP2_SESSION(session)); if (session->closed) { - return; + goto cancel; } else if (session->closing) { /* * There might be leftover callbacks waiting to be received @@ -1610,23 +1615,24 @@ http_do_bio(isc_nm_http_session_t *session, isc_nmhandle_t *send_httphandle, if (session->sending == 0) { finish_http_session(session); } - return; + goto cancel; + } else if (nghttp2_session_want_read(session->ngsession) == 0 && + nghttp2_session_want_write(session->ngsession) == 0 && + session->pending_write_data == NULL) + { + session->closing = true; + if (session->handle != NULL) { + isc_nm_read_stop(session->handle); + } + if (session->sending == 0) { + finish_http_session(session); + } + goto cancel; } - if (send_cb != NULL) { - INSIST(VALID_NMHANDLE(send_httphandle)); - http_send_outgoing(session, send_httphandle, send_cb, - send_cbarg); - return; - } - - INSIST(send_httphandle == NULL); - INSIST(send_cb == NULL); - INSIST(send_cbarg == NULL); - - if (session->pending_write_data != NULL && session->sending == 0) { - http_send_outgoing(session, NULL, NULL, NULL); - return; + else if (session->buf != NULL) + { + remaining = isc_buffer_remaininglength(session->buf); } if (nghttp2_session_want_read(session->ngsession) != 0) { @@ -1635,9 +1641,7 @@ http_do_bio(isc_nm_http_session_t *session, isc_nmhandle_t *send_httphandle, isc__nmsocket_timer_start(session->handle->sock); isc_nm_read(session->handle, http_readcb, session); session->reading = true; - } else if (session->buf != NULL) { - size_t remaining = - isc_buffer_remaininglength(session->buf); + } else if (session->buf != NULL && remaining > 0) { /* Leftover data in the buffer, use it */ size_t remaining_after = 0; ssize_t readlen = 0; @@ -1661,8 +1665,12 @@ http_do_bio(isc_nm_http_session_t *session, isc_nmhandle_t *send_httphandle, http_log_flooding_peer(session); failed_read_cb(ISC_R_RANGE, session); } else if ((size_t)readlen == remaining) { - isc_buffer_free(&session->buf); - http_do_bio(session, NULL, NULL, NULL); + isc_buffer_clear(session->buf); + isc_buffer_compact(session->buf); + http_do_bio(session, send_httphandle, send_cb, + send_cbarg); + isc__nm_httpsession_detach(&tmpsess); + return; } else if (remaining_after > 0 && remaining_after < remaining) { @@ -1679,39 +1687,36 @@ http_do_bio(isc_nm_http_session_t *session, isc_nmhandle_t *send_httphandle, * it and that could overwhelm the server. */ http_do_bio_async(session); - } else { - http_send_outgoing(session, NULL, NULL, NULL); } - isc__nm_httpsession_detach(&tmpsess); - return; - } else { + } else if (session->handle != NULL) { + INSIST(VALID_NMHANDLE(session->handle)); /* * Resume reading, it's idempotent, wait for more */ isc__nmsocket_timer_start(session->handle->sock); isc_nm_read(session->handle, http_readcb, session); } - } else { + } else if (session->handle != NULL) { + INSIST(VALID_NMHANDLE(session->handle)); /* We don't want more data, stop reading for now */ isc_nm_read_stop(session->handle); } /* we might have some data to send after processing */ - http_send_outgoing(session, NULL, NULL, NULL); - - if (nghttp2_session_want_read(session->ngsession) == 0 && - nghttp2_session_want_write(session->ngsession) == 0 && - session->pending_write_data == NULL) - { - session->closing = true; - isc_nm_read_stop(session->handle); - if (session->sending == 0) { - finish_http_session(session); - } - } + http_send_outgoing(session, send_httphandle, send_cb, send_cbarg); return; +cancel: + if (send_cb == NULL) { + return; + } + req = isc__nm_uvreq_get(send_httphandle->sock); + + req->cb.send = send_cb; + req->cbarg = send_cbarg; + isc_nmhandle_attach(send_httphandle, &req->handle); + isc__nm_sendcb(send_httphandle->sock, req, ISC_R_CANCELED, true); } static void From 0e1b02868a63d2c4f6f0c414b20d4b999adbcc46 Mon Sep 17 00:00:00 2001 From: Artem Boldariev Date: Mon, 24 Feb 2025 18:32:23 +0200 Subject: [PATCH 2/5] DoH: Limit the number of delayed IO processing requests Previously, a function for continuing IO processing on the next UV tick was introduced (http_do_bio_async()). The intention behind this function was to ensure that http_do_bio() is eventually called at least once in the future. However, the current implementation allows queueing multiple such delayed requests needlessly. There is currently no need for these excessive requests as http_do_bio() can requeue them if needed. At the same time, each such request can lead to a memory allocation, particularly in BIND 9.18. This commit ensures that the number of enqueued delayed IO processing requests never exceeds one in order to avoid potentially bombarding IO threads with the delayed requests needlessly. --- lib/isc/netmgr/http.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/lib/isc/netmgr/http.c b/lib/isc/netmgr/http.c index 21a52936dc..1e440837b3 100644 --- a/lib/isc/netmgr/http.c +++ b/lib/isc/netmgr/http.c @@ -190,6 +190,8 @@ struct isc_nm_http_session { size_t data_in_flight; + bool async_queued; + /* * The statistical values below are for usage on server-side * only. They are meant to detect clients that are taking too many @@ -1725,6 +1727,8 @@ http_do_bio_async_cb(void *arg) { REQUIRE(VALID_HTTP2_SESSION(session)); + session->async_queued = false; + if (session->handle != NULL && !isc__nmsocket_closing(session->handle->sock)) { @@ -1741,10 +1745,12 @@ http_do_bio_async(isc_nm_http_session_t *session) { REQUIRE(VALID_HTTP2_SESSION(session)); if (session->handle == NULL || - isc__nmsocket_closing(session->handle->sock)) + isc__nmsocket_closing(session->handle->sock) || + session->async_queued) { return; } + session->async_queued = true; isc__nm_httpsession_attach(session, &tmpsess); isc_async_run(session->handle->sock->worker->loop, http_do_bio_async_cb, tmpsess); From c5f7968856f3d0cf37c5882ac19fa70333bae4cc Mon Sep 17 00:00:00 2001 From: Artem Boldariev Date: Tue, 25 Feb 2025 09:52:19 +0200 Subject: [PATCH 3/5] DoH: Flush HTTP write buffer on an outgoing DNS message MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Previously, the code would try to avoid sending any data regardless of what it is unless: a) The flush limit is reached; b) There are no sends in flight. This strategy is used to avoid too numerous send requests with little amount of data. However, it has been proven to be too aggressive and, in fact, harms performance in some cases (e.g., on longer (≥1h) runs of "stress:long:rpz:doh+udp:linux:*"). Now, additionally to the listed cases, we also: c) Flush the buffer and perform a send operation when there is an outgoing DNS message passed to the code (which is indicated by the presence of a send callback). That helps improve performance for "stress:long:rpz:doh+udp:linux:*" tests. --- lib/isc/netmgr/http.c | 25 +++++++++++++------------ 1 file changed, 13 insertions(+), 12 deletions(-) diff --git a/lib/isc/netmgr/http.c b/lib/isc/netmgr/http.c index 1e440837b3..bf2abe5e4b 100644 --- a/lib/isc/netmgr/http.c +++ b/lib/isc/netmgr/http.c @@ -1467,26 +1467,27 @@ http_send_outgoing(isc_nm_http_session_t *session, isc_nmhandle_t *httphandle, * to avoid hitting unnecessary limitations on a TLS record size * within some tools (e.g. flamethrower). */ - if (max_total_write_size >= FLUSH_HTTP_WRITE_BUFFER_AFTER) { + if (cb != NULL) { /* - * Case 1: We have at least FLUSH_HTTP_WRITE_BUFFER_AFTER - * bytes to send. Let's flush it. + * Case 0: The callback is specified, that means that a DNS + * message is ready. Let's flush the the buffer. + */ + total = max_total_write_size; + } else if (max_total_write_size >= FLUSH_HTTP_WRITE_BUFFER_AFTER) { + /* + * Case 1: We have equal or more than + * FLUSH_HTTP_WRITE_BUFFER_AFTER bytes to send. Let's flush it. */ total = max_total_write_size; } else if (session->sending > 0 && total > 0) { /* * Case 2: There is one or more write requests in flight and - * we have some new data form nghttp2 to send. Let's put the - * write callback (if any) into the pending write callbacks - * list. Then let's return from the function: as soon as the + * we have some new data form nghttp2 to send. + * Then let's return from the function: as soon as the * "in-flight" write callback get's called or we have reached * FLUSH_HTTP_WRITE_BUFFER_AFTER bytes in the write buffer, we - * will flush the buffer. - */ - if (cb != NULL) { - http_append_pending_send_request(session, httphandle, - cb, cbarg); - } + * will flush the buffer. */ + INSIST(cb == NULL); goto nothing_to_send; } else if (session->sending == 0 && total == 0 && session->pending_write_data != NULL) From 217a1ebd79d90d2a3eebf44256fd15ab61a7d2a9 Mon Sep 17 00:00:00 2001 From: Artem Boldariev Date: Wed, 26 Feb 2025 14:43:34 +0200 Subject: [PATCH 4/5] DoH: remove obsolete INSIST() check The check, while not active by default, is not valid since the commit 8b8f4d500d9c1d41d95d34a79c8935823978114c. See 'if (total == 0) { ...' below branch to understand why. --- lib/isc/netmgr/http.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/lib/isc/netmgr/http.c b/lib/isc/netmgr/http.c index bf2abe5e4b..b1ed237a0b 100644 --- a/lib/isc/netmgr/http.c +++ b/lib/isc/netmgr/http.c @@ -1522,8 +1522,6 @@ http_send_outgoing(isc_nm_http_session_t *session, isc_nmhandle_t *httphandle, (total == 0 && session->sending == 0) || (total > 0 && session->sending == 0)); } -#else - INSIST(ISC_LIST_EMPTY(session->pending_write_callbacks)); #endif /* ENABLE_HTTP_WRITE_BUFFERING */ if (total == 0) { From eaad0aefe668408d8ae0792796852cc7bccaff0f Mon Sep 17 00:00:00 2001 From: Artem Boldariev Date: Tue, 25 Feb 2025 19:58:24 +0200 Subject: [PATCH 5/5] DoH: Bump the active streams processing limit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This commit bumps the total number of active streams (= the opened streams for which a request is received, but response is not ready) to 60% of the total streams limit. The previous limit turned out to be too tight as revealed by longer (≥1h) runs of "stress:long:rpz:doh+udp:linux:*" tests. --- lib/isc/netmgr/http.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/isc/netmgr/http.c b/lib/isc/netmgr/http.c index b1ed237a0b..24210c0572 100644 --- a/lib/isc/netmgr/http.c +++ b/lib/isc/netmgr/http.c @@ -1583,7 +1583,7 @@ http_too_many_active_streams(isc_nm_http_session_t *session) { */ const uint64_t max_active_streams = ISC_MAX(ISC_NETMGR_MAX_STREAM_CLIENTS_PER_CONN, - session->max_concurrent_streams / 3); + (session->max_concurrent_streams * 6) / 10); /* 60% */ if (session->client) { return false;