Subject: [quicwg/base-drafts] Clarify OnAckReceived pseudocode (#2140)

From: Benjamin Saunders <[email-masked]>
Date: Wed, 12 Dec 2018 22:44:47 -0800

I ran into a few points of confusion in the existing code:
- The time to update and scope of `largest_acked_packet` is not explicit
- `ProcessECN` references elements of `sent_packets` after they are removed by `OnPacketAcked`
- `DetectLostPackets` is passed an undefined variable

The fixes are trivial, except for the last: it isn't clear to me whether `DetectLostPackets` was intended to be called with `ack.largest_acked` or `largest_acked_packet`. These only differ when ACKs are reordered, and because the prior invocation of `DetectLostPackets` with a higher `largest_acked` would have already deemed lost all packets that the later invocation might deem lost, I don't think the behavior differs even then. Hence, removing the argument and consistently referencing `largest_acked_packet` seems to be the simpler option.
You can view, comment on, or merge this pull request online at:

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

-- Commit Summary --

  * Process ECN before forgetting newly acked packets
  * Clarify use of largest_acked_packet

-- File Changes --

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

-- Patch Links --

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

---

From: Martin Thomson <[email-masked]>
Date: Wed, 12 Dec 2018 23:02:04 -0800

martinthomson requested changes on this pull request.

> @@ -720,9 +720,14 @@ Pseudocode for OnAckReceived and UpdateRtt follow:
     // ack-eliciting, update the RTT.
     if (sent_packets[ack.largest_acked] &&
         sent_packets[ack.largest_acked].ack_eliciting):
+      largest_acked_packet = ack.largest_acked

This doesn't work because the largest_acked in the ACK might be newly acknowledged, but not the largest ever acknowledged.

```suggestion
      if ack.largest_acked > largest_acked_packet:
         largest_acked_packet = ack.largest_acked
```

>        latest_rtt = now - sent_packets[ack.largest_acked].time
       UpdateRtt(latest_rtt, ack.ack_delay)
 
+    // Process ECN information if present.
+    if (ACK frame contains ECN information):
+       ProcessECN(ack)

This is a separate change.  I think that you should make this a separate PR.

-- 
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/2140#pullrequestreview-184522784

---

From: Benjamin Saunders <[email-masked]>
Date: Wed, 12 Dec 2018 23:12:35 -0800

Split the `ProcessECN` tweak into https://github.com/quicwg/base-drafts/pull/2142, corrected the `largest_acked_packet` update logic, and added initialization for `largest_acked_packet`.

-- 
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/2140#issuecomment-446865673

---

From: janaiyengar <[email-masked]>
Date: Thu, 13 Dec 2018 19:39:27 +0000 (UTC)

janaiyengar approved this pull request.

A minor nit, but this is a nice cleanup. Thanks! I'll approve, but @ianswett should take a look as well.

> @@ -634,7 +634,7 @@ largest_sent_packet:
 : The packet number of the most recently sent packet.
 
 largest_acked_packet:
-: The largest packet number acknowledged in an ACK frame.
+: The largest packet number acknowledged by any ACK frame.

```suggestion
: The largest packet number acknowledged in the connection so far.
```

-- 
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/2140#pullrequestreview-184834092

---

From: Martin Thomson <[email-masked]>
Date: Thu, 13 Dec 2018 14:24:16 -0800

martinthomson 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/2140#pullrequestreview-184896913

---

From: Martin Thomson <[email-masked]>
Date: Thu, 13 Dec 2018 15:59:28 -0800

martinthomson commented on this pull request.

 largest_acked_packet:

Isn't this in the packet number space?

 largest_acked_packet:

I think that you mean "packet number space" rather than connection.

-- 
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/2140#discussion_r241605239

---

From: ianswett <[email-masked]>
Date: Mon, 17 Dec 2018 11:50:03 -0800

ianswett approved this pull request.

> @@ -716,6 +717,9 @@ Pseudocode for OnAckReceived and UpdateRtt follow:
 
 ~~~
   OnAckReceived(ack):
+    largest_acked_packet = max(largest_acked_packet,

I think this accurately describes the text we have, but I think we should add some text that you MAY use the largest acked from the most recent ACK, which could reduce spurious retransmits when one packet arrives far out of order?

-- 
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/2140#pullrequestreview-185768633

---

From: ianswett <[email-masked]>
Date: Mon, 17 Dec 2018 12:16:59 -0800

ianswett commented on this pull request.

 ~~~
   OnAckReceived(ack):

To clarify, I'm ok doing this in a follow up PR.

-- 
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/2140#discussion_r242301100

---

From: Benjamin Saunders <[email-masked]>
Date: Tue, 18 Dec 2018 10:20:02 -0800

Ralith commented on this pull request.

 ~~~
   OnAckReceived(ack):

> you MAY use the largest acked from the most recent ACK, which could reduce spurious retransmits when one packet arrives far out of order

Any reason not to make this the sole recommendation? I had some trouble coming up with a case where that might occur, which is why I didn't take that route originally, but I think I see it now: when a reordered ACK newly acknowledges a packet such that the newly acknowledged packet is > ack.largest_acked - kPacketThreshold, but <= largest_acked_packet - kPacketThreshold.

-- 
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/2140#discussion_r242649354

---

From: Benjamin Saunders <[email-masked]>
Date: Tue, 18 Dec 2018 10:21:52 -0800

~~~
   OnAckReceived(ack):

Er, wait, that's nonsense. Still having trouble seeing the case.

-- 
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/2140#discussion_r242649957

---

From: ianswett <[email-masked]>
Date: Tue, 18 Dec 2018 13:57:54 -0800

~~~
   OnAckReceived(ack):

It's the case when one packet is far out of order, and then subsequent packets arrive in order.  ie: 100 is received, then 1, 2, 3, 4, 5, etc.  I don't fully understand the cause, but I've seen small UDP packets jump ahead of large ones in a number of traces.

But I suspect Jana and I will merge this as is and we can add follow-up text about that issue if we think it's valuable.

-- 
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/2140#discussion_r242721000

---

From: ianswett <[email-masked]>
Date: Tue, 18 Dec 2018 23:15:02 +0000 (UTC)

@ianswett pushed 1 commit.

8fa3bf5283e9bd6df2b3cae3db44237e80b472e3  Merge branch 'master' into ack-cleanup

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/2140/files/659bb52e170fc6859a4e771704beefbf7a840f41..8fa3bf5283e9bd6df2b3cae3db44237e80b472e3

---

From: janaiyengar <[email-masked]>
Date: Tue, 18 Dec 2018 15:16:34 -0800

Merged #2140 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/2140#event-2034356402