From 3c498245896452231c33a38c021ede8ae1622dcc Mon Sep 17 00:00:00 2001 From: Artem Boldariev Date: Thu, 13 Feb 2025 14:53:18 +0200 Subject: [PATCH 1/4] DoH: Track the amount of in flight outgoing data Previously we would limit the amount of incoming data to process based solely on the presence of not completed send requests. That worked, however, it was found to severely degrade performance in certain cases, as was revealed during extended testing. Now we switch to keeping track of how much data is in flight (or ready to be in flight) and limit the amount of processed incoming data when the amount of in flight data surpasses the given threshold, similarly to like we do in other transports. (cherry picked from commit 05e8a508188116e8e367aaf5e68575c8cebb4207) --- lib/isc/netmgr/http.c | 27 +++++++++++++++++++++++---- 1 file changed, 23 insertions(+), 4 deletions(-) diff --git a/lib/isc/netmgr/http.c b/lib/isc/netmgr/http.c index a00f4a7229..55fcfbaa12 100644 --- a/lib/isc/netmgr/http.c +++ b/lib/isc/netmgr/http.c @@ -188,6 +188,8 @@ struct isc_nm_http_session { isc__nm_http_pending_callbacks_t pending_write_callbacks; isc_buffer_t *pending_write_data; + size_t data_in_flight; + /* * The statistical values below are for usage on server-side * only. They are meant to detect clients that are taking too many @@ -1045,6 +1047,19 @@ client_submit_request(isc_nm_http_session_t *session, http_cstream_t *stream) { return ISC_R_SUCCESS; } +static inline size_t +http_in_flight_data_size(isc_nm_http_session_t *session) { + size_t in_flight = 0; + + if (session->pending_write_data != NULL) { + in_flight += isc_buffer_usedlength(session->pending_write_data); + } + + in_flight += session->data_in_flight; + + return in_flight; +} + static ssize_t http_process_input_data(isc_nm_http_session_t *session, isc_buffer_t *input_data) { @@ -1096,13 +1111,14 @@ http_process_input_data(isc_nm_http_session_t *session, (session->received - session->processed); /* - * If there are non completed send requests in flight -let's - * not process any incoming data, as it could lead to piling - * up too much send data in send buffers. With many clients + * If there is too much outgoing data in flight - let's not + * process any incoming data, as it could lead to piling up + * too much send data in send buffers. With many clients * connected it can lead to excessive memory consumption on * the server instance. */ - if (session->sending > 0) { + const size_t in_flight = http_in_flight_data_size(session); + if (in_flight >= ISC_NETMGR_TCP_SENDBUF_SIZE) { break; } @@ -1314,6 +1330,8 @@ http_writecb(isc_nmhandle_t *handle, isc_result_t result, void *arg) { isc_nmhandle_detach(&req->httphandle); } + session->data_in_flight -= + isc_buffer_usedlength(req->pending_write_data); isc_buffer_free(&req->pending_write_data); session->processed += req->submitted; isc_mem_put(session->mctx, req, sizeof(*req)); @@ -1488,6 +1506,7 @@ http_send_outgoing(isc_nm_http_session_t *session, isc_nmhandle_t *httphandle, session->sending++; isc_buffer_usedregion(send->pending_write_data, &send_data); + session->data_in_flight += send_data.length; isc_nm_send(transphandle, &send_data, http_writecb, send); return true; nothing_to_send: From f9aa7a298da78c74b787f27e4c9b12a779c7a8db Mon Sep 17 00:00:00 2001 From: Artem Boldariev Date: Thu, 13 Feb 2025 15:05:10 +0200 Subject: [PATCH 2/4] DoH: change how the active streams number is calculated This commit changes the way how the number of active HTTP streams is calculated and allows it to scale with the values of the maximum amount of streams per connection, instead of effectively capping at STREAM_CLIENTS_PER_CONN. The original limit, which is intended to define the pipelining limit for TCP/DoT. However, it appeared to be too restrictive for DoH, as it works quite differently and implements pipelining at protocol level by the means of multiplexing multiple streams. That renders each stream to be effectively a separate connection from the point of view of the rest of the codebase. (cherry picked from commit a22bc2d7d4974d730e4a7267d2f85e74db53c688) --- lib/isc/netmgr/http.c | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/lib/isc/netmgr/http.c b/lib/isc/netmgr/http.c index 55fcfbaa12..1006581d48 100644 --- a/lib/isc/netmgr/http.c +++ b/lib/isc/netmgr/http.c @@ -1517,8 +1517,20 @@ nothing_to_send: static inline bool http_too_many_active_streams(isc_nm_http_session_t *session) { const uint64_t active_streams = session->received - session->processed; - const uint64_t max_active_streams = ISC_MIN( - STREAM_CLIENTS_PER_CONN, session->max_concurrent_streams); + /* + * The motivation behind capping the maximum active streams number + * to a third of maximum streams is to allow the value to scale + * with the max number of streams. + * + * We do not want to have too many active streams at once as every + * stream is processed as a separate virtual connection by the + * higher level code. If a client sends a bulk of requests without + * waiting for the previous ones to complete we might want to + * throttle it as it might be not a friend knocking at the + * door. We already have some job to do for it. + */ + const uint64_t max_active_streams = ISC_MAX( + STREAM_CLIENTS_PER_CONN, session->max_concurrent_streams / 3); if (session->client) { return false; From 0b9e8e6063e0369008f34d039d158edef517258c Mon Sep 17 00:00:00 2001 From: Artem Boldariev Date: Wed, 12 Feb 2025 22:58:42 +0200 Subject: [PATCH 3/4] DoH: Fix missing send callback calls When handling outgoing data, there were a couple of rarely executed code paths that would not take into account that the callback MUST be called. It could lead to potential memory leaks and consequent shutdown hangs. (cherry picked from commit 8b8f4d500d9c1d41d95d34a79c8935823978114c) --- lib/isc/netmgr/http.c | 53 +++++++++++++++++++++++++++++++++---------- 1 file changed, 41 insertions(+), 12 deletions(-) diff --git a/lib/isc/netmgr/http.c b/lib/isc/netmgr/http.c index 1006581d48..2df182edbc 100644 --- a/lib/isc/netmgr/http.c +++ b/lib/isc/netmgr/http.c @@ -1361,6 +1361,23 @@ move_pending_send_callbacks(isc_nm_http_session_t *session, ISC_LIST_INIT(session->pending_write_callbacks); } +static inline void +http_append_pending_send_request(isc_nm_http_session_t *session, + isc_nmhandle_t *httphandle, isc_nm_cb_t cb, + void *cbarg) { + REQUIRE(VALID_HTTP2_SESSION(session)); + REQUIRE(VALID_NMHANDLE(httphandle)); + REQUIRE(cb != NULL); + + isc__nm_uvreq_t *newcb = isc__nm_uvreq_get(httphandle->sock->mgr, + httphandle->sock); + + newcb->cb.send = cb; + newcb->cbarg = cbarg; + isc_nmhandle_attach(httphandle, &newcb->handle); + ISC_LIST_APPEND(session->pending_write_callbacks, newcb, link); +} + static bool http_send_outgoing(isc_nm_http_session_t *session, isc_nmhandle_t *httphandle, isc_nm_cb_t cb, void *cbarg) { @@ -1372,10 +1389,25 @@ http_send_outgoing(isc_nm_http_session_t *session, isc_nmhandle_t *httphandle, size_t max_total_write_size = 0; #endif /* ENABLE_HTTP_WRITE_BUFFERING */ - if (!http_session_active(session) || - (!nghttp2_session_want_write(session->ngsession) && - session->pending_write_data == NULL)) + if (!http_session_active(session)) { + if (cb != NULL) { + isc__nm_uvreq_t *req = isc__nm_uvreq_get( + httphandle->sock->mgr, httphandle->sock); + + req->cb.send = cb; + req->cbarg = cbarg; + isc_nmhandle_attach(httphandle, &req->handle); + isc__nm_sendcb(httphandle->sock, req, ISC_R_CANCELED, + true); + } + return false; + } else if (!nghttp2_session_want_write(session->ngsession) && + session->pending_write_data == NULL) { + if (cb != NULL) { + http_append_pending_send_request(session, httphandle, + cb, cbarg); + } return false; } @@ -1435,15 +1467,8 @@ http_send_outgoing(isc_nm_http_session_t *session, isc_nmhandle_t *httphandle, * FLUSH_HTTP_WRITE_BUFFER_AFTER bytes in the write buffer, we * will flush the buffer. */ if (cb != NULL) { - isc__nm_uvreq_t *newcb = isc__nm_uvreq_get( - httphandle->sock->mgr, httphandle->sock); - - INSIST(VALID_NMHANDLE(httphandle)); - newcb->cb.send = cb; - newcb->cbarg = cbarg; - isc_nmhandle_attach(httphandle, &newcb->handle); - ISC_LIST_APPEND(session->pending_write_callbacks, newcb, - link); + http_append_pending_send_request(session, httphandle, + cb, cbarg); } goto nothing_to_send; } else if (session->sending == 0 && total == 0 && @@ -1480,6 +1505,10 @@ http_send_outgoing(isc_nm_http_session_t *session, isc_nmhandle_t *httphandle, if (total == 0) { /* No data returned */ + if (cb != NULL) { + http_append_pending_send_request(session, httphandle, + cb, cbarg); + } goto nothing_to_send; } From 66bdddc51a5e6acc4ff6746dab62333f44241b43 Mon Sep 17 00:00:00 2001 From: Artem Boldariev Date: Wed, 19 Feb 2025 12:28:37 +0200 Subject: [PATCH 4/4] DoH: http_send_outgoing() return value is not used The value returned by http_send_outgoing() is not used anywhere, so we make it not return anything (void). Probably it is an omission from older times. (cherry picked from commit 2adabe835a290c021948a43a4c2c25ce2806aef2) --- lib/isc/netmgr/http.c | 22 ++++++++++------------ 1 file changed, 10 insertions(+), 12 deletions(-) diff --git a/lib/isc/netmgr/http.c b/lib/isc/netmgr/http.c index 2df182edbc..33ce20809c 100644 --- a/lib/isc/netmgr/http.c +++ b/lib/isc/netmgr/http.c @@ -232,7 +232,7 @@ typedef struct isc_http_send_req { #define HTTP_HANDLER_MAGIC ISC_MAGIC('H', 'T', 'H', 'L') #define VALID_HTTP_HANDLER(t) ISC_MAGIC_VALID(t, HTTP_HANDLER_MAGIC) -static bool +static void http_send_outgoing(isc_nm_http_session_t *session, isc_nmhandle_t *httphandle, isc_nm_cb_t cb, void *cbarg); @@ -1378,7 +1378,7 @@ http_append_pending_send_request(isc_nm_http_session_t *session, ISC_LIST_APPEND(session->pending_write_callbacks, newcb, link); } -static bool +static void http_send_outgoing(isc_nm_http_session_t *session, isc_nmhandle_t *httphandle, isc_nm_cb_t cb, void *cbarg) { isc_http_send_req_t *send = NULL; @@ -1400,7 +1400,7 @@ http_send_outgoing(isc_nm_http_session_t *session, isc_nmhandle_t *httphandle, isc__nm_sendcb(httphandle->sock, req, ISC_R_CANCELED, true); } - return false; + return; } else if (!nghttp2_session_want_write(session->ngsession) && session->pending_write_data == NULL) { @@ -1408,7 +1408,7 @@ http_send_outgoing(isc_nm_http_session_t *session, isc_nmhandle_t *httphandle, http_append_pending_send_request(session, httphandle, cb, cbarg); } - return false; + return; } /* We need to attach to the session->handle earlier because as an @@ -1537,10 +1537,9 @@ http_send_outgoing(isc_nm_http_session_t *session, isc_nmhandle_t *httphandle, isc_buffer_usedregion(send->pending_write_data, &send_data); session->data_in_flight += send_data.length; isc_nm_send(transphandle, &send_data, http_writecb, send); - return true; + return; nothing_to_send: isc_nmhandle_detach(&transphandle); - return false; } static inline bool @@ -1595,8 +1594,8 @@ http_do_bio(isc_nm_http_session_t *session, isc_nmhandle_t *send_httphandle, if (send_cb != NULL) { INSIST(VALID_NMHANDLE(send_httphandle)); - (void)http_send_outgoing(session, send_httphandle, send_cb, - send_cbarg); + http_send_outgoing(session, send_httphandle, send_cb, + send_cbarg); return; } @@ -1605,7 +1604,7 @@ http_do_bio(isc_nm_http_session_t *session, isc_nmhandle_t *send_httphandle, INSIST(send_cbarg == NULL); if (session->pending_write_data != NULL && session->sending == 0) { - (void)http_send_outgoing(session, NULL, NULL, NULL); + http_send_outgoing(session, NULL, NULL, NULL); return; } @@ -1660,8 +1659,7 @@ http_do_bio(isc_nm_http_session_t *session, isc_nmhandle_t *send_httphandle, */ http_do_bio_async(session); } else { - (void)http_send_outgoing(session, NULL, NULL, - NULL); + http_send_outgoing(session, NULL, NULL, NULL); } isc__nm_httpsession_detach(&tmpsess); @@ -1677,7 +1675,7 @@ http_do_bio(isc_nm_http_session_t *session, isc_nmhandle_t *send_httphandle, } /* we might have some data to send after processing */ - (void)http_send_outgoing(session, NULL, NULL, NULL); + http_send_outgoing(session, NULL, NULL, NULL); if (nghttp2_session_want_read(session->ngsession) == 0 && nghttp2_session_want_write(session->ngsession) == 0 &&