BBRv2 RFC Draft 02 Issues

[Wesley Rosenblum]

I'm implementing BBRv2 for a QUIC protocol implementation (s2n-quic), and I've encountered a number of issues in revision 02 of the draft RFC. Some of these issues may be duplicates of those highlighted by Junho Choi in his April 26 email. I’ve grouped the issues into a few sections:

Logical issues:

• BBR.bw_hi seems to be initialized to `Infinity`, but the value is never lowered, only raised (in `BBRAdaptUpperBounds`). This causes issues with BBRAdaptUpperBounds, since it returns early if BBR.bw_hi == Infinity. Since that will always be true, BBR.inflight_hi is not raised either. When and how should BBR.bw_hi be lowered?
• In §4.3.4.4, BBR.probe_rtt_min_delay and BBR.min_rtt are only updated if the rs.rtt sample is strictly less than the current value. This differs from the v2alpha code that updates the min rtt if it is less than or equal to the current value. I believe the v2alpha code is correct, as an RTT sample equal to the current min RTT would indicate that RTT is still recently encountered, and should not result in the windowed min RTT value expiring.  

Missing definitions:

• packet.lost is referenced in §4.5.6.2, but is not defined in §2.2 or in [draft-cheng-iccrg-delivery-rate-estimation]
• C.lost is referenced in §4.5.6.2, but is not defined in §2.1 or in [draft-cheng-iccrg-delivery-rate-estimation]
• BBRUpdateAggregationBudget in §4.6.4.2 is not defined
• BBR.cycle_idx in §4.6.4.2 is not defined

Typos/Editorial issues:

• §2 contains the statement “The variables starting with C, P, or rs not defined below are defined in [draft-cheng-iccrg-delivery-rate-estimation].” However, there are no variables starting with `P`. I believe this is referring to the variables in §2.2 that start with `packet`
• There is missing punctuation in this statement from §4.3.1.1: “A second method BBR uses for estimating the bottleneck is full is by looking at sustained packet losses Specifically for a case where the following criteria are all met:”
• BBR.rounds_since_probe in §4.5.1 is not defined or used anywhere else, should this be BBR.rounds_since_bw_probe?
• §3.4.1: "the lowest que pressure is achieved” should be “the lowest queue pressure is achieved”
• §4.5.6.3: “BBRBeta * BBR.infligh_lo” should be “BBRBeta * BBR.inflight_lo”
• §4.6.4.2: “The BBR BBR.max_inflight is” should be “BBR.max_inflight is”
• §4.5.6.3: BBR.loss_in_round() should be BBR.loss_in_round 
• §4.3.3.6: BBR.bw_probe_up_cnt is not defined or used anywhere, should that be BBR.probe_up_cnt?
• §4.3.4.5: “we know that infligh is already below” should be “we know that inflight is already below”
• In §4.3.3.6, BBR.bw_hi is set to `rs.bw`, which is not defined or used anywhere else. Should this be `rs.delivery_rate`?

Other:

• The pseudocode for BBRHandleLostPacket(packet) in §4.5.6.2 references an (rs) RateSample variable. It was only clear from looking at the v2alpha code that this RateSample is constructed just the purpose of passing the rs.tx_inflight, rs.lost, and rs.is_app_limited values to IsInflightTooHigh(rs). I found this confusing, it might be better to just pass those three values directly in the pseudocode, rather than involve a RateSample.
• The definition of “recovery” used in the §4.6.4.4 and when to exit recovery seem TCP specific, and differs from the definition in RFC 9000: QUIC: A UDP-Based Multiplexed and Secure Transport, which says that recovery ends when a packet sent during the recovery period is acknowledged, not when there are no losses in a round. Since I would imagine this RFC would be more likely to be used for transport protocols that allow for user space congestion controllers (such as QUIC), it might be better to be less TCP specific.
• BBRInflight in §4.6.4.2 is defined with a single input (gain), but is used in §4.3.3.6 with an additional bandwidth input

Thanks for working on this draft RFC, I look forward to its continued development!

Thanks,
Wesley Rosenblum

[evol...@web.de]

Hello Wesley,

I'm also working on implementation of BBRv2 draft in OMNET++  and I've encountered almost the same issues and couple more.

I think it make sense to replicate your list of issues in iccrg group of IETF: https://mailarchive.ietf.org/arch/browse/iccrg 

Best regards,

Ekaterina Volodina

[Wesley Rosenblum]

