> On 1 Aug 2023, at 11:45, Roman Arutyunyan <arut@nginx.com> wrote:
>
> # HG changeset patch
> # User Roman Arutyunyan <arut@nginx.com>
> # Date 1690873324 -14400
> # Tue Aug 01 11:02:04 2023 +0400
> # Node ID cd0ef56b0f1afaa54d7d2756dad2182628445e04
> # Parent 741deb8ff8257914312ab134f3a0b69256c661f4
> QUIC: fixed probe-congestion deadlock.
>
> When probe timeout expired while congestion window was exhausted, probe PINGs
> could not be sent. As a result, lost packets could not be declared lost and
> congestion window could not be freed for new packets. This deadlock
> continued until connection idle timeout expiration.
>
> Now PINGs are sent separately from the frame queue without congestion control.

Which is supported by this clause in RFC 9002, section 7:

An endpoint MUST NOT send a packet if it would cause bytes_in_flight (see
Appendix B.2) to be larger than the congestion window, unless the packet
is sent on a PTO timer expiration

>
> diff --git a/src/event/quic/ngx_event_quic_ack.c b/src/event/quic/ngx_event_quic_ack.c
> --- a/src/event/quic/ngx_event_quic_ack.c
> +++ b/src/event/quic/ngx_event_quic_ack.c
> @@ -820,9 +820,9 @@ ngx_quic_pto_handler(ngx_event_t *ev)
> {
> ngx_uint_t i;
> ngx_msec_t now;
> - ngx_queue_t *q, *next;
> + ngx_queue_t *q;
> ngx_connection_t *c;
> - ngx_quic_frame_t *f;
> + ngx_quic_frame_t *f, frame;
> ngx_quic_send_ctx_t *ctx;
> ngx_quic_connection_t *qc;
>
> @@ -859,63 +859,23 @@ ngx_quic_pto_handler(ngx_event_t *ev)
> "quic pto %s pto_count:%ui",
> ngx_quic_level_name(ctx->level), qc->pto_count);
>
> - for (q = ngx_queue_head(&ctx->frames);
> - q != ngx_queue_sentinel(&ctx->frames);
> - /* void */)
> - {
> - next = ngx_queue_next(q);
> - f = ngx_queue_data(q, ngx_quic_frame_t, queue);
> + ngx_memzero(&frame, sizeof(ngx_quic_frame_t));
>
> - if (f->type == NGX_QUIC_FT_PING) {
> - ngx_queue_remove(q);
> - ngx_quic_free_frame(c, f);
> - }
> -
> - q = next;
> - }
> -
> - for (q = ngx_queue_head(&ctx->sent);
> - q != ngx_queue_sentinel(&ctx->sent);
> - /* void */)
> - {
> - next = ngx_queue_next(q);
> - f = ngx_queue_data(q, ngx_quic_frame_t, queue);
> + frame.level = ctx->level;
> + frame.type = NGX_QUIC_FT_PING;
>
> - if (f->type == NGX_QUIC_FT_PING) {
> - ngx_quic_congestion_lost(c, f);
> - ngx_queue_remove(q);
> - ngx_quic_free_frame(c, f);
> - }
> -
> - q = next;
> - }

Removing of handling PING frames in at least ctx->sent looks premature.
Consider PTO happened after sending PING in response to PATH_CHALLENGE
on an active path, as part of RFC 9000, 9.3.3. In this case, such PING
won't be considered lost, which means we won't enter recovery period.

> -
> - /* enforce 2 udp datagrams */

The comment is removed, though it's obvious now and should go probably.

> -
> - f = ngx_quic_alloc_frame(c);
> - if (f == NULL) {
> - break;
> + if (ngx_quic_frame_sendto(c, &frame, 0, qc->path) != NGX_OK
> + || ngx_quic_frame_sendto(c, &frame, 0, qc->path) != NGX_OK)
> + {
> + ngx_quic_close_connection(c, NGX_ERROR);
> + return;
> }
> -
> - f->level = ctx->level;
> - f->type = NGX_QUIC_FT_PING;
> - f->flush = 1;
> -
> - ngx_quic_queue_frame(qc, f);
> -
> - f = ngx_quic_alloc_frame(c);
> - if (f == NULL) {
> - break;
> - }
> -
> - f->level = ctx->level;
> - f->type = NGX_QUIC_FT_PING;
> -
> - ngx_quic_queue_frame(qc, f);
> }
>
> qc->pto_count++;
>
> + ngx_quic_set_lost_timer(c);
> +
> ngx_quic_connstate_dbg(c);
> }
>
> diff --git a/src/event/quic/ngx_event_quic_output.c b/src/event/quic/ngx_event_quic_output.c
> --- a/src/event/quic/ngx_event_quic_output.c
> +++ b/src/event/quic/ngx_event_quic_output.c
> @@ -645,10 +645,6 @@ ngx_quic_output_packet(ngx_connection_t
> f->plen = 0;
>
> nframes++;
> -
> - if (f->flush) {
> - break;
> - }
> }
>
> if (nframes == 0) {
> diff --git a/src/event/quic/ngx_event_quic_transport.h b/src/event/quic/ngx_event_quic_transport.h
> --- a/src/event/quic/ngx_event_quic_transport.h
> +++ b/src/event/quic/ngx_event_quic_transport.h
> @@ -271,7 +271,6 @@ struct ngx_quic_frame_s {
> ssize_t len;
> unsigned need_ack:1;
> unsigned pkt_need_ack:1;
> - unsigned flush:1;
>
> ngx_chain_t *data;
> union {

Otherwise, looks good.

--
Sergey Kandaurov
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
https://mailman.nginx.org/mailman/listinfo/nginx-devel