Subject: Re: [quicwg/base-drafts] Move Generating Acknowledgements to Transport (#2916)

From: Jana Iyengar <[email-masked]>
Date: Tue, 10 Sep 2019 18:18:23 -0700

janaiyengar commented on this pull request.

> +An endpoint measures the delays intentionally introduced between when an
+ACK-eliciting packet is received and the corresponding acknowledgment is sent.
+The endpoint encodes this delay for the largest acknowledged packet in the
+Ack Delay field of an ACK frame (see Section 19.3). This allows the receiver
+of the ACK to adjust for any intentional delays, which is important for
+delayed acknowledgements, when estimating the path RTT. A packet might be
+held in the OS kernel or elsewhere on the host before being processed.
+An endpoint SHOULD NOT include these unintentional delays when populating
+the Ack Delay field in an ACK frame.
+
+An endpoint MUST NOT excessively delay acknowledgements of ack-eliciting
+packets.  The maximum ack delay is communicated in the max_ack_delay transport
+parameter; see Section 18.1.  max_ack_delay implies an explicit contract:
+an endpoint promises to never delay acknowledgments of an ack-eliciting packet
+by more than the indicated value. If it does, any excess accrues to the RTT
+estimate and could result in spurious retransmissions from the peer.

I think this should have been "delayed retransmissions".

-- 
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/2916#discussion_r323024778

---

From: Jana Iyengar <[email-masked]>
Date: Tue, 10 Sep 2019 18:34:31 -0700

@janaiyengar pushed 1 commit.

b2b2c22c32e6624cbd8a5b301d4a398953c573fd  Addresses comments and some text rework

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/2916/files/8174d8c940af257f7b3f38ddf7c64da27e3d9d9e..b2b2c22c32e6624cbd8a5b301d4a398953c573fd

---

From: Jana Iyengar <[email-masked]>
Date: Tue, 10 Sep 2019 18:46:02 -0700

86992963effb71a0bb6eeca65f2a1a3d70408c42  re-adding (with some rework) text that was dropped inadvertently

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/2916/files/b2b2c22c32e6624cbd8a5b301d4a398953c573fd..86992963effb71a0bb6eeca65f2a1a3d70408c42

---

From: Jana Iyengar <[email-masked]>
Date: Tue, 10 Sep 2019 19:03:14 -0700

d60f633a45413a6234a192703a7a9b7d40cf8ec1  rebase cleanup

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/2916/files/55f1ad92f03dfb96d7c07e6e8550faf4f6285399..d60f633a45413a6234a192703a7a9b7d40cf8ec1

---

From: Jana Iyengar <[email-masked]>
Date: Tue, 10 Sep 2019 19:03:44 -0700

Ok, I've addressed comments and rebased, 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/2916#issuecomment-530187964

---

From: Jana Iyengar <[email-masked]>
Date: Tue, 10 Sep 2019 19:04:09 -0700

@martinthomson, 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/2916#issuecomment-530188067

---

From: Martin Thomson <[email-masked]>
Date: Tue, 10 Sep 2019 19:42:12 -0700

martinthomson approved this pull request.

>  ### Sending ACK Frames
 
-An endpoint sends ACK frames to acknowledge packets it has received and
-processed.
+An ACK frame SHOULD be sent immediately upon receipt of a second ack-eliciting
+packet. Loss detection does not assume that the peer sends an ACK immediately on
+receiving a second ack-eliciting packet.

These are both good and useful statements, but they sort of stand awkwardly next to each other as if they are strangers.

> An endpoint SHOULD immediately send an ACK frame when two ack-eliciting packets have been received without being acknowledged.  Though the algorithms in {{QUIC-RECOVERY}} do not depend on receivers following this guidance, frequent feedback improves loss and congestion response.

I tried to say something about the fact that this also provides some assurances about acknowledgement behaviour that senders could use when innovating with new algorithms, but it ended up being quite hard to do that without distracting too much from the core purpose of the section.  Also, the SHOULD got in the way: there is no real assurance, there we have to rely on the fact that this is a good recommendation that most deployments will likely respect.

> @@ -3020,9 +3059,34 @@ This ensures an acknowledgment is sent at least once per RTT when packets
 needing acknowledgement are received.  The sender can use the receiver's
 `max_ack_delay` value in determining timeouts for timer-based retransmission.
 
-Strategies and implications of the frequency of generating acknowledgments are
-discussed in more detail in {{QUIC-RECOVERY}}.
+### Managing ACK Ranges
+
+When an ACK frame is sent, one or more ranges of acknowledged packets are
+included.  Including older packets reduces the chance of spurious retransmits
+caused by losing previously sent ACK frames, at the cost of larger ACK frames.
+
+ACK frames SHOULD always acknowledge the most recently received packets, and the
+more out-of-order the packets are, the more important it is to send an updated
+ACK frame quickly, to prevent the peer from declaring a packet as lost and
+spuriously retransmitting the frames it contains.
+
+Below is one recommended approach for determining what packets to include in an

"Below" is a little imprecise.  Is this the next section, or next several sections?

```suggestion
{{ack-tracking}} and {{ack-limiting}} describe an exemplary approach for determining what packets to acknowledge in each
```

+### Receiver Tracking of ACK Frames

```suggestion
### Receiver Tracking of ACK Frames {#ack-tracking}
```

+### Receiver Tracking of ACK Frames
+
+When a packet containing an ACK frame is sent, the largest acknowledged in that
+frame may be saved.  When a packet containing an ACK frame is acknowledged, the
+receiver can stop acknowledging packets less than or equal to the largest
+acknowledged in the sent ACK frame.
+
+In cases without ACK frame loss, this algorithm allows for a minimum of 1 RTT
+of reordering. In cases with ACK frame loss and reordering, this approach does
+not guarantee that every acknowledgement is seen by the sender before it is no
+longer included in the ACK frame. Packets could be received out of order and
+all subsequent ACK frames containing them could be lost. In this case, the
+loss recovery algorithm could cause spurious retransmits, but the sender will
+continue making forward progress.
 
 ### Limiting ACK Ranges

```suggestion
### Limiting ACK Ranges {#ack-limiting}
```

> +### Measuring and Reporting Host Delay {#host-delay}
+
+An endpoint measures the delays intentionally introduced between when an
+ACK-eliciting packet is received and the corresponding acknowledgment is sent.
+The endpoint encodes this delay for the largest acknowledged packet in the Ack
+Delay field of an ACK frame (see {{frame-ack}}). This allows the receiver of the
+ACK to adjust for any intentional delays, which is important for getting a
+better estimate of the path RTT when acknowledgments are delayed. A packet might
+be held in the OS kernel or elsewhere on the host before being processed.  An
+endpoint MUST NOT include delays that is does not control when populating the
+Ack Delay field in an ACK frame.
+
+An endpoint MUST NOT excessively delay acknowledgements of ack-eliciting
+packets.  An endpoint commits to a maximum delay using the max_ack_delay
+transport parameter; see {{transport-parameter-definitions}}.  max_ack_delay
+implies an explicit contract: an endpoint promises to never delay

implies explicit is odd:

```suggestion
declares an explicit contract: an endpoint promises to never delay
```

-- 
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/2916#pullrequestreview-286527818

---

From: ianswett <[email-masked]>
Date: Wed, 11 Sep 2019 08:00:13 -0700

@ianswett pushed 1 commit.

17196dc451c9479302e53b33d69a821fd0af3372  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/2916/files/d60f633a45413a6234a192703a7a9b7d40cf8ec1..17196dc451c9479302e53b33d69a821fd0af3372

---

From: ianswett <[email-masked]>
Date: Wed, 11 Sep 2019 08:00:35 -0700

02bbcdf106e9532e3b2cd52ab70591b40e4f23e1  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/2916/files/17196dc451c9479302e53b33d69a821fd0af3372..02bbcdf106e9532e3b2cd52ab70591b40e4f23e1

---

From: ianswett <[email-masked]>
Date: Wed, 11 Sep 2019 08:00:48 -0700

35a607298e7fc8bb2be98a5eaecb5a2773aeb027  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/2916/files/02bbcdf106e9532e3b2cd52ab70591b40e4f23e1..35a607298e7fc8bb2be98a5eaecb5a2773aeb027

---

From: ianswett <[email-masked]>
Date: Wed, 11 Sep 2019 08:01:05 -0700

ac3e43754ac89bf9eb9441c8623508091e100211  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/2916/files/35a607298e7fc8bb2be98a5eaecb5a2773aeb027..ac3e43754ac89bf9eb9441c8623508091e100211

---

From: ianswett <[email-masked]>
Date: Wed, 11 Sep 2019 08:02:21 -0700

6910bcea398723851d7ace7b8d4ff7e2ad9c88a5  Reflow a line

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/2916/files/ac3e43754ac89bf9eb9441c8623508091e100211..6910bcea398723851d7ace7b8d4ff7e2ad9c88a5

---

From: Martin Thomson <[email-masked]>
Date: Wed, 11 Sep 2019 17:41:50 +0000 (UTC)

martinthomson commented on this pull request.

@ianswett, not planning to do anything about this?  I'll merge it now if you think we should just leave this alone.

-- 
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/2916#discussion_r323372108

---

From: ianswett <[email-masked]>
Date: Wed, 11 Sep 2019 11:07:48 -0700

ianswett commented on this pull request.

I want to do another larger pass over this text, but I'd like to keep it as is for now.

-- 
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/2916#discussion_r323384037

---

From: Jana Iyengar <[email-masked]>
Date: Wed, 11 Sep 2019 12:09:38 -0700

I'm trying out something -- give me a few minutes

-- 
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/2916#discussion_r323411496

---

From: Jana Iyengar <[email-masked]>
Date: Wed, 11 Sep 2019 12:49:15 -0700

5a8af76fb64d5ccc9c16516c409390ccd59f8192  moving text around a bit and mt's comment

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/2916/files/6910bcea398723851d7ace7b8d4ff7e2ad9c88a5..5a8af76fb64d5ccc9c16516c409390ccd59f8192

---

From: Jana Iyengar <[email-masked]>
Date: Wed, 11 Sep 2019 12:50:14 -0700

@ianswett @martinthomson PTAL

Ok, a few changes, look at the last commit. I've moved some text around and I think I've captured the spirit of this more clearly.

-- 
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/2916#pullrequestreview-287032284

---

From: Nick Banks <[email-masked]>
Date: Wed, 11 Sep 2019 12:58:18 -0700

nibanks commented on this pull request.

-Packets containing only ACK frames are not congestion controlled, so there are
-limits on how frequently they can be sent.  An endpoint MUST NOT send more than
-one ACK-frame-only packet in response to receiving an ACK-eliciting packet
-(one containing frames other than ACK and/or PADDING).  An endpoint MUST NOT
-send a packet containing only an ACK frame in response to a non-ACK-eliciting
-packet (one containing only ACK and/or PADDING frames), even if there are
-packet gaps which precede the received packet. Limiting ACK frames avoids an
-infinite feedback loop of acknowledgements, which could prevent the connection
-from ever becoming idle. However, the endpoint acknowledges non-ACK-eliciting
-packets when it sends an ACK frame.
+A receiver's delayed acknowledgment timer SHOULD NOT exceed the current RTT
+estimate or the value it indicates in the `max_ack_delay` transport parameter.
+This ensures an acknowledgment is sent at least once per RTT when packets
+needing acknowledgement are received.  The sender can use the receiver's
+`max_ack_delay` value in determining timeouts for timer-based retransmission.

Does this text `SHOULD NOT exceed the current RTT estimate` mean the peer could/should use a min of the max_ack_delay and the current RTT when computing probe timeouts?
```
PTO = smoothed_rtt + max(4*rttvar, kGranularity) + min(max_ack_delay, smoothed_rtt)
```

-- 
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/2916#pullrequestreview-287036888

---

From: ianswett <[email-masked]>
Date: Wed, 11 Sep 2019 13:20:11 -0700

Yes, probably.  This text is pre-existing, and maybe with max_ack_delay the 1 ack per RTT recommendation should be removed, but that's not an editorial change.

-- 
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/2916#discussion_r323439994

---

From: ianswett <[email-masked]>
Date: Wed, 11 Sep 2019 13:21:52 -0700

Thanks for the update Jana, LGTM.

I think we should open an issue for Nick's question.  I wonder if we should remove the text about sending an ACK per RTT entirely?  ie: what if the RTT is 1us?

-- 
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/2916#pullrequestreview-287049551

---

From: Jana Iyengar <[email-masked]>
Date: Wed, 11 Sep 2019 14:47:37 -0700

@ianswett : I think the ACK per RTT is sensible.... if the RTT is 1us, you still need to send an ACK in that timeframe if the sender is to send more packets. But yes, let's discuss that in a separate issue.

-- 
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/2916#issuecomment-530579562

---

From: Jana Iyengar <[email-masked]>
Date: Wed, 11 Sep 2019 14:47:45 -0700

Merged #2916 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/2916#event-2627262516