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

[Jana Iyengar <[e-mail]>]

@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