> On 9 May 2023, at 16:58, Sergey Kandaurov <pluknet@nginx.com> wrote:
>
>>
>> On 9 May 2023, at 14:11, Sergey Kandaurov <pluknet@nginx.com> wrote:
>>
>>>
>>> On 3 May 2023, at 17:00, Roman Arutyunyan <arut@nginx.com> wrote:
>>>
>>> Hi,
>>>
>>> On Mon, Apr 24, 2023 at 04:15:21PM +0400, Sergey Kandaurov wrote:
>>>>
>>>>
>>>
>>>> [..]
>>>> # HG changeset patch
>>>> # User Sergey Kandaurov <pluknet@nginx.com>
>>>> # Date 1682338151 -14400
>>>> # Mon Apr 24 16:09:11 2023 +0400
>>>> # Branch quic
>>>> # Node ID 808fe808e276496a9b026690c141201720744ab3
>>>> # Parent f49aba6e3fb54843d3e3bd5df26dbb45f5d3d687
>>>> QUIC: separated path validation retransmit backoff.
>>>>
>>>> Path validation packets containing PATH_CHALLENGE frames are sent separately
>>>> from regular frame queue, because of the need to use a decicated path and pad
>>>> the packets. The packets are sent periodically, separately from the regular
>>>> probe/lost detection mechanism. A path validation packet is resent up to 3
>>>> times, each time after PTO expiration, with increasing per-path PTO backoff.
>>>>
>>>> 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
>>>> @@ -736,7 +736,8 @@ ngx_quic_set_lost_timer(ngx_connection_t
>>>>
>>>> q = ngx_queue_last(&ctx->sent);
>>>> f = ngx_queue_data(q, ngx_quic_frame_t, queue);
>>>> - w = (ngx_msec_int_t) (f->last + ngx_quic_pto(c, ctx) - now);
>>>> + w = (ngx_msec_int_t) (f->last + (ngx_quic_pto(c, ctx) << qc->pto_count)
>>>> + - now);
>>>>
>>>> if (w < 0) {
>>>> w = 0;
>>>> @@ -785,10 +786,9 @@ ngx_quic_pto(ngx_connection_t *c, ngx_qu
>>>>
>>>> duration = qc->avg_rtt;
>>>> duration += ngx_max(4 * qc->rttvar, NGX_QUIC_TIME_GRANULARITY);
>>>> - duration <<= qc->pto_count;
>>>>
>>>> if (ctx->level == ssl_encryption_application && c->ssl->handshaked) {
>>>> - duration += qc->ctp.max_ack_delay << qc->pto_count;
>>>> + duration += qc->ctp.max_ack_delay;
>>>> }
>>>>
>>>> return duration;
>>>> @@ -846,7 +846,9 @@ ngx_quic_pto_handler(ngx_event_t *ev)
>>>> continue;
>>>> }
>>>>
>>>> - if ((ngx_msec_int_t) (f->last + ngx_quic_pto(c, ctx) - now) > 0) {
>>>> + if ((ngx_msec_int_t) (f->last + (ngx_quic_pto(c, ctx) << qc->pto_count)
>>>> + - now) > 0)
>>>> + {
>>>> continue;
>>>> }
>>>>
>>>> diff --git a/src/event/quic/ngx_event_quic_migration.c b/src/event/quic/ngx_event_quic_migration.c
>>>> --- a/src/event/quic/ngx_event_quic_migration.c
>>>> +++ b/src/event/quic/ngx_event_quic_migration.c
>>>> @@ -496,6 +496,7 @@ ngx_quic_validate_path(ngx_connection_t
>>>> "quic initiated validation of path seq:%uL", path->seqnum);
>>>>
>>>> path->validating = 1;
>>>> + path->tries = 0;
>>>>
>>>> if (RAND_bytes(path->challenge1, 8) != 1) {
>>>> return NGX_ERROR;
>>>> @@ -513,7 +514,6 @@ ngx_quic_validate_path(ngx_connection_t
>>>> pto = ngx_quic_pto(c, ctx);
>>>>
>>>> path->expires = ngx_current_msec + pto;
>>>> - path->tries = NGX_QUIC_PATH_RETRIES;
>>>>
>>>> if (!qc->path_validation.timer_set) {
>>>> ngx_add_timer(&qc->path_validation, pto);
>>>> @@ -578,7 +578,6 @@ ngx_quic_path_validation_handler(ngx_eve
>>>> qc = ngx_quic_get_connection(c);
>>>>
>>>> ctx = ngx_quic_get_send_ctx(qc, ssl_encryption_application);
>>>> - pto = ngx_quic_pto(c, ctx);
>>>>
>>>> next = -1;
>>>> now = ngx_current_msec;
>>>> @@ -605,7 +604,9 @@ ngx_quic_path_validation_handler(ngx_eve
>>>> continue;
>>>> }
>>>>
>>>> - if (--path->tries) {
>>>> + if (++path->tries < NGX_QUIC_PATH_RETRIES) {
>>>> + pto = ngx_quic_pto(c, ctx) << path->tries;
>>>
>>> Here we schedule a timer for 2 * PTO or 4 * PTO. Technically, our path
>>> validation code allows several paths to be validated at the same time. If that
>>> happens, 2 * PTO and 4 * PTO will be too much for the first attempt for the new
>>> path, where the timeout is just PTO. As a result, the last path may wait 2x
>>> or 4x longer than needed.
>>
>> Agree it needs to be addressed.
>>
>> Below is the first glance to set the next available pv timer:
>> 1) in ngx_quic_validate_path(), a new path may own a shorter timer
>> than already established, especially after several timeouts
>> Previously, new path validation could be scheduled late.
>> 2) in ngx_quic_handle_path_response_frame(), a validated path
>> might left a spurious timer, while nothing more to validate
>> or remaining paths have more distant timer
>> This one is less severe, just extra work added.
>>
>> Unlike the two above, pv handler has a more complex
>> logic I decided not to touch it.
>>
>
> Updated patch, with timers logic moved inside
> similar to ngx_quic_set_lost_timer().
>
> # HG changeset patch
> # User Sergey Kandaurov <pluknet@nginx.com>
> # Date 1683636833 -14400
> # Tue May 09 16:53:53 2023 +0400
> # Branch quic
> # Node ID a40ce69032547110b1a10e953ca55dfc684f131d
> # Parent c9a6960c548501b5234ca7f7ff5ae23fad75b023
> QUIC: reschedule path validation on path insertion/removal.
>
> Two issues fixed:
> - new path validation could be scheduled late
> - a validated path could leave a spurious timer
>
> diff --git a/src/event/quic/ngx_event_quic_migration.c b/src/event/quic/ngx_event_quic_migration.c
> --- a/src/event/quic/ngx_event_quic_migration.c
> +++ b/src/event/quic/ngx_event_quic_migration.c
> @@ -16,6 +16,7 @@ static ngx_int_t ngx_quic_validate_path(
> ngx_quic_path_t *path);
> static ngx_int_t ngx_quic_send_path_challenge(ngx_connection_t *c,
> ngx_quic_path_t *path);
> +static void ngx_quic_set_path_timer(ngx_connection_t *c);
> static ngx_quic_path_t *ngx_quic_get_path(ngx_connection_t *c, ngx_uint_t tag);
>
>
> @@ -169,6 +170,8 @@ valid:
> path->validating = 0;
> path->limited = 0;
>
> + ngx_quic_set_path_timer(c);
> +
> return NGX_OK;
> }
>
> @@ -515,9 +518,7 @@ ngx_quic_validate_path(ngx_connection_t
>
> path->expires = ngx_current_msec + pto;
>
> - if (!qc->path_validation.timer_set) {
> - ngx_add_timer(&qc->path_validation, pto);
> - }
> + ngx_quic_set_path_timer(c);
>
> return NGX_OK;
> }
> @@ -563,6 +564,46 @@ ngx_quic_send_path_challenge(ngx_connect
> }
>
>
> +static void
> +ngx_quic_set_path_timer(ngx_connection_t *c)
> +{
> + ngx_msec_t now;
> + ngx_queue_t *q;
> + ngx_msec_int_t left, next;
> + ngx_quic_path_t *path;
> + ngx_quic_connection_t *qc;
> +
> + qc = ngx_quic_get_connection(c);
> +
> + now = ngx_current_msec;
> + next = -1;
> +
> + for (q = ngx_queue_head(&qc->paths);
> + q != ngx_queue_sentinel(&qc->paths);
> + q = ngx_queue_next(q))
> + {
> + path = ngx_queue_data(q, ngx_quic_path_t, queue);
> +
> + if (!path->validating) {
> + continue;
> + }
> +
> + left = path->expires - now;
> +
> + if (next == -1 || left < next) {
> + next = left;
> + }
> + }
> +
> + if (next == -1) {
> + ngx_del_timer(&qc->path_validation);
> +
> + } else {
> + ngx_add_timer(&qc->path_validation, next);
> + }
> +}
> +
> +
> void
> ngx_quic_path_validation_handler(ngx_event_t *ev)
> {
>

Addendum to handle negation expiration time.

Previously, when updating path validation timer, another path being
in the process of validation could already expire, notably when both
were scheduled on apparent migration.
This resulted in negative expiration time and could left a path
without a timer.

diff --git a/src/event/quic/ngx_event_quic_migration.c b/src/event/quic/ngx_event_quic_migration.c
--- a/src/event/quic/ngx_event_quic_migration.c
+++ b/src/event/quic/ngx_event_quic_migration.c
@@ -586,17 +586,18 @@ ngx_quic_set_path_timer(ngx_connection_t
}

left = path->expires - now;
+ left = ngx_max(left, 1);

if (next == -1 || left < next) {
next = left;
}
}

- if (next == -1) {
- ngx_del_timer(&qc->path_validation);
+ if (next != -1) {
+ ngx_add_timer(&qc->path_validation, next);

- } else {
- ngx_add_timer(&qc->path_validation, next);
+ } else if (qc->path_validation.timer_set) {
+ ngx_del_timer(&qc->path_validation);
}
}


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