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

From: Martin Thomson <[email-masked]>
Date: Tue, 25 Aug 2020 17:24:21 -0700

You can view, comment on, or merge this pull request online at:

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

-- Commit Summary --

  * Add some MUSTs
  * Update draft-ietf-quic-recovery.md
  * Update draft-ietf-quic-recovery.md
  * Update draft-ietf-quic-recovery.md
  * Update draft-ietf-quic-recovery.md
  * Attempt to define slow start/congestion avoidance
  * detected/increased
  * Integrate the changes from #3978
  * Update draft-ietf-quic-recovery.md
  * Update draft-ietf-quic-recovery.md
  * Update draft-ietf-quic-recovery.md
  * Update draft-ietf-quic-recovery.md
  * ssthresh being initiallly infinite means that you start in slow start
  * Clarify NewReno
  * Remove a sentence
  * Update draft-ietf-quic-recovery.md
  * Reword congestion signal response
  * Merge branch &#39;master&#39; into define-ss-ca
  * Merge remote-tracking branch &#39;origin/ianswett-must-new-reno&#39; into define-ss-ca
  * A Congestion Control state machine

-- File Changes --

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

-- Patch Links --

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

---

From: Martin Thomson <[email-masked]>
Date: Tue, 25 Aug 2020 17:30:04 -0700

@martinthomson pushed 1 commit.

ec01b201edfa755c39d8b172eda6aa55c7c7a502  Make pseudocode match directly

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

---

From: Jana Iyengar <[email-masked]>
Date: Tue, 25 Aug 2020 17:42:59 -0700

@janaiyengar commented on this pull request.

Thanks for doing this -- a few minor comments. Overall LGTM, but let's wait until iterations on #3978 end and we know where that lands.

> +
+The sender exits slow start and enters a recovery period when a packet is lost
+or when the ECN-CE count reported by its peer increases.
+
+A sender re-enters slow start any time the congestion window is less than the
+slow start threshold, which only occurs after persistent congestion is
+declared.
+
+### Recovery Period {#recovery-period}
+
+A sender enters the recovery period when it detects the loss of a packet or the
+ECN-CE count reported by its peer increases. A sender that is already in a
+recovery period stays in it and does not re-enter it.
+
+On entering a recovery period, a sender MUST set the slow start threshold to
+the half the value of the congestion window at the moment that loss is

```suggestion
half the value of the congestion window at the moment that loss is
```

 The recovery period aims to limit congestion window reduction to once per round
-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 remains does

```suggestion
trip. Therefore during a recovery period, the congestion window does
```

> +sent during the recovery period is acknowledged. This is slightly different
+from TCP's definition of recovery, which ends when the lost packet that started
+recovery is acknowledged.

```suggestion
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}}).
```

 ~~~
 OnCongestionEvent(sent_time):
-  // Start a new congestion event if packet was sent after the
-  // start of the previous congestion recovery period.
-  if (!InCongestionRecovery(sent_time)):
-    congestion_recovery_start_time = now()
-    congestion_window *= kLossReductionFactor
-    congestion_window = max(congestion_window, kMinimumWindow)
-    ssthresh = congestion_window
-    // A packet can be sent to speed up loss recovery.
-    MaybeSendOnePacket()
+  // No reaction when recovery period.

```suggestion
  // No reaction if already in a recovery period.
```

