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

[ianswett <[e-mail]>]

@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