Subject: [quicwg/base-drafts] Persistent Congestion Time Threshold (#2365)

From: Nick Banks <[email-masked]>
Date: Wed, 23 Jan 2019 15:53:11 +0000 (UTC)

Updates the persistent congestion logic to be time based instead of PTO count based, to account for the cases where PTOs don't actually occur.

This is the simplest change I could come up with that fixes the main issue, though I think a completely separate timer would still be better than piggy-backing off the `CongestionEvent`. For instance, in the case the app is periodically sending (and therefore no PTOs occur), if the ACK eventually comes much later than when persistent congestion occurs (or should occur) the CC window only decreases when the ACK arrives; not when persistent congestion started. If you have a separate timer for persistent congestion the window could be reduced immediately instead of waiting for the ACK. For now, though, I chose to go with the simpler change/solution that still mostly gets the job done.
You can view, comment on, or merge this pull request online at:

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

-- Commit Summary --

  * Persistent Congestion Time Threshold

-- File Changes --

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

-- Patch Links --

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

---

From: ianswett <[email-masked]>
Date: Wed, 23 Jan 2019 20:56:39 +0000 (UTC)

ianswett commented on this pull request.

Thanks Nick, a few suggestions.

> @@ -1003,14 +1003,21 @@ packets might cause the sender's bytes in flight to exceed the congestion window
 until an acknowledgement is received that establishes loss or delivery of
 packets.
 
-When an ACK frame is received that establishes loss of all in-flight packets
-sent prior to a threshold number of consecutive PTOs (pto_count is more than
-kPersistentCongestionThreshold, see {{cc-consts-of-interest}}), the network is
-considered to be experiencing persistent congestion, and the sender's congestion
-window MUST be reduced to the minimum congestion window (kMinimumWindow).  This
-response of collapsing the congestion window on persistent congestion is
-functionally similar to a sender's response on a Retransmission Timeout (RTO) in
-TCP {{RFC5681}}.
+## Persistent Congestion
+
+The network is considered to be experiencing persistent congestion when the
+sender goes a period of time without receiving any ACK frames for its in-flight,
+ack-eliciting packets.  Generally, this can be calculated as a number of
+consecutive PTOs (pto_count is more than  kPersistentCongestionThreshold, see
+{{cc-consts-of-interest}}), but since a PTO only occurs if no other

How about "but since the PTO timer is reset when a new ack-eliciting packet is sent,"

> @@ -1132,6 +1144,8 @@ increases bytes_in_flight.
 
 ~~~
    OnPacketSentCC(bytes_sent):
+     if (bytes_in_flight == 0):
+       probe_start_time = Now()

It's important not to start this if the packet isn't ack-eliciting, but I will say that requirement makes this more complex, since you have to track the first ack-eliciting packet to be sent.

> @@ -1147,6 +1161,8 @@ acked_packet from sent_packets.
    OnPacketAckedCC(acked_packet):
      // Remove from bytes_in_flight.
      bytes_in_flight -= acked_packet.size
+     if (bytes_in_flight != 0):
+       probe_start_time = Now()

I don't think it's necessary to check bytes_in_flight != 0 here?

> @@ -1003,14 +1003,21 @@ packets might cause the sender's bytes in flight to exceed the congestion window
 until an acknowledgement is received that establishes loss or delivery of
 packets.
 
-When an ACK frame is received that establishes loss of all in-flight packets
-sent prior to a threshold number of consecutive PTOs (pto_count is more than
-kPersistentCongestionThreshold, see {{cc-consts-of-interest}}), the network is
-considered to be experiencing persistent congestion, and the sender's congestion
-window MUST be reduced to the minimum congestion window (kMinimumWindow).  This
-response of collapsing the congestion window on persistent congestion is
-functionally similar to a sender's response on a Retransmission Timeout (RTO) in
-TCP {{RFC5681}}.
+## Persistent Congestion

I think this should be a sub-section of Probe Timeout?

-- 
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/2365#pullrequestreview-195737638

---

From: Nick Banks <[email-masked]>
Date: Wed, 23 Jan 2019 13:01:04 -0800

nibanks commented on this pull request.

Yeah, you are right. I'm not sure what I was thinking when I added this. I will remove it.

-- 
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/2365#discussion_r250370155

---

From: Nick Banks <[email-masked]>
Date: Wed, 23 Jan 2019 13:01:22 -0800

~~~

So, are you suggestion a change? Perhaps a comment?

-- 
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/2365#discussion_r250370259

---

From: Nick Banks <[email-masked]>
Date: Wed, 23 Jan 2019 13:02:32 -0800

I'm not super opinionated either way, but I feel like coupling the two things go us into this problem in the first place. They are related but still fairly independent in my mind. Especially, if we ever go with the model of a separate timer.

-- 
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/2365#discussion_r250370612

---

From: Nick Banks <[email-masked]>
Date: Wed, 23 Jan 2019 13:02:41 -0800

Sure.

-- 
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/2365#discussion_r250370657

---

From: MikkelFJ <[email-masked]>
Date: Wed, 23 Jan 2019 13:05:42 -0800

mikkelfj commented on this pull request.

I'm getting used to that word ...

-- 
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/2365#pullrequestreview-195743429

---

From: Nick Banks <[email-masked]>
Date: Wed, 23 Jan 2019 13:08:39 -0800

@nibanks pushed 1 commit.

8c4cfdb6f762119b2736b89115448f5dec8690f9  Some of Ian's comments

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/2365/files/a2ab05c21bc360e2953d8b01705c839e32975b4f..8c4cfdb6f762119b2736b89115448f5dec8690f9

---

From: MikkelFJ <[email-masked]>
Date: Wed, 23 Jan 2019 13:23:33 -0800

I'm wondering how many concurrent timers there are. Is it always at most one timer?
The probe timer is updated fairly frequently and that is actually a heavy operation if it is inserted into red-black tree (as is popular though suboptimal) when there are many concurrent connections.

Another approach would be to poll the state periodically and take action at those intervals. This could be done by not resetting the timer but inspecting state once it expires. Granted, a timeout might also be costly.

-- 
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/2365#issuecomment-456971499

---

From: MikkelFJ <[email-masked]>
Date: Wed, 23 Jan 2019 13:28:30 -0800

+ack-eliciting packets have been sent, an explicit time out must be used to

```suggestion
ack-eliciting packets have been sent, an explicit timeout must be used to
```

+ack-eliciting packets have been sent, an explicit time out must be used to
+account for those cases where PTOs don't occur.

```suggestion
account for those cases where PTOs do not occur.
```

 ~~~

 
 ~~~

`if (ack_eliciting)` is used elsewhere.

-- 
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/2365#pullrequestreview-195744311

---

From: Nick Banks <[email-masked]>
Date: Wed, 23 Jan 2019 13:31:29 -0800

~~~

That doesn't quite work. What if the first packet isn't ack_eliciting but the second is? The first would still increment the counter, so when the second comes in it wouldn't be 0 any more.

-- 
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/2365#discussion_r250379912

---

From: Nick Banks <[email-masked]>
Date: Wed, 23 Jan 2019 13:32:03 -0800

Could you make these suggestions on the latest changes so I can accept them more easily?

-- 
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/2365#discussion_r250380055

---

From: MikkelFJ <[email-masked]>
Date: Wed, 23 Jan 2019 14:15:40 -0800

-
+{{cc-consts-of-interest}}), but since the PTO timer is reset when a new
+ack-eliciting packet is sent, an explicit time out must be used to account for

