Hi,
On Mon, Apr 24, 2023 at 04:15:21PM +0400, Sergey Kandaurov wrote:
>
>
> > On 28 Mar 2023, at 18:51, Roman Arutyunyan <a...@nginx.com> wrote:
> >
> > # HG changeset patch
> > # User Roman Arutyunyan <a...@nginx.com>
> > # Date 1679925333 -14400
> > # Mon Mar 27 17:55:33 2023 +0400
> > # Branch quic
> > # Node ID f76e83412133085a6c82fce2c3e15b2c34a6e959
> > # Parent 5fd628b89bb7fb5c95afa1dc914385f7ab79f6a3
> > QUIC: changed path validation timeout.
> >
> > 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 also resent separately from the regular
> > probe/lost detection mechanism. A path validation packet is resent 3 times,
> > each time after PTO expiration. Assuming constant PTO, the overall maximum
> > waiting time is 3 * PTO. According to RFC 9000, 8.2.4. Failed Path
> > Validation,
> > the following value is recommended as a validation timeout:
> >
> > A value of three times the larger of the current PTO
> > or the PTO for the new path (using kInitialRtt, as
> > defined in [QUIC-RECOVERY]) is RECOMMENDED.
> >
> > The change adds PTO of the new path to the equation as the lower bound.
> > Also, max_ack_delay is now always accounted for, unlike previously, when
> > it was only used when there are packets in flight. As mentioned before,
> > PACH_CHALLENGE is not considered in-flight by nginx since it's processed
> > separately, but technically it is.
>
> I don't like an idea to make a separate function to calculate
> time for path validation retransmits. It looks like an existing
> function could be reused.
>
> I tend to think checking for inflight packets in ngx_quic_pto()
> isn't correct at the first place. The condition comes from
> the GetPtoTimeAndSpace example in 9002, A.8:
>
> : GetPtoTimeAndSpace():
> : duration = (smoothed_rtt + max(4 * rttvar, kGranularity))
> : * (2 ^ pto_count)
> : // Anti-deadlock PTO starts from the current time
> : if (no ack-eliciting packets in flight):
> : assert(!PeerCompletedAddressValidation())
> : if (has handshake keys):
> : return (now() + duration), Handshake
> : else:
> : return (now() + duration), Initial
> : <..>
> : return pto_timeout, pto_space
>
> But PeerCompletedAddressValidation is always true for the server.
> The above anti-deadlock measure seems to only make sense for a client
> when it has no new data to send, but forced to send something to rise
> an anti-amplification limit for the server. This thought is supported
> by commentaries in places of GetPtoTimeAndSpace use.
>
> Removing the condition from ngx_quic_pto() makes possible to unify
> the function to use it for both regular PTO and path validation.
>
> Next is to make retransmits similar to a new connection establishment.
> Per RFC 9000, 8.2.1:
> : An endpoint SHOULD NOT probe a new path with packets containing a
> : PATH_CHALLENGE frame more frequently than it would send an Initial packet.
>
> I think we can improve path validation to use a separate backoff,
> path->tries can be used to base a backoff upon it.
>
> Since PATH_CHALLENGE are resent separately from the regular probe/lost
> detection mechanism, this needs to be moved out from ngx_quic_pto().
>
> This makes the following series based on your patch.
> We could set an overall maximum waiting time of 3 * PTO and test it
> in pv handler in addition to the check for NGX_QUIC_PATH_RETRIES.
Jftr, discussed all of the above in person. Agreed to implement that.
> # HG changeset patch
> # User Sergey Kandaurov <pluk...@nginx.com>
> # Date 1682332923 -14400
> # Mon Apr 24 14:42:03 2023 +0400
> # Branch quic
> # Node ID f49aba6e3fb54843d3e3bd5df26dbb45f5d3d687
> # Parent d6861ecf8a9cf4e98d9ed6f4435054d106b29f48
> QUIC: removed check for in-flight packets in computing PTO.
>
> The check is needed for clients in order to unblock a server due to
> anti-amplification limits, and it seems to make no sense for servers.
> See RFC 9002, A.6 and A.8 for a further explanation.
>
> This makes max_ack_delay to now always account, notably including
> PATH_CHALLENGE timers as noted in the last paragraph of 9000, 9.4,
> unlike when it was only used when there are packets in flight.
>
> While here, fixed nearby style.
>
> 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
> @@ -782,15 +782,11 @@ ngx_quic_pto(ngx_connection_t *c, ngx_qu
> qc = ngx_quic_get_connection(c);
>
> /* RFC 9002, Appendix A.8. Setting the Loss Detection Timer */
> +
> duration = qc->avg_rtt;
> -
> duration += ngx_max(4 * qc->rttvar, NGX_QUIC_TIME_GRANULARITY);
> duration <<= qc->pto_count;
>
> - if (qc->congestion.in_flight == 0) { /* no in-flight packets */
> - return duration;
> - }
> -
> if (ctx->level == ssl_encryption_application && c->ssl->handshaked) {
> duration += qc->ctp.max_ack_delay << qc->pto_count;
> }
Looks ok.
> # HG changeset patch
> # User Sergey Kandaurov <pluk...@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. Luckily, this is not possible, because when we start
validation of a path, the validation for the old path is stopped. This
allows us to simplify the PATH_RESPONSE and timeout handlers to only handle
qc->path. I suggest to add a patch for this. It will also affect PMTUD,
since the same handler will be responsible for MTU packets. Also, it's
probably a good idea to suspend PMTUD for the old path when we switch to a
new one. This will happend naturally if we support only one path in those
handlers.
> path->expires = ngx_current_msec + pto;
>
> if (next == -1 || pto < next) {
> # HG changeset patch
> # User Sergey Kandaurov <pluk...@nginx.com>
> # Date 1682338293 -14400
> # Mon Apr 24 16:11:33 2023 +0400
> # Branch quic
> # Node ID 760ee5baed4d1370a92f5d3a2b82d4a28ac8bae5
> # Parent 808fe808e276496a9b026690c141201720744ab3
> QUIC: lower bound path validation PTO.
>
> According to RFC 9000, 8.2.4. Failed Path Validation,
> the following value is recommended as a validation timeout:
>
> A value of three times the larger of the current PTO
> or the PTO for the new path (using kInitialRtt, as
> defined in [QUIC-RECOVERY]) is RECOMMENDED.
>
> The change adds PTO of the new path to the equation as the lower bound.
>
> 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
> @@ -511,7 +511,7 @@ ngx_quic_validate_path(ngx_connection_t
> }
>
> ctx = ngx_quic_get_send_ctx(qc, ssl_encryption_application);
> - pto = ngx_quic_pto(c, ctx);
> + pto = ngx_max(ngx_quic_pto(c, ctx), 1000);
>
> path->expires = ngx_current_msec + pto;
>
> @@ -605,7 +605,7 @@ ngx_quic_path_validation_handler(ngx_eve
> }
>
> if (++path->tries < NGX_QUIC_PATH_RETRIES) {
> - pto = ngx_quic_pto(c, ctx) << path->tries;
> + pto = ngx_max(ngx_quic_pto(c, ctx), 1000) << path->tries;
>
> path->expires = ngx_current_msec + pto;
Looks ok.
>
>
>
>
> --
> Sergey Kandaurov
> _______________________________________________
> nginx-devel mailing list
> nginx-devel@nginx.org
> https://mailman.nginx.org/mailman/listinfo/nginx-devel
--
Roman Arutyunyan
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
https://mailman.nginx.org/mailman/listinfo/nginx-devel
