From 96e8ea12455d6a6c31c9be6bbbe0dfcd76c79b57 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 9a1249feba..ced80dd9ae 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 @@ -1054,6 +1056,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) { @@ -1105,13 +1120,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; } @@ -1320,6 +1336,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)); @@ -1509,6 +1527,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; From 6b9387e2ee69f6c6a1bde502dcd096d8135bb862 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 ced80dd9ae..50f2f2b737 100644 --- a/lib/isc/netmgr/http.c +++ b/lib/isc/netmgr/http.c @@ -1539,9 +1539,21 @@ 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; + /* + * 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_MIN(ISC_NETMGR_MAX_STREAM_CLIENTS_PER_CONN, - session->max_concurrent_streams); + ISC_MAX(ISC_NETMGR_MAX_STREAM_CLIENTS_PER_CONN, + session->max_concurrent_streams / 3); if (session->client) { return false; From 47e9b477428e752de09760e76bc53ecd42dcad90 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, 40 insertions(+), 13 deletions(-) diff --git a/lib/isc/netmgr/http.c b/lib/isc/netmgr/http.c index 50f2f2b737..e591d3a537 100644 --- a/lib/isc/netmgr/http.c +++ b/lib/isc/netmgr/http.c @@ -1367,6 +1367,22 @@ 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); + + 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) { @@ -1378,10 +1394,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); + + 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; } @@ -1448,16 +1479,8 @@ http_send_outgoing(isc_nm_http_session_t *session, isc_nmhandle_t *httphandle, * will flush the buffer. */ if (cb != NULL) { - isc__nm_uvreq_t *newcb = NULL; - - INSIST(VALID_NMHANDLE(httphandle)); - - newcb = isc__nm_uvreq_get(httphandle->sock); - 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 && @@ -1499,6 +1522,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 788e9252614d68a7a3eb0a435e4e3c8bb396e567 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 e591d3a537..eea87aa5d8 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); @@ -1383,7 +1383,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; @@ -1405,7 +1405,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) { @@ -1413,7 +1413,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; } /* @@ -1556,11 +1556,10 @@ 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 @@ -1616,8 +1615,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; } @@ -1626,7 +1625,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; } @@ -1681,8 +1680,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); @@ -1700,7 +1698,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 &&