Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(s2n-quic-core): Advance max bw filter only once per probe bw cycle #1698

Merged
merged 1 commit into from
Apr 6, 2023

Conversation

WesleyRosenblum
Copy link
Contributor

@WesleyRosenblum WesleyRosenblum commented Apr 6, 2023

Description of changes:

BBRv2 uses a WindowedMaxFilter of size 2 to track the maximum bandwidth sample that had been obtained over the current bandwidth probing cycle and the previous bandwidth probing cycle. The way this is supposed to work is each time BBR completes a cycle from ProbeBW_Down -> ProbeBW_Cruise -> ProbeBW_Refill -> ProbeBW_Up, a counter is incremented.

This can be seen in the BBR draft RFC pseudo-code. BBRAdvanceMaxBwFilter() increments the counter:

if (BBR.ack_phase == ACKS_PROBE_STOPPING and BBR.round_start)
      /* end of samples from bw probing phase */
      if (IsInAProbeBWState() and !rs.is_app_limited)
        BBRAdvanceMaxBwFilter()

Unfortunetly this pseudocode does not change the BBR.ack_phase, which allows the BBRAdvanceMaxBwFilter() function to be called multiple times within the same bandwidth probing cycle.

This is addressed in the Linux TCP BBRv2 implementation by setting the ACK phase to the Init phase, ensuring that the max bandwidth counter is only incremented once:

if (bbr->ack_phase == BBR_ACKS_PROBE_STOPPING && bbr->round_start) {
	/* End of samples from bw probing phase. */
	bbr->bw_probe_samples = 0;
	bbr->ack_phase = BBR_ACKS_INIT;

This change follows the Linux TCP BBRv2 implementation and transitions the ack phase to AckPhase::Init from AckPhase::ProbeStopping

Testing:

Added a unit test and updated others

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@WesleyRosenblum WesleyRosenblum merged commit ab1f235 into main Apr 6, 2023
@WesleyRosenblum WesleyRosenblum deleted the WesleyRosenblum/ackphase branch April 6, 2023 19:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants