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

From: Kazu Yamamoto <[email-masked]>
Date: Tue, 21 Jul 2020 21:22:38 -0700

This should fix #3920.
You can view, comment on, or merge this pull request online at:

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

-- Commit Summary --

  * adding comment for max_ack_delay.
  * Using 0 for max_ack_delay in Initial and Handshake.

-- File Changes --

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

-- Patch Links --

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

---

From: Martin Thomson <[email-masked]>
Date: Tue, 21 Jul 2020 21:26:56 -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-452985336

---

From: Marten Seemann <[email-masked]>
Date: Tue, 21 Jul 2020 22:20:29 -0700

@marten-seemann 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-453001411

---

From: Kazuho Oku <[email-masked]>
Date: Tue, 21 Jul 2020 23:07:12 -0700

@kazuho 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.

These comments look fine, but I am not sure if I agree with the rest of the changes.

IIUC, persistent congestion is a path-wide event rather than a event specific to the PN space. I think we need to have max_ack_delay even if that might turn out to be a bit conservative when 0-RTT / 1-RTT packets are not involved.

Then, there is no point in passing `pn_space` to `InPersistentCongestion` or to the functions that invoke `InPersistentCongestion`.

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

---

From: Kazu Yamamoto <[email-masked]>
Date: Wed, 22 Jul 2020 04:40:32 -0700

@kazu-yamamoto commented on this pull request.

My purpose is to make pseudocode consistent with the spec. It seems to me that the spec should be changed if your observation is correct. I'm happy to drop the commit when the spec is changed.

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

---

From: Kazuho Oku <[email-masked]>
Date: Wed, 22 Jul 2020 06:24:20 -0700

I'd argue that the spec is clear about the intent, and the current state of this PR is going to introduce a contradiction from what the spec says.

Quoting from [section 4.1](https://quicwg.org/base-drafts/draft-ietf-quic-recovery.html#section-4.1-1), "congestion control and round-trip time (RTT) measurement are unified across packet number spaces." As can be seen, persistent congestion by definition is a path-wide event. Not an event specific to the PN space in which a loss has been newly observed.

Based on that, my understanding is that consistently applying max_ack_delay exchanged by the Transport Parameter is the intent of [the formula found in in section 7.8](https://quicwg.org/base-drafts/draft-ietf-quic-recovery.html#section-7.8-3), and that the definition that currently exists in the WG draft is correct.

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

---

From: Martin Thomson <[email-masked]>
Date: Wed, 22 Jul 2020 19:15:03 -0700

@martinthomson requested changes on this pull request.

> +     ack_delay = 0
+     if (pn_space == ApplicationData):
+       ack_delay = max_ack_delay

Based on discussions about PC elsewhere, I realized my error.  This needs to be reverted.  In fact, I would suggest that we initialize max_ack_delay to 25ms for that reason.

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

---

From: Kazu Yamamoto <[email-masked]>
Date: Tue, 28 Jul 2020 21:49:36 -0700

@kazu-yamamoto pushed 0 commits.

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

---

From: Kazu Yamamoto <[email-masked]>
Date: Tue, 28 Jul 2020 21:50:41 -0700

I agree with @kazuho. The spec is clear. I have dropped the second commit.

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

---

From: Jana Iyengar <[email-masked]>
Date: Fri, 31 Jul 2020 12:44:18 -0700

@janaiyengar commented on this pull request.

I would move this comment out and to the definition of max_ack_delay further up in this section.

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