Subject: [quicwg/base-drafts] Notify the congestion controller of losses first (#3540)

From: ianswett <[email-masked]>
Date: Sun, 22 Mar 2020 13:27:45 -0700

Fixes #3495 and #3288 by notifying the congestion controller of lost packets prior to notifying it of acknowledged packets and passing all lost packets into InPersistentCongestion().
You can view, comment on, or merge this pull request online at:

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

-- Commit Summary --

  * Notify the congestion controller of losses first

-- File Changes --

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

-- Patch Links --

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

---

From: ianswett <[email-masked]>
Date: Sun, 22 Mar 2020 13:36:48 -0700

@ianswett pushed 1 commit.

0c944ea9badc434888c169b4609687d404ad2fcb  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/3540/files/6290d1447ecdc63e4b0a702d0a0bd27d01904d6a..0c944ea9badc434888c169b4609687d404ad2fcb

---

From: Alessandro Ghedini <[email-masked]>
Date: Mon, 23 Mar 2020 10:54:21 -0700

Thanks. I think `AreAllPacketsLost` will still need some clarifications but I haven't been able to find a nice way to word that, so for now this is good.

-- 
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/3540#issuecomment-602759578

---

From: Christoph Paasch <[email-masked]>
Date: Fri, 27 Mar 2020 09:26:25 -0700

Looks good to me wrt to issue #3495

-- 
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/3540#issuecomment-605093862

---

From: martinduke <[email-masked]>
Date: Fri, 27 Mar 2020 12:17:37 -0700

@martinduke approved this pull request.

It mostly looks good.

One note: the pseudocode doesn't really address when we set sent_packets as acked. If we call DetectLostPackets() first, this becomes quite relevant as it iterates through unacked sent_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/3540#pullrequestreview-383137069

---

From: ianswett <[email-masked]>
Date: Sun, 29 Mar 2020 16:48:35 -0700

5f520e2d9207eb156fb3a6fec2a11318c975b36e  temp update

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3540/files/0c944ea9badc434888c169b4609687d404ad2fcb..5f520e2d9207eb156fb3a6fec2a11318c975b36e

---

From: ianswett <[email-masked]>
Date: Sun, 29 Mar 2020 16:53:27 -0700

de8eca5e5b745c44c1c52b1c811f5068c565c112  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/3540/files/5f520e2d9207eb156fb3a6fec2a11318c975b36e..de8eca5e5b745c44c1c52b1c811f5068c565c112

---

From: ianswett <[email-masked]>
Date: Sun, 29 Mar 2020 16:55:33 -0700

2594cd133feb9ef21f93fda32bca9dc6c997a494  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/3540/files/de8eca5e5b745c44c1c52b1c811f5068c565c112..2594cd133feb9ef21f93fda32bca9dc6c997a494

---

From: ianswett <[email-masked]>
Date: Sun, 29 Mar 2020 16:57:35 -0700

05d9705c4dce2707f3bd8456b410e082c717a41c  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/3540/files/2594cd133feb9ef21f93fda32bca9dc6c997a494..05d9705c4dce2707f3bd8456b410e082c717a41c

---

From: ianswett <[email-masked]>
Date: Sun, 29 Mar 2020 17:01:12 -0700

902703f7d3bc8931ba05e877e88a322f29c1d9b0  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/3540/files/05d9705c4dce2707f3bd8456b410e082c717a41c..902703f7d3bc8931ba05e877e88a322f29c1d9b0

---

From: ianswett <[email-masked]>
Date: Sun, 29 Mar 2020 17:05:45 -0700

@martinduke  I think I addressed your comment by commenting that DetectNewlyAckedPackets is responsible for removing packets from sent_packets.  This aligns with the API for DetectLostPackets.  I also changed OnPacketAcked to OnPacketsAcked.

"// Determine which packets have been acknowledged and remove
  // them from sent_packets.
  newly_acked_packets = DetectNewlyAckedPackets(ack, pn_space)"

-- 
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/3540#issuecomment-605723121

---

From: Jana Iyengar <[email-masked]>
Date: Mon, 30 Mar 2020 23:30:25 -0700

@janaiyengar commented on this pull request.

This looks like a good fix. A few comments.

> +DetectLostPackets is called every time an ACK is received or the time threshold
+loss detection timer expires and operates on the sent_packets for that packet
+number space.

```suggestion
DetectLostPackets is called every time an ACK is received or the time threshold
loss detection timer expires. This function operates on the sent_packets for that
packet number space and returns a list of packets newly detected as lost.
```

> @@ -1265,7 +1244,8 @@ OnLossDetectionTimeout():
     GetEarliestTimeAndSpace(loss_time)
   if (earliest_loss_time != 0):
     // Time threshold loss Detection
-    DetectLostPackets(pn_space)
+    lost_packets = DetectLostPackets(pn_space)
+    OnPacketsLost(lost_packets)

```suggestion
    if (!lost_packets.empty()):
      OnPacketsLost(lost_packets)
```

> @@ -1517,7 +1494,7 @@ Invoked from DetectLostPackets when packets are deemed lost.
      CongestionEvent(largest_lost_packet.time_sent)

How about also changing this to the following, and removing the line before this (last() is a language-specific method, so I'm suggesting largest())?
```suggestion
     CongestionEvent(lost_packets.largest().time_sent)
```

> @@ -1108,14 +1108,16 @@ OnAckReceived(ack, pn_space):
     largest_acked_packet[pn_space] =
         max(largest_acked_packet[pn_space], ack.largest_acked)
 
+  // Determine which packets have been acknowledged and remove

```suggestion
  // DetectNewlyAckedPackets finds packets have been newly acknowledged and removes
```

> @@ -1108,14 +1108,16 @@ OnAckReceived(ack, pn_space):
     largest_acked_packet[pn_space] =
         max(largest_acked_packet[pn_space], ack.largest_acked)
 
+  // Determine which packets have been acknowledged and remove
+  // them from sent_packets.
+  newly_acked_packets = DetectNewlyAckedPackets(ack, pn_space)

```suggestion
  newly_acked_packets = DetectAndRemoveAckedPackets(ack, pn_space)
```
here and below

> @@ -1128,13 +1130,12 @@ OnAckReceived(ack, pn_space):
   if (ACK frame contains ECN information):
       ProcessECN(ack, pn_space)
 
-  for acked_packet in newly_acked_packets:
-    OnPacketAcked(acked_packet.packet_number, pn_space)
-
-  DetectLostPackets(pn_space)
+  lost_packets = DetectLostPackets(pn_space)

```suggestion
  lost_packets = DetectAndRemoveLostPackets(pn_space)
```
here and below

> @@ -1434,26 +1411,27 @@ increases bytes_in_flight.
 ## On Packet Acknowledgement
 
 Invoked from loss detection's OnPacketAcked and is supplied with the

```suggestion
Invoked from loss detection's OnAckReceived and is supplied with the
```

> @@ -1499,15 +1477,14 @@ Invoked when an ACK frame with an ECN section is received from the peer.
 Invoked from DetectLostPackets when packets are deemed lost.
 
 ~~~
-   InPersistentCongestion(largest_lost_packet):
+   InPersistentCongestion(lost_packets):
      pto = smoothed_rtt + max(4 * rttvar, kGranularity) +
        max_ack_delay
      congestion_period = pto * kPersistentCongestionThreshold
      // Determine if all packets in the time period before the
      // newest lost packet, including the edges, are marked

```suggestion
     // largest newly lost packet, including the edges, are marked
```

 ~~~
-   InCongestionRecovery(sent_time):
+   InLossRecovery(sent_time):

```suggestion
   InCongestionRecovery(sent_time):
```
Revert this change. Was this deliberate? The term congestion recovery is used earlier in the spec and the term is defined.

> -     if (InCongestionRecovery(acked_packet.time_sent)):
-       // Do not increase congestion window in recovery period.
-       return
-     if (IsAppOrFlowControlLimited()):
-       // Do not increase congestion_window if application
-       // limited or flow control limited.
-       return
-     if (congestion_window < ssthresh):
-       // Slow start.
-       congestion_window += acked_packet.size
-     else:
+   OnPacketsAcked(acked_packets):
+     for (packet in acked_packets):
+       // Remove from bytes_in_flight.
+       bytes_in_flight -= packet.size
+       if (InLossRecovery(packet.time_sent)):

```suggestion
       if (InCongestionRecovery(packet.time_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/3540#pullrequestreview-384421422

---

From: Marten Seemann <[email-masked]>
Date: Tue, 31 Mar 2020 03:25:55 -0700

@marten-seemann 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/3540#pullrequestreview-384598953

---

From: Marten Seemann <[email-masked]>
Date: Tue, 31 Mar 2020 03:44:18 -0700

@marten-seemann commented on this pull request.

`DetectLostPackets` used to call `OnPacketsLost(lost_packets)`. What do we gain by moving this out of `DetectLostPackets`?

I believe this does not change the order in which the congestion controller is informed about the losses. This could have been achieved by interchanging the two calls to `OnPacketsLost` and `OnPacketsAcked` in `OnAckReceived`. Or am I missing something 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/3540#pullrequestreview-384611313

---

From: ianswett <[email-masked]>
Date: Tue, 31 Mar 2020 09:03:44 -0700

35fad6f470bf6fd2316189ecb7a9ce376c93f4d9  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/3540/files/902703f7d3bc8931ba05e877e88a322f29c1d9b0..35fad6f470bf6fd2316189ecb7a9ce376c93f4d9

---

From: ianswett <[email-masked]>
Date: Tue, 31 Mar 2020 09:03:59 -0700

71bebc7cd1d63193b6e788deffb65aee681ab8cc  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/3540/files/35fad6f470bf6fd2316189ecb7a9ce376c93f4d9..71bebc7cd1d63193b6e788deffb65aee681ab8cc

---

From: ianswett <[email-masked]>
Date: Tue, 31 Mar 2020 09:04:06 -0700

c942fd8027c3ddbd03c658aca614282cb1b3f490  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/3540/files/71bebc7cd1d63193b6e788deffb65aee681ab8cc..c942fd8027c3ddbd03c658aca614282cb1b3f490

---

From: ianswett <[email-masked]>
Date: Tue, 31 Mar 2020 09:05:07 -0700

@ianswett commented on this pull request.

If a timeout occurred, then lost_packets should never be empty I believe?  I could add an assert for that if you'd like?

-- 
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/3540#discussion_r401033422

---

From: ianswett <[email-masked]>
Date: Tue, 31 Mar 2020 09:06:05 -0700

~~~

This was deliberate.  I've always heard it called loss recovery or just 'the recovery period'

But I can revert 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/3540#discussion_r401034180

---

From: ianswett <[email-masked]>
Date: Tue, 31 Mar 2020 09:07:19 -0700

~~~

I'm not sure largest makes this less clearer, at least to me.

-- 
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/3540#discussion_r401035082

---

From: ianswett <[email-masked]>
Date: Tue, 31 Mar 2020 09:08:39 -0700

I'm not a big fan of largest here and above.  If you want to make the change, maybe another PR, since this one is already quite large.

-- 
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/3540#discussion_r401036031

---

From: ianswett <[email-masked]>
Date: Tue, 31 Mar 2020 09:08:49 -0700

5d0f1778e301bc0aaae4c0a8d192814852000fda  Apply suggestions from code review

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3540/files/c942fd8027c3ddbd03c658aca614282cb1b3f490..5d0f1778e301bc0aaae4c0a8d192814852000fda

---

From: ianswett <[email-masked]>
Date: Tue, 31 Mar 2020 09:09:37 -0700

ef5ea9cf87e6742d05a8347c6a58e07d6bc8a27c  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/3540/files/5d0f1778e301bc0aaae4c0a8d192814852000fda..ef5ea9cf87e6742d05a8347c6a58e07d6bc8a27c

---

From: ianswett <[email-masked]>
Date: Tue, 31 Mar 2020 09:10:52 -0700

949bbab5c32eef55d23296330477c49bb60661ca  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/3540/files/ef5ea9cf87e6742d05a8347c6a58e07d6bc8a27c..949bbab5c32eef55d23296330477c49bb60661ca

---

From: ianswett <[email-masked]>
Date: Tue, 31 Mar 2020 09:17:38 -0700

6c7ff3f90cef21648886155cd108e7eecf981e56  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/3540/files/949bbab5c32eef55d23296330477c49bb60661ca..6c7ff3f90cef21648886155cd108e7eecf981e56

---

From: ianswett <[email-masked]>
Date: Tue, 31 Mar 2020 09:21:05 -0700

@marten-seemann my reasoning was I thought it was critical to determine what packets were newly acked before invoking loss detection.  In some cases it may be(depending upon details of one's loss detection), but since we update largest_acked_packet[pn_space] outside the code which detects newly acked packets, it's not critical to the pseudocode as it stands today.

That being said, I think the parallelism is nice, so I'd prefer to keep 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/3540#issuecomment-606730634

---

From: ianswett <[email-masked]>
Date: Tue, 31 Mar 2020 09:23:27 -0700

3634c6b298b8e1e57fd89cc5247750d182767512  Apply suggestions from code review

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3540/files/6c7ff3f90cef21648886155cd108e7eecf981e56..3634c6b298b8e1e57fd89cc5247750d182767512

---

From: ianswett <[email-masked]>
Date: Tue, 31 Mar 2020 09:26:16 -0700

2503587c41e812f77339cc30f3909abad187cb8d  Use largest() consistently

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3540/files/3634c6b298b8e1e57fd89cc5247750d182767512..2503587c41e812f77339cc30f3909abad187cb8d

---

From: ianswett <[email-masked]>
Date: Tue, 31 Mar 2020 09:26:53 -0700

a90f17cacdfe043735ff599d4b16c3d510148259  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/3540/files/2503587c41e812f77339cc30f3909abad187cb8d..a90f17cacdfe043735ff599d4b16c3d510148259

---

From: ianswett <[email-masked]>
Date: Tue, 31 Mar 2020 09:30:19 -0700

ae9bb473c0f1d882375d7e1e6acab6444f96b42f  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/3540/files/a90f17cacdfe043735ff599d4b16c3d510148259..ae9bb473c0f1d882375d7e1e6acab6444f96b42f

---

From: Jana Iyengar <[email-masked]>
Date: Tue, 31 Mar 2020 16:16:44 -0700

Hm, that's true. I was thinking about stream cancellations, but that doesn't matter here. assert SGTM

-- 
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/3540#discussion_r401269053

---

From: Jana Iyengar <[email-masked]>
Date: Tue, 31 Mar 2020 16:17:20 -0700

@janaiyengar approved this pull request.

LGTM, modulo adding the assert, which is a good idea.

-- 
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/3540#pullrequestreview-385168628

---

From: Jana Iyengar <[email-masked]>
Date: Tue, 31 Mar 2020 16:45:51 -0700

@marten-seemann : It does change the order -- it does CC increase action after the decrease

-- 
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/3540#issuecomment-606944811

---

From: ianswett <[email-masked]>
Date: Tue, 31 Mar 2020 17:05:13 -0700

b34f5f9be7847b80ae890f03012f95775b946de3  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/3540/files/ae9bb473c0f1d882375d7e1e6acab6444f96b42f..b34f5f9be7847b80ae890f03012f95775b946de3

---

From: ianswett <[email-masked]>
Date: Tue, 31 Mar 2020 17:05:33 -0700

Assert added, merging.

-- 
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/3540#issuecomment-606951234

---

From: ianswett <[email-masked]>
Date: Tue, 31 Mar 2020 17:07:34 -0700

Merged #3540 into master.

-- 
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/3540#event-3185633741