On Tue, May 09, 2023 at 07:07:09PM +0400, Sergey Kandaurov wrote:
>
> > 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);
> }
> }

Looks good

--
Roman Arutyunyan
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
https://mailman.nginx.org/mailman/listinfo/nginx-devel