Subject: [quicwg/base-drafts] Editorial fixes (#1551)

From: ianswett <[email-masked]>
Date: Wed, 11 Jul 2018 19:14:51 -0700

Some editorial improvements from re-reading the document in it's entirety.
You can view, comment on, or merge this pull request online at:

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

-- Commit Summary --

  * Editorial fixes

-- File Changes --

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

-- Patch Links --

https://github.com/quicwg/base-drafts/pull/1551.patch
https://github.com/quicwg/base-drafts/pull/1551.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/1551

---

From: Martin Thomson <[email-masked]>
Date: Wed, 11 Jul 2018 19:25:15 -0700

martinthomson commented on this pull request.

A few suggestions.

> @@ -216,9 +215,9 @@ implemented in QUIC.
 
 An unacknowledged packet is marked as lost when an acknowledgment is received
 for a packet that was sent a threshold number of packets (kReorderingThreshold)
-after the unacknowledged packet. Receipt of the ack indicates that a later
-packet was received, while kReorderingThreshold provides some tolerance for
-reordering of packets in the network.
+and/or a threshold amount of time after the unacknowledged packet. Receipt of the
+ack indicates that a later packet was received, the reordering threshold

s/the ack/an acknowledgement/

Is this sentence missing an "If" at the start?  Maybe instead, "The reordering threshold provides some tolerance for reordering when acknowledgments indicate that packets were reordered."

> @@ -227,12 +226,13 @@ We derive this recommendation from TCP loss recovery {{?RFC5681}}
 reordering, causing a sender to detect spurious losses. Detecting spurious
 losses leads to unnecessary retransmissions and may result in degraded
 performance due to the actions of the congestion controller upon detecting
-loss. Implementers MAY use algorithms developed for TCP, such as TCP-NCR
-{{?RFC4653}}, to improve QUIC's reordering resilience, though care should be
-taken to map TCP specifics to QUIC correctly. Similarly, using time-based loss
-detection to deal with reordering, such as in PR-TCP, should be more readily
-usable in QUIC. Making QUIC deal with such networks is important open research,
-and implementers are encouraged to explore this space.
+loss and spurious retransmissions. Implementers MAY use algorithms developed
+for TCP, such as TCP-NCR {{?RFC4653}}, to improve QUIC's reordering resilience,
+though care should be taken to map TCP specifics to QUIC correctly.
+
+QUIC implementations may use time-based loss detection to deal with reordering,
+such as in PR-TCP. Making QUIC deal with such networks is important open
+research, and implementers are encouraged to explore this space.

MAY?  Cite and expand PR-TCP?

This paragraph looks like a good candidate for excision.  It's not really necessary for this document to encourage research.

> @@ -249,7 +249,7 @@ retransmittable packets are not acknowledged during this time, then these
 packets MUST be marked as lost.
 
 An endpoint SHOULD set the alarm such that a packet is marked as lost no earlier
-than 1.25 * max(SRTT, latest_RTT) since when it was sent.
+than 1.125 * max(SRTT, latest_RTT) since when it was sent.

This isn't really editorial.

> @@ -628,7 +629,7 @@ are as follows:
   this packet, but it is not retransmittable.
 
 * is_handshake_packet: A boolean that indicates whether a packet contains
-  handshake data.
+  a CRYPTO frame in a long header packet.

Maybe say " A boolean that indicates whether the packet contains cryptographic handshake messages critical to the completion of the QUIC handshake.  In this version of QUIC, this includes any packet with the long header that includes a CRYPTO frame."

> @@ -653,7 +654,7 @@ Pseudocode for OnPacketSent follows:
 
 ### On Receiving an Acknowledgment
 
-When an ACK frame is received, it may acknowledge 0 or more packets.
+When an ACK frame is received, it may acknowledge 0 or more new packets.

... it could newly acknowledge any number of packets.

-- 
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/1551#pullrequestreview-136482465

---

From: ianswett <[email-masked]>
Date: Wed, 11 Jul 2018 19:35:06 -0700

ianswett commented on this pull request.

True, but it's meant to align with the general 1/8 RTT reordering recommendation for time based loss detection.

-- 
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/1551#discussion_r201893416

---

From: ianswett <[email-masked]>
Date: Wed, 11 Jul 2018 19:43:07 -0700

@ianswett pushed 1 commit.

eec0d2f  Fixes

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/1551/files/4e6aec4c4bed67127b23b8b123e5cfe4e4eb24a1..eec0d2f342d891289f96db5d31483837c8e9651f

---

From: Mike Bishop <[email-masked]>
Date: Thu, 12 Jul 2018 08:33:29 -0700

MikeBishop commented on this pull request.

One comment throughout -- the use of lowercase "ack" reads oddly to me.  Is that typical of TCP docs?  I'd tend to either spell out "acknowledge[ment]" or refer to the (all-caps) "ACK" frame.

> @@ -216,23 +215,22 @@ implemented in QUIC.
 
 An unacknowledged packet is marked as lost when an acknowledgment is received
 for a packet that was sent a threshold number of packets (kReorderingThreshold)
-after the unacknowledged packet. Receipt of the ack indicates that a later
-packet was received, while kReorderingThreshold provides some tolerance for
-reordering of packets in the network.
-
-The RECOMMENDED initial value for kReorderingThreshold is 3.
-
-We derive this recommendation from TCP loss recovery {{?RFC5681}}
-{{?RFC6675}}. It is possible for networks to exhibit higher degrees of
-reordering, causing a sender to detect spurious losses. Detecting spurious
-losses leads to unnecessary retransmissions and may result in degraded
+and/or a threshold amount of time after the unacknowledged packet. Receipt of the
+ack indicates that a later packet was received, the reordering threshold

Comma splice; I'd either re-add the conjunction ("while" seemed fine) or replace the comma with a semicolon.

> -reordering of packets in the network.
-
-The RECOMMENDED initial value for kReorderingThreshold is 3.
-
-We derive this recommendation from TCP loss recovery {{?RFC5681}}
-{{?RFC6675}}. It is possible for networks to exhibit higher degrees of
-reordering, causing a sender to detect spurious losses. Detecting spurious
-losses leads to unnecessary retransmissions and may result in degraded
+and/or a threshold amount of time after the unacknowledged packet. Receipt of the
+ack indicates that a later packet was received, the reordering threshold
+provides some tolerance for reordering of packets in the network.
+
+The RECOMMENDED initial value for kReorderingThreshold is 3, based on 
+TCP loss recovery {{?RFC5681}} {{?RFC6675}}. Some networks may exhibit higher
+degrees of reordering, causing a sender to detect spurious losses. Detecting
+spurious losses leads to unnecessary retransmissions and may result in degraded

Perhaps it's simply a transport term of art, but talking about "detecting spurious losses" has the wrong connotation to me.  When we say that you've detected a spurious retransmission, it means you realized that retransmission was actually unnecessary; detecting spurious losses kind of feels like realizing a packet you thought was lost actually wasn't.

I know you're actually trying to talk about the initial incorrect conclusion, not the later realization it was incorrect.  Maybe it would be clearer to say that highly reordering networks "caus[e] a sender to incorrectly mark packets as lost.  Incorrect loss detection leads to...."?

-- 
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/1551#pullrequestreview-136704033

---

From: ianswett <[email-masked]>
Date: Sat, 14 Jul 2018 08:06:28 -0700

d3aa88a  1.125

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

---

From: ianswett <[email-masked]>
Date: Sat, 14 Jul 2018 12:33:20 -0700

-

Agreed, its either confusing or wrong, depending on your perspective.

How about spuriously detecting losses?

-- 
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/1551#discussion_r202523506

---

From: ianswett <[email-masked]>
Date: Sat, 14 Jul 2018 12:39:53 -0700

ccbbde3  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/1551/files/d3aa88aadb1cae4fa0bd3acd009bd560362e52dd..ccbbde30ea2ca4c3619f3f8f8728565d9acbb67f

---

From: ianswett <[email-masked]>
Date: Sat, 14 Jul 2018 12:43:09 -0700

6418fb7  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/1551/files/ccbbde30ea2ca4c3619f3f8f8728565d9acbb67f..6418fb72a4ea035bd1570b310ee0b4881bf618de

---

From: ianswett <[email-masked]>
Date: Sat, 14 Jul 2018 12:43:43 -0700

I think I addressed all the comments and I reverted the 1.25 -> 1.125 change and put it in my recovery slides.

-- 
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/1551#issuecomment-405045465

---

From: ianswett <[email-masked]>
Date: Sat, 14 Jul 2018 12:46:19 -0700

8fade2b  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/1551/files/6418fb72a4ea035bd1570b310ee0b4881bf618de..8fade2b55f7efb62b158f1fa9a75b5ff22b2fdca

---

From: MikkelFJ <[email-masked]>
Date: Sat, 14 Jul 2018 12:47:35 -0700

mikkelfj commented on this pull request.

-

somehow reminds me of https://en.wikiquote.org/wiki/Colossal_Cave_Adventure

-- 
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/1551#discussion_r202523791

---

From: janaiyengar <[email-masked]>
Date: Sat, 14 Jul 2018 14:40:52 -0700

janaiyengar approved this pull request.

-- 
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/1551#pullrequestreview-137249204

---

From: janaiyengar <[email-masked]>
Date: Sat, 14 Jul 2018 14:58:21 -0700

janaiyengar commented on this pull request.

> +the acknowledgement indicates that a later packet was received, while the
+reordering threshold provides some tolerance for reordering of packets in the
+network.
+
+The RECOMMENDED initial value for kReorderingThreshold is 3, based on
+TCP loss recovery {{?RFC5681}} {{?RFC6675}}. Some networks may exhibit higher
+degrees of reordering, causing a sender to detect spurious losses. Spuriously
+declaring packets lost leads to unnecessary retransmissions and may result in
+degraded performance due to the actions of the congestion controller upon
+detecting loss. Implementers MAY use algorithms developed for TCP, such as
+TCP-NCR {{?RFC4653}}, to improve QUIC's reordering resilience.
+
+QUIC implementations may use time-based loss detection to deal with reordering.
+time-based loss detection allows a certain amount of reordering above
+max(SRTT, latest_rtt).  The RECOMMENDED reordering tolerance is
+1/8 max(SRTT, latest_rtt).

This should be reworded. How about: "QUIC implementations can use time-based loss detection to handle reordering based on time elapsed since the packet was sent, instead of a packet reordering threshold. The RECOMMENDED time threshold, expressed as a fraction of the round-trip time (kTimeReorderingFraction), is 1/8." (The details of SRTT, etc are in the algo below, so I think being generic is ok here.)

-- 
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/1551#pullrequestreview-137249484

---

From: janaiyengar <[email-masked]>
Date: Sat, 14 Jul 2018 14:59:39 -0700

janaiyengar requested changes on this pull request.

Unapproving PR 

-- 
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/1551#pullrequestreview-137249511

---

From: ianswett <[email-masked]>
Date: Sat, 14 Jul 2018 15:18:57 -0700

dcba32d  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/1551/files/8fade2b55f7efb62b158f1fa9a75b5ff22b2fdca..dcba32d0efbc2135a448880c5fbe54ffc9c657f6

---

From: ianswett <[email-masked]>
Date: Sat, 14 Jul 2018 15:19:10 -0700

Suggestion accepted

-- 
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/1551#pullrequestreview-137250050

---

From: ianswett <[email-masked]>
Date: Sun, 15 Jul 2018 08:43:47 -0700

@MikeBishop I agree the use of ack is somewhat inconsistent.  I'd like to clean that up in a separate PR.

-- 
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/1551#issuecomment-405099976

---

From: janaiyengar <[email-masked]>
Date: Sun, 15 Jul 2018 09:27:07 -0700

one nit, but lgtm

> @@ -228,10 +228,10 @@ degraded performance due to the actions of the congestion controller upon
 detecting loss. Implementers MAY use algorithms developed for TCP, such as
 TCP-NCR {{?RFC4653}}, to improve QUIC's reordering resilience.
 
-QUIC implementations may use time-based loss detection to deal with reordering.
-time-based loss detection allows a certain amount of reordering above
-max(SRTT, latest_rtt).  The RECOMMENDED reordering tolerance is
-1/8 max(SRTT, latest_rtt).
+QUIC implementations can use time-based loss detection to handle reordering
+based on time elapsed since the packet was sent, instead of a packet reordering

slight rewording suggestion: "QUIC implementations can use time-based loss detection instead of a packet reordering threshold to handle reordering based on time elapsed since the packet was sent."

-- 
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/1551#pullrequestreview-137271440

---

From: ianswett <[email-masked]>
Date: Sun, 15 Jul 2018 10:39:48 -0700

5432507  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/1551/files/dcba32d0efbc2135a448880c5fbe54ffc9c657f6..54325072f5bf5fa000cb01785dd170a6ca6a61d6

---

From: ianswett <[email-masked]>
Date: Sun, 15 Jul 2018 10:40:01 -0700

Merged #1551.

-- 
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/1551#event-1734285069