Subject: Re: [quicwg/base-drafts] State machine for CC (#4055)

From: Martin Thomson <[email-masked]>
Date: Tue, 01 Sep 2020 15:42:48 -0700

@martinthomson pushed 4 commits.

fb9943441478fa295c11752125ef7fa89e7a321b  Merge branch 'master' into define-ss-ca
3ad13ceb7a26bc88631f24385cb628089330c589  Sprinkle some NewReno pixie dust
780b910b6f38c8bbe7d568520bf219acdc906169  Merge branch 'define-ss-ca' into cc-fsm
bc37ce7a8c440d05788db947e84f455ded8e901e  Merge remote-tracking branch 'origin/cc-fsm' into cc-fsm

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/4055/files/5d3f651cf191c411ebe674e03cebcd279c5279b5..bc37ce7a8c440d05788db947e84f455ded8e901e

---

From: ianswett <[email-masked]>
Date: Tue, 01 Sep 2020 16:45:05 -0700

@ianswett commented on this pull request.

This is a large change, but is largely good.  Some suggestions, particularly around the use of Recovery as a state vs the "recovery period"

-While in slow start, a NewReno sender increases the congestion window by the
-number of bytes acknowledged when each acknowledgment is processed, resulting in
+The congestion controller has three distinct states, as shown in
+{{fig-cc-fsm}}.
+
+~~~
+                 Start or         +------------+

Start seems odd to me, maybe "New Path"?

```suggestion
                 New Path or         +------------+
```

>  exponential growth of the congestion window.
 
-A sender MUST exit slow start and enter recovery when a loss is detected or when
-the ECN-CE counter reported by the peer increases.
+The sender exits slow start and enters a recovery period when a packet is lost

I think we need to keep the MUST?

```suggestion
The sender MUST exit slow start and enter a recovery period when a packet is lost
```

-A sender MUST exit congestion avoidance and enter recovery when a loss is
-detected or when the ECN-CE counter reported by the peer increases.
+On entering a recovery period, a sender MUST set the slow start threshold to
+half the value of the congestion window at the moment that loss is

```suggestion
half the value of the congestion window when loss is
```

-## Congestion Avoidance
+### Recovery Period {#recovery-period}

I think this should be "Recovery", not recovery period, since "Recovery" is a state, just like Slow Start is a state.  When we only referred to recovery period in a few spots, it seemed ok, but I've always heard the phrase "enter recovery", never "enters a recovery period".

-A sender MUST exit congestion avoidance and enter recovery when a loss is
-detected or when the ECN-CE counter reported by the peer increases.
+On entering a recovery period, a sender MUST set the slow start threshold to
+half the value of the congestion window at the moment that loss is
+detected. The congestion window MUST be set to the reduced value of the slow
+start threshold before exiting the recovery period. Implementations MAY set the

I prefer the organization the existing text for this paragraph, and starting the new paragraph with "Implementations MAY..."

> @@ -1653,20 +1697,21 @@ OnPacketAcked(acked_packet):
 ## On New Congestion Event
 
 Invoked from ProcessECN and OnPacketsLost when a new congestion event is
-detected. May start a new recovery period and reduces the congestion
-window.
+detected. If not in a recovery period, this starts a recovery period and

This is a good example of 'recovery' the state vs  the 'recovery period'.

```suggestion
detected. If not already in recovery, this starts a recovery period and
```

-- 
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/4055#pullrequestreview-480182596

---

From: Martin Thomson <[email-masked]>
Date: Tue, 01 Sep 2020 17:11:08 -0700

@martinthomson pushed 1 commit.

e094c5f27c67bd0703f1f1c3011ab8c0127248a4  Ian's suggestions

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

---

From: Martin Thomson <[email-masked]>
Date: Tue, 01 Sep 2020 17:12:31 -0700

@martinthomson commented on this pull request.

Just a consequence of many iterations of merging and rebasing.  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/4055#discussion_r481500140

---

From: Martin Thomson <[email-masked]>
Date: Tue, 01 Sep 2020 17:13:33 -0700

e43ad7c0a3b1d7cbdcd058b866eb02966f7fe85e  Reshuffle paragraphs, reflow

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

---

From: ianswett <[email-masked]>
Date: Wed, 02 Sep 2020 07:11:31 -0700

@ianswett approved this pull request.

Looks good, but it's a bit change, so I'd like @janaiyengar review as well.

-Implementations MAY set the congestion window immediately on entering a recovery
-period or use other mechanisms, such as Proportional Rate Reduction
-({{?PRR=RFC6937}}), to reduce it more gradually. If the congestion window is
-reduced immediately, a single packet can be sent prior to reduction. This speeds
-up loss recovery if the data in the lost packet is retransmitted and is similar
-to TCP as described in Section 5 of {{?RFC6675}}.
+Implementations MAY set the congestion window immediately upon entering a

```suggestion
Implementations MAY reduce the congestion window immediately upon entering a
```

-- 
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/4055#pullrequestreview-480853123

---

From: mirjak <[email-masked]>
Date: Wed, 02 Sep 2020 07:49:48 -0700

@mirjak commented on this pull request.

> -trip. Therefore during recovery, the congestion window remains unchanged
-irrespective of new losses or increases in the ECN-CE counter.
+trip. Therefore during a recovery period, the congestion window does not change
+in response to new losses or increases in the ECN-CE count.
+
+A recovery period ends and the sender enters congestion avoidance when a packet
+sent during the recovery period is acknowledged. This is slightly different
+from TCP's definition of recovery, which ends when the lost segment that
+started recovery is acknowledged ({{?RFC5681}}).
+
+### Congestion Avoidance
+
+A NewReno sender is in congestion avoidance any time the congestion window is
+at or above the slow start threshold and not in a recovery period.
+
+A sender in congestion avoidance uses an Additive Increase Multiplicative

```suggestion
A NewReno sender in congestion avoidance uses an Additive Increase Multiplicative
```

-- 
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/4055#pullrequestreview-480900201

---

From: mirjak <[email-masked]>
Date: Wed, 02 Sep 2020 07:50:04 -0700

The recovery period aims to limit congestion window reduction to once per round

```suggestion
A sender is in congestion avoidance any time the congestion window is
```

-- 
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/4055#pullrequestreview-480900454

---

From: Jana Iyengar <[email-masked]>
Date: Fri, 04 Sep 2020 13:11:35 -0700

@janaiyengar pushed 1 commit.

45fbd4389d76e213ca305439d5572b2f30bc9624  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/4055/files/e43ad7c0a3b1d7cbdcd058b866eb02966f7fe85e..45fbd4389d76e213ca305439d5572b2f30bc9624

---

From: Jana Iyengar <[email-masked]>
Date: Fri, 04 Sep 2020 13:15:42 -0700

0194e02c796f0a9d31789a20e448870fceb74232  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/4055/files/45fbd4389d76e213ca305439d5572b2f30bc9624..0194e02c796f0a9d31789a20e448870fceb74232

---

From: Jana Iyengar <[email-masked]>
Date: Fri, 04 Sep 2020 13:16:25 -0700

@janaiyengar commented on this pull request.

This is specific to NewReno, so I would leave this as it is.

-- 
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/4055#discussion_r483827188

---

From: mirjak <[email-masked]>
Date: Mon, 07 Sep 2020 01:19:44 -0700

How is this specific to NewReno? It's the definition of the ssthresh that you are in slow start when your cwnd is below that 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/4055#discussion_r484268552

---

From: Jana Iyengar <[email-masked]>
Date: Mon, 07 Sep 2020 18:04:51 -0700

It's specific to a sender that uses slow start with an ssthresh variable. BBR does not have an ssthresh, for example. Since we're only defining NewReno in this document, there isn't anything incorrect with saying "NewReno sender", though it might be slightly more specific than you might like.

-- 
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/4055#discussion_r484599022

---

From: Jana Iyengar <[email-masked]>
Date: Mon, 07 Sep 2020 18:05:28 -0700

@janaiyengar 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/4055#pullrequestreview-483739127

---

From: Jana Iyengar <[email-masked]>
Date: Mon, 07 Sep 2020 18:05:37 -0700

Merged #4055 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/4055#event-3738687714