Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Stop using initial timeout #4263

Merged
merged 3 commits into from
Nov 3, 2020
Merged

Stop using initial timeout #4263

merged 3 commits into from
Nov 3, 2020

Conversation

martinthomson
Copy link
Member

Address validation can use 3*PTO, just taking the maximum of the old and
new path PTO. The idea of initial timeout is easily replaced in
recovery by instead describing the effect on the PTO. The net effect is
the same, except that new paths often have to include the maximum ACK
delay, which this doesn't incorporate.

Closes #4261.
Closes #4262.

Address validation can use 3*PTO, just taking the maximum of the old and
new path PTO.  The idea of initial timeout is easily replaced in
recovery by instead describing the effect on the PTO.  The net effect is
the same, except that new paths often have to include the maximum ACK
delay, which this doesn't incorporate.

Closes #4261.
Closes #4262.
@martinthomson martinthomson added editorial An issue that does not affect the design of the protocol; does not require consensus. -transport -recovery labels Oct 27, 2020
@@ -632,8 +632,9 @@ in most cases and is less likely to spuriously retransmit data.

Resumed connections over the same network MAY use the previous connection's
final smoothed RTT value as the resumed connection's initial RTT. When no
previous RTT is available, the initial RTT SHOULD be set to 333ms, resulting in
a 1 second initial timeout, as recommended in {{?RFC6298}}.
previous RTT is available, the initial RTT SHOULD be set to 333ms. This
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer the existing text to the updated text. Is there something you're adding that is important?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"initial timeout" seems to be a concept that doesn't exist in 6298, but it seems to be implied something. And it is not defined. So I wanted to remove it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I forgot... The PTO is 3*kInitialRtt only for the handshake. A new path will be after the handshake is complete, so the value will have max_ack_delay added.


~~~
validation_timeout = max(3*PTO, 6*kInitialRtt)
validation_timeout = 3*max(PTO_current, PTO_new)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PTO_current and PTO_new aren't defined, so even thought I understand the intent, I'm no longer sure this pseudocode is helpful. Possibly it'd be better to remove it?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is probably good. I will do that.

Copy link
Contributor

@janaiyengar janaiyengar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer this, but let's take the corrected explicit computation from Ian's PR.

validation_timeout = max(3*PTO, 6*kInitialRtt)
~~~
current Probe Timeout (PTO) or the PTO for the new path (that is, using
kInitialRtt as defined in {{QUIC-RECOVERY}}) is RECOMMENDED.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add the computation back in? With the corrected values of course:

That is, validation timeout = max(3*PTO, 9*kInitialRtt)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But that is wrong. Because the PTO on the new path is 9*kInitialRtt on the handshake, but 9*kInitialRtt + 3*max_ack_delay on migration.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh hm. It's actually more than that, since rttvar is also set to kInitialRtt/2 initially. Ok, let's leave it as you have it then.

draft-ietf-quic-recovery.md Outdated Show resolved Hide resolved
Co-authored-by: Jana Iyengar <jri.ietf@gmail.com>
Copy link
Contributor

@janaiyengar janaiyengar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is correct. LGTM, but waiting for @ianswett to take a look.

@janaiyengar janaiyengar merged commit 5a506a5 into master Nov 3, 2020
@janaiyengar janaiyengar deleted the initial-timeout-begone branch November 3, 2020 23:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
-recovery -transport editorial An issue that does not affect the design of the protocol; does not require consensus.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Initial timeout in path validation failure
4 participants