Subject: [quicwg/base-drafts] MUST pace or limit bursts to 10 packets (#3106)

From: ianswett <[email-masked]>
Date: Wed, 16 Oct 2019 17:02:08 -0700

Fixes part or all of #3094
You can view, comment on, or merge this pull request online at:

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

-- Commit Summary --

  * MUST pace or limit bursts to 10 packets

-- File Changes --

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

-- Patch Links --

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

---

From: ianswett <[email-masked]>
Date: Wed, 16 Oct 2019 21:46:57 -0700

@ianswett pushed 2 commits.

321ec027758ad0149e2f4de07ca251cf739ab67f  Alternative > alternate (#3105)
dc891d82d0b1bdd5ea8351a4c0d55e326fbf18d9  Merge branch 'master' into ianswett-limit-bursts

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3106/files/a7ad371908364e16474ec746a33aea4c30afc3d9..dc891d82d0b1bdd5ea8351a4c0d55e326fbf18d9

---

From: Nick Banks <[email-masked]>
Date: Thu, 17 Oct 2019 08:10:28 -0700

nibanks approved this pull request.

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/3106#pullrequestreview-303333911

---

From: ianswett <[email-masked]>
Date: Thu, 17 Oct 2019 09:43:05 -0700

@ianswett pushed 1 commit.

8aa3f261bac737b25867c964a8eb941f362be36a  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/3106/files/dc891d82d0b1bdd5ea8351a4c0d55e326fbf18d9..8aa3f261bac737b25867c964a8eb941f362be36a

---

From: Jana Iyengar <[email-masked]>
Date: Thu, 17 Oct 2019 10:13:22 -0700

@ianswett : I realized after we did this PR yesterday that this does not fix #3094. I It reduces burstiness in general,  which is still a good thing, but it doesn't change slow start increase. I'll send a PR or ask Vidhi if she wants to. 

-- 
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/3106#issuecomment-543273653

---

From: Jana Iyengar <[email-masked]>
Date: Thu, 17 Oct 2019 14:57:45 -0700

janaiyengar approved this pull request.

Let's get this in -- it addresses some burstiness anyway.

-- 
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/3106#pullrequestreview-303570582

---

From: Jana Iyengar <[email-masked]>
Date: Thu, 17 Oct 2019 14:57:58 -0700

Merged #3106 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/3106#event-2722890075

---

From: MikkelFJ <[email-masked]>
Date: Thu, 17 Oct 2019 21:56:16 -0700

mikkelfj commented on this pull request.

> @@ -753,6 +753,12 @@ delivery of ACK frames is important for efficient loss recovery. Packets
 containing only ACK frames should therefore not be paced, to avoid delaying
 their delivery to the peer.
 
+Sending multiple packets into the network without any delay between them
+creates a packet burst that might cause short-term congestion and losses.
+Implementations MUST either use pacing or limit such bursts to minimum

to minimum -> to the minimum

-- 
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/3106#pullrequestreview-303664818

---

From: ianswett <[email-masked]>
Date: Fri, 18 Oct 2019 15:17:23 -0700

ianswett commented on this pull request.

Done in head.

-- 
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/3106#discussion_r336694343

---

From: mirjak <[email-masked]>
Date: Fri, 25 Oct 2019 00:39:15 -0700

mirjak commented on this pull request.

+Sending multiple packets into the network without any delay between them
+creates a packet burst that might cause short-term congestion and losses.
+Implementations MUST either use pacing or limit such bursts to minimum
+of 10 * kMaxDatagramSize and max(2* kMaxDatagramSize, 14720)), the same
+as the recommended initial congestion window.
+

Sorry again late here... To we recommend a fixed value of 10 packets or do we recommend to have the burst limit the same as the initial window? E.g. if we decide in future to increase our recommendation for the initial window, should that also increase the recommendation for the burst limit? I guess to be really smart about this, you would need to say that you set the burst but initial window but decrease if you see tail loss in the initial burst (but that might be an overkill). 

-- 
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/3106#pullrequestreview-307029301

---

From: Martin Thomson <[email-masked]>
Date: Fri, 25 Oct 2019 00:57:36 -0700

martinthomson commented on this pull request.

Or you could say that there are different initial window and burst limits, which is probably OK.  I don't think that we need to be that smart though.

-- 
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/3106#discussion_r338928420

---

From: ianswett <[email-masked]>
Date: Fri, 25 Oct 2019 09:55:13 -0700

That's a good point.  We could swap the sentence and say "same as the initial congestion window, which is recommended to be ..." if you think it's preferable.  But I'm also not sure it matters much.

-- 
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/3106#discussion_r339146939

---

From: Martin Thomson <[email-masked]>
Date: Fri, 25 Oct 2019 18:31:37 -0700

I think that explicitly saying "Initial congestion window" here as the primary target of the MUST requirement would be better.

-- 
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/3106#discussion_r339280586

---

From: mirjak <[email-masked]>
Date: Mon, 28 Oct 2019 06:59:11 -0700

Fine for me. 

-- 
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/3106#discussion_r339577123

---

From: ianswett <[email-masked]>
Date: Mon, 28 Oct 2019 07:13:46 -0700

PR created, PTAL: https://github.com/quicwg/base-drafts/pull/3160

-- 
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/3106#discussion_r339585036