> +                                        v
+ +------------+     Loss or       +------------+
+ | Congestion |  ECN-CE increase  |  Recovery  |
+ | Avoidance  |------------------>|   Period   |
+ +------------+                   +------------+
+           ^                            |
+           |                            |
+           +----------------------------+
+              Acknowledgment of packet
+                sent during recovery
+~~~
+{: #fig-cc-fsm title="Congestion Control States and Transitions"}
+
+These states and the transitions between them are described in subsequent
+sections.
+

Perhaps add a note about persistent congestion, since that's not an explicit state in this machine. Maybe something like "Note that a sender can declare persistent congestion in any of these states (see {{persistent-congestion}}), after which it enters slow start.".

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

---

From: Martin Thomson <[email-masked]>
Date: Tue, 25 Aug 2020 17:49:21 -0700

a995b265247af1c7187263df77413919bb41741a  Corrections and 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/ec01b201edfa755c39d8b172eda6aa55c7c7a502..a995b265247af1c7187263df77413919bb41741a

---

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

@kazu-yamamoto commented on this pull request.

> -threshold is set to the new congestion window.  QUIC re-enters slow start
-any time the congestion window is less than the slow start threshold,
-which only occurs after persistent congestion is declared.
+~~~
+                 Start or         +------------+
+            persistent congestion |   Slow     |
+        (O)---------------------->|   Start    |
+                                  +------------+
+                                        |
+                                Loss or |
+                        ECN-CE increase |
+                                        v
+ +------------+     Loss or       +------------+
+ | Congestion |  ECN-CE increase  |  Recovery  |
+ | Avoidance  |------------------>|   Period   |
+ +------------+                   +------------+

It would be nice if you drew what happens in Recovery Period in the case of  Loss or ECN-CE increase. In my understanding, Recovery Period continues and both cwin and ssthresh are not changed. I'm not sure if congestion_recovery_start_time 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/4055#pullrequestreview-475090478

---

From: Martin Thomson <[email-masked]>
Date: Tue, 25 Aug 2020 18:38:13 -0700

@martinthomson commented on this pull request.

Nothing happens.  The only real actions occur on entry (and maybe on exit, depending on when you decide to apply the congestion window decrease).  The text and pseudocode capture this; I had trouble fitting anything more into the diagram.

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

---

From: Kazu Yamamoto <[email-masked]>
Date: Tue, 25 Aug 2020 19:08:50 -0700

OK.

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

---

From: mirjak <[email-masked]>
Date: Wed, 26 Aug 2020 01:32:37 -0700

@mirjak commented on this pull request.

Shouldn't this also show how to transition out of 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/4055#discussion_r477128928

---

From: Mike Bishop <[email-masked]>
Date: Wed, 26 Aug 2020 07:24:04 -0700

@MikeBishop commented on this pull request.

```suggestion
            Start                 +------------+
             (O)----------------->|   Slow     |
       +------------------------->|   Start    |
       |                          +------------+
       | Persistent                     |
       | congestion             Loss or |
       |                ECN-CE increase |
       |                                v
```

It's probably worth calling persistent congestion out as a separate transition rather than a restart.  Though it's more complex than that, since I believe persistent congestion is also checked only on a loss event.

-## Recovery Period {#recovery-period}
+A sender enters the recovery period when it detects the loss of a packet or the
+ECN-CE count reported by its peer increases. A sender that is already in a
+recovery period stays in it and does not re-enter it.

Switching between "the"/"a" here.

Looks like that's been added.

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

---

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

```suggestion
A sender enters a recovery period when it detects the loss of a packet or the
```

You could enter persistent congestion from any state, so this particular transition won't be adequate. You could make draw a parallel box called persistent-congestion on the left to which {Slow Start, Congestion Avoidance, Recovery} can all transition, which would then transition back up to Slow Start.

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

---

From: Martin Thomson <[email-masked]>
Date: Wed, 26 Aug 2020 15:49:32 -0700

5d3f651cf191c411ebe674e03cebcd279c5279b5  a/the

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

---

From: Martin Thomson <[email-masked]>
Date: Wed, 26 Aug 2020 15:50:57 -0700

When you encounter persistent congestion, you reset the state machine.  That is what the text below the figure says.  It's not a new state because then you are in Slow Start again.  I think that is the only sensible solution here.

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

---

From: Jana Iyengar <[email-masked]>
Date: Thu, 27 Aug 2020 11:12:25 -0700

Good point. It's certainly not a new state for the congestion controller; it's an event, much like 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/4055#discussion_r478605668