> 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:
>>>
>>>
>>>> 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.
>>
>> Jftr, discussed all of the above in person. Agreed to implement that.
>>
>>> [..]
>>> # 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)
{
--
Sergey Kandaurov
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
https://mailman.nginx.org/mailman/listinfo/nginx-devel
