Subject: [quicwg/base-drafts] use max_ack_delay = 0 before receving the TPs (#2646)

From: Marten Seemann <[email-masked]>
Date: Tue, 23 Apr 2019 19:14:03 -0700

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

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

-- Commit Summary --

  * use max_ack_delay = 0 before receving the TPs

-- File Changes --

    M draft-ietf-quic-recovery.md (14)

-- Patch Links --

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

---

From: ianswett <[email-masked]>
Date: Tue, 23 Apr 2019 19:45:25 -0700

ianswett commented on this pull request.

I think this is fine, but for it to make sense, max_ack_delay should also default to 0 in the transport doc.

-- 
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/2646#pullrequestreview-229878327

---

From: Kazuho Oku <[email-masked]>
Date: Tue, 23 Apr 2019 20:04:25 -0700

kazuho commented on this pull request.

> @@ -1028,9 +1029,9 @@ min_rtt:
 
 max_ack_delay:
 : The maximum amount of time by which the receiver intends to delay
-  acknowledgments, in milliseconds.  The actual ack_delay in a
-  received ACK frame may be larger due to late timers, reordering,
-  or lost ACKs.
+  acknowledgments. The value is initialized to 0, and updated when the peer's
+  transport parameters are received. The actual ACK Delay in a received ACK
+  frame may be larger due to late timers, reordering, or lost ACKs.

How about clarifying tha the value is zero for "crypto packets".

Some reasons:
* endpoints are expected to send ACKs immediately for crypto packets, even after TP.max_ack_delay is known.
* Retransmissions of 0-RTT packets should respect max_ack_delay obtained in the previous connection.

-- 
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/2646#pullrequestreview-229881550

---

From: Martin Thomson <[email-masked]>
Date: Tue, 23 Apr 2019 20:22:27 -0700

Let's do this right and set the value to 0 initially in the transport doc.  The "crypto packet" hack is cute, but it's another special case, where this doesn't need a special case, just an initial value.

-- 
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/2646#issuecomment-486052265

---

From: Marten Seemann <[email-masked]>
Date: Tue, 23 Apr 2019 20:32:07 -0700

I think this PR is orthogonal to the question what the default value for this TP is. We use default values to allow peers to skip sending certain transport parameters.
This PR is about initializing the max_ack_delay variable in the recovery code before we even received the TPs.

-- 
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/2646#issuecomment-486053816

---

From: Kazuho Oku <[email-masked]>
Date: Tue, 23 Apr 2019 22:08:53 -0700

@martinthomson 
> Let's do this right and set the value to 0 initially in the transport doc. The "crypto packet" hack is cute, but it's another special case, where this doesn't need a special case, just an initial v…

I think that using max-ack-delay 0 for crypto packets (to be precise, Initial and Hanshake packets)  is _not_ a hack. It's preferable.

The problem of respecting TP.max_ack_delay for Initial and Handshake packets, once receiving TP from the peer, is that loss recovery would kick in `TP.max_ack_delay` seconds later than they can (note: peer is required to send ACK ASAP).

-- 
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/2646#issuecomment-486069860

---

From: Martin Thomson <[email-masked]>
Date: Tue, 23 Apr 2019 22:34:12 -0700

I see what you mean.  I'm still concerned about the special case-y nature of this.  Can we segregate by packet number space instead?  Is there any reason not to just say that this only applies to Initial and Handshake packets?  By my rough reckoning, these should all be acknowledged immediately.  Having the transport parameters only apply to 1-RTT (and maybe 0-RTT, which I haven't thought through) is probably right.

-- 
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/2646#issuecomment-486074471

---

From: MikkelFJ <[email-masked]>
Date: Tue, 23 Apr 2019 23:13:43 -0700

mikkelfj commented on this pull request.

 max_ack_delay:

I think this text needs to clarify that the value is updated to the default 25ms if TPs are received with the max_ack_delay setting.

-- 
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/2646#discussion_r277967109

---

From: Kazuho Oku <[email-masked]>
Date: Tue, 23 Apr 2019 23:16:37 -0700

>  Is there any reason not to just say that this only applies to Initial and Handshake packets?

I think that just saying that is fine.

-- 
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/2646#issuecomment-486083441

---

From: Marten Seemann <[email-masked]>
Date: Tue, 23 Apr 2019 23:19:00 -0700

I agree. I'll update my PR.

-- 
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/2646#issuecomment-486083947

---

From: ianswett <[email-masked]>
Date: Thu, 25 Apr 2019 06:45:42 -0700

It seems like this doesn't reflect the suggestion of making it 0ms for Initial and Handshake PN spaces, rather than discussing what to do before a transport param was received.  Were you going to update the PR, or keep it as is?

> @@ -309,7 +309,8 @@ parameter, see Section 18.1 of {{QUIC-TRANSPORT}}.  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.
+the peer.  Before receiving the transport parameters, endpoints use a value of 0

