From b703cfd170a202b7f67b117f0667f7579dd5890a Mon Sep 17 00:00:00 2001 From: ianswett Date: Tue, 2 Oct 2018 12:55:45 -0400 Subject: [PATCH 01/10] Track smallest_newly_acked to prevent spurious RTO Fixes #1774 --- draft-ietf-quic-recovery.md | 24 ++++++++++++++---------- 1 file changed, 14 insertions(+), 10 deletions(-) diff --git a/draft-ietf-quic-recovery.md b/draft-ietf-quic-recovery.md index 7fc1a57fb1..8862fe9196 100644 --- a/draft-ietf-quic-recovery.md +++ b/draft-ietf-quic-recovery.md @@ -690,9 +690,22 @@ Pseudocode for OnAckReceived and UpdateRtt follow: if (sent_packets[ack.largest_acked]): latest_rtt = now - sent_packets[ack.largest_acked].time UpdateRtt(latest_rtt, ack.ack_delay) - // Find all newly acked packets. + // Find all newly acked packets and track the smallest + smallest_newly_acked = 2^62 for acked_packet in DetermineNewlyAckedPackets(): + smallest_newly_acked = + min(smallest_newly_acked, acked_packet.packet_number) OnPacketAcked(acked_packet.packet_number) + + if smallest_newly_acked != 2^62 + // If any packets sent prior to the RTO were acked, then + // the RTO was spurious. Otherwise, inform congestion control. + if (rto_count > 0 && + smallest_newly_acked > largest_sent_before_rto): + OnRetransmissionTimeoutVerified(smallest_newly_acked) + handshake_count = 0 + tlp_count = 0 + rto_count = 0 DetectLostPackets(ack.largest_acked_packet) SetLossDetectionTimer() @@ -737,15 +750,6 @@ Pseudocode for OnPacketAcked follows: OnPacketAcked(acked_packet): if (!acked_packet.is_ack_only): OnPacketAckedCC(acked_packet) - // If a packet sent prior to RTO was acked, then the RTO - // was spurious. Otherwise, inform congestion control. - if (rto_count > 0 && - acked_packet.packet_number > largest_sent_before_rto): - OnRetransmissionTimeoutVerified( - acked_packet.packet_number) - handshake_count = 0 - tlp_count = 0 - rto_count = 0 sent_packets.remove(acked_packet.packet_number) ~~~ From bae572957ba5e27a55836b2f4209291ee6cd030c Mon Sep 17 00:00:00 2001 From: ianswett Date: Tue, 2 Oct 2018 13:53:01 -0400 Subject: [PATCH 02/10] Update draft-ietf-quic-recovery.md --- 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 8862fe9196..696b94e7ca 100644 --- a/draft-ietf-quic-recovery.md +++ b/draft-ietf-quic-recovery.md @@ -696,7 +696,7 @@ Pseudocode for OnAckReceived and UpdateRtt follow: smallest_newly_acked = min(smallest_newly_acked, acked_packet.packet_number) OnPacketAcked(acked_packet.packet_number) - + if smallest_newly_acked != 2^62 // If any packets sent prior to the RTO were acked, then // the RTO was spurious. Otherwise, inform congestion control. From 7175373fa3dde589fc01b575e45b2af466bc2e28 Mon Sep 17 00:00:00 2001 From: ianswett Date: Tue, 2 Oct 2018 13:55:27 -0400 Subject: [PATCH 03/10] Update draft-ietf-quic-recovery.md --- draft-ietf-quic-recovery.md | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/draft-ietf-quic-recovery.md b/draft-ietf-quic-recovery.md index 696b94e7ca..ed2f5dd836 100644 --- a/draft-ietf-quic-recovery.md +++ b/draft-ietf-quic-recovery.md @@ -530,6 +530,9 @@ kDelayedAckTimeout: kInitialRtt: : The RTT used before an RTT sample is taken. The RECOMMENDED value is 100ms. +kInvalidPacketNumber: +: An invalid packet number. May be any packet number that is never sent. + ### Variables of interest Variables required to implement the congestion control mechanisms @@ -691,13 +694,13 @@ Pseudocode for OnAckReceived and UpdateRtt follow: latest_rtt = now - sent_packets[ack.largest_acked].time UpdateRtt(latest_rtt, ack.ack_delay) // Find all newly acked packets and track the smallest - smallest_newly_acked = 2^62 + smallest_newly_acked = kInvalidPacketNumber for acked_packet in DetermineNewlyAckedPackets(): smallest_newly_acked = min(smallest_newly_acked, acked_packet.packet_number) OnPacketAcked(acked_packet.packet_number) - if smallest_newly_acked != 2^62 + if smallest_newly_acked != kInvalidPacketNumber // If any packets sent prior to the RTO were acked, then // the RTO was spurious. Otherwise, inform congestion control. if (rto_count > 0 && From 39abdea949f3949b15ffb1103ca87f906c91b541 Mon Sep 17 00:00:00 2001 From: ianswett Date: Tue, 2 Oct 2018 13:56:25 -0400 Subject: [PATCH 04/10] Update draft-ietf-quic-recovery.md --- 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 ed2f5dd836..11458c7294 100644 --- a/draft-ietf-quic-recovery.md +++ b/draft-ietf-quic-recovery.md @@ -531,7 +531,7 @@ kInitialRtt: : The RTT used before an RTT sample is taken. The RECOMMENDED value is 100ms. kInvalidPacketNumber: -: An invalid packet number. May be any packet number that is never sent. +: An invalid packet number. Any number larger than the largest packet number. ### Variables of interest From b7db26c0a480dbb0b9ea9a91e040ea1795364813 Mon Sep 17 00:00:00 2001 From: ianswett Date: Tue, 2 Oct 2018 14:03:41 -0400 Subject: [PATCH 05/10] Update draft-ietf-quic-recovery.md --- draft-ietf-quic-recovery.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/draft-ietf-quic-recovery.md b/draft-ietf-quic-recovery.md index 11458c7294..dbe0bb5f05 100644 --- a/draft-ietf-quic-recovery.md +++ b/draft-ietf-quic-recovery.md @@ -701,8 +701,8 @@ Pseudocode for OnAckReceived and UpdateRtt follow: OnPacketAcked(acked_packet.packet_number) if smallest_newly_acked != kInvalidPacketNumber - // If any packets sent prior to the RTO were acked, then - // the RTO was spurious. Otherwise, inform congestion control. + // If any packets sent prior to RTO were acked, then the + // RTO was spurious. Otherwise, inform congestion control. if (rto_count > 0 && smallest_newly_acked > largest_sent_before_rto): OnRetransmissionTimeoutVerified(smallest_newly_acked) From 69ae1bcda07f5261d912297db95cebb939d591ff Mon Sep 17 00:00:00 2001 From: ianswett Date: Tue, 2 Oct 2018 14:57:44 -0400 Subject: [PATCH 06/10] Update draft-ietf-quic-recovery.md --- draft-ietf-quic-recovery.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/draft-ietf-quic-recovery.md b/draft-ietf-quic-recovery.md index dbe0bb5f05..badc9db0f5 100644 --- a/draft-ietf-quic-recovery.md +++ b/draft-ietf-quic-recovery.md @@ -419,7 +419,9 @@ necessary to add this delay explicitly in the TLP and RTO computation. When an acknowledgment is received for a packet sent on an RTO event, any unacknowledged packets with lower packet numbers than those acknowledged MUST be -marked as lost. +marked as lost. If an acknowledgement for a packet sent on an RTO is received +at the same time packets sent prior to the first RTO are acknowledged, the RTO +is considered spurious and standard loss detection rules apply. A packet sent when an RTO timer expires MAY carry new data if available or unacknowledged data to potentially reduce recovery time. Since this packet is From 9aef36f0ff5a88304f66f5d13b8bb5f01eb007f3 Mon Sep 17 00:00:00 2001 From: ianswett Date: Wed, 3 Oct 2018 15:44:07 -0400 Subject: [PATCH 07/10] Update draft-ietf-quic-recovery.md --- draft-ietf-quic-recovery.md | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/draft-ietf-quic-recovery.md b/draft-ietf-quic-recovery.md index badc9db0f5..ea3b37ba15 100644 --- a/draft-ietf-quic-recovery.md +++ b/draft-ietf-quic-recovery.md @@ -532,9 +532,6 @@ kDelayedAckTimeout: kInitialRtt: : The RTT used before an RTT sample is taken. The RECOMMENDED value is 100ms. -kInvalidPacketNumber: -: An invalid packet number. Any number larger than the largest packet number. - ### Variables of interest Variables required to implement the congestion control mechanisms @@ -695,14 +692,17 @@ Pseudocode for OnAckReceived and UpdateRtt follow: if (sent_packets[ack.largest_acked]): latest_rtt = now - sent_packets[ack.largest_acked].time UpdateRtt(latest_rtt, ack.ack_delay) - // Find all newly acked packets and track the smallest - smallest_newly_acked = kInvalidPacketNumber - for acked_packet in DetermineNewlyAckedPackets(): + + // Find all newly acked packets in this ACK frame + newly_acked_packets = DetermineNewlyAckedPackets(ack) + // Find the smallest packet that is newly acked in this ACK frame. + smallest_newly_acked = FindSmallestNewlyAcked(newly_acked_packets) + for acked_packet in newly_acked_packets: smallest_newly_acked = min(smallest_newly_acked, acked_packet.packet_number) OnPacketAcked(acked_packet.packet_number) - if smallest_newly_acked != kInvalidPacketNumber + if !newly_acket_packets.empty(): // If any packets sent prior to RTO were acked, then the // RTO was spurious. Otherwise, inform congestion control. if (rto_count > 0 && From 7b4ed8c83580cec4cd80ce914b15beb84699ffef Mon Sep 17 00:00:00 2001 From: ianswett Date: Wed, 3 Oct 2018 15:45:58 -0400 Subject: [PATCH 08/10] Update draft-ietf-quic-recovery.md --- draft-ietf-quic-recovery.md | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/draft-ietf-quic-recovery.md b/draft-ietf-quic-recovery.md index ea3b37ba15..c0a3d07417 100644 --- a/draft-ietf-quic-recovery.md +++ b/draft-ietf-quic-recovery.md @@ -695,14 +695,12 @@ Pseudocode for OnAckReceived and UpdateRtt follow: // Find all newly acked packets in this ACK frame newly_acked_packets = DetermineNewlyAckedPackets(ack) - // Find the smallest packet that is newly acked in this ACK frame. - smallest_newly_acked = FindSmallestNewlyAcked(newly_acked_packets) for acked_packet in newly_acked_packets: - smallest_newly_acked = - min(smallest_newly_acked, acked_packet.packet_number) OnPacketAcked(acked_packet.packet_number) if !newly_acket_packets.empty(): + // Find the smallest packet that is newly acked in this ACK frame. + smallest_newly_acked = FindSmallestNewlyAcked(newly_acked_packets) // If any packets sent prior to RTO were acked, then the // RTO was spurious. Otherwise, inform congestion control. if (rto_count > 0 && From 4f058e8319e5c93a3d325206fe238349c5d3df0d Mon Sep 17 00:00:00 2001 From: ianswett Date: Wed, 3 Oct 2018 15:52:34 -0400 Subject: [PATCH 09/10] Update draft-ietf-quic-recovery.md --- draft-ietf-quic-recovery.md | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/draft-ietf-quic-recovery.md b/draft-ietf-quic-recovery.md index c0a3d07417..4286f2b446 100644 --- a/draft-ietf-quic-recovery.md +++ b/draft-ietf-quic-recovery.md @@ -699,8 +699,9 @@ Pseudocode for OnAckReceived and UpdateRtt follow: OnPacketAcked(acked_packet.packet_number) if !newly_acket_packets.empty(): - // Find the smallest packet that is newly acked in this ACK frame. - smallest_newly_acked = FindSmallestNewlyAcked(newly_acked_packets) + // Find the smallest newly acknowledged packet + smallest_newly_acked = + FindSmallestNewlyAcked(newly_acked_packets) // If any packets sent prior to RTO were acked, then the // RTO was spurious. Otherwise, inform congestion control. if (rto_count > 0 && From f226f3625116638758c6cc778635229145238e70 Mon Sep 17 00:00:00 2001 From: janaiyengar Date: Wed, 3 Oct 2018 16:09:57 -0400 Subject: [PATCH 10/10] acket --- 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 4286f2b446..39b6dd970a 100644 --- a/draft-ietf-quic-recovery.md +++ b/draft-ietf-quic-recovery.md @@ -698,7 +698,7 @@ Pseudocode for OnAckReceived and UpdateRtt follow: for acked_packet in newly_acked_packets: OnPacketAcked(acked_packet.packet_number) - if !newly_acket_packets.empty(): + if !newly_acked_packets.empty(): // Find the smallest newly acknowledged packet smallest_newly_acked = FindSmallestNewlyAcked(newly_acked_packets)