Subject: [quicwg/base-drafts] Unified ACK frame (#1706)

From: Martin Thomson <[email-masked]>
Date: Mon, 27 Aug 2018 01:11:06 -0700

This takes the design discussed on the list and enacts it.  This turns
out to be quite simple.  Lots of red in this change.

This treats ECT(1) as equivalent to unmarked, meaning that QUIC won't be
able to use ECT(1) without an extension or revision.  That keeps the
encoding simpler.

Closes #1439.
You can view, comment on, or merge this pull request online at:

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

-- Commit Summary --

  * Unified ACK frame

-- File Changes --

    M draft-ietf-quic-recovery.md (47)
    M draft-ietf-quic-transport.md (213)

-- Patch Links --

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

---

From: Magnus Westerlund <[email-masked]>
Date: Mon, 27 Aug 2018 02:39:19 -0700

gloinul commented on this pull request.

-~~~
-  largest = previous_smallest - gap - 2
-~~~
-
-If the calculated value for largest or smallest packet number for any ACK Block
-is negative, an endpoint MUST generate a connection error of type
-FRAME_ENCODING_ERROR indicating an error in an ACK frame.
+: Packets that are ECT(0) marked are acknowledged using a type of 3.

Should this read .. using a type of 2? 

-The value of the Gap field establishes the largest packet number value for the
-ACK block that follows the gap using the following formula:
+ECT(0) Marked (2):

I wonder if this type should be called just ECT Marked (2). And then make it clear that only packet with an expected ECT codepoint is to be reported. The default behavior is that only ECT(0) SHALL be reported using this type. Future extensions may change which code points that are reported using this type. That way extensions, such as a L4S congestion control extension in the future can define that ECT(1) is the one to be reported using this type after the end-point has agreed on 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/1706#pullrequestreview-149628073

---

From: MikkelFJ <[email-masked]>
Date: Mon, 27 Aug 2018 02:51:33 -0700

mikkelfj commented on this pull request.

-~~~

type of 2?

-- 
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/1706#pullrequestreview-149648995

---

From: Martin Thomson <[email-masked]>
Date: Mon, 27 Aug 2018 02:55:14 -0700

@martinthomson pushed 1 commit.

12f063a  ECT(0) = Type 2

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/1706/files/0580d78fb6ed4ed06dd756e02ec401799a892b56..12f063a04fc50258b9f0ffd6575e30de82613903

---

From: MikkelFJ <[email-masked]>
Date: Mon, 27 Aug 2018 02:55:30 -0700

-: A variable-length integer indicating the number of contiguous packets
-  preceding the Largest Acknowledged that are being acknowledged.
+The range of acknowledged values in each ACK Block is inclusive of its largest
+value and is described in numerically decreasing order.  Each ACK Block includes

acknowledged values -> acknowledged packet numbers?

-- 
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/1706#pullrequestreview-149649367

---

From: MikkelFJ <[email-masked]>
Date: Mon, 27 Aug 2018 03:23:53 -0700

-Each Gap indicates a range of packets that are not being acknowledged.  The
-number of packets in the gap is one higher than the encoded value of the Gap
-Field.
+: Unmarked packets are those packets that are acknowledged, but for which the
+  ECN marking is either not present, ECT(1), or unknown.

@gloinul suggested generic ECT - how about a generic term such as negotiated ECT code point and have a TP section stating that ECT is always ECT(0) by default but may change in future versions? An agreed ECT mark could also be accepting both ECT(0) and ECT(1), but it is not important 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/1706#pullrequestreview-149658018

---

From: mirjak <[email-masked]>
Date: Mon, 27 Aug 2018 04:54:43 -0700

mirjak commented on this pull request.

> @@ -1875,46 +1874,20 @@ Congestion Experienced (CE) codepoint set by a network device that is
 experiencing congestion.
 
 On receiving a packet with an ECT or CE codepoint, an endpoint that can access
-the IP ECN codepoints increases the corresponding ECT(0), ECT(1), or CE count,
-and includes these counters in subsequent (see {{processing-and-ack}}) ACK_ECN
-frames (see {{frame-ack-ecn}}).
+the IP ECN codepoints records the markings for that packet.  This information is
+reported in ACK frames (see {{processing-and-ack}}).  Packets bearing ECT(1) are
+reported as though they were not marked.

I'm not sure if this is the best choice. I think it would be more future-proof to have the ACK block type (2) as meaning either ECT(0) or ECT(1) marked because that makes it possible to switch over to a mechanism that uses ECT(1) as default marking without changing the feedback (which is the current idea of these the codepoint - basically you mark all you packets either ECT(0) or all them ECT(1) but you don't use both in the same connection as ECN Nonce did). 

However, no providing feedback on one of the marking, also means testing is less easy. If you could use both of the codepoints at least for testing you can detect if there is a box overwriting the codepoint to either ECT(0) or ECT(1). However, I guess you can find another mechanism to get this kind of feedback if needed, e.g. tell the other endpoint what the default ECT is that you are planning to use in the TP and then have a separate feedback frame that tells you if the other unexpected ECT was observed.

The other option is of course to use a third bit for one of the cases, e.g. if the ACK block type is (2) you need to check the next bit that indicates you if this was a ECT(0) or ECT(1). Or you could use the third bit to indicate either a gap/loss or CE mark as both blocks should usually be rather small and both indicate congestion.

-- 
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/1706#pullrequestreview-149680023

---

From: Mike Bishop <[email-masked]>
Date: Mon, 27 Aug 2018 10:25:13 -0700

MikeBishop commented on this pull request.

> @@ -3281,7 +3254,13 @@ An ACK frame is shown below.
 +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
 |                       ACK Block Count (i)                   ...
 +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
-|                          ACK Blocks (*)                     ...
+|                          ACK Block 1 (i)                   ...

Consistency nit:  I believe the other `...`s line up with the edge of the graphic, as in the removed line.  You're left-shifting them here.

 ACK Blocks:
 
-: Contains one or more blocks of packet numbers which have been successfully
-  received, see {{ack-block-section}}.
+: An ACK block is encoded as a variable-length integer.  The encoding of the ACK

Here and above, it's not clear when you capitalize Block and when you don't.  I'd suggest capitalizing references to the the field itself, but the structure contained inside is a lower-case ACK block.  Always capitalizing would make sense, too.

