From fb64708249f8e089121082ba0de6e4cd0c6648c1 Mon Sep 17 00:00:00 2001 From: Martin Thomson Date: Wed, 2 Sep 2020 12:03:03 +1000 Subject: [PATCH 1/7] Pseudocode for persistent congestion is too hard This removes that function, relying instead on comments and the definition in text (which people have noted is now very clear). I have also reset a couple of extra variables in this case. It is important to catch all of these because if you don't you get poor outcomes. The only question I have is regarding bytes in flight. I think that we need to keep tracking those bytes, or the overall accounting gets messy. That is what our implementation does anyway. Closes #4010. Closes #3972. --- draft-ietf-quic-recovery.md | 20 +++++--------------- 1 file changed, 5 insertions(+), 15 deletions(-) diff --git a/draft-ietf-quic-recovery.md b/draft-ietf-quic-recovery.md index e5b90186af..6b3767f1c4 100644 --- a/draft-ietf-quic-recovery.md +++ b/draft-ietf-quic-recovery.md @@ -1690,27 +1690,17 @@ ProcessECN(ack, pn_space): Invoked when DetectAndRemoveLostPackets deems packets lost. ~~~ -InPersistentCongestion(largest_lost): - // Persistent congestion cannot be declared on the - // first RTT sample. - if (is first RTT sample): - return false - pto = smoothed_rtt + max(4 * rttvar, kGranularity) + - max_ack_delay - congestion_period = pto * kPersistentCongestionThreshold - // Determine if all packets in the time period before the - // largest newly lost packet, including the edges and - // across all packet number spaces, are marked lost. - return AreAllPacketsLost(largest_lost, congestion_period) - OnPacketsLost(lost_packets): // Remove lost packets from bytes_in_flight. for lost_packet in lost_packets: bytes_in_flight -= lost_packet.sent_bytes OnCongestionEvent(lost_packets.largest().time_sent) - // Collapse congestion window if persistent congestion - if (InPersistentCongestion(lost_packets.largest())): + // Reset the congestion window if the loss of these + // packets indicates persistent congestion. + if (InPersistentCongestion(lost_packets)): congestion_window = kMinimumWindow + ssthresh = infinite + congestion_recovery_start_time = 0 ~~~ ## Upon dropping Initial or Handshake keys From a8adc5d8d5ccab2cb05dc3a371057c71df275a76 Mon Sep 17 00:00:00 2001 From: Martin Thomson Date: Wed, 9 Sep 2020 08:32:40 +1000 Subject: [PATCH 2/7] oops --- draft-ietf-quic-recovery.md | 1 - 1 file changed, 1 deletion(-) diff --git a/draft-ietf-quic-recovery.md b/draft-ietf-quic-recovery.md index 6b3767f1c4..30fd0505a1 100644 --- a/draft-ietf-quic-recovery.md +++ b/draft-ietf-quic-recovery.md @@ -1699,7 +1699,6 @@ OnPacketsLost(lost_packets): // packets indicates persistent congestion. if (InPersistentCongestion(lost_packets)): congestion_window = kMinimumWindow - ssthresh = infinite congestion_recovery_start_time = 0 ~~~ From 779d9238f65c3373619615b3b05257196e811e8a Mon Sep 17 00:00:00 2001 From: Martin Thomson Date: Wed, 9 Sep 2020 08:38:19 +1000 Subject: [PATCH 3/7] Filter out packets sent before an RTT sample --- draft-ietf-quic-recovery.md | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/draft-ietf-quic-recovery.md b/draft-ietf-quic-recovery.md index 30fd0505a1..8bda3e7265 100644 --- a/draft-ietf-quic-recovery.md +++ b/draft-ietf-quic-recovery.md @@ -1589,6 +1589,9 @@ ssthresh: the mode is slow start and the window grows by the number of bytes acknowledged. +first_rtt_sample: +: The time that the first RTT sample was obtained. + ## Initialization @@ -1600,6 +1603,7 @@ congestion_window = kInitialWindow bytes_in_flight = 0 congestion_recovery_start_time = 0 ssthresh = infinite +first_rtt_sample = 0 for pn_space in [ Initial, Handshake, ApplicationData ]: ecn_ce_counters[pn_space] = 0 ~~~ @@ -1626,6 +1630,9 @@ InCongestionRecovery(sent_time): return sent_time <= congestion_recovery_start_time OnPacketsAcked(acked_packets): + if (first_rtt_sample == 0): + first_rtt_sample = now() + for acked_packet in acked_packets: OnPacketAcked(acked_packet) @@ -1695,8 +1702,14 @@ OnPacketsLost(lost_packets): for lost_packet in lost_packets: bytes_in_flight -= lost_packet.sent_bytes OnCongestionEvent(lost_packets.largest().time_sent) + // Reset the congestion window if the loss of these // packets indicates persistent congestion. + // Disregard packets sent prior to getting an RTT sample. + assert(first_rtt_sample != 0) + for lost in lost_packets: + if lost.time_sent <= first_rtt_sample: + lost_packets.remove(lost) if (InPersistentCongestion(lost_packets)): congestion_window = kMinimumWindow congestion_recovery_start_time = 0 From 20642c995d59efe11f6c5fbb575aa130e72f73f7 Mon Sep 17 00:00:00 2001 From: Martin Thomson Date: Wed, 9 Sep 2020 08:40:40 +1000 Subject: [PATCH 4/7] Make a copy for this --- draft-ietf-quic-recovery.md | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/draft-ietf-quic-recovery.md b/draft-ietf-quic-recovery.md index 8bda3e7265..c9f01d7fbf 100644 --- a/draft-ietf-quic-recovery.md +++ b/draft-ietf-quic-recovery.md @@ -1707,10 +1707,11 @@ OnPacketsLost(lost_packets): // packets indicates persistent congestion. // Disregard packets sent prior to getting an RTT sample. assert(first_rtt_sample != 0) - for lost in lost_packets: + pc_lost = lost_packets + for lost in pc_lost: if lost.time_sent <= first_rtt_sample: - lost_packets.remove(lost) - if (InPersistentCongestion(lost_packets)): + pc_lost.remove(lost) + if (InPersistentCongestion(pc_lost)): congestion_window = kMinimumWindow congestion_recovery_start_time = 0 ~~~ From c4bd59fee3d0c7b7d6508403d7b7b627b94eeb19 Mon Sep 17 00:00:00 2001 From: Martin Thomson Date: Wed, 9 Sep 2020 08:47:37 +1000 Subject: [PATCH 5/7] Copy inline --- draft-ietf-quic-recovery.md | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/draft-ietf-quic-recovery.md b/draft-ietf-quic-recovery.md index bb0dac6abf..d83697623e 100644 --- a/draft-ietf-quic-recovery.md +++ b/draft-ietf-quic-recovery.md @@ -1751,10 +1751,10 @@ OnPacketsLost(lost_packets): // packets indicates persistent congestion. // Disregard packets sent prior to getting an RTT sample. assert(first_rtt_sample != 0) - pc_lost = lost_packets - for lost in pc_lost: - if lost.time_sent <= first_rtt_sample: - pc_lost.remove(lost) + pc_lost = [] + for lost in lost_packets: + if lost.time_sent > first_rtt_sample: + pc_lost.insert(lost) if (InPersistentCongestion(pc_lost)): congestion_window = kMinimumWindow congestion_recovery_start_time = 0 From 9c2d7fb58100b6519a5d3fb523541c98d939cbcf Mon Sep 17 00:00:00 2001 From: Martin Thomson Date: Wed, 9 Sep 2020 08:48:54 +1000 Subject: [PATCH 6/7] curlies --- draft-ietf-quic-recovery.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/draft-ietf-quic-recovery.md b/draft-ietf-quic-recovery.md index d83697623e..fdde6e95a4 100644 --- a/draft-ietf-quic-recovery.md +++ b/draft-ietf-quic-recovery.md @@ -1751,7 +1751,7 @@ OnPacketsLost(lost_packets): // packets indicates persistent congestion. // Disregard packets sent prior to getting an RTT sample. assert(first_rtt_sample != 0) - pc_lost = [] + pc_lost = {} for lost in lost_packets: if lost.time_sent > first_rtt_sample: pc_lost.insert(lost) From 900535aec0ef510a8cac858ef4a7a17055467656 Mon Sep 17 00:00:00 2001 From: Martin Thomson Date: Wed, 9 Sep 2020 08:54:08 +1000 Subject: [PATCH 7/7] Invert comment --- draft-ietf-quic-recovery.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/draft-ietf-quic-recovery.md b/draft-ietf-quic-recovery.md index fdde6e95a4..d60fec5dec 100644 --- a/draft-ietf-quic-recovery.md +++ b/draft-ietf-quic-recovery.md @@ -1749,7 +1749,7 @@ OnPacketsLost(lost_packets): // Reset the congestion window if the loss of these // packets indicates persistent congestion. - // Disregard packets sent prior to getting an RTT sample. + // Only consider packets sent after getting an RTT sample. assert(first_rtt_sample != 0) pc_lost = {} for lost in lost_packets: