On Fri, Apr 25, 2025 at 10:51:12PM +0400, Sergey Kandaurov wrote:
> On Fri, Apr 25, 2025 at 01:55:11PM +0300, Vladimir Homutov via nginx-devel
> wrote:
> > missing attachments
> >
>
> Thanks for the provided debug, it is really helpful.
>
> Looking into debug, it becomes clear that once we sent something
> (be it a large response or MTU probe) that temporarily exceeds
> the congestion window (which is permitted for MTU probes),
> we can no longer send acknowledgments.
>
> This is especially visible if we don't receive ACKs for some reason,
> such as in your case,
> to decrease our inflight counter, so the connection becomes stalled.
>
> Below is a fix I have made without looking into your version
> (no offense, it is purely for aesthetic reasons).
>
> Although it appears to be quite similar to yours, it's somewhat less
> intrusive. I tried not to break sending segments for exceeded
> congestion, as well as to send correct frames in the ack_only mode.
indeed, my patch misses the part about queue management, this is
required part.
I'm agree regarding segmentation - sending pure ACK is definitely not
the case for optimizations;
otherwise, looks good for me.
>
>
> diff --git a/src/event/quic/ngx_event_quic_output.c
> b/src/event/quic/ngx_event_quic_output.c
> index a92a539f3..3fc2091e0 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_int_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);
> @@ -131,8 +132,7 @@ ngx_quic_create_datagrams(ngx_connection_t *c)
> ngx_memzero(preserved_pnum, sizeof(preserved_pnum));
> #endif
>
> - while (cg->in_flight < cg->window) {
> -
> + do {
> p = dst;
>
> len = ngx_quic_path_limit(c, path, path->mtu);
> @@ -158,7 +158,8 @@ 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,
> + cg->in_flight >= cg->window);
> if (n == NGX_ERROR) {
> return NGX_ERROR;
> }
> @@ -187,7 +188,8 @@ ngx_quic_create_datagrams(ngx_connection_t *c)
> ngx_quic_commit_send(c);
>
> path->sent += len;
> - }
> +
> + } while (cg->in_flight < cg->window);
>
> return NGX_OK;
> }
> @@ -315,6 +317,10 @@ ngx_quic_allow_segmentation(ngx_connection_t *c)
>
> bytes += f->len;
>
> + if (qc->congestion->in_flight + bytes >= qc->congestion->window)) {
> + return 0;
> + }
> +
> if (bytes > len * 3) {
> /* require at least ~3 full packets to batch */
> return 1;
> @@ -364,7 +370,7 @@ ngx_quic_create_segments(ngx_connection_t *c)
>
> if (len && cg->in_flight + (p - dst) < cg->window) {
>
> - n = ngx_quic_output_packet(c, ctx, p, len, len);
> + n = ngx_quic_output_packet(c, ctx, p, len, len, 0);
> if (n == NGX_ERROR) {
> return NGX_ERROR;
> }
> @@ -521,7 +527,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_int_t ack_only)
> {
> size_t len, pad, min_payload, max_payload;
> u_char *p;
> @@ -585,6 +591,10 @@ ngx_quic_output_packet(ngx_connection_t *c,
> ngx_quic_send_ctx_t *ctx,
> {
> f = ngx_queue_data(q, ngx_quic_frame_t, queue);
>
> + if (ack_only && f->type != NGX_QUIC_FT_ACK) {
> + continue;
> + }
> +
> if (len >= max_payload) {
> break;
> }
> @@ -651,14 +661,20 @@ ngx_quic_output_packet(ngx_connection_t *c,
> ngx_quic_send_ctx_t *ctx,
> f->plen = res.len;
> }
>
> - while (nframes--) {
> + while (nframes) {
> q = ngx_queue_head(&ctx->frames);
> f = ngx_queue_data(q, ngx_quic_frame_t, queue);
>
> + if (ack_only && f->type != NGX_QUIC_FT_ACK) {
> + continue;
> + }
> +
> f->pkt_need_ack = pkt.need_ack;
>
> ngx_queue_remove(q);
> ngx_queue_insert_tail(&ctx->sending, q);
> +
> + nframes--;
> }
>
> return res.len;
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
https://mailman.nginx.org/mailman/listinfo/nginx-devel
