[nginx] QUIC: do not increase underutilized congestion window.
Roman Arutyunyan
arut at nginx.com
Fri Apr 25 11:29:13 UTC 2025
Hi Vladimir,
Thanks for your patch, I will look into this. The issue indeed seems possible
and needs to be addressed. The mentiobned change cd5e4fa14 is correct though
since without it CWND used to skyrocket while being underutilized. And this
certainly consealed other issues.
Do you have a reliable way to trigger this?
On Fri, Apr 25, 2025 at 01:55:11PM +0300, Vladimir Homutov via nginx-devel wrote:
> missing attachments
>
> commit 0c7c9d6732a5fe3a3208286c8904db1851ac2cba
> Author: Vladimir Homutov <vl at inspert.ru>
> Date: Fri Apr 25 13:35:00 2025 +0300
>
> QUIC: fixed possible deadlock with ACKs not sent due to congestion.
>
> The commit cd5e4fa1446dff86fafc3b6ffcc11afd527a024f (QUIC: do not increase
> underutilized congestion window) lead to increased possibility of deadlock,
> caused by the fact that quic does not output any packets in congestion.
>
> If both client and server follow the same logic, it is possible that both
> ends are in the same state: waiting for ACKs to increase window, while
> being unable to send even ACK to unblock the other side.
>
> Since packets that contain ACK frames only are not congestion controlled,
> we are definitely allowed to send them, since we also need to respect
> the max_ack_delay. Currently, the max_ack_delay may be triggered and
> push handler is called, but the output does not send anything, because
> window is closed, thus nothing is sent.
>
> The patch allows to send ACK-only packets in case when the window is closed.
> Probably, this needs to be attached to the actual trigger of max_ack_delay
> timer, but it looks like suggested changes is goed enough for practical reasons.
>
> Also, RFC 9000 13.2.1 says:
> Since packets containing only ACK frames are not congestion controlled,
> an endpoint MUST NOT send more than one such packet in response to
> receiving an ack-eliciting packet.
>
> Probably, this also needs to be accounted.
>
> diff --git a/src/event/quic/ngx_event_quic_output.c b/src/event/quic/ngx_event_quic_output.c
> index a92a539f3..4e51518c0 100644
> --- a/src/event/quic/ngx_event_quic_output.c
> +++ b/src/event/quic/ngx_event_quic_output.c
> @@ -55,7 +55,8 @@ static ssize_t ngx_quic_send_segments(ngx_connection_t *c, u_char *buf,
> size_t len, struct sockaddr *sockaddr, socklen_t socklen, size_t segment);
> #endif
> static ssize_t ngx_quic_output_packet(ngx_connection_t *c,
> - ngx_quic_send_ctx_t *ctx, u_char *data, size_t max, size_t min);
> + ngx_quic_send_ctx_t *ctx, u_char *data, size_t max, size_t min,
> + ngx_uint_t ack_only);
> static void ngx_quic_init_packet(ngx_connection_t *c, ngx_quic_send_ctx_t *ctx,
> ngx_quic_header_t *pkt, ngx_quic_path_t *path);
> static ngx_uint_t ngx_quic_get_padding_level(ngx_connection_t *c);
> @@ -116,7 +117,7 @@ ngx_quic_create_datagrams(ngx_connection_t *c)
> ssize_t n;
> u_char *p;
> uint64_t preserved_pnum[NGX_QUIC_SEND_CTX_LAST];
> - ngx_uint_t i, pad;
> + ngx_uint_t i, pad, ack_only;
> ngx_quic_path_t *path;
> ngx_quic_send_ctx_t *ctx;
> ngx_quic_congestion_t *cg;
> @@ -131,7 +132,9 @@ ngx_quic_create_datagrams(ngx_connection_t *c)
> ngx_memzero(preserved_pnum, sizeof(preserved_pnum));
> #endif
>
> - while (cg->in_flight < cg->window) {
> + ack_only = (cg->in_flight >= cg->window);
> +
> + while ((cg->in_flight < cg->window) || ack_only) {
>
> p = dst;
>
> @@ -158,7 +161,7 @@ ngx_quic_create_datagrams(ngx_connection_t *c)
> return NGX_OK;
> }
>
> - n = ngx_quic_output_packet(c, ctx, p, len, min);
> + n = ngx_quic_output_packet(c, ctx, p, len, min, ack_only);
> if (n == NGX_ERROR) {
> return NGX_ERROR;
> }
> @@ -186,6 +189,9 @@ ngx_quic_create_datagrams(ngx_connection_t *c)
>
> ngx_quic_commit_send(c);
>
> + /* send pure acks just once */
> + ack_only = 0;
> +
> path->sent += len;
> }
>
> @@ -331,7 +337,7 @@ ngx_quic_create_segments(ngx_connection_t *c)
> size_t len, segsize;
> ssize_t n;
> u_char *p, *end;
> - ngx_uint_t nseg, level;
> + ngx_uint_t nseg, level, ack_only;
> ngx_quic_path_t *path;
> ngx_quic_send_ctx_t *ctx;
> ngx_quic_congestion_t *cg;
> @@ -358,13 +364,15 @@ ngx_quic_create_segments(ngx_connection_t *c)
> level = ctx - qc->send_ctx;
> preserved_pnum[level] = ctx->pnum;
>
> + ack_only = (cg->in_flight >= cg->window);
> +
> for ( ;; ) {
>
> len = ngx_min(segsize, (size_t) (end - p));
>
> - if (len && cg->in_flight + (p - dst) < cg->window) {
> + if (len && ((cg->in_flight + (p - dst) < cg->window) || ack_only)) {
>
> - n = ngx_quic_output_packet(c, ctx, p, len, len);
> + n = ngx_quic_output_packet(c, ctx, p, len, len, ack_only);
> if (n == NGX_ERROR) {
> return NGX_ERROR;
> }
> @@ -397,6 +405,9 @@ ngx_quic_create_segments(ngx_connection_t *c)
>
> ngx_quic_commit_send(c);
>
> + /* send pure acks just once */
> + ack_only = 0;
> +
> path->sent += n;
>
> p = dst;
> @@ -521,7 +532,7 @@ ngx_quic_get_padding_level(ngx_connection_t *c)
>
> static ssize_t
> ngx_quic_output_packet(ngx_connection_t *c, ngx_quic_send_ctx_t *ctx,
> - u_char *data, size_t max, size_t min)
> + u_char *data, size_t max, size_t min, ngx_uint_t ack_only)
> {
> size_t len, pad, min_payload, max_payload;
> u_char *p;
> @@ -589,6 +600,12 @@ ngx_quic_output_packet(ngx_connection_t *c, ngx_quic_send_ctx_t *ctx,
> break;
> }
>
> + if (ack_only
> + && (f->type != NGX_QUIC_FT_ACK && f->type != NGX_QUIC_FT_ACK_ECN))
> + {
> + continue;
> + }
> +
> if (len + f->len > max_payload) {
> rc = ngx_quic_split_frame(c, f, max_payload - len);
>
> _______________________________________________
> nginx-devel mailing list
> nginx-devel at nginx.org
> https://mailman.nginx.org/mailman/listinfo/nginx-devel
--
Roman
More information about the nginx-devel
mailing list