This discusses transport param defaults, not Initial and Handshake packets.  Did you intend to keep this?

-- 
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/2646#pullrequestreview-230655632

---

From: Marten Seemann <[email-masked]>
Date: Sat, 27 Apr 2019 19:13:54 -0700

@marten-seemann pushed 1 commit.

62d0a5519deafafa31d8c2a33cdb474cab861248  don't use max_ack_delay for Initial and Handshake packets

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/2646/files/470eed0ab5542c82e9f655dd12545efc082b2998..62d0a5519deafafa31d8c2a33cdb474cab861248

---

From: Marten Seemann <[email-masked]>
Date: Sat, 27 Apr 2019 19:14:11 -0700

I updated the PR such that max_ack_delay is only used for the 1-RTT packet number space.

-- 
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/2646#issuecomment-487334592

---

From: MikkelFJ <[email-masked]>
Date: Sat, 27 Apr 2019 23:20:40 -0700

Note that the ack frames `ack.ack_delay` is also not defined before TP's are ready:

>The value of the ACK Delay field is scaled by multiplying the encoded value by 2 to the power of the value of the ack_delay_exponent transport parameter set by the sender of the ACK frame.

-- 
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/2646#issuecomment-487348616

---

From: Marten Seemann <[email-masked]>
Date: Sun, 28 Apr 2019 00:09:22 -0700

@mikkelfj That is not correct, see https://github.com/quicwg/base-drafts/issues/2638#issuecomment-487351648.

-- 
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/2646#issuecomment-487351736

---

From: MikkelFJ <[email-masked]>
Date: Sun, 28 Apr 2019 01:43:43 -0700

OK, but given the latest commit `ack_delay = min(ack_delay, max_ack_delay)`, `max_ack_delay = 0` for Initial, Handshake, does it even make sense to define the exponent for that value rather the specifying ack.ack_delay must be zero for 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/2646#issuecomment-487358605

---

From: Marten Seemann <[email-masked]>
Date: Sun, 28 Apr 2019 19:26:20 -0700

3779c9c70db02edadd3aeac1852f7f402b76d19a  don't use a default ack_delay_exponent for Initial and Handshake packets

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/2646/files/62d0a5519deafafa31d8c2a33cdb474cab861248..3779c9c70db02edadd3aeac1852f7f402b76d19a

---

From: Marten Seemann <[email-masked]>
Date: Sun, 28 Apr 2019 19:29:23 -0700

@mikkelfj This is true, there's no sense in defining an ack_delay_exponent for Initial and Handshake packets if we're going to ignore the ACK delay anyway. Should be safe to recommend to just send a 0 for these packet number spaces.
I updated the PR accordingly. Since this now also touches the transport document, I guess we'll have to apply the late-stage consensus process to get this merged.

-- 
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/2646#issuecomment-487437831

---

From: ianswett <[email-masked]>
Date: Mon, 29 Apr 2019 20:16:30 +0000 (UTC)

ianswett approved this pull request.

One small suggestion, but LG.

 max_ack_delay:

Instead of crypto packets, I'd suggest packets in the ApplicationData packet number space?

> @@ -1111,7 +1111,10 @@ OnAckReceived(ack, pn_space):
       IncludesAckEliciting(newly_acked_packets))
     latest_rtt =
       now - sent_packets[pn_space][ack.largest_acked].time_sent
-    UpdateRtt(ack.ack_delay)
+    ack_delay = 0
+    if pn_space == 1-RTT:

```suggestion
    if pn_space == ApplicationData:
```

-- 
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/2646#pullrequestreview-231878002

---

From: Marten Seemann <[email-masked]>
Date: Mon, 29 Apr 2019 17:24:23 -0700

680e20fe6c4f1f8b26f59025d96b72443b00e968  it's called ApplicationData packet number space, not 1-RTT

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/2646/files/3779c9c70db02edadd3aeac1852f7f402b76d19a..680e20fe6c4f1f8b26f59025d96b72443b00e968

---

From: Marten Seemann <[email-masked]>
Date: Mon, 29 Apr 2019 17:27:58 -0700

b466b28f055fea0a7409596771a4db3729391a07  max_ack_delay applies to ApplicationData pn space, not on 1-RTT packets

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/2646/files/680e20fe6c4f1f8b26f59025d96b72443b00e968..b466b28f055fea0a7409596771a4db3729391a07

---

From: Marten Seemann <[email-masked]>
Date: Mon, 29 Apr 2019 17:28:09 -0700

marten-seemann commented on this pull request.

 max_ack_delay:

@ianswett Done.

-- 
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/2646#discussion_r279585598

---

From: ianswett <[email-masked]>
Date: Mon, 29 Apr 2019 17:48:13 -0700

Still LGTM.

-- 
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/2646#pullrequestreview-231963288