> On 28 Mar 2023, at 18:51, Roman Arutyunyan <arut@nginx.com> wrote:
>
> # HG changeset patch
> # User Roman Arutyunyan <arut@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.

# HG changeset patch
# User Sergey Kandaurov <pluknet@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;
}
# 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;
+
path->expires = ngx_current_msec + pto;

if (next == -1 || pto < next) {
# HG changeset patch
# User Sergey Kandaurov <pluknet@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;




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