Adds substantive descriptions of loss detection mechanisms #801

janaiyengar · 2017-09-27T23:29:06Z

This isn't perfect, but I'd like to commit soon and iterate.

ianswett

Lots of comments, but I like the content and structure.

ianswett · 2017-09-28T13:22:23Z

draft-ietf-quic-recovery.md

-  * Handshake packets, which contain STREAM frames for stream 0, are
-    critical to QUIC transport and crypto negotiation, so a separate alarm
-    period is used for them.
+QUIC uses both ack information and timeouts to detect lost packets, and this


Also, ack information and timeouts is correct, but not super-descriptive and it might be worth saying once that this is sender side, even though it's obvious to us. Maybe something like: "QUIC senders use both received acknowledgements and timeouts to detect and recover from lost packets. This section provides..."

ianswett · 2017-09-28T13:34:26Z

draft-ietf-quic-recovery.md

+
+### Fast Retransmit
+
+An unacknowledged packet is marked as lost when an acknowledgment is received


Given I just had the conversation about "acknowledgement" and what it meant, do we need to clarify it somewhere in this doc?

I'm adding a TODO, but not doing this in this PR. I think we're going to need an early terminology section.

ianswett · 2017-09-28T13:36:28Z

draft-ietf-quic-recovery.md

+packet was received, while kReorderingThreshold provides some tolerance for
+reordering of packets in the network. 
+
+The RECOMMENDED value for kReorderingThreshold is 3.


RECOMMENDED initial value? Esp given you talk about changing it below.

ianswett · 2017-09-28T13:38:33Z

draft-ietf-quic-recovery.md

+
+We derive this default from recommendations for TCP loss recovery {{!RFC5681}}
+{{!RFC6675}}. It is possible for networks to exhibit higher degrees of
+reordering, causing a sender to spuriously detect losses. Spurious loss


nit: I think "detect spurious losses" reads a bit better, but I'm not sure.

Also, Spurious loss detection is sort of odd. I'd go with "Detecting spurious losses leads..."

ianswett · 2017-09-28T13:49:15Z

draft-ietf-quic-recovery.md

+Unacknowledged packets close to the tail may have fewer than
+kReorderingThreshold number of packets sent after them. Loss of such packets
+cannot be detected via Fast Retransmit. To enable ack-based loss detection of
+such packets, receipt of an acknowledgment for the last outstanding packet


Do you want to clarify that it's the last outstanding ackable packet?

Also, now that I read through this, it seems like it'd be worth doing early retransmit for any packets within kReorderingThreshold from the tail, but that's a sidethought.

Hm -- yeah, I think this is important. I'm adding ackable in a few places, including in TLP.

ianswett · 2017-09-28T14:19:49Z

draft-ietf-quic-recovery.md

+
+A Retransmission Timeout (RTO) alarm is the final backstop for loss
+detection. The algorithm used in QUIC is based on the RTO algorithm for TCP
+{{!RFC5681}} and is additionally resilient to spurious RTOs.


since you mention spurious RTOs, maybe reference 5682 as well?

ianswett · 2017-09-28T14:21:27Z

draft-ietf-quic-recovery.md

+
+* When the final TLP packet is sent, RTO is set to max(SRTT + 4*RTTVAR, minRTO)
+
+* When an RTO alarm fires, RTO is set to twice its current value.


nit; "RTO is doubled." seems a bit less ambiguous.

ianswett · 2017-09-28T14:24:42Z

draft-ietf-quic-recovery.md

+them.
+
+The initial flight has no prior RTT sample.  A client SHOULD remember
+the previous RTT it observed when resumption is attempted and use that for an


This is mostly a good recommendation, but if the client changes networks, does it still apply?

ianswett · 2017-09-28T14:26:47Z

draft-ietf-quic-recovery.md

+
+Endpoints MUST retransmit handshake frames if not acknowledged within a
+time limit. This time limit will start as the largest of twice the RTT value
+and MinTLPTimeout.  Each consecutive handshake retransmission doubles the


MinTLPTimeout is not defined in this text, it's just used above as 10ms when calculating the TLP alarm.

ianswett · 2017-09-28T14:28:39Z

draft-ietf-quic-recovery.md

+initial RTT value.  If no previous RTT is available, the initial RTT defaults
+to 100ms.
+
+Endpoints MUST retransmit handshake frames if not acknowledged within a


Currently we retransmit "all unacknowledged handshake frames", so we should probably say that?

lucas-clemente · 2017-09-29T09:20:29Z

draft-ietf-quic-recovery.md

+in the underlying network RTT.
+
+When an ack is received after one or more consecutive RTO alarm events, any
+packets with lower packet numbers than those acked MUST be marked as lost.


Does this apply even if the ACK doesn't ACK the RTO packets, i.e. it was an earlier delayed ACK?

Good point, I think it should not. In that case, we should fall out of RTO mode and go back into standard ack based recovery.

I've modified the text here, PTAL.

martinthomson · 2017-10-09T06:35:58Z

I can't work out what circle is complaining about here. Can you rebase this branch and push again? It might be easier to see any problem with the new CI setup.

martinthomson · 2017-10-09T08:17:52Z

draft-ietf-quic-recovery.md

-  * Handshake packets, which contain STREAM frames for stream 0, are
-    critical to QUIC transport and crypto negotiation, so a separate alarm
-    period is used for them.
+QUIC uses both ack information and timeouts to detect lost packets, and this


s/acked/acknowledged/g
s/ack information/acknowledgment information/g
s/ack-based/ACK-based/g or s/ack-based/acknowledgment-based/g

ianswett

Great improvement. We can tackle the TODOs next.

janaiyengar added the -recovery label Sep 27, 2017

janaiyengar self-assigned this Sep 27, 2017

janaiyengar requested a review from ianswett September 27, 2017 23:29

ianswett reviewed Sep 28, 2017

View reviewed changes

lucas-clemente reviewed Sep 29, 2017

View reviewed changes

ianswett mentioned this pull request Oct 1, 2017

bytes_in_flight for TLPs and RTOs #790

Closed

martinthomson reviewed Oct 9, 2017

View reviewed changes

janaiyengar mentioned this pull request Oct 10, 2017

Cleaning up Loss Detection overview #536

Closed

Jana Iyengar added 6 commits October 10, 2017 21:47

Added text to overview

4293f84

TLP text added

5ab54a4

More changes

6086394

finished RTO and HTO text

bc0f691

Ian's comments

92712da

lucas comment

3d757f4

janaiyengar force-pushed the newrecovery branch from 8cb4f4c to 3d757f4 Compare October 11, 2017 04:47

trailing whitespaces

f4602dc

ianswett approved these changes Oct 11, 2017

View reviewed changes

ianswett merged commit 8cf88aa into master Oct 11, 2017

martinthomson deleted the newrecovery branch October 12, 2017 23:52

ianswett mentioned this pull request Nov 13, 2017

Default RTT and Slow Start #590

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds substantive descriptions of loss detection mechanisms #801

Adds substantive descriptions of loss detection mechanisms #801

janaiyengar commented Sep 27, 2017

ianswett left a comment

ianswett Sep 28, 2017

ianswett Sep 28, 2017

janaiyengar Oct 9, 2017

ianswett Sep 28, 2017

ianswett Sep 28, 2017

ianswett Sep 28, 2017

ianswett Sep 28, 2017

janaiyengar Oct 10, 2017

ianswett Sep 28, 2017

ianswett Sep 28, 2017

ianswett Sep 28, 2017

ianswett Sep 28, 2017

ianswett Sep 28, 2017

lucas-clemente Sep 29, 2017 •

edited

Loading

ianswett Sep 29, 2017

janaiyengar Oct 11, 2017

martinthomson commented Oct 9, 2017

martinthomson Oct 9, 2017

ianswett left a comment


		### Fast Retransmit

		An unacknowledged packet is marked as lost when an acknowledgment is received


		* When the final TLP packet is sent, RTO is set to max(SRTT + 4*RTTVAR, minRTO)

		* When an RTO alarm fires, RTO is set to twice its current value.

Adds substantive descriptions of loss detection mechanisms #801

Adds substantive descriptions of loss detection mechanisms #801

Conversation

janaiyengar commented Sep 27, 2017

ianswett left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucas-clemente Sep 29, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martinthomson commented Oct 9, 2017

Choose a reason for hiding this comment

ianswett left a comment

Choose a reason for hiding this comment

lucas-clemente Sep 29, 2017 •

edited

Loading