Subject: Re: [quicwg/base-drafts] Fixing max_ack_delay (#3938)

From: Kazu Yamamoto <[email-masked]>
Date: Sun, 02 Aug 2020 17:24:35 -0700

@kazu-yamamoto pushed 1 commit.

5956eaa304742a74dd96c6315f9f55b133e23cd4  moving a comment from code to its definition.

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3938/files/23dd580e62d723461842986ebdd77ee26da23371..5956eaa304742a74dd96c6315f9f55b133e23cd4

---

From: Kazu Yamamoto <[email-masked]>
Date: Sun, 02 Aug 2020 17:25:12 -0700

@kazu-yamamoto commented on this pull request.

> @@ -1129,6 +1129,10 @@ follows:
    smoothed_rtt = kInitialRtt
    rttvar = kInitialRtt / 2
    min_rtt = 0
+   // max_ack_delay is initialized to 0 and
+   // updated when transport parameters are exchanged.
+   // If a peer does not specify max_ack_delay,
+   // it is set to 25ms.

It's also fine with me. And done in 5956eaa.

-- 
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/3938#discussion_r464142645

---

From: ianswett <[email-masked]>
Date: Mon, 03 Aug 2020 09:21:07 -0700

@ianswett approved this pull request.

I think this is good now, but would like @janaiyengar to confirm.

-- 
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/3938#pullrequestreview-460162847

---

From: Kazu Yamamoto <[email-masked]>
Date: Wed, 12 Aug 2020 19:18:32 -0700

@janaiyengar Gentle ping

-- 
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/3938#issuecomment-673208861

---

From: Kazuho Oku <[email-masked]>
Date: Wed, 12 Aug 2020 19:41:09 -0700

@kazuho commented on this pull request.

I am not sure if we want to adopt this PR (in current form).

> @@ -1094,7 +1094,9 @@ max_ack_delay:
 : The maximum amount of time by which the receiver intends to delay
   acknowledgments for packets in the Application Data packet number space. The
   actual ack_delay in a received ACK frame may be larger due to late timers,
-  reordering, or lost ACK frames.
+  reordering, or lost ACK frames. max_ack_delay is initialized to 0 and
+  updated when transport parameters are exchanged. If a peer does not
+  specify max_ack_delay, it is set to 25ms.

As @martinthomson points out, this is a deviation from the definition of max_ack_delay in the transport draft, that says that the value defaults to 25ms and is updated when receiving the transport parameters from the peer.

We could have different definitions, but IMO it is unnecessary in this case, based on our agreement that persistent congestion is a connection-wide event and that it can be applied during the handshake.

-- 
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/3938#pullrequestreview-466406959

---

From: ianswett <[email-masked]>
Date: Thu, 13 Aug 2020 06:58:10 -0700

@ianswett commented on this pull request.

Good point.  How about a reference to the section max_ack_delay is discussed in transport instead of this 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/3938#discussion_r469971375

---

From: Kazuho Oku <[email-masked]>
Date: Thu, 13 Aug 2020 07:59:00 -0700

@ianswett That SGTM.

-- 
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/3938#discussion_r470016230

---

From: Jana Iyengar <[email-masked]>
Date: Tue, 18 Aug 2020 12:51:45 -0700

@janaiyengar requested changes on this pull request.

Agreed that we can't take the proposed text. I've suggested some text.

(apologies about not getting to this sooner, I was trying to prioritize the design issues assuming that I'd have gotten to the editorial ones sooner than now.)

I'd suggest some text like this, to avoid repeating the details here.
```
The maximum amount of time by which the receiver intends to delay acknowledgments
for packets in the Application Data packet number space, as defined by the eponymous
transport parameter (Section 18.2 of {{QUIC-TRANSPORT}}). Note that the actual
ack_delay in a received ACK frame may be larger due to late timers, reordering, or lost
ACK frames.
```

-- 
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/3938#pullrequestreview-469772612

---

From: Kazu Yamamoto <[email-masked]>
Date: Wed, 19 Aug 2020 19:55:53 -0700

I think that I am still confused.

In recovery, `max_ack_delay` is used in three places:

- RTT calculation
- PTO calculation
- Persistent congestion

Sec 18.2 of transport says that `max_ack_delay` is default to 25ms and updated if a peer specifies the TP.

Sec 13.2.1 of transport says there is a special rule: "For Initial and Handshake packets, a max_ack_delay of 0 is used." According to the context, this rule is applied to both RTT calculation and PTO calculation.

In the pseudocode of recovery, `max_ack_delay` is set to 0 initially. But I think that 25ms should be used for persistent congestion.

The special rule is use for PTO calculation, which is good.

But RTT calculation just depends on the initial value of 0. Isn't it necessary to consider 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/3938#issuecomment-676892583

---

From: Kazuho Oku <[email-masked]>
Date: Wed, 19 Aug 2020 20:29:55 -0700

@kazu-yamamoto Nice catch regarding inconsistencies.

> In the pseudocode of recovery, `max_ack_delay` is set to 0 initially.

Maybe this is the only error that we have? I think it might be a good idea to
* remove this initialization from the pseudo code, and
* within the definition of `max_ack_delay` at [A.3 of recovery](https://quicwg.org/base-drafts/draft-ietf-quic-recovery.html#section-a.3), add a reference to the Transport Parameter definition 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/3938#issuecomment-676942748

---

From: Kazu Yamamoto <[email-masked]>
Date: Wed, 19 Aug 2020 22:24:40 -0700

> - remove this initialization from the pseudo code, and

Agreed.

> - within the definition of `max_ack_delay` at A.3 of recovery, add a reference to the Transport Parameter definition in the transport doc.

@janaiyengar suggested a good text. I'm planning to adopt it.

It seems to me that you assume that the calculation of smooth RTT in the pseudocode is correct. If it is correct, I think Sec 13.2.1 of transport should say that the special rule is only applied to RTO calculation, not to RTT calculation.

-- 
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/3938#issuecomment-677122630

---

From: Kazuho Oku <[email-masked]>
Date: Wed, 19 Aug 2020 22:30:17 -0700

> If it is correct, I think Sec 13.2.1 of transport should say that the special rule is only applied to RTO calculation, not to RTT calculation.

I think that the pseudo code gives you what the transport draft says. When processing an Initial or a Handshake packet, `ack_delay` being passed to the UpdateRtt function of the pseudo code is always set to zero. That effectively eliminates the side effect caused by a non-zero `max_ack_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/3938#issuecomment-677141111

---

From: Kazu Yamamoto <[email-masked]>
Date: Wed, 19 Aug 2020 22:42:18 -0700

OK. I understand. But honestly, it is difficult at least for me to understand the intention of the pseudocode. If `UpdateRTT` had a guard clause where `ack_delay` is set to 0 if the packet space is Initial or Handshake, it would much help. But this function does not take the packet space as argument, sigh.

-- 
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/3938#issuecomment-677174800

---

From: Kazu Yamamoto <[email-masked]>
Date: Wed, 19 Aug 2020 22:58:02 -0700

@kazu-yamamoto pushed 3 commits.

76f3262f9c6b3a7f8ef9e7d7aed8c2505486430c  adopting Jana's suggestion.
3d5626733b33b844e9e7d66a3e8bcc3ce187a5fb  removing the initialization code of max_ack_delay.
175a16cae6018ac4d9bdc308f4773b9d5b174171  adding a comment about ack_delay.

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3938/files/5956eaa304742a74dd96c6315f9f55b133e23cd4..175a16cae6018ac4d9bdc308f4773b9d5b174171

---

From: Kazu Yamamoto <[email-masked]>
Date: Wed, 19 Aug 2020 22:58:50 -0700

I updated this PR. Please review this again.

-- 
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/3938#issuecomment-677223313

---

From: Kazuho Oku <[email-masked]>
Date: Thu, 20 Aug 2020 16:24:59 -0700

LGTM modulo the grammatical point below.

> @@ -1092,9 +1092,10 @@ min_rtt:
 
 max_ack_delay:
 : The maximum amount of time by which the receiver intends to delay
-  acknowledgments for packets in the Application Data packet number space. The
-  actual ack_delay in a received ACK frame may be larger due to late timers,
-  reordering, or lost ACK frames.
+  acknowledgments for packets in the Application Data packet number
+  space, as defined by the eponymous transport parameter (Section 18.2
+  of {{QUIC-TRANSPORT}}). Note that the actual ack_delay in a received
+  ACK frame may be larger due to late timers, reordering, or lost

```suggestion
  ACK frame may be larger due to late timers, reordering, or loss
```

-- 
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/3938#pullrequestreview-472046561

---

From: Kazu Yamamoto <[email-masked]>
Date: Sun, 23 Aug 2020 17:21:53 -0700

544efe50534d4dea3ac11993846ff8ffd32992eb  fixing the typo pointed out by kazuho.

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3938/files/175a16cae6018ac4d9bdc308f4773b9d5b174171..544efe50534d4dea3ac11993846ff8ffd32992eb

---

From: Kazu Yamamoto <[email-masked]>
Date: Sun, 23 Aug 2020 17:22:03 -0700

max_ack_delay:

Thanks. Fixed.

-- 
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/3938#discussion_r475287489

---

From: Martin Thomson <[email-masked]>
Date: Sun, 23 Aug 2020 21:07:19 -0700

@martinthomson 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/3938#pullrequestreview-473107005

---

From: Jana Iyengar <[email-masked]>
Date: Mon, 24 Aug 2020 19:08:13 -0700

@janaiyengar approved this pull request.

Thank you, @kazu-yamamoto !

-- 
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/3938#pullrequestreview-474043758

---

From: Jana Iyengar <[email-masked]>
Date: Mon, 24 Aug 2020 19:08:42 -0700

I'd like to merge this, @kazu-yamamoto ; can you fix the conflicts?

-- 
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/3938#issuecomment-679459586

---

From: Martin Thomson <[email-masked]>
Date: Mon, 24 Aug 2020 19:10:02 -0700

@martinthomson pushed 1 commit.

215b1bb3d5c6b3b161c85977342bbf6721a129cf  Merge branch 'master' into fixi-max-ack-delay

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3938/files/544efe50534d4dea3ac11993846ff8ffd32992eb..215b1bb3d5c6b3b161c85977342bbf6721a129cf

---

From: Martin Thomson <[email-masked]>
Date: Mon, 24 Aug 2020 19:10:02 -0700

Resolved :)

-- 
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/3938#issuecomment-679459972

---

From: Kazu Yamamoto <[email-masked]>
Date: Mon, 24 Aug 2020 19:48:04 -0700

@martinthomson Thanks. I think that it's @janaiyengar's turn 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/3938#issuecomment-679470205

---

From: Jana Iyengar <[email-masked]>
Date: Wed, 26 Aug 2020 12:20:40 -0700

@janaiyengar commented on this pull request.

> @@ -1264,6 +1264,7 @@ UpdateRtt(ack_delay):
   // min_rtt ignores acknowledgment delay.
   min_rtt = min(min_rtt, latest_rtt)
   // Limit ack_delay by max_ack_delay
+  // Note that ack_delay is 0 for Initial and Handshake.

```suggestion
  // Note that ack_delay is 0 for acknowledgements of Initial and Handshake 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/3938#pullrequestreview-475789260

---

From: Jana Iyengar <[email-masked]>
Date: Wed, 26 Aug 2020 12:21:32 -0700

@janaiyengar pushed 1 commit.

cca47c5970e8aea1c51aa55036f45f59d2e8630e  Update draft-ietf-quic-recovery.md

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3938/files/215b1bb3d5c6b3b161c85977342bbf6721a129cf..cca47c5970e8aea1c51aa55036f45f59d2e8630e

---

From: Jana Iyengar <[email-masked]>
Date: Wed, 26 Aug 2020 12:29:13 -0700

Merged #3938 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/3938#event-3696302182