Subject: [quicwg/base-drafts] Add max_ack_delay transport parameter (#1781)

From: ianswett <[email-masked]>
Date: Thu, 20 Sep 2018 22:35:35 +0000 (UTC)

Fixes #981
You can view, comment on, or merge this pull request online at:

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

-- Commit Summary --

  * Add max_ack_delay transport parameter

-- File Changes --

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

-- Patch Links --

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

---

From: ianswett <[email-masked]>
Date: Thu, 20 Sep 2018 15:46:13 -0700

@ianswett pushed 1 commit.

b0cbe91  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/1781/files/3568a75a1e94a1c9227488ef60ca4c8f300bd848..b0cbe91ed75f99a14d1130df1cfc915b976824e9

---

From: Marten Seemann <[email-masked]>
Date: Thu, 20 Sep 2018 15:47:31 -0700

marten-seemann commented on this pull request.

> @@ -1674,6 +1675,12 @@ disable_migration (0x0009):
   address other than that used to perform the handshake.  This parameter is a
   zero-length value.
 
+max_ack_delay (0x000c):
+
+: An 8 bit unsigned integer value indicating the maximum amount of time in

duration?

-- 
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/1781#pullrequestreview-157494057

---

From: Nick Banks <[email-masked]>
Date: Thu, 20 Sep 2018 16:12:45 -0700

nibanks 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/1781#pullrequestreview-157499530

---

From: ianswett <[email-masked]>
Date: Thu, 20 Sep 2018 16:21:47 -0700

ianswett commented on this pull request.

+max_ack_delay (0x000c):
+

To clarify, "the maximum duration in milliseconds..."?  I think both are fine, but do you think that's substantially clearer?

-- 
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/1781#discussion_r219346773

---

From: ianswett <[email-masked]>
Date: Thu, 20 Sep 2018 17:09:24 -0700

7a0d476  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/1781/files/b0cbe91ed75f99a14d1130df1cfc915b976824e9..7a0d476421b1aefdcbf8d69e69cc7ac143907820

---

From: ianswett <[email-masked]>
Date: Thu, 20 Sep 2018 17:12:48 -0700

18cbe48  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/1781/files/7a0d476421b1aefdcbf8d69e69cc7ac143907820..18cbe487fca9dd095c35e12d7c0a9fb8169207ce

---

From: Martin Thomson <[email-masked]>
Date: Fri, 21 Sep 2018 16:38:12 -0700

martinthomson commented on this pull request.

> @@ -3461,9 +3468,14 @@ ACK frames in response to other packets.  Implementations MUST NOT send more
 than one packet containing only an ACK frame per received packet that contains
 frames other than ACK and PADDING frames.  Packets containing frames besides
 ACK and PADDING MUST be acknowledged immediately or when a delayed ack timer
-expires. The delayed ack timer MUST NOT delay an ACK for longer than an RTT or
-the alarm granularity.  This ensures an ACK frame is sent at least once per RTT
-if new packets needing acknowledgement were received.
+expires.
+
+The receiver's delayed ack timer MUST NOT delay an ACK for longer than an RTT
+or the value indicated by the `max_ack_delay` transport parameter.

you say that it might exceed `max_ack_delay` in the recovery PR, so which is it?

> @@ -3461,9 +3468,14 @@ ACK frames in response to other packets.  Implementations MUST NOT send more
 than one packet containing only an ACK frame per received packet that contains
 frames other than ACK and PADDING frames.  Packets containing frames besides
 ACK and PADDING MUST be acknowledged immediately or when a delayed ack timer
-expires. The delayed ack timer MUST NOT delay an ACK for longer than an RTT or
-the alarm granularity.  This ensures an ACK frame is sent at least once per RTT
-if new packets needing acknowledgement were received.
+expires.
+
+The receiver's delayed ack timer MUST NOT delay an ACK for longer than an RTT
+or the value indicated by the `max_ack_delay` transport parameter.
+This ensures an ACK frame is sent at least once per RTT if new packets
+needing acknowledgement were received and ensures the sender has a predictable
+limit on the ack delay for timer based retransmissions.  ACKs should be sent

I think we're gonna need caps on that should.

-- 
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/1781#pullrequestreview-157886983

---

From: ianswett <[email-masked]>
Date: Sat, 22 Sep 2018 16:55:26 -0700

787b030  Martin's comments

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/1781/files/18cbe487fca9dd095c35e12d7c0a9fb8169207ce..787b0306bc9c403b3062584c91bcc52d327e9b44

---

From: ianswett <[email-masked]>
Date: Sat, 22 Sep 2018 16:56:24 -0700

Thanks Martin, tweaked based on your comments.

-- 
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/1781#issuecomment-423781559

---

From: Christian Huitema <[email-masked]>
Date: Sun, 23 Sep 2018 15:18:16 -0700

huitema approved this pull request.

Looks good, follows what we agreed in NYC. Modulo mandatory bike shedding on the description of the parameter.

+max_ack_delay (0x000c):
+

How about, "the maximum ACK delay in milliseconds"?

-- 
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/1781#pullrequestreview-157953859

---

From: ianswett <[email-masked]>
Date: Sun, 23 Sep 2018 17:52:40 -0700

+max_ack_delay (0x000c):
+

I think it's helpful to clarify what ACK delay is here, so I'd like to avoid using the same words in the parameter when possible.

-- 
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/1781#discussion_r219718314

---

From: Christian Huitema <[email-masked]>
Date: Sun, 23 Sep 2018 18:42:11 -0700

huitema commented on this pull request.

+max_ack_delay (0x000c):
+

I agree that it would be helpful. The only reference to ACK Delay in the transport draft is in the description of the ACK frame. The ACK generation is described in the recovery draft, using a parameter kDelayedAckTimeout. Max ACK Delay is also defined in the recovery draft, but the section 4.1.1 appears obsolete. In light of that, the proper phrase is probably "maximum time that the peer will wait before acknowledging a packet" plus a reference to the appropriate section of the recovery draft. This PR should probably also update the recovery draft.

-- 
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/1781#discussion_r219720183

---

From: Rui Paulo <[email-masked]>
Date: Sun, 23 Sep 2018 23:20:15 -0700

This might be another color of the bikeshed but if the ack delay is in microseconds, shouldn't the max ack delay also be in microseconds?

-- 
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/1781#issuecomment-423884761

---

From: Martin Thomson <[email-masked]>
Date: Mon, 24 Sep 2018 15:37:43 -0700

@martinthomson pushed 1 commit.

2402202  Some cleanup

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/1781/files/787b0306bc9c403b3062584c91bcc52d327e9b44..240220233afd7127747bc3756ea9047c4302462e

---

From: Martin Thomson <[email-masked]>
Date: Mon, 24 Sep 2018 15:38:23 -0700

I don't think that we need microsecond precision here.  It just makes expressing the value harder.

-- 
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/1781#issuecomment-424148600

---

From: Martin Thomson <[email-masked]>
Date: Mon, 24 Sep 2018 15:38:29 -0700

Merged #1781 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/1781#event-1864351030