> -|                    Additional ACK Block (i)                 ...
-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
-|                             Gap (i)                         ...
-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
-|                    Additional ACK Block (i)                 ...
-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
-                               ...
-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
-|                             Gap (i)                         ...
-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
-|                    Additional ACK Block (i)                 ...
-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
-~~~
-{: #ack-block-format title="ACK Block Section"}
+```
+ack_type_i = decode_varint() & 3

Just for completeness, maybe include the left-shift to get the size here 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/1706#pullrequestreview-149801906

---

From: Martin Thomson <[email-masked]>
Date: Mon, 27 Aug 2018 17:01:30 -0700

ac76592  Editorial feedback

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/1706/files/12f063a04fc50258b9f0ffd6575e30de82613903..ac7659251f6fd46c0f08683fc1e7eb9dcacd8a43

---

From: Martin Thomson <[email-masked]>
Date: Tue, 28 Aug 2018 00:06:23 +0000 (UTC)

@mirjak, this simplification is intentional.  Taking another bit is surprisingly expensive.  We're already down to a mere 16 values in a range before expanding the size of acknowledgments.  While 16 is enough that you can avoid expanding ACK sizes, 8 is poor.

Those testing can negotiate use of ECT(1), or negotiate the use of supplementary signals.  For instance, if you want to mark occasional packets with ECT(1), then do that and read "unmarked" as ECT(1).  If you want to ensure that you don't overcount that as a result of bleaching, then negotiate the use of a supplementary mechanism.

I liked @mikkelfj's suggestion here about negotiated default marking.

-- 
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/1706#issuecomment-416407974

---

From: Magnus Westerlund <[email-masked]>
Date: Tue, 28 Aug 2018 05:31:45 -0700

@martinthomson including a TP for the sender to indicate to the receiver what ECT codepoint will be used in marking is fine and would cater for the case when the sender only needs to implement the ECT(1) experiment and no additional receiver changes are needed. However, if the concern is that this is becoming to complicated, I think it is fine to leave the signaling to a future extension.

-- 
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/1706#issuecomment-416566127

---

From: ianswett <[email-masked]>
Date: Tue, 28 Aug 2018 06:34:42 -0700

ianswett commented on this pull request.

Looking good, I think you missed a few spots to update in recovery.

 ~~~
-   ProcessECN(ack):
-     // If the ECN-CE counter reported by the peer has increased,
-     // this could be a new congestion event.
-     if (ack.ce_counter > ecn_ce_counter):
-       ecn_ce_counter = ack.ce_counter
-       // Start a new congestion event if the last acknowledged
-       // packet is past the end of the previous recovery epoch.
+   ProcessECN(packet):

This is now simple enough it might be easier to inline it?

 ~~~
-   ProcessECN(ack):
-     // If the ECN-CE counter reported by the peer has increased,
-     // this could be a new congestion event.
-     if (ack.ce_counter > ecn_ce_counter):
-       ecn_ce_counter = ack.ce_counter
-       // Start a new congestion event if the last acknowledged
-       // packet is past the end of the previous recovery epoch.
+   ProcessECN(packet):

This is now simple enough I believe it'd be easier to read inlined and call CongestionEvent() in OnPacketAcked.

 ~~~
-   ProcessECN(ack):
-     // If the ECN-CE counter reported by the peer has increased,
-     // this could be a new congestion event.
-     if (ack.ce_counter > ecn_ce_counter):
-       ecn_ce_counter = ack.ce_counter
-       // Start a new congestion event if the last acknowledged
-       // packet is past the end of the previous recovery epoch.
+   ProcessECN(packet):

Also, please update the Recovery Period section from "Recovery is a period of time beginning with detection of a lost packet or an increase in the ECN-CE counter." to "Recovery is a period of time beginning with detection of a lost  packet or receipt of an ECN-CE marked packet."  Or something similar

 ~~~
-   ProcessECN(ack):
-     // If the ECN-CE counter reported by the peer has increased,
-     // this could be a new congestion event.
-     if (ack.ce_counter > ecn_ce_counter):
-       ecn_ce_counter = ack.ce_counter
-       // Start a new congestion event if the last acknowledged
-       // packet is past the end of the previous recovery epoch.
+   ProcessECN(packet):

And at the end of that paragraph "new losses or increases in the ECN-CE counter." -> " new losses or ECN-CE marks."

-The ACK Block Section consists of alternating Gap and ACK Block fields in
-descending packet number order.  A First Ack Block field is followed by a
-variable number of alternating Gap and Additional ACK Blocks.  The number of Gap
-and Additional ACK Block fields is determined by the ACK Block Count field.
+Each ACK Block in an ACK frame describes a contiguous range of packets, as well
+as the observed type of the packets in that range.  Each range of packets is

I think of packet types as Initial/Retry/etc.  How about ack state?  Or something else that doesn't collide with the existing packet type?

Also, "Each range of packets is attributed a type," overlaps with the first sentence.  How about "Each packet range's type is encoded into the two least..."

-- 
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/1706#pullrequestreview-150106809

---

From: Martin Thomson <[email-masked]>
Date: Tue, 28 Aug 2018 17:11:55 -0700

@martinthomson pushed 3 commits.

8ff41ed  Refactor recovery draft to remove ProcessECN
78cd18f  Try to clean up description of ACK Blocks
2ebf211  Gee, I'm glad no one checked these...

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/1706/files/ac7659251f6fd46c0f08683fc1e7eb9dcacd8a43..2ebf211e28aef7a7805042e6c4beec0774e71248