Thanks, I've shared it in the ICCRG group as well: https://mailarchive.ietf.org/arch/msg/iccrg/MTdZgI0ZR11Qf_zqYpyTr38ttDo/

-Wesley

[Wesley Rosenblum]

Noticed another issue:

§4.3.4.4: BBRSaveCwnd() is called in BBRCheckProbeRTT(), but the return value is not used. Might be cleaner to just have BBRSaveCwnd() set BBR.prior_cwnd

Thanks,

Wesley Rosenblum

[evol...@web.de]

Hello Wesley,

thank you for mention this issue.

BBRSaveCwnd() is also called in BBROnEnterRTO() & BBROnEnterFastRecovery(), where BBR.prior_cwnd will be saved.

In my implementation I do the same: BBR.prior_cwnd = BBRSaveCwnd() in BBRCheckProbeRTT().

I would prefer to do it in BBRSaveCwnd() like https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/tree/net/ipv4/tcp_bbr.c#n320 does, but I try to be conform to bbr2 draft.

Best regards,

Ekaterina Volodina

[Junho Choi]

bbr00 (bbrv1) spec also has the same issue, so I think it's just not updated.

https://datatracker.ietf.org/doc/html/draft-cardwell-iccrg-bbr-congestion-control-00#section-4.3.5

--
You received this message because you are subscribed to the Google Groups "BBR Development" group.
To unsubscribe from this group and stop receiving emails from it, send an email to bbr-dev+u...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/bbr-dev/a8ec7ac5-2135-451c-a600-640457c0f288n%40googlegroups.com.

--

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

[Wesley Rosenblum]

One more, though this might just need clarification:

In §4.6.4.4: BBRModulateCwndForRecovery(), it sets cwnd = max(cwnd - rs.newly_lost, 1). I'm wondering why it isn't cwnd = max(cwnd - rs.newly_lost, BBRMinPipeCwnd)? I thought the cwnd should not drop below BBRMinPipeCwnd?

