Subject: [quicwg/base-drafts] Out of order relative to other ack-eliciting packets (#4000)

From: ianswett <[email-masked]>
Date: Sun, 16 Aug 2020 12:46:04 -0700

I believe this was the existing intent, so not filing an issue, people can feel free to disagree.

The case I&#39;m concerned about is an ACK arrives with PN 10, then PN 9 arrives.  I do not believe PN 9 should not be considered &#39;out-of-order&#39; and trigger an immediate ACK.  Recovery is designed with this case in mind, but when re-reading this text, I thought it was unclear.
You can view, comment on, or merge this pull request online at:

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

-- Commit Summary --

  * Out of order relative to other ack-eliciting packets

-- File Changes --

    M draft-ietf-quic-transport.md (10)

-- Patch Links --

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

---

From: Nick Banks <[email-masked]>
Date: Sun, 16 Aug 2020 12:50:31 -0700

What are the downsides of not doing this and generally having logic that triggers an immediate ACK if any out-of-order packet is received?

-- 
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/4000#issuecomment-674569731

---

From: Nick Banks <[email-masked]>
Date: Sun, 16 Aug 2020 12:51:50 -0700

Also, how would you recommend implementing this logic, assuming you already have the general logic? Keep a flag `LastPacketReceivedWasAckEliciting` and only trigger the immediate ACK if that is `true`?

-- 
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/4000#issuecomment-674569854

---

From: ianswett <[email-masked]>
Date: Sun, 16 Aug 2020 12:58:38 -0700

The code that sends ACK frames would track the largest received ack-eliciting packet to determine if packets are out of order, not the largest received packet.

This is similar to PTO, which is armed from the largest ack-eliciting packet, not the largest 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/4000#issuecomment-674570627

---

From: ianswett <[email-masked]>
Date: Sun, 16 Aug 2020 13:00:09 -0700

The text is already very clear that if you receive an ACK-only packet out of order, you MUST NOT send an ACK.  The goal if this PR is to clarify that ACK-only packets don't count when deciding when ack-eliciting packets are 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/4000#issuecomment-674570787

---

From: Kazuho Oku <[email-masked]>
Date: Sun, 16 Aug 2020 18:39:30 -0700

@kazuho commented on this pull request.

I think we need to change section 1.2 too. There, we have a pretty clear definition of what out-of-order means, that does not align with what we want to say.

_Out-of-order packet: a packet that does not increase the largest received packet number for its packet number space (Section 12.3) by exactly one. ([section 1.2](https://quicwg.org/base-drafts/draft-ietf-quic-transport.html#section-1.2-3.8))_

-- 
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/4000#pullrequestreview-468121792

---

From: Martin Thomson <[email-masked]>
Date: Sun, 16 Aug 2020 18:40:11 -0700

@martinthomson commented on this pull request.

I'm ambivalent on this.  Having immediate acknowledgment if an ack-eliciting frame is reordered relative to a non-ack-eliciting frame isn't going to cause me to lose much sleep if that is how people implement this "SHOULD".

> @@ -3558,11 +3558,11 @@ packets are eventually acknowledged when the endpoint sends an ACK frame in
 response to other events.
 
 In order to assist loss detection at the sender, an endpoint SHOULD send an ACK
-frame immediately on receiving an ack-eliciting packet that is out of order. The
-endpoint SHOULD NOT continue sending ACK frames immediately unless more
-ack-eliciting packets are received out of order.  If every subsequent
-ack-eliciting packet arrives out of order, then an ACK frame SHOULD be sent
-immediately for every received ack-eliciting packet.
+frame immediately on receiving an ack-eliciting packet that is out of order from

```suggestion
frame immediately on receiving an ack-eliciting packet that is out of order relative to
```

-- 
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/4000#pullrequestreview-468121697

---

From: Kazuho Oku <[email-masked]>
Date: Sun, 16 Aug 2020 19:05:08 -0700

Followup on my [review comment](https://github.com/quicwg/base-drafts/pull/4000#pullrequestreview-468121792), I think what we need to do is to clarify that the scope of out-of-order packets is limited to ack-eliciting packets.

We can say that a out-order packet is defined as "a newly received ack-eliciting packet carrying a packet number that is either smaller than those of any previously-received ack-eliciting packets, or is more than one greater than any of those packets."

-- 
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/4000#issuecomment-674618540

---

From: Martin Thomson <[email-masked]>
Date: Sun, 16 Aug 2020 20:35:53 -0700

How do you tell if the absent packets are ack-eliciting?  I mean, most scenarios we use this definition are for the receiver, which can't know whether a missing packet is ack-eliciting.

-- 
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/4000#issuecomment-674638169

---

From: Kazuho Oku <[email-masked]>
Date: Sun, 16 Aug 2020 22:07:37 -0700

@martinthomson I've left my two cents regarding that concern on the [issue comment](https://github.com/quicwg/base-drafts/issues/4001#issuecomment-674657546).

-- 
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/4000#issuecomment-674658049

---

From: Mike Bishop <[email-masked]>
Date: Mon, 17 Aug 2020 08:43:20 -0700

> What are the downsides of not doing this and generally having logic that triggers an immediate ACK if any out-of-order packet is received?

@nibanks, if I'm understanding correctly, the concern is that an implementation that sends ACK-only packets immediately but paces others will routinely have ACK-only packets skipping ahead of ACK-eliciting packets.  If you react to all reordering, you'll send a lot of unneeded immediate ACKs when interacting with such an implementation.

-- 
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/4000#issuecomment-674956818

---

From: Nick Banks <[email-masked]>
Date: Mon, 17 Aug 2020 08:54:39 -0700

> @nibanks, if I'm understanding correctly, the concern is that an implementation that sends ACK-only packets immediately but paces others will routinely have ACK-only packets skipping ahead of ACK-e…

Yeah, that makes sense. Thanks.

-- 
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/4000#issuecomment-674962941

---

From: ianswett <[email-masked]>
Date: Mon, 17 Aug 2020 09:12:57 -0700

@MikeBishop has it right.  It doesn't cause a correctness issue, but it could result in a large number of unnecessary ACK-only packets being sent.

-- 
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/4000#issuecomment-674972594

---

From: ianswett <[email-masked]>
Date: Mon, 17 Aug 2020 12:58:29 -0700

@ianswett pushed 1 commit.

3ca0b89c8bd6caddc68559084fbfbd1095899436  Update draft-ietf-quic-transport.md

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/4000/files/e0e024dd0456160102bdf39314417ebd048361ab..3ca0b89c8bd6caddc68559084fbfbd1095899436

---

From: ianswett <[email-masked]>
Date: Mon, 17 Aug 2020 13:06:30 -0700

5cdb166db5f2d9ccc5745129511d91a12b334937  Martin's suggestion

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/4000/files/3ca0b89c8bd6caddc68559084fbfbd1095899436..5cdb166db5f2d9ccc5745129511d91a12b334937

---

From: ianswett <[email-masked]>
Date: Mon, 17 Aug 2020 13:16:31 -0700

ea6359f86f09fce5b206fe27f6ff5753b9b1ccd8  Update draft-ietf-quic-transport.md

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/4000/files/5cdb166db5f2d9ccc5745129511d91a12b334937..ea6359f86f09fce5b206fe27f6ff5753b9b1ccd8

---

From: Mike Bishop <[email-masked]>
Date: Mon, 17 Aug 2020 13:20:12 -0700

@MikeBishop pushed 1 commit.

b89274cb3f88af6986917455310fa0438649d5b1  Line break to enable bullets

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/4000/files/ea6359f86f09fce5b206fe27f6ff5753b9b1ccd8..b89274cb3f88af6986917455310fa0438649d5b1

---

From: ianswett <[email-masked]>
Date: Mon, 17 Aug 2020 13:20:29 -0700

I ended up largely taking @martinthomson suggestion from https://github.com/quicwg/base-drafts/issues/4001#issuecomment-674661218, and no longer relying on out-of-order, but didn't remove the definition entirely.  I can if people prefer.

-- 
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/4000#issuecomment-675092849

---

From: Martin Thomson <[email-masked]>
Date: Mon, 17 Aug 2020 15:00:59 -0700

Thanks for writing this down.

> +* When the received packet has a packet number less than another ack-eliciting
+  packet that has been received.
+* When the packet has a packet number larger than any other ack-eliciting packet
+  that has been received and there are missing packets between that other packet
+  and this packet.

I think that this list construction shouldn't be capitalized:

```suggestion
* when the received packet has a packet number less than another ack-eliciting
  packet that has been received, or
* when the packet has a packet number larger than the highest-numbered
  ack-eliciting packet that has been received and there are missing packets between
  that packet and this packet.
```

> +The endpoint SHOULD NOT continue sending ACK frames immediately unless one of
+the above criteria are satisfied.

This is something I'm less comfortable with, because the delayed-ack draft immediately has us violate this.  I would be more comfortable if this said that an endpoint only sends an immediate acknowledgment if it has received two or more unacknowledged ack-eliciting packets or something like 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/4000#pullrequestreview-468859922

---

From: ianswett <[email-masked]>
Date: Tue, 18 Aug 2020 10:34:04 -0700

a881299b06fe25abaee03fb192693583b4f6af93  Update draft-ietf-quic-transport.md

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/4000/files/b89274cb3f88af6986917455310fa0438649d5b1..a881299b06fe25abaee03fb192693583b4f6af93

---

From: ianswett <[email-masked]>
Date: Tue, 18 Aug 2020 10:37:45 -0700

@ianswett commented on this pull request.

This is an effort to keep the existing text, but rework it to refer to the criteria that's been reworked to no longer mention out of order.

Specifically, the existing text is "The endpoint SHOULD NOT continue sending ACK frames immediately unless more ack-eliciting packets are received out of order."

The goal of this text is: you should send one immediate ACK if the above happens, but don't keep sending immediate ACKs for every subsequent ack-eliciting packet, because that would be excessive.

Suggestions welcome.

-- 
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/4000#discussion_r472368830

---

From: ianswett <[email-masked]>
Date: Tue, 18 Aug 2020 10:39:01 -0700

d7a4c50c6b159ba69a75f6ef136c7de09e702b9c  Update draft-ietf-quic-transport.md

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/4000/files/a881299b06fe25abaee03fb192693583b4f6af93..d7a4c50c6b159ba69a75f6ef136c7de09e702b9c

---

From: Kazuho Oku <[email-masked]>
Date: Tue, 18 Aug 2020 14:36:27 -0700

Changes to the text looks good to me.

As @martinthomson suggested, I think we can remove the definition of "out-of-order packet" in section 1.2 now that we do not rely on it.

The draft assumes that people understand what "out-of-order *delivery*" means, when we discuss about frames or stream IDs arriving out-of-order. We can rely on that for packets too.

-- 
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/4000#pullrequestreview-469847689

---

From: ianswett <[email-masked]>
Date: Tue, 18 Aug 2020 14:58:44 -0700

c467dba1750a3bece8cae6ba082bc464fe0f5026  Remove out-of-order definition

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/4000/files/d7a4c50c6b159ba69a75f6ef136c7de09e702b9c..c467dba1750a3bece8cae6ba082bc464fe0f5026

---

From: ianswett <[email-masked]>
Date: Tue, 18 Aug 2020 14:59:14 -0700

Thanks @kazuho, removed.

-- 
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/4000#issuecomment-675741020

---

From: ianswett <[email-masked]>
Date: Tue, 18 Aug 2020 15:12:38 -0700

b7b431b4c364e0751ab0360eb041717e78e1ecad  Remove redundant SHOULD NOT

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/4000/files/c467dba1750a3bece8cae6ba082bc464fe0f5026..b7b431b4c364e0751ab0360eb041717e78e1ecad

---

From: Kazuho Oku <[email-masked]>
Date: Tue, 18 Aug 2020 19:31:10 -0700

@kazuho approved this pull request.

@ianswett Thank you for all your work.

-- 
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/4000#pullrequestreview-470082849

---

From: martinduke <[email-masked]>
Date: Fri, 28 Aug 2020 13:39:39 -0700

@martinduke 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/4000#pullrequestreview-478012760

---

From: Jana Iyengar <[email-masked]>
Date: Fri, 28 Aug 2020 17:47:07 -0700

@janaiyengar 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/4000#pullrequestreview-478083053