Specify MaxAckDelay for TLP and RTO #991

ianswett · 2017-12-05T20:01:15Z

Closes #981

janaiyengar

Thanks for writing this up. A few comments.

janaiyengar · 2017-12-05T20:12:21Z

draft-ietf-quic-recovery.md

-
-(TODO: Add negotiability of delayed ack timeout.)
+MaxAckDelay is the maximum ack delay supplied in an incoming ack frame.
+Ack delays that are aren't used for an RTT sample or reference ack-only packets


nit: It's not clear what it means to say Ack delays that reference ack-only packets... I would leave this at "aren't used for an RTT sample.", and remove "or reference ack-only packets"

janaiyengar · 2017-12-05T20:13:11Z

draft-ietf-quic-recovery.md

-  was sent, and a bytes field indicating the packet's size.  sent_packets is
-  ordered by packet number, and packets remain in sent_packets until
-  acknowledged or lost.
+  was sent, a boolean indicating whether the packet is ack only, and a bytes


s/ack only/ackable. I think this constraint needs to be more than just ack only packets.

As discussed, in a future PR.

janaiyengar · 2017-12-05T20:13:46Z

draft-ietf-quic-recovery.md

@@ -569,6 +573,7 @@ Pseudocode for OnPacketSent follows:
   largest_sent_packet = packet_number
   sent_packets[packet_number].packet_number = packet_number
   sent_packets[packet_number].time = now
+   sent_packets[packet_number].ack_only = is_ack_only


nit: is_ackable

janaiyengar · 2017-12-05T20:15:27Z

draft-ietf-quic-recovery.md

@@ -602,6 +607,10 @@ Pseudocode for OnAckReceived and UpdateRtt follow:
     // Adjust for ack delay if it's plausible.
     if (latest_rtt - min_rtt > ack_delay):
       latest_rtt -= ack_delay
+       // Only save into max ack delay if it's used
+       // for rtt calculation and is not ack only.


janaiyengar

Ok, let's leave the ackable discussion to later; I'm fine with ack only. There's one nit I'll leave you to address.

janaiyengar

Thanks for adding more explanatory text. A few comments.

janaiyengar · 2017-12-05T20:51:17Z

draft-ietf-quic-recovery.md

-exactly 1*SRTT may generate spurious probes, and 2*SRTT is simply the next
-integral value of RTT.
+QUIC diverges from TCP by calculating MaxAckDelay dynamically, instead of
+assuming a constant value for all connections.  It includes this in all probe


TCP doesn't assume a constant value for all connections. I would say something like "instead of implicitly including it in the SRTT and RTTVAR estimates".

I state that below, but I updated this text to be a bit more specific.

janaiyengar · 2017-12-05T20:52:01Z

draft-ietf-quic-recovery.md

-The values of 2 and 1.5 are based on
-{{?LOSS-PROBE=I-D.dukkipati-tcpm-tcp-loss-probe}}, but implementations MAY
-experiment with other constants.
+A PTO value of at least 1.5*SRTT ensures that the ACK is overdue.  The of 1.5


missing word: "The value of".

janaiyengar · 2017-12-05T20:53:22Z

draft-ietf-quic-recovery.md

@@ -372,10 +366,17 @@ one significantly increases resilience to packet drop in both directions, thus
 reducing the probability of consecutive RTO events.

 QUIC's RTO algorithm differs from TCP in that the firing of an RTO alarm is not
-considered a strong enough signal of packet loss. An RTO alarm fires only when
+considered a strong enough signal of packet loss, so does not result in an
+immediate change to CWND or recovery state. An RTO alarm fires only when


is CWND defined by here? or should it be "congestion window"? I'd prefer spelling it out in any case.

janaiyengar · 2017-12-05T20:53:40Z

draft-ietf-quic-recovery.md

 there's a prolonged period of network silence, which could be caused by a change
 in the underlying network RTT.

+QUIC also diverges from TCP by including MaxAckDelay in the RTO period.  QUIC is
+able to explicitly model the ack delay via the ack delay field in the ack frame.
+Because QUIC is subtracting this delay from both SRTT and it is expected to


s/is subtracting/subtracts/
It doesn't subtract this delay from SRTT... also this sentence is a bit of a run-on. Suggestion: "Since QUIC ignores this delay in its SRTT and RTTVAR computations, it is necessary to add this delay explicitly in the RTO computation.".

Agreed, fixed.

ianswett

One comment I didn't fully address, but PTAL

ianswett · 2017-12-05T21:01:42Z

draft-ietf-quic-recovery.md

-exactly 1*SRTT may generate spurious probes, and 2*SRTT is simply the next
-integral value of RTT.
+QUIC diverges from TCP by calculating MaxAckDelay dynamically, instead of
+assuming a constant value for all connections.  It includes this in all probe


I state that below, but I updated this text to be a bit more specific.

ianswett · 2017-12-05T21:01:48Z

draft-ietf-quic-recovery.md

@@ -372,10 +366,17 @@ one significantly increases resilience to packet drop in both directions, thus
 reducing the probability of consecutive RTO events.

 QUIC's RTO algorithm differs from TCP in that the firing of an RTO alarm is not
-considered a strong enough signal of packet loss. An RTO alarm fires only when
+considered a strong enough signal of packet loss, so does not result in an
+immediate change to CWND or recovery state. An RTO alarm fires only when