```suggestion
ack-eliciting packet is sent, an explicit timeout must be used to account for
```

-- 
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/2365#pullrequestreview-195771286

---

From: MikkelFJ <[email-masked]>
Date: Wed, 23 Jan 2019 14:15:59 -0800

+those cases where PTOs don't occur.

```suggestion
those cases where PTOs do not occur.
```

-- 
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/2365#pullrequestreview-195771420

---

From: MikkelFJ <[email-masked]>
Date: Wed, 23 Jan 2019 14:17:31 -0800

~~~

right

-- 
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/2365#discussion_r250394529

---

From: Nick Banks <[email-masked]>
Date: Wed, 23 Jan 2019 14:17:47 -0800

d85c452a6e30223dfd8d016283de7ec379d7031f  mikkelfj's suggestion

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/2365/files/8c4cfdb6f762119b2736b89115448f5dec8690f9..d85c452a6e30223dfd8d016283de7ec379d7031f

---

From: Nick Banks <[email-masked]>
Date: Wed, 23 Jan 2019 14:18:02 -0800

548e4e316c08d3ca8916d79a7ebde11bd91980df  mikkelfj's suggestion

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/2365/files/d85c452a6e30223dfd8d016283de7ec379d7031f..548e4e316c08d3ca8916d79a7ebde11bd91980df

---

From: MikkelFJ <[email-masked]>
Date: Wed, 23 Jan 2019 14:21:42 -0800

> @@ -1120,6 +1130,7 @@ variables as follows:
 ~~~
    congestion_window = kInitialWindow
    bytes_in_flight = 0

`probe_pending = 0`

-- 
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/2365#pullrequestreview-195773445

---

From: MikkelFJ <[email-masked]>
Date: Wed, 23 Jan 2019 14:24:18 -0800

~~~

```
if (ack_eliciting && (bytes_in_flight == 0 || probe_pending)):
    probe_pending = 0
    probe_start_time = Now()
else:
   probe_pending = 1

-- 
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/2365#discussion_r250396625

---

From: MikkelFJ <[email-masked]>
Date: Wed, 23 Jan 2019 14:24:58 -0800

> @@ -1147,6 +1160,8 @@ acked_packet from sent_packets.
+     // Reset probe start time

probe_pending = 0

-- 
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/2365#pullrequestreview-195774514

---

From: MikkelFJ <[email-masked]>
Date: Wed, 23 Jan 2019 14:26:24 -0800

I'm not sure this is correct, but the comments combined should avoid starting the probe timer when a packet is not eliciting

-- 
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/2365#pullrequestreview-195775057

---

From: MikkelFJ <[email-masked]>
Date: Wed, 23 Jan 2019 22:35:32 +0000 (UTC)

~~~

It's too late, I keep getting this wrong - wait

-- 
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/2365#discussion_r250399936

---

From: MikkelFJ <[email-masked]>
Date: Wed, 23 Jan 2019 14:40:55 -0800

~~~

Edited above comment, I think it is ok now

-- 
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/2365#discussion_r250401633

---

From: Nick Banks <[email-masked]>
Date: Sun, 27 Jan 2019 23:33:54 -0800

@nibanks pushed 2 commits.

b61e20c0cadad293c040b029a5c4d36b6c83c30e  Jana's offline suggestions
ad66ac022422ed1a14de1c1a5729c2b085c8be00  Merge branch 'pr/persistent-congestion' of https://github.com/nibanks/base-drafts into pr/persistent-congestion

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/2365/files/548e4e316c08d3ca8916d79a7ebde11bd91980df..ad66ac022422ed1a14de1c1a5729c2b085c8be00

---

From: Nick Banks <[email-masked]>
Date: Sun, 27 Jan 2019 23:34:48 -0800

d750a396b207d2ea72623860d26e34713559aacb  tab to spaces

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

---

From: ianswett <[email-masked]>
Date: Mon, 28 Jan 2019 08:37:56 +0000 (UTC)

> @@ -1202,15 +1205,26 @@ Invoked by loss detection from DetectLostPackets when new packets
 are detected lost.
 
 ~~~
+   InPersistentCongestion(oldest_loss_time):
+     return

This expression is hard to read, I think it's worth breaking it up some.
 elapsed_time = Now() - oldest_loss_time
 pto = smoothed_rtt + 4 * rttvar + max_ack_delay
 return elapsed_time > pto * (2 ^ kPersistentCongestionThreshold - 1)

-- 
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/2365#pullrequestreview-196918793

---

From: Nick Banks <[email-masked]>
Date: Mon, 28 Jan 2019 22:42:05 +0000 (UTC)

are detected lost.
 
 ~~~

Sure. I can do that.

-- 
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/2365#discussion_r251625398

---

From: Nick Banks <[email-masked]>
Date: Mon, 28 Jan 2019 14:47:24 -0800

c668cc159432c846cae7fe1e3802d55f97d7509a  Ian's suggestion

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

---

From: ianswett <[email-masked]>
Date: Mon, 28 Jan 2019 15:06:33 -0800

I think it makes sense to fix #2255 with this PR as well?

-- 
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/2365#issuecomment-458338657

---

From: Nick Banks <[email-masked]>
Date: Mon, 28 Jan 2019 15:23:01 -0800

Why is the build failing?
```
1211: Line is 73 characters; limit is 65
     return elapsed_time > pto * (2 ^ kPersistentCongestionThreshold - 1)
```
I though the limit was 80 characters?

Also, @ianswett what exactly do you want to add to fix #2255? Perhaps we could just have a separate PR for that?

-- 
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/2365#issuecomment-458342782

---

From: ianswett <[email-masked]>
Date: Mon, 28 Jan 2019 15:32:22 -0800

I think the pseudocode is good, but I have some suggestions on the text.

> @@ -1003,15 +1003,21 @@ packets might cause the sender's bytes in flight to exceed the congestion window

The old text was more correct than this text in the sense that it's important that not only does time pass, but all the packets are lost within that period of time.

How about:
"When an ACK frame is received that establishes loss of all in-flight packets sent over a long enough period of time, the network is considered to be experiencing persistent congestion."

-response of collapsing the congestion window on persistent congestion is
-functionally similar to a sender's response on a Retransmission Timeout (RTO) in
-TCP {{RFC5681}}.
-
+## Persistent Congestion
+
+The network is considered to be experiencing persistent congestion when the
+sender goes a period of time without receiving any ACK frames for its in-flight,
+ack-eliciting packets.  Generally, this can be calculated as a number of
+consecutive PTOs (pto_count is more than  kPersistentCongestionThreshold, see
+{{cc-consts-of-interest}}), but since the PTO timer is reset when a new
+ack-eliciting packet is sent, an explicit timeout must be used to account for
+those cases where PTOs do not occur.
+
+When persistent congestion is encountered the sender's congestion window MUST be

```suggestion
When persistent congestion is established, the sender's congestion window MUST be
```

> @@ -1003,15 +1003,21 @@ packets might cause the sender's bytes in flight to exceed the congestion window

Suggestion: replace "Generally, this can be calculated as a number of consecutive PTOs (),..." with
"Commonly, this can be established by consecutive PTOs,..."

-- 
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/2365#pullrequestreview-197306266

---

From: ianswett <[email-masked]>
Date: Mon, 28 Jan 2019 15:33:35 -0800

#2255 should only need the pseudocode fixed up, so your current PR should be sufficient once the text is in good shape.

-- 
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/2365#issuecomment-458345534

---

From: Nick Banks <[email-masked]>
Date: Mon, 28 Jan 2019 15:38:10 -0800

For your suggestion, do you really need the word "all" in there. To me, the loss of any in-flight packets over a long enough time seems to be the requirement (at least how we have the pseudocode). It think the removal the word "all" might be better. Other than that, I am fine with making the change.

-- 
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/2365#discussion_r251639439

---

From: Nick Banks <[email-masked]>
Date: Mon, 28 Jan 2019 15:39:00 -0800

be4375040bdb8826b9301103fc9fbc6b7ed6d989  Ian's suggestion

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

---

From: ianswett <[email-masked]>
Date: Mon, 28 Jan 2019 15:40:14 -0800

It has to be all packets lost within that period of time(ie: total blackout).  And now that you mention it, that means the pseudocode isn't quite right either.

As #2255 details, the goal is to be consistent with the previous behavior in terms of when persistent congestion would be established.

-- 
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/2365#discussion_r251639885

---

From: Nick Banks <[email-masked]>
Date: Mon, 28 Jan 2019 15:40:34 -0800

>     OnPacketsLost(lost_packets):
      // Remove lost packets from bytes_in_flight.
      for (lost_packet : lost_packets):
        bytes_in_flight -= lost_packet.size
+     smallest_lost_packet = lost_packets.first()
      largest_lost_packet = lost_packets.last()

Slight tangent to this PR: what do folks think about changing these variables to `oldest_lost_packet` and `newest_lost_packet`?

-- 
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/2365#pullrequestreview-197315648

---

From: ianswett <[email-masked]>
Date: Mon, 28 Jan 2019 15:42:57 -0800

I don't have a strong preference either way.

-- 
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/2365#discussion_r251640472

---

From: Nick Banks <[email-masked]>
Date: Mon, 28 Jan 2019 15:44:08 -0800

I'm not sure how we'd update the pseudocode to indicate loss of all packets. I'd have to think a bit more about it.

-- 
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/2365#discussion_r251640734

---

From: Nick Banks <[email-masked]>
Date: Mon, 28 Jan 2019 15:51:31 -0800

08d85ed469478812c63a9793df1a4aa500a6304d  More of Ian's suggestions
4ec86205ff8dce9a9ecbfef0ca97a7f7ee6d4c32  Merge branch 'pr/persistent-congestion' of https://github.com/nibanks/base-drafts into pr/persistent-congestion

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

---

From: Marten Seemann <[email-masked]>
Date: Mon, 28 Jan 2019 17:02:17 -0800

marten-seemann commented on this pull request.

This seems to change the way we do RTO verification in a fundamental way: Currently, we **only** collapse the congestion window when we receive an ACK that actually establishes loss of the probe packets.
This PR changes this behavior, since `InPersistentCongestion` is called from `OnPacketsLost`, which is also called from `OnLossDetectionTimeout`, i.e. **without** receiving any 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/2365#pullrequestreview-197335572

---

From: Nick Banks <[email-masked]>
Date: Mon, 28 Jan 2019 17:10:11 -0800

@marten-seemann Personally, I don't think `DetectLostPackets` should be called from `OnLossDetectionTimeout` any more, but I'd like @janaiyengar to chime in. My understanding of the new PTO design is that ONLY ACK frames are supposed to trigger loss. Timers only cause additional packets to be sent, in order to trigger ACKs from the peer.

-- 
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/2365#issuecomment-458366731

---

From: Marten Seemann <[email-masked]>
Date: Mon, 28 Jan 2019 17:19:32 -0800

@nibanks I think the reason we call `DetectLostPackets` from `OnLossDetectionTimeout` is in order for Early Retransmit to work. We might need some more code refactoring here to achieve the desired behavior. Let's discuss this with @janaiyengar.

-- 
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/2365#issuecomment-458368824

---

From: Nick Banks <[email-masked]>
Date: Tue, 29 Jan 2019 01:48:06 +0000 (UTC)

1ecd70bebd64eba1e221385d1490b43f3a684309  Fix build error

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/2365/files/4ec86205ff8dce9a9ecbfef0ca97a7f7ee6d4c32..1ecd70bebd64eba1e221385d1490b43f3a684309

---

From: Nick Banks <[email-masked]>
Date: Mon, 28 Jan 2019 22:23:42 -0800

87b2c5b80d45f24a52ca9b1988cba971e9b35ee7  Merge branch 'master' into pr/persistent-congestion

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/2365/files/1ecd70bebd64eba1e221385d1490b43f3a684309..87b2c5b80d45f24a52ca9b1988cba971e9b35ee7

---

From: ianswett <[email-masked]>
Date: Tue, 29 Jan 2019 10:47:44 +0000 (UTC)

>  When an ACK frame is received that establishes loss of all in-flight packets
-sent prior to a threshold number of consecutive PTOs (pto_count is more than
-kPersistentCongestionThreshold, see {{cc-consts-of-interest}}), the network is
-considered to be experiencing persistent congestion, and the sender's congestion
-window MUST be reduced to the minimum congestion window (kMinimumWindow).  This
-response of collapsing the congestion window on persistent congestion is
-functionally similar to a sender's response on a Retransmission Timeout (RTO) in
-TCP {{RFC5681}}.
+sent over a long enough period of time, the network is considered to be
+experiencing persistent congestion.  Commonly, this can be established by
+consecutive PTOs (pto_count is more than  kPersistentCongestionThreshold, see

```suggestion
consecutive PTOs (pto_count is more than kPersistentCongestionThreshold, see
```

-- 
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/2365#pullrequestreview-197486572

---

From: ianswett <[email-masked]>
Date: Tue, 29 Jan 2019 10:48:16 +0000 (UTC)

```suggestion
those cases where PTOs do not occur or are substantially delayed.
```

-- 
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/2365#pullrequestreview-197486828

---

From: ianswett <[email-masked]>
Date: Tue, 29 Jan 2019 10:55:23 +0000 (UTC)

A few more suggestions, but I think this is getting close.

I don't think it's really a timeout.  How about period or duration or time?

Also, I'd remove the () here and then define what the actual period is at the end of this paragraph.

> @@ -1214,15 +1218,27 @@ Invoked by loss detection from DetectLostPackets when new packets
 are detected lost.
 
 ~~~
+   InPersistentCongestion(oldest_loss_time):
+     elapsed_time = Now() - oldest_loss_time

```suggestion
     congestion_period = Now() - oldest_loss_time
```

-- 
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/2365#pullrequestreview-197487691

---

From: Nick Banks <[email-masked]>
Date: Tue, 29 Jan 2019 03:02:22 -0800

b5f342e5d7b846ec670ba57cbd63fe8f6c6f5f50  Ian's suggestion

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/2365/files/87b2c5b80d45f24a52ca9b1988cba971e9b35ee7..b5f342e5d7b846ec670ba57cbd63fe8f6c6f5f50

---

From: Nick Banks <[email-masked]>
Date: Tue, 29 Jan 2019 03:03:20 -0800

e3623457424d74e7f8a70643136ac718edbeef48  Ian's suggestion

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

---

From: Nick Banks <[email-masked]>
Date: Tue, 29 Jan 2019 11:03:46 +0000 (UTC)

a0e507aaa28c7fa2fd19a321e9165dbfc3a6fc69  Ian's suggestion

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

---

From: Nick Banks <[email-masked]>
Date: Tue, 29 Jan 2019 14:42:16 -0800

I think time or duration might fit.

-- 
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/2365#discussion_r252049017

---

From: Nick Banks <[email-masked]>
Date: Tue, 29 Jan 2019 14:57:38 -0800

6fa9b5268221766fb7038af8cc5df440d1608898  More of Ian's suggestions

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

---

From: ianswett <[email-masked]>
Date: Wed, 30 Jan 2019 08:22:53 +0000 (UTC)

```suggestion
consecutive PTOs
```

-- 
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/2365#pullrequestreview-197949397

---

From: ianswett <[email-masked]>
Date: Wed, 30 Jan 2019 00:24:33 -0800

+ack-eliciting packet is sent, an explicit duration must be used to account for
+those cases where PTOs do not occur or are substantially delayed.

Add: This duration is the equivalent of kPersistentCongestionThreshold consecutive PTOS, smoothed_rtt + 4 * rttvar + max_ack_delay * ((2 ^ kPersistentCongestionThreshold) - 1)

-- 
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/2365#pullrequestreview-197949963

---

From: ianswett <[email-masked]>
Date: Wed, 30 Jan 2019 00:25:15 -0800

are detected lost.
 
 ~~~
+   InPersistentCongestion(oldest_loss_time):
+     congestion_period = Now() - oldest_loss_time
+     pto = smoothed_rtt + 4 * rttvar + max_ack_delay
+     return
+       congestion_period >
+       pto * (2 ^ kPersistentCongestionThreshold - 1)

```suggestion
         pto * (2 ^ kPersistentCongestionThreshold - 1)
```

-- 
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/2365#pullrequestreview-197950226

---

From: Nick Banks <[email-masked]>
Date: Wed, 30 Jan 2019 01:16:43 -0800

f64efb82c0c5e559b671d6c618645e185873bc1e  Ian's suggestion

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

---

From: Nick Banks <[email-masked]>
Date: Wed, 30 Jan 2019 01:28:02 -0800

931c0cc812b0512bd4b6a494e6a66608c00f1ce2  Ian's suggestions

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/2365/files/f64efb82c0c5e559b671d6c618645e185873bc1e..931c0cc812b0512bd4b6a494e6a66608c00f1ce2

---

From: ianswett <[email-masked]>
Date: Wed, 30 Jan 2019 12:03:34 +0000 (UTC)

-sent prior to a threshold number of consecutive PTOs (pto_count is more than
-kPersistentCongestionThreshold, see {{cc-consts-of-interest}}), the network is
-considered to be experiencing persistent congestion, and the sender's congestion
-window MUST be reduced to the minimum congestion window (kMinimumWindow).  This
-response of collapsing the congestion window on persistent congestion is
-functionally similar to a sender's response on a Retransmission Timeout (RTO) in
-TCP {{RFC5681}}.
+sent over a long enough period of time, the network is considered to be
+experiencing persistent congestion.  Commonly, this can be established by
+consecutive PTOs, but since the PTO timer is reset when a new ack-eliciting
+packet is sent, an explicit duration must be used to account for those cases
+where PTOs do not occur or are substantially delayed.  This duration is the
+equivalent of kPersistentCongestionThreshold consecutive PTOS, smoothed_rtt +

```suggestion
equivalent of kPersistentCongestionThreshold consecutive PTOS: smoothed_rtt +
```

-- 
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/2365#pullrequestreview-198040855

---

From: ianswett <[email-masked]>
Date: Wed, 30 Jan 2019 04:09:50 -0800

// Start a new congestion epoch if the last lost packet
      // is past the end of the previous recovery epoch.
-     CongestionEvent(largest_lost_packet.time_sent)
+     CongestionEvent(newest_lost_packet.time_sent)
+
+     // Collapse congestion window if persistent congestion
+     if (InPersistentCongestion(oldest_lost_packet.time_sent)):

I'm starting to think we should pass in congestion_period and make it oldest_lost_packet - newest_lost_packet.

I think using Now() may be incorrect.  What if one packet is lost and then a few ACKs are lost.  In this case, Now() is substantially larger than oldest_lost_packet, but in fact only one packet was lost.

-- 
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/2365#pullrequestreview-198043282

---

From: ianswett <[email-masked]>
Date: Wed, 30 Jan 2019 12:38:18 +0000 (UTC)

+4 * rttvar + max_ack_delay * ((2 ^ kPersistentCongestionThreshold) - 1).

Q for here and below: Should max_ack_delay be added after exponential backoff, instead of before?  Adding it after incentivizes implementations to indicate a realistic value, since an unrealistic one just adds to rttvar, which seems desirable.

-- 
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/2365#pullrequestreview-198054430

---

From: Nick Banks <[email-masked]>
Date: Wed, 30 Jan 2019 13:24:41 -0800

The order is the same in `SetLossDetectionTimer`:
```    // Calculate PTO duration
    timeout =
      smoothed_rtt + 4 * rttvar + max_ack_delay
    timeout = max(timeout, kGranularity)
    timeout = timeout * (2 ^ pto_count)```
Though, on a side issue, I feel the `kGranularity` check should be moved to the end of that calculation...

-- 
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/2365#discussion_r252443584

---

From: Nick Banks <[email-masked]>
Date: Wed, 30 Jan 2019 13:25:07 -0800

ed4ad7ebbad7ba063d0dde2aad67e24fcccd8308  Ian's suggestion

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/2365/files/931c0cc812b0512bd4b6a494e6a66608c00f1ce2..ed4ad7ebbad7ba063d0dde2aad67e24fcccd8308

---

From: Nick Banks <[email-masked]>
Date: Wed, 30 Jan 2019 13:28:45 -0800

Since this only happens in response to an ACK, IF we made any change, I think the change would need to be congestion_time equals the time between the oldest packet lost and the oldest packet acknowledged. That being said, I'm not convinced we really need to make that change. For instance, when comparing to TCP RTO, it's a timer from the oldest packet (or ACK) to Now().

-- 
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/2365#discussion_r252444932

---

From: ianswett <[email-masked]>
Date: Wed, 30 Jan 2019 14:10:45 -0800

Agreed, what you have matches the current pseudocode.  I'll open an issue for kGranularity and max_ack_delay and when they should be added.

-- 
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/2365#discussion_r252458850