Subject: [quicwg/base-drafts] Don't update RTT for ack-only packets (#984)

From: Martin Thomson <[email-masked]>
Date: Tue, 05 Dec 2017 01:01:46 +0000 (UTC)

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

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

-- Commit Summary --

  * Don't update RTT for ack-only packets

-- File Changes --

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

-- Patch Links --

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

---

From: ianswett <[email-masked]>
Date: Tue, 05 Dec 2017 01:07:30 +0000 (UTC)

ianswett commented on this pull request.

I'm not sure this is clearly the right choice.  The reason ack only packets shouldn't be considered for creating a model of ack delay is different from whether they should be considered for RTT in general.

-- 
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/984#pullrequestreview-81042974

---

From: Marten Seemann <[email-masked]>
Date: Mon, 04 Dec 2017 17:08:03 -0800

I don't think ack-only packets and acks for non-retransmittable packets are necessarily the same.

-- 
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/984#issuecomment-349161207

---

From: janaiyengar <[email-masked]>
Date: Mon, 04 Dec 2017 17:28:47 -0800

janaiyengar commented on this pull request.

Thanks for getting to this, you beat me to it :-) A few nits.

> @@ -577,15 +577,22 @@ Pseudocode for OnPacketSent follows:
 
 ### On Ack Receipt
 
-When an ack is received, it may acknowledge 0 or more packets.
+When an acknowledgment is received, it may acknowledge 0 or more packets.
+
+The first acknowledgment for the largest acknowledged packet is used to update

nit: "first acknowledgement received for", just to clarify that it's the first one we've seen, not the first one sent.

> @@ -577,15 +577,22 @@ Pseudocode for OnPacketSent follows:
 
 ### On Ack Receipt
 
-When an ack is received, it may acknowledge 0 or more packets.
+When an acknowledgment is received, it may acknowledge 0 or more packets.
+
+The first acknowledgment for the largest acknowledged packet is used to update
+the estimate of minimum and smoothed RTT.  However, if the largest acknowledged
+packet would not have caused an acknowledgment to be sent, RTT estimates are not
+updated, because the acknowledgment could be significantly delayed.

Use normative language: "... RTT estimates MUST NOT be updated..."

 Pseudocode for OnAckReceived and UpdateRtt follow:
 
 ~~~
    OnAckReceived(ack):
      largest_acked_packet = ack.largest_acked
-     // If the largest acked is newly acked, update the RTT.
-     if (sent_packets[ack.largest_acked]):
+     // If the largest acked is newly acked and is a packet
+     // that would normally cause an ack, update the RTT.
+     if (sent_packets[ack.largest_acked] and
+         not ack_only(sent_packets[ack.largest_acked])):

We use "ackable" as a term in this doc to refer to a packet that triggers an ack. I just realized that it's not yet defined in the doc, but I would use that term ("and is_ackable(sent_packets[ack.largest_acked])"). I'm happy to write up a definition for "ackable" in a PR.

Alternatively, you can leave this as you have it, and I can change it when I actually define the term. Up to you.

-- 
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/984#pullrequestreview-81042886

---

From: Martin Thomson <[email-masked]>
Date: Mon, 04 Dec 2017 17:45:36 -0800

@martinthomson pushed 2 commits.

81e0bba  Make normative, clearer
2651b66  Use the as-yet-undefined is_ackable()

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/984/files/28baaa07fca849b846375035db84cae81e69929e..2651b66e761eb8fbbce58b8594faeb37117746a9

---

From: Martin Thomson <[email-masked]>
Date: Tue, 05 Dec 2017 01:47:53 +0000 (UTC)

@ianswett, I was operating on the assumption that a long ACK delay would reduce the accuracy of the RTT estimate.  I agree that this might be too blunt an instrument though.  I'll let you sort it out.

-- 
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/984#issuecomment-349168461

---

From: Martin Thomson <[email-masked]>
Date: Wed, 06 Dec 2017 01:18:48 +0000 (UTC)

Closed by #991.

-- 
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/984#issuecomment-349497550

---

From: Martin Thomson <[email-masked]>
Date: Tue, 05 Dec 2017 17:18:49 -0800

Closed #984.

-- 
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/984#event-1373887817