ianswett · 2017-12-05T21:02:56Z

draft-ietf-quic-recovery.md

 there's a prolonged period of network silence, which could be caused by a change
 in the underlying network RTT.

+QUIC also diverges from TCP by including MaxAckDelay in the RTO period.  QUIC is
+able to explicitly model the ack delay via the ack delay field in the ack frame.
+Because QUIC is subtracting this delay from both SRTT and it is expected to


Agreed, fixed.

ianswett · 2017-12-05T21:03:14Z

draft-ietf-quic-recovery.md

-  was sent, and a bytes field indicating the packet's size.  sent_packets is
-  ordered by packet number, and packets remain in sent_packets until
-  acknowledged or lost.
+  was sent, a boolean indicating whether the packet is ack only, and a bytes


As discussed, in a future PR.

ianswett · 2017-12-05T21:03:19Z

draft-ietf-quic-recovery.md

@@ -569,6 +573,7 @@ Pseudocode for OnPacketSent follows:
   largest_sent_packet = packet_number
   sent_packets[packet_number].packet_number = packet_number
   sent_packets[packet_number].time = now
+   sent_packets[packet_number].ack_only = is_ack_only


ianswett · 2017-12-05T21:03:23Z

draft-ietf-quic-recovery.md

@@ -602,6 +607,10 @@ Pseudocode for OnAckReceived and UpdateRtt follow:
     // Adjust for ack delay if it's plausible.
     if (latest_rtt - min_rtt > ack_delay):
       latest_rtt -= ack_delay
+       // Only save into max ack delay if it's used
+       // for rtt calculation and is not ack only.


ianswett · 2017-12-05T21:22:32Z

Also fixes #912

janaiyengar

one nit, but lgtm

janaiyengar · 2017-12-05T22:25:15Z

draft-ietf-quic-recovery.md

-integral value of RTT.
+QUIC diverges from TCP by calculating MaxAckDelay dynamically, instead of
+assuming a constant value for all connections.  It includes this in all probe
+timeouts, because it assume the ack delay may come into play, regardless of


s/assume/assumes

janaiyengar · 2017-12-05T22:27:28Z

draft-ietf-quic-recovery.md

+change in the underlying network RTT.
+
+QUIC also diverges from TCP by including MaxAckDelay in the RTO period.  QUIC is
+able to explicitly model the ack delay via the ack delay field in the ack frame.


s/model the ack delay/model delay at the receiver/
s/ack frame/ACK frame/

Specify MaxAckDelay for TLP and RTO

989207a

Closes #981

ianswett requested a review from janaiyengar December 5, 2017 20:01

ianswett added 2 commits December 5, 2017 15:06

Update draft-ietf-quic-recovery.md

372c318

Update draft-ietf-quic-recovery.md

7c508ce

janaiyengar reviewed Dec 5, 2017

View reviewed changes

ianswett added 7 commits December 5, 2017 15:37

Add RTO rationale

e1d7bac

Update draft-ietf-quic-recovery.md

2578425

Update draft-ietf-quic-recovery.md

f14f41f

Update draft-ietf-quic-recovery.md

a1b6798

Update draft-ietf-quic-recovery.md

3ba73be

Update draft-ietf-quic-recovery.md

ddcf6d9

Update draft-ietf-quic-recovery.md

6f644ac

janaiyengar reviewed Dec 5, 2017

View reviewed changes

ianswett commented Dec 5, 2017

View reviewed changes

ianswett added 2 commits December 5, 2017 16:04

Comments

c58974b

Update draft-ietf-quic-recovery.md

69547db

janaiyengar approved these changes Dec 5, 2017

View reviewed changes

s/ack/ACK

ba55348

ianswett merged commit e29b203 into master Dec 5, 2017

martinthomson mentioned this pull request Dec 6, 2017

Don't update RTT for ack-only packets #984

Closed

martinthomson deleted the ianswett-maxackdelay branch December 6, 2017 19:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Specify MaxAckDelay for TLP and RTO #991

Specify MaxAckDelay for TLP and RTO #991

ianswett commented Dec 5, 2017

janaiyengar left a comment

janaiyengar Dec 5, 2017

janaiyengar Dec 5, 2017

ianswett Dec 5, 2017

janaiyengar Dec 5, 2017

ianswett Dec 5, 2017

janaiyengar Dec 5, 2017

ianswett Dec 5, 2017

janaiyengar left a comment

janaiyengar left a comment

janaiyengar Dec 5, 2017

ianswett Dec 5, 2017

janaiyengar Dec 5, 2017

janaiyengar Dec 5, 2017

ianswett Dec 5, 2017

janaiyengar Dec 5, 2017

ianswett Dec 5, 2017

ianswett left a comment

ianswett Dec 5, 2017

ianswett Dec 5, 2017

ianswett Dec 5, 2017

ianswett Dec 5, 2017

ianswett Dec 5, 2017

ianswett Dec 5, 2017

ianswett commented Dec 5, 2017

janaiyengar left a comment

janaiyengar Dec 5, 2017

janaiyengar Dec 5, 2017

Specify MaxAckDelay for TLP and RTO #991

Specify MaxAckDelay for TLP and RTO #991

Conversation

ianswett commented Dec 5, 2017

janaiyengar 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

janaiyengar left a comment

Choose a reason for hiding this comment

janaiyengar 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

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

ianswett commented Dec 5, 2017

janaiyengar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment