[PATCH 4 of 5] QUIC: fixed probe-congestion deadlock

[Sergey Kandaurov]

> On 1 Aug 2023, at 11:45, Roman Arutyunyan <arut at nginx.com> wrote:
> 
> # HG changeset patch
> # User Roman Arutyunyan <arut at 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