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

From: Jana Iyengar <[email-masked]>
Date: Tue, 06 Aug 2019 14:53:44 -0700

janaiyengar commented on this pull request.

Looks good -- a few nits to address

> @@ -376,26 +298,25 @@ min_rtt is set to the latest_rtt on the first sample in a connection, and to the
 lesser of min_rtt and latest_rtt on subsequent samples.
 
 An endpoint uses only locally observed times in computing the min_rtt and does
-not adjust for host delays reported by the peer ({{host-delay}}).  Doing so
-allows the endpoint to set a lower bound for the smoothed_rtt based entirely on
-what it observes (see {{smoothed-rtt}}), and limits potential underestimation
-due to erroneously-reported delays by the peer.
+not adjust for host delays reported by the peer.  Doing so allows the endpoint

why remove this ref? Can you add the right ref in the transport doc?

> @@ -2947,6 +2997,25 @@ to unnecessarily retransmit some data.  Standard QUIC algorithms
 acknowledged.  Therefore, the receiver SHOULD repeatedly acknowledge newly
 received packets in preference to packets received in the past.
 
+### 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 Section 19.3). This allows the receiver

change "Section 19.1" to a real reference

> @@ -2947,6 +2997,25 @@ to unnecessarily retransmit some data.  Standard QUIC algorithms
 acknowledged.  Therefore, the receiver SHOULD repeatedly acknowledge newly
 received packets in preference to packets received in the past.
 
+### 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 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:

change to a real reference

-- 
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-271634173

---

From: Martin Thomson <[email-masked]>
Date: Tue, 06 Aug 2019 19:25:10 -0700

martinthomson commented on this pull request.

> @@ -2880,6 +2880,28 @@ packet.
 expectations about what implementations do with packets that have errors after
 valid frames? -->
 
+
+## Generating Acknowledgements {#generating-acks}
+
+An acknowledgement SHOULD be sent immediately upon receipt of a second
+ack-eliciting packet. QUIC recovery algorithms do not assume the peer sends
+an ACK immediately when receiving a second ack-eliciting packet.

This is an odd lead-in.  Do you want to start by saying a few more general things?

1. endpoints send acknowledgements for all packets they receive and process
2. loss recovery doesn't depend on immediate acknowledgment, but performance can be improved by acknowledging immediately in some cases
3. packets that aren't ack-eliciting are only acknowledged when sending an ACK for other reasons
4. there are recommendations to send immediate acknowledgments in a dazzling array of cases
   a. when receiving the second ack-eliciting packet
   b. when it appears as though there is loss or reordering, though not for every packet
   c. when new CRYPTO data is available to send (aka the incoming CRYPTO flight is outstanding)
   d. ...
5. When sending a packet for any reason, it's generally a good idea to send an ACK if one has not been sent recently.

> +Below is one recommended approach for determining what packets to include in an
+ACK frame.
+
+### 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 may cause spurious retransmits, but the sender will

```suggestion
loss recovery algorithm could cause spurious retransmits, but the sender will
```

```suggestion
Ack Delay field of an ACK frame (see {{frame-ack}}). This allows the receiver
```

I think that you mean "which is important for getting a better estimate of the path RTT when acknowledgments are delayed"

```suggestion
An endpoint MUST NOT include delays that is does not control when populating
```

I think that we can use MUST NOT here.  Also, I think that outside of endpoint control is the key.

```suggestion
packets.  An endpoint commits to a maximum delay using the max_ack_delay transport
```

```suggestion
parameter; see {{transport-parameter-definitions}}.  max_ack_delay implies an explicit contract:
```

+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.

Do you really get spurious retransmissions if your estimate of RTT is too high?  I thought that the effect would just come down to poor performance, maybe over-commitment of memory, and that sort of thing.

-- 
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-271699720

---

From: Jana Iyengar <[email-masked]>
Date: Tue, 27 Aug 2019 15:11:45 -0700

@janaiyengar pushed 1 commit.

19f882473caf114683c31c8f1e5f46fa28ce93c5  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/34f06f467b1e4aee411948339734d339846da4c9..19f882473caf114683c31c8f1e5f46fa28ce93c5

---

From: Jana Iyengar <[email-masked]>
Date: Tue, 27 Aug 2019 15:11:58 -0700

c11a209b3e632c74927806abaa5c86b833a0d2be  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/19f882473caf114683c31c8f1e5f46fa28ce93c5..c11a209b3e632c74927806abaa5c86b833a0d2be

---

From: Jana Iyengar <[email-masked]>
Date: Tue, 27 Aug 2019 15:12:39 -0700

9f8b0c28f217fc28aa953056be809ee36083b11b  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/c11a209b3e632c74927806abaa5c86b833a0d2be..9f8b0c28f217fc28aa953056be809ee36083b11b

---

From: Jana Iyengar <[email-masked]>
Date: Tue, 27 Aug 2019 15:13:22 -0700

7d4c4cc90b7f402563a72ee67ddf69c2512f310a  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/9f8b0c28f217fc28aa953056be809ee36083b11b..7d4c4cc90b7f402563a72ee67ddf69c2512f310a

---

From: Jana Iyengar <[email-masked]>
Date: Tue, 27 Aug 2019 15:14:20 -0700

8174d8c940af257f7b3f38ddf7c64da27e3d9d9e  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/7d4c4cc90b7f402563a72ee67ddf69c2512f310a..8174d8c940af257f7b3f38ddf7c64da27e3d9d9e