comments on draft-cardwell-iccrg-bbr-congestion-control-02

[Junho Choi]

Hello,

I am working on bbr2 cc on quiche (https://github.com/cloudflare/quiche/pull/1218).

Basically I tried to follow the draft as-is, but I have found some issues.

I understand the spec and code is still evolving so it may be already updated,

but I believe it's still worth reporting here.

- cwnd: is it bytes vs packets? Section 2.8 BBRMinPipeCwnd() is 4 x MSS

  so I think cwnd is based on bytes, but other pseudocode is mostly using

  packets_in_flight, in the following places:

   - Section 4.3.2: BBRCheckDrain()
   - Section 4.3.3.4: see "Estimated queue"

   - Section 4.3.3.4: BBRHandleProbeRTT()

   - Section 4.3.3.4: MarkConnectionAppLimited()

   - Section 4.4.3: BBRHandleRestartFromIdle()

   - Section 4.6.4.4: BBROnEnterRTO()

  Other parameters (bw, tx_inflight) may have the same problem.

- Section 4.2.1:

  BBROnInit() also need to initialize the following:

    BBR.bw_lo = Infinity

    BBR.bw_hi = Infinity

    BBR.inflight_lo = Infinity

    BBR.inflight_hi = Infinity

    BBR.probe_up_cnt = Infinity

    BBR.ack_phase = ACKS_INIT

- Section 4.3.1.3:

   BBRCheckStartupHighLoss() pseudo code is missing

- Section 4.3.3.4: MarkConnectionAppLimited()

   C.app_limited = C.delivered + packets_in_flight ? : 1

  draft-cheng-iccrg-delivery-rate-estimation-02 used C.pipe but here

  packets_in_flight is used. Any reason?

  Also, C.delivered is represented as bytes, not packets in Section 4.5.1:

     C.delivered += packet.size

- Section 4.3.3.6: BBRAdaptUpperBounds():

  After the following code:

    if (BBR.ack_phase == ACKS_PROBE_STOPPING and BBR.round_start)

      /* end of samples from bw probing phase */

  ==> add the following 2 lines:

      BBR.bw_probe_samples = 0

      BBR.ack_phase = ACKS_INIT

 Also,

      BBR.bw_hi = rs.bw

 ==> "rs.bw" not mentioned anywhere. Is it "rs.delivery_rate"?

  In linux tcp, it is "BBR.inflight_hi = rs.tx_in_flight"

- Section 4.5.1: BBRUpdateRound()

   BBR.rounds_since_probe++

   ==> BBR.rounds_since_bw_probe++

- Section 4.5.6.3: BBRUpdateLatestDeliverySignals():

   "rs.prior_delivered" is not mentioned. I think it needs to be added in Section 2.3

- Section 4.5.6.3: BBRUpdateLatestDeliverySignals():

  "rs.losses" is not mentioned. or is it a typo of "rs.lost" ??

- Section 4.5.6.3: BBRAdaptLowerBoundsFromCongestion()

  if (BBR.loss_in_round())

  ==> if (BBR.loss_in_round)

- Section 4.5.6.3: BBRAdaptLowerBoundsFromCongestion()

   BBR.infligh_lo

  ==> BBR.inflight_lo (looks like a typo)

- Section 4.5.6.3: BBRAdapLowerBoundsFromCongestion()

   BBRIsProbingBW()

   ==> IsInAProbeBWState() ??

   Also, IsInAProbeBWState() may need BBR prefix

   ==> BBRIsInAProbeBWState()

- Section 4.6.4.2: BBRQuantizationBudget()

   if (BBR.state == ProbeBW && BBR.cycle_idx == ProbeBW_UP)

    ==> if (BBR.state == ProbeBW_UP)

   looks like the latest tcp bbr2 code has cycle_idx and the state definition is

   different, but it's not mentioned in the draft.

- Section 4.6.4.2:

   BBRUpdateAggregationBudget() pseudo code is missing

That's all I can find now. Thanks!

--
Junho Choi <junho dot choi at gmail.com> | https://saturnsoft.net