Subject: [quicwg/base-drafts] Track smallest_newly_acked to prevent spurious RTO (#1822)

From: ianswett <[email-masked]>
Date: Tue, 02 Oct 2018 09:56:06 -0700

Fixes #1774
You can view, comment on, or merge this pull request online at:

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

-- Commit Summary --

  * Track smallest_newly_acked to prevent spurious RTO

-- File Changes --

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

-- Patch Links --

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

---

From: Marten Seemann <[email-masked]>
Date: Tue, 02 Oct 2018 10:46:51 -0700

marten-seemann commented on this pull request.

>        OnPacketAcked(acked_packet.packet_number)
+    
+    if smallest_newly_acked != 2^62

I still think we should make 0 the sentinel value.

> @@ -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():

Not an issue of this PR, but I noticed that `DetermineNewlyAckedPackets` is not defined. Do we need to fix 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/1822#pullrequestreview-160826558

---

From: ianswett <[email-masked]>
Date: Tue, 02 Oct 2018 17:48:40 +0000 (UTC)

ianswett commented on this pull request.

I can comment what it does more clearly, but I don't want to define it.  There are multiple different ways to determine newly acked packets based on one's data structure choices, so I think it's very implementation dependent.

-- 
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/1822#discussion_r222049231

---

From: ianswett <[email-masked]>
Date: Tue, 02 Oct 2018 10:49:02 -0700

+    

I was going to, but 0 is a valid packet number.

-- 
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/1822#discussion_r222049387

---

From: Marten Seemann <[email-masked]>
Date: Tue, 02 Oct 2018 10:51:08 -0700

+    

My point is that it shouldn't be, in order to avoid ugly code like this here, and at many other places.

-- 
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/1822#discussion_r222050125

---

From: ianswett <[email-masked]>
Date: Tue, 02 Oct 2018 10:53:12 -0700

@ianswett pushed 1 commit.

bae5729  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/1822/files/b703cfd170a202b7f67b117f0667f7579dd5890a..bae572957ba5e27a55836b2f4209291ee6cd030c

---

From: ianswett <[email-masked]>
Date: Tue, 02 Oct 2018 10:53:37 -0700

+    

I'm not disagreeing, but I don't think this is the PR to change 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/1822#discussion_r222050988

---

From: ianswett <[email-masked]>
Date: Tue, 02 Oct 2018 10:55:44 -0700

7175373  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/1822/files/bae572957ba5e27a55836b2f4209291ee6cd030c..7175373fa3dde589fc01b575e45b2af466bc2e28

---

From: ianswett <[email-masked]>
Date: Tue, 02 Oct 2018 10:56:29 -0700

39abdea  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/1822/files/7175373fa3dde589fc01b575e45b2af466bc2e28..39abdea949f3949b15ffb1103ca87f906c91b541

---

From: ianswett <[email-masked]>
Date: Tue, 02 Oct 2018 10:56:53 -0700

Changed to a constant for readability.

-- 
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/1822#issuecomment-426370766

---

From: ianswett <[email-masked]>
Date: Tue, 02 Oct 2018 11:03:51 -0700

b7db26c  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/1822/files/39abdea949f3949b15ffb1103ca87f906c91b541..b7db26c0a480dbb0b9ea9a91e040ea1795364813

---

From: ianswett <[email-masked]>
Date: Tue, 02 Oct 2018 11:57:52 -0700

69ae1bc  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/1822/files/b7db26c0a480dbb0b9ea9a91e040ea1795364813..69ae1bcda07f5261d912297db95cebb939d591ff

---

From: janaiyengar <[email-masked]>
Date: Wed, 03 Oct 2018 12:26:25 -0700

janaiyengar commented on this pull request.

Yes, I agree with @ianswett 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/1822#discussion_r222437530

---

From: janaiyengar <[email-masked]>
Date: Wed, 03 Oct 2018 12:33:34 -0700

>      for acked_packet in DetermineNewlyAckedPackets():
+      smallest_newly_acked =
+        min(smallest_newly_acked, acked_packet.packet_number)

This assumes an ordering in the processing of the acked packets data structure. I would make this simpler:

// Find all newly acked packets in this ACK frame
newly_acked = DetermineNewlyAckedPackets(ack)

// Find the smallest packet that is newly acked in this ACK frame.
smallest_newly_acked = FindSmallestNewlyAcked(newly_acked)

You don't have to define either function, but add a comment that describes what they do.

-- 
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/1822#pullrequestreview-161326463

---

From: janaiyengar <[email-masked]>
Date: Wed, 03 Oct 2018 12:33:57 -0700

> @@ -530,6 +532,9 @@ 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.

not needed if you use my suggestion below.

-- 
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/1822#pullrequestreview-161326597

---

From: ianswett <[email-masked]>
Date: Wed, 03 Oct 2018 12:42:09 -0700

+      smallest_newly_acked =

I don't believe it does assume ordering, that's why I'm using a min, but your suggestion works 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/1822#discussion_r222442024

---

From: ianswett <[email-masked]>
Date: Wed, 03 Oct 2018 12:44:15 -0700

9aef36f  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/1822/files/69ae1bcda07f5261d912297db95cebb939d591ff..9aef36f0ff5a88304f66f5d13b8bb5f01eb007f3

---

From: ianswett <[email-masked]>
Date: Wed, 03 Oct 2018 12:46:05 -0700

7b4ed8c  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/1822/files/9aef36f0ff5a88304f66f5d13b8bb5f01eb007f3..7b4ed8c83580cec4cd80ce914b15beb84699ffef

---

From: ianswett <[email-masked]>
Date: Wed, 03 Oct 2018 12:52:42 -0700

4f058e8  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/1822/files/7b4ed8c83580cec4cd80ce914b15beb84699ffef..4f058e8319e5c93a3d325206fe238349c5d3df0d

---

From: ianswett <[email-masked]>
Date: Wed, 03 Oct 2018 12:57:28 -0700

Pseudocode updated, PTAL

-- 
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/1822#issuecomment-426778614

---

From: janaiyengar <[email-masked]>
Date: Wed, 03 Oct 2018 13:10:07 -0700

@janaiyengar pushed 1 commit.

f226f36  acket

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

---

From: janaiyengar <[email-masked]>
Date: Wed, 03 Oct 2018 13:11:26 -0700

Merged #1822 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/1822#event-1882913977