Welcome! Log In Create A New Profile

Advanced

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

Sergey Kandaurov
August 14, 2023 05:34AM
> On 14 Aug 2023, at 08:35, Roman Arutyunyan <arut@nginx.com> wrote:
>
> Hi,
>
> On Mon, Aug 14, 2023 at 12:18:48AM +0400, Sergey Kandaurov wrote:
>>
>>> 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
>
> Thanks. Updated commit message:
>
> 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,
> as specified by 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 (see Section 6.2) or when entering
> recovery (see Section 7.3.2).
>
>>> 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.
>
> The only reason why the code above was added was to prevent accumulation of
> multiple PINGs in the queue on multiple PTO expirations. Now that PINGs
> are no longer queued, there's no problem. The PINGs sent in response to
> PATH_CHALLENGE will stay in the queue until they are finally sent.
>

The code to remove PING frames sent on PTO predates the code for sending
PINGs in response to PATH_CHALLENGE, such that such PINGs are accumulated
in the sent queue as well and get removed on PTO event, entering recovery
period; they are considered lost on PTO event while they are actually not
to be (yet), so this is actually a correct change. Removing this code
will make such PINGs to stay in the sent queue available for resending
on packet loss, though this barely makes sense after PTO's PINGs.
What actually matters is that removing this code would fix entering
recovery period on PTO, which should not happen as discussed above.

Pondering on that more, I agree to remove this code, please commit.

>>> -
>>> - /* 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
> _______________________________________________
> nginx-devel mailing list
> nginx-devel@nginx.org
> https://mailman.nginx.org/mailman/listinfo/nginx-devel

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

[PATCH 0 of 5] QUIC congestion control fixes

Roman Arutyunyan 485 August 01, 2023 03:46AM

[PATCH 1 of 5] QUIC: avoid accessing freed frame

Roman Arutyunyan 124 August 01, 2023 03:46AM

Re: [PATCH 1 of 5] QUIC: avoid accessing freed frame

Sergey Kandaurov 121 August 12, 2023 09:28AM

[PATCH 2 of 5] QUIC: eliminated spurious recovery period

Roman Arutyunyan 131 August 01, 2023 03:46AM

Re: [PATCH 2 of 5] QUIC: eliminated spurious recovery period

Sergey Kandaurov 108 August 12, 2023 09:30AM

[PATCH 3 of 5] QUIC: fixed PTO expiration condition

Roman Arutyunyan 122 August 01, 2023 03:46AM

Re: [PATCH 3 of 5] QUIC: fixed PTO expiration condition

Sergey Kandaurov 117 August 12, 2023 11:10AM

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

Roman Arutyunyan 118 August 01, 2023 03:46AM

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

Sergey Kandaurov 117 August 13, 2023 04:20PM

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

Roman Arutyunyan 123 August 14, 2023 12:38AM

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

Sergey Kandaurov 128 August 14, 2023 05:34AM

[PATCH 5 of 5] QUIC: ignore blocked status in congestion event handlers

Roman Arutyunyan 122 August 01, 2023 03:46AM

Re: [PATCH 5 of 5] QUIC: ignore blocked status in congestion event handlers

Sergey Kandaurov 118 August 13, 2023 05:14PM



Sorry, you do not have permission to post/reply in this forum.

Online Users

Guests: 153
Record Number of Users: 8 on April 13, 2023
Record Number of Guests: 500 on July 15, 2024
Powered by nginx      Powered by FreeBSD      PHP Powered      Powered by MariaDB      ipv6 ready