This is another case were it was implemented this way in BBRv1 (https://github.com/torvalds/linux/blob/master/net/ipv4/tcp_bbr.c#L491-L492). Perhaps this just needs to be updated?

Thanks,

Wesley

[Wesley Rosenblum]

Regarding the issue with BBR.bw_hi never being lowered from Infinity:

> BBR.bw_hi seems to be initialized to `Infinity`, but the value is never lowered, only raised (in `BBRAdaptUpperBounds`). This causes issues with BBRAdaptUpperBounds, since it returns early if BBR.bw_hi == Infinity. Since that will always be true, BBR.inflight_hi is not raised either. When and how should BBR.bw_hi be lowered?

Would it be appropriate to set BBR.bw_hi = rs.delivery_rate in BBRHandleInflightTooHigh()? That would be similar to how BBR.inflight_hi is lowered from infinity

Thanks,

Wesley

[evol...@web.de]

Hello Wesley, as I saw in other implementations BBR.bw_hi and BBR.inflight_hi were initialized to zero.

I would ask, in which case it can raise to infinity?

Regards

Ekaterina 

[Wesley Rosenblum]

I see BBR.inflight_hi initialized to infinity in tcp_bbr2.c, along with BBR.bw_lo, and BBR.inflight_lo. BBR.bw_hi is initialized to 0, but from this comment from Neal Cardwell, BBR.bw_hi in the current TCP implementation actually corresponds to max_bw in the RFC draft. 

Also, I'm not sure why the check for if (BBR.inflight_hi == Infinity or BBR.bw_hi == Infinity) in BBRAdaptUpperBounds would exist if these values weren't initialized to infinity.

-Wesley

You received this message because you are subscribed to a topic in the Google Groups "BBR Development" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/bbr-dev/xmley7VkeoE/unsubscribe.
To unsubscribe from this group and all its topics, send an email to bbr-dev+u...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/bbr-dev/aedf28e7-67f6-4613-a45d-0a83db54be61n%40googlegroups.com.

[Wesley Rosenblum]

Another logical issue:

BBRHeadroom is defined as 0.85 in §2.8, with the intention of lowering inflight_hi to 0.85 * inflight_hi. 

But in BBRInflightWithHeadroom()defined in §4.3.3.6, the pseudocode ends up lowering inflight_hi to inflight_hi - BBRHeadroom * inflight_hi or 0.15 * inflight_hi

This may have happened because tcp_bbr2.c has the same math implementation as the draft RFC, but defines BBRHeadroom as 0.15

-Wesley

[Yuanlai Zhou]

Hi Wesley, 

     I'm also implementing BBRv2 for simulation. I found that the BBR.filled_pipe was initialized as false and set as true in BBRCheckStartupFullBandwidth() and BBRCheckStartupHighLoss(), but never set back to false in other place. How do you set BBR.filled_pipe from true to false? or BBR.filled_pipe will keep as true after the two function set it as true?

Thank you,

Yuanlai

[Wesley Rosenblum]

Hi Yuanlai,

I think it is expected that once `BBR.filled_pipe` is set to true, it remains that way, since after the initial Startup period the ProbeBW_Up state is used to probe for additional bandwidth

-Wesley

[Yuanlai Zhou]

Hi Wesley,

    Thanks for the quick response. I agreed that the ProbeBW_Up state is used to probe for additional bandwidth, but in ProbeRTT, the cwnd_gain is set as 0.5, the BBR_bw will drop a lot and the BBR.filled_pipe should be false? In addition, in exit_ProbeRTT function, BBR.filled_pipe will be used to decide where to go. So, should we check the pipe status when exit_ProbeRTT() was called?

Thanks,

Yuanlai

--

You received this message because you are subscribed to a topic in the Google Groups "BBR Development" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/bbr-dev/xmley7VkeoE/unsubscribe.
To unsubscribe from this group and all its topics, send an email to bbr-dev+u...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/bbr-dev/0983df81-93ea-4ac5-a709-17a41f7e6023n%40googlegroups.com.

[MUHAMMAD AHSAN]

Hi

Do we have bbrv2 available to be compiled under linux kernel 5 or 6 ?

I mean tcp_bbrv2.c  file ?

Regards,
Ahsan

You received this message because you are subscribed to the Google Groups "BBR Development" group.

To unsubscribe from this group and stop receiving emails from it, send an email to bbr-dev+u...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/bbr-dev/CAF5v5N_msg6CokR19t7KenB%3DhQZZz_pakZi2k4Cm54855L%2BUzA%40mail.gmail.com.

[Yuanlai Zhou]

I think you want this:

bbr/tcp_bbr2.c at v2alpha · google/bbr · GitHub

[Wesley Rosenblum]

> I agreed that the ProbeBW_Up state is used to probe for additional bandwidth, but in ProbeRTT, the cwnd_gain is set as 0.5, the BBR_bw will drop a lot and the BBR.filled_pipe should be false? 

BBR.filled_pipe means the pipe was filled at some point in the connection, not that the pipe is currently filled. So once it is true, it should not be set to false

> In addition, in exit_ProbeRTT function, BBR.filled_pipe will be used to decide where to go. So, should we check the pipe status when exit_ProbeRTT() was called?

You check the value of BBR.filled_pipe when exiting ProbeRTT to decide if you need to continue in Startup to try to fill the pipe some more, or if the pipe was already filled in the past then you return to ProbeBW

-Wesley

[evol...@web.de]

Hello Wesley,

I’m wondering, how did you choose finally to initialize BBR.bw_hi to Zero or to Infinity?

And may be you can share some cwnd plots to compare?

Thanks,

Ekaterina

[Wesley Rosenblum]

We are still initializing BBR.bw_hi to Infinity, which currently means it has no effect. BBR.inflight_hi is the only long term bound used. 

-Wesley

[Wesley Rosenblum]

Another issue I noticed recently:

BBRAdaptUpperBounds() in §4.3.3.6 calls BBRAdvanceMaxBwFilter() if BBR.ack_phase == ACKS_PROBE_STOPPING, with the intention of advancing the Max BW filter once after the bandwidth probing has concluded for the current cycle. However, the BBR.ack_phase does not subsequently get updated, meaning BBRAdvanceMaxBwFilter() may be invoked multiple times per probing cycle.

I noticed this is addressed in the Linux TCP BBRv2 implementation by setting BBR.ack_phase to "Init" so the Max BW filter is only advanced once per probe cycle.

-Wesley

--

You received this message because you are subscribed to a topic in the Google Groups "BBR Development" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/bbr-dev/xmley7VkeoE/unsubscribe.
To unsubscribe from this group and all its topics, send an email to bbr-dev+u...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/bbr-dev/4fdcaa6f-88db-4aec-b3c4-a036c101d202n%40googlegroups.com.

[Wesley Rosenblum]

Bumping this thread in light of the impending draft update for BBRv3. Looking forward to reading the new revision!

-Wesley