Subject: [quicwg/base-drafts] Specify MaxAckDelay for TLP and RTO (#991)

From: ianswett <[email-masked]>
Date: Tue, 05 Dec 2017 20:01:17 +0000 (UTC)

Closes #981
You can view, comment on, or merge this pull request online at:

  https://github.com/quicwg/base-drafts/pull/991

-- Commit Summary --

  * Specify MaxAckDelay for TLP and RTO

-- File Changes --

    M draft-ietf-quic-recovery.md (26)

-- Patch Links --

https://github.com/quicwg/base-drafts/pull/991.patch
https://github.com/quicwg/base-drafts/pull/991.diff

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/991

---

From: ianswett <[email-masked]>
Date: Tue, 05 Dec 2017 20:06:21 +0000 (UTC)

@ianswett pushed 1 commit.

372c318  Update draft-ietf-quic-recovery.md

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/991/files/989207a697ce6e4379da3774876e22e275d252bf..372c3189eeb4366b51eab91b29ed62d5c2e3aa9e

---

From: ianswett <[email-masked]>
Date: Tue, 05 Dec 2017 20:06:58 +0000 (UTC)

7c508ce  Update draft-ietf-quic-recovery.md

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/991/files/372c3189eeb4366b51eab91b29ed62d5c2e3aa9e..7c508ce53f9a978c637cb25ba28b030b3e69e62b

---

From: janaiyengar <[email-masked]>
Date: Tue, 05 Dec 2017 12:15:52 -0800

janaiyengar commented on this pull request.

Thanks for writing this up. A few comments.

 * If there are more than one unacknowledged packets, PTO SHOULD be scheduled for
   max(2*SRTT, 10ms).
 
 * If RTO ({{rto}}) is earlier, schedule a TLP alarm in its place. That is,
   PTO SHOULD be scheduled for min(RTO, PTO).
 
-kDelayedAckTimeout is the expected delayed ACK timer.  When there is exactly one
-unacknowledged packet, the alarm duration includes time for a delayed
-acknowledgment to be received by including kDelayedAckTimeout.
-
-The RECOMMENDED value for kDelayedAckTimeout is 25ms.
-
-(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"

> @@ -517,9 +520,9 @@ transmit or exceeding the reordering window in time.
 sent_packets:
 : An association of packet numbers to information about them, including a number
   field indicating the packet number, a time field indicating the time a packet
-  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.

> @@ -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

> @@ -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.

ditto

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/991#pullrequestreview-81320437

---

From: janaiyengar <[email-masked]>
Date: Tue, 05 Dec 2017 12:24:03 -0800

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.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/991#pullrequestreview-81323628

---

From: ianswett <[email-masked]>
Date: Tue, 05 Dec 2017 20:37:58 +0000 (UTC)

e1d7bac  Add RTO rationale

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/991/files/7c508ce53f9a978c637cb25ba28b030b3e69e62b..e1d7bacc65c3850daaaa47e5d1ddde7c6ef70897

---

From: ianswett <[email-masked]>
Date: Tue, 05 Dec 2017 20:47:07 +0000 (UTC)

2578425  Update draft-ietf-quic-recovery.md

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/991/files/e1d7bacc65c3850daaaa47e5d1ddde7c6ef70897..25784256da325462780bd85ac61fc7d056e15e0f

---

From: ianswett <[email-masked]>
Date: Tue, 05 Dec 2017 12:47:50 -0800

f14f41f  Update draft-ietf-quic-recovery.md

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/991/files/25784256da325462780bd85ac61fc7d056e15e0f..f14f41f3b49e52cb820ca3000443827d36c090b1

---

From: ianswett <[email-masked]>
Date: Tue, 05 Dec 2017 20:48:35 +0000 (UTC)

a1b6798  Update draft-ietf-quic-recovery.md

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/991/files/f14f41f3b49e52cb820ca3000443827d36c090b1..a1b67988615e938f59178161b2c0472c3192802d

---

From: ianswett <[email-masked]>
Date: Tue, 05 Dec 2017 20:49:34 +0000 (UTC)

3ba73be  Update draft-ietf-quic-recovery.md

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/991/files/a1b67988615e938f59178161b2c0472c3192802d..3ba73bed8669b4990a3df9a8b1da5108fa154a6b

---

From: ianswett <[email-masked]>
Date: Tue, 05 Dec 2017 12:50:48 -0800

ddcf6d9  Update draft-ietf-quic-recovery.md

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/991/files/3ba73bed8669b4990a3df9a8b1da5108fa154a6b..ddcf6d977ca47ea4e363844f5c9f116619719163

---

From: ianswett <[email-masked]>
Date: Tue, 05 Dec 2017 20:56:15 +0000 (UTC)

6f644ac  Update draft-ietf-quic-recovery.md

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/991/files/ddcf6d977ca47ea4e363844f5c9f116619719163..6f644ac4c06453dd5653b717d3aa25180829fc41

---

From: janaiyengar <[email-masked]>
Date: Tue, 05 Dec 2017 20:58:04 +0000 (UTC)

Thanks for adding more explanatory text. A few comments.

-A PTO value of at least 2*SRTT ensures that the ACK is overdue. Using a PTO of
-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".

-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".

> @@ -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.

>  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.".

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/991#pullrequestreview-81331270

---

From: ianswett <[email-masked]>
Date: Tue, 05 Dec 2017 21:03:55 +0000 (UTC)

ianswett commented on this pull request.

One comment I didn't fully address, but PTAL

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

Done

Agreed, fixed.

 sent_packets:

As discussed, in a future PR.

ack

ack

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/991#pullrequestreview-81334071

---

From: ianswett <[email-masked]>
Date: Tue, 05 Dec 2017 21:04:52 +0000 (UTC)

c58974b  Comments

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/991/files/6f644ac4c06453dd5653b717d3aa25180829fc41..c58974b553d5599045faf1ed2e6496106c7ab45c

---

From: ianswett <[email-masked]>
Date: Tue, 05 Dec 2017 21:22:05 +0000 (UTC)

69547db  Update draft-ietf-quic-recovery.md

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/991/files/c58974b553d5599045faf1ed2e6496106c7ab45c..69547db50be6e4441b8d70bf17a42fd771915565

---

From: ianswett <[email-masked]>
Date: Tue, 05 Dec 2017 21:22:35 +0000 (UTC)

Also fixes #912 

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/991#issuecomment-349445672

---

From: janaiyengar <[email-masked]>
Date: Tue, 05 Dec 2017 22:28:38 +0000 (UTC)

janaiyengar approved this pull request.

one nit, but lgtm

+timeouts, because it assume the ack delay may come into play, regardless of

s/assume/assumes

> @@ -372,9 +366,15 @@ one significantly increases resilience to packet drop in both directions, thus
 
 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
-there's a prolonged period of network silence, which could be caused by a change
-in the underlying network RTT.
+considered a strong enough signal of packet loss, so does not result in an
+immediate change to congestion window or recovery state. An RTO alarm fires only
+when 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.

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

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/991#pullrequestreview-81357591

---

From: ianswett <[email-masked]>
Date: Tue, 05 Dec 2017 15:30:07 -0800

ba55348  s/ack/ACK

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/991/files/69547db50be6e4441b8d70bf17a42fd771915565..ba5534853ad3e685d3d7ddf3f1e83ef3a628a72f

---

From: ianswett <[email-masked]>
Date: Tue, 05 Dec 2017 15:31:18 -0800

Merged #991.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/991#event-1373767728