Subject: Re: [quicwg/base-drafts] Editorial rework (#2080)

From: ianswett <[email-masked]>
Date: Sat, 01 Dec 2018 05:53:18 -0800

ianswett commented on this pull request.

Thanks Jana, I think this is a nice improvement on my PR

> -fraction of an RTT, but implementations MAY experiment with absolute
-thresholds. The RECOMMENDED time threshold, expressed as a fraction
-of the round-trip time (kTimeThreshold), is 1/8.
-
-An endpoint SHOULD declare packets lost no earlier than
-(1 + kTimeThreshold) * max(SRTT, latest_RTT) after when they were
-sent.  If packets sent prior to the largest acknowledged packet cannot yet
-be declared lost, then a timer SHOULD be set for the remaining time.
+{{?RFC6675}}, and RACK {{?RACK=I-D.ietf-tcpm-rack}}. This section provides an
+overview of how these algorithms are implemented in QUIC.
+
+When an ACK frame is received that newly acknowledges a packet, a QUIC sender
+uses two thresholds to determine if prior packets should be declared lost.  A
+packet is declared lost under the following conditions:
+
+* it has a smaller packet number than the newly acknowledged packet,

I would prefer merging the first two bullets because I think it might flow better.
ie: "The packet is retransmittable, unacknowledged, and was sent prior to an acknowledged packet."

The removal of the mention of packet number was to capture the concept a bit more and we talk about packet number in the next bullet.

Also, we typically capitalize the first letter after the *, which I think looks better.

> @@ -596,13 +570,14 @@ kMaxTLPs:
   The RECOMMENDED value is 2.
 
 kPacketThreshold:
-: Maximum reordering in packet number space before FACK style loss detection
+: Maximum reordering in packets  before packet threshold loss detection

```suggestion
: Maximum reordering in packets before packet threshold loss detection
```

> -#### Time Threshold
-
-Time threshold loss detection uses a time threshold to determine how much
-reordering to tolerate.  In this document, the threshold is expressed as a
-fraction of an RTT, but implementations MAY experiment with absolute
-thresholds. The RECOMMENDED time threshold, expressed as a fraction
-of the round-trip time (kTimeThreshold), is 1/8.
-
-An endpoint SHOULD declare packets lost no earlier than
-(1 + kTimeThreshold) * max(SRTT, latest_RTT) after when they were
-sent.  If packets sent prior to the largest acknowledged packet cannot yet
-be declared lost, then a timer SHOULD be set for the remaining time.
+{{?RFC6675}}, and RACK {{?RACK=I-D.ietf-tcpm-rack}}. This section provides an
+overview of how these algorithms are implemented in QUIC.
+
+When an ACK frame is received that newly acknowledges a packet, a QUIC sender

I'm now thinking we should drop newly because we also trigger loss on a timer.  If there's something subtle we're trying to say there, we should probably make it more explicit?

At some point, we discussed basing loss detection on the largest recently acknowledged packet(ie: in the last ACK), but I don't think that's what the pseudocode does and I don't think that's what the current gQUIC code does.  If we want to discuss that, maybe add a paragraph that the algorithm may be enhanced by doing so and it may reduce spurious retransmits in some cases?

> +upon detecting loss.  Implementations that detect spurious retransmissions and
+increase the reordering threshold in packets or time MAY choose to start with
+smaller initial reordering thresholds to minimize recovery latency.
+
+### Packet Threshold
+
+The RECOMMENDED initial value for the packet reordering threshold
+(kPacketThreshold) is 3, based on TCP loss detection {{?RFC5681}} {{?RFC6675}}.
+
+Some networks may exhibit higher degrees of reordering, causing a sender to
+detect spurious losses.  Implementers MAY use algorithms developed for TCP, such
+as TCP-NCR {{?RFC4653}}, to improve QUIC's reordering resilience.
+
+### Time Threshold {#time-threshold}
+
+Ack-based loss detection uses a time threshold to determine how much reordering

I find changing this sentence to start with "Ack-based" is confusing, because there's also the packet number threshold.

How about dropping this first sentence entirely, since I think it's said better above?

-- 
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/2080#pullrequestreview-180523967

---

From: janaiyengar <[email-masked]>
Date: Tue, 04 Dec 2018 02:31:43 +0000 (UTC)

@janaiyengar pushed 1 commit.

8a695c9e94c50c800ef44ed1f0153a6fc68bce6e  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/2080/files/1088bb6e26d782d13ee05b85aa2985111ee33c5c..8a695c9e94c50c800ef44ed1f0153a6fc68bce6e

---

From: janaiyengar <[email-masked]>
Date: Mon, 03 Dec 2018 18:42:44 -0800

janaiyengar commented on this pull request.

-

This is a good point. In fact, I think we may want to drop the text that says this is triggered on an receipt of an ACK only, because that's not necessary. Sure, that's when we check it (and when the ER timer fires), but that's not required.  I've changed the text above to reflect this.

-- 
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/2080#discussion_r238513348

---

From: janaiyengar <[email-masked]>
Date: Mon, 03 Dec 2018 18:43:21 -0800

Done, and moved this para below. The ordering is better now I think.

-- 
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/2080#discussion_r238513431

---

From: janaiyengar <[email-masked]>
Date: Mon, 03 Dec 2018 18:43:43 -0800

006843e6dd84ca4a6123814deac552c7fdb2a716  Ian comments

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/2080/files/8a695c9e94c50c800ef44ed1f0153a6fc68bce6e..006843e6dd84ca4a6123814deac552c7fdb2a716

---

From: janaiyengar <[email-masked]>
Date: Mon, 03 Dec 2018 18:45:07 -0800

d0959523e25f3958808ce205c68a2bb066737334  edit

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/2080/files/006843e6dd84ca4a6123814deac552c7fdb2a716..d0959523e25f3958808ce205c68a2bb066737334

---

From: janaiyengar <[email-masked]>
Date: Mon, 03 Dec 2018 18:49:30 -0800

000f6127997a5836baf513180cfbd7b64f10bf17  lint

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/2080/files/d0959523e25f3958808ce205c68a2bb066737334..000f6127997a5836baf513180cfbd7b64f10bf17

---

From: ianswett <[email-masked]>
Date: Mon, 03 Dec 2018 18:50:19 -0800

ianswett 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/2080#pullrequestreview-181073043

---

From: janaiyengar <[email-masked]>
Date: Mon, 03 Dec 2018 18:51:40 -0800

Merged #2080 into ianswett-both-thresholds.

-- 
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/2080#event-2003152916