Subject: [quicwg/base-drafts] Attempt to define slow start/congestion avoidance (#4005)

From: Martin Thomson <[email-masked]>
Date: Sun, 16 Aug 2020 22:08:52 -0700

This essentially rewrites these small sections.

The definitions are new.  The text on how the congestion window
increases is just tweaked.  The reaction to loss is moved to the
congestion avoidance section, to avoid repetition.

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

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

-- Commit Summary --

  * Attempt to define slow start/congestion avoidance

-- File Changes --

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

-- Patch Links --

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

---

From: mirjak <[email-masked]>
Date: Mon, 17 Aug 2020 02:36:27 -0700

@mirjak commented on this pull request.

> -Additive Increase Multiplicative Decrease (AIMD) approach that increases
-the congestion window by one maximum packet size per congestion window
-acknowledged.  When a loss or ECN-CE marking is detected, NewReno halves
-the congestion window, sets the slow start threshold to the new
-congestion window, and then enters the recovery period.
+The congestion controller is in congestion avoidance any time the congestion
+window is at or above the slow start threshold.
+
+Congestion avoidance uses an Additive Increase Multiplicative Decrease (AIMD)
+approach that increases the congestion window by one maximum packet size per
+congestion window acknowledged.
+
+When first entering congestion avoidance, when a packet is declared lost, or
+when the ECN-CE count is detected, the congestion controller halves the
+congestion window, reduces the slow start threshold to the resulting size, and
+enters the recovery period.

I'm just realising this now but this is actually not how TCP works. When you enter recovery you halve the ssthresh (basically to remember the right value for later). Then what TCP does is that is keeps sending one new packet for each ACK received (conservation principle: not know which or how many packets have been ACKed but know there must be at least one successful received and leaving the network) and then when all losses are repaired it sets the cwnd to ssthresh and leaves recovery. If you halve cwnd immediately when entering recovery, you can't send data for half an RTT until you received enough ACKs to be below the cwnd. This is usually not what you want.

-- 
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/4005#pullrequestreview-468312505

---

From: ianswett <[email-masked]>
Date: Mon, 17 Aug 2020 08:10:17 -0700

@ianswett approved this pull request.

This is likely to conflict with #3978

 ## Congestion Avoidance
 
-Slow start exits to congestion avoidance.  Congestion avoidance uses an

```suggestion
when the ECN-CE marking is detected, the congestion controller halves the
```

I think you're correct, do you have a good RFC reference handy?  I couldn't easily find this text.

-- 
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/4005#pullrequestreview-468544666

---

From: Mike Bishop <[email-masked]>
Date: Mon, 17 Aug 2020 08:32:28 -0700

@MikeBishop commented on this pull request.

Having read this text, I think your comment on the issue is correct -- there are three states here.

> @@ -763,24 +763,33 @@ The RECOMMENDED value is 2 * max_datagram_size.
 
 ## Slow Start
 
-While in slow start, QUIC increases the congestion window by the
-number of bytes acknowledged when each acknowledgment is processed, resulting
-in exponential growth of the congestion window.
+The congestion controller is in slow start any time the congestion window is
+below the slow start threshold.  The congestion controller begins in slow start.

...because the ICW is less than SSThresh, presumably?  Might be worth mentioning that explicitly.

 ## Congestion Avoidance

```suggestion
when the ECN-CE count is increased, the congestion controller halves the
```

-- 
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/4005#pullrequestreview-468567203

---

From: Martin Thomson <[email-masked]>
Date: Mon, 17 Aug 2020 15:33:31 -0700

@martinthomson pushed 1 commit.

2f3927e55f3a01aae44ee42ed324f49788214509  detected/increased

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/4005/files/2a9cb75e13ebfaf79dd9cf4d7296ae7b3ae612fd..2f3927e55f3a01aae44ee42ed324f49788214509

---

From: Martin Thomson <[email-masked]>
Date: Mon, 17 Aug 2020 15:33:47 -0700

@martinthomson commented on this pull request.

 ## Congestion Avoidance

Taking Mike's suggestion.

-- 
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/4005#discussion_r471811021

---

From: Martin Thomson <[email-masked]>
Date: Mon, 17 Aug 2020 15:36:33 -0700

I'll rebase this on #3978 when that lands.  The only thing that I think this will clobber (and so I need to capture) are the MUST statements that define AIMD.

-- 
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/4005#issuecomment-675148846

---

From: Martin Thomson <[email-masked]>
Date: Mon, 17 Aug 2020 18:27:14 -0700

755f59a06e4e0f507c3963bf1515720db5b3940e  Integrate the changes from #3978

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/4005/files/2f3927e55f3a01aae44ee42ed324f49788214509..755f59a06e4e0f507c3963bf1515720db5b3940e

---

From: ianswett <[email-masked]>
Date: Tue, 18 Aug 2020 07:49:53 -0700

LG, one suggestion on ssthresh init.

 ## Slow Start

+1 to this suggestion, since I had the same thought.  ssthresh is initialized to infinity, which is in the pseudocode, but not in this text I'm now noticing.
```suggestion
below the slow start threshold.  The congestion controller begins in slow start
because the slow start threshold is initialized to an infinite 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/4005#pullrequestreview-469531925

---

From: mirjak <[email-masked]>
Date: Tue, 18 Aug 2020 08:00:26 -0700

https://tools.ietf.org/html/rfc5681#section-4.3

-- 
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/4005#discussion_r472264340

---

From: Martin Thomson <[email-masked]>
Date: Tue, 18 Aug 2020 16:02:08 -0700

2775fdd582e3f7f977f1774300bd1f3486e072e1  ssthresh being initiallly infinite means that you start in slow start

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/4005/files/755f59a06e4e0f507c3963bf1515720db5b3940e..2775fdd582e3f7f977f1774300bd1f3486e072e1

---

From: ianswett <[email-masked]>
Date: Tue, 18 Aug 2020 16:08:10 -0700

-- 
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/4005#pullrequestreview-469888827

---

From: Martin Thomson <[email-masked]>
Date: Tue, 18 Aug 2020 16:32:04 -0700

@janaiyengar just explained to me why this difference exists, and I found the argument convincing.  I don't know whether this is worth calling out though, it seems like it might be a little tricky to get down in text.

-- 
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/4005#discussion_r472549667

---

From: mirjak <[email-masked]>
Date: Wed, 19 Aug 2020 02:42:28 -0700

I think the MUST requirement is that the cwnd is set to ssthresh when LEAVING recovery. In order to decrement the cwnd gradually we could say that only one new packet for two ACKed ones should be send which then would be mostly inline with TCP or we can leave the detailed scheme to the implementation and just put some warnings to not overload the link in recovery. Alternatively you could also e.g. halve the cwnd and then pace out packets over one RTT (which is assume is what implementation do right 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/4005#discussion_r472901102

---

From: ianswett <[email-masked]>
Date: Wed, 19 Aug 2020 09:47:04 -0700

@ianswett commented on this pull request.

If we're going to make a change, I think it needs to be on #3978, which is a design PR that's going to be sent out for consensus call soon.

-- 
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/4005#discussion_r473175220

---

From: Martin Thomson <[email-masked]>
Date: Wed, 19 Aug 2020 17:17:59 -0700

Yes.  This PR is intended to be a strictly editorial conversion.  I hope that I have faithfully translated the text from #3978.

-- 
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/4005#discussion_r473465267

---

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

@martinthomson This is a very good change. Why don't you include the diagram in #4005. I drew the same diagram when I tried to understand 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/4005#issuecomment-676849391

---

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

@janaiyengar commented on this pull request.

(This is being discussed in #3978.)

-- 
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/4005#discussion_r476032423

---

From: Jana Iyengar <[email-masked]>
Date: Mon, 24 Aug 2020 18:23:17 -0700

> +When first entering congestion avoidance, when a packet is declared lost, or
+when the ECN-CE count is increased, the congestion controller described in this
+document halves the congestion window, reduces the slow start threshold to the
+resulting size, and enters the recovery period. A congestion controller MUST
+reduce the congestion window when a loss or ECN-CE marking is detected.

```suggestion
When first entering congestion avoidance, when a packet is declared lost, or
when the ECN-CE count is increased, the congestion controller described in this
document MUST halve the congestion window, set the slow start threshold to the
size of the reduced congestion window, and enter the recovery period.
```

-- 
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/4005#pullrequestreview-474030278

---

From: Jana Iyengar <[email-masked]>
Date: Mon, 24 Aug 2020 18:34:34 -0700

-the congestion window, sets the slow start threshold to the new
-congestion window, and then enters the recovery period.
+The congestion controller is in congestion avoidance any time the congestion
+window is at or above the slow start threshold.
+
+Congestion avoidance uses an Additive Increase Multiplicative Decrease (AIMD)
+approach that increases the congestion window by one maximum datagram size per
+congestion window acknowledged. A congestion controller MUST NOT increase the
+congestion window by more than this amount.
+
+When first entering congestion avoidance, when a packet is declared lost, or
+when the ECN-CE count is increased, the congestion controller described in this
+document halves the congestion window, reduces the slow start threshold to the
+resulting size, and enters the recovery period. A congestion controller MUST
+reduce the congestion window when a loss or ECN-CE marking is detected.
 
 ## Recovery Period

```suggestion
## Recovery Period {#recovery}
```

-- 
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/4005#pullrequestreview-474033629

---

From: Martin Thomson <[email-masked]>
Date: Mon, 24 Aug 2020 18:44:49 -0700

bb961751b3168fc43ba08df5a268811452559628  Reword congestion signal response

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/4005/files/2775fdd582e3f7f977f1774300bd1f3486e072e1..bb961751b3168fc43ba08df5a268811452559628

---

From: Jana Iyengar <[email-masked]>
Date: Mon, 24 Aug 2020 18:53:45 -0700

@janaiyengar approved this pull request.

I think this is good to merge. @martinthomson ? @ianswett ?

-- 
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/4005#pullrequestreview-474039473

---

From: Martin Thomson <[email-masked]>
Date: Mon, 24 Aug 2020 18:57:35 -0700

This rolls up the normative statements in #3978, so why don't we wait for that to land first.

-- 
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/4005#issuecomment-679456530

---

From: Martin Thomson <[email-masked]>
Date: Mon, 24 Aug 2020 18:58:54 -0700

4478c2b99a2c5207451c52366b3d05482ca4f7f4  Merge branch 'master' into define-ss-ca

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/4005/files/bb961751b3168fc43ba08df5a268811452559628..4478c2b99a2c5207451c52366b3d05482ca4f7f4