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

Draft: Implement BBR #935

Closed
wants to merge 7 commits into from
Closed

Draft: Implement BBR #935

wants to merge 7 commits into from

Conversation

rafibaum
Copy link

@rafibaum rafibaum commented May 17, 2021

I've recently been working on a version of BBR for quiche for a research project. It's broadly based on information found in the draft BBR RFC, the Linux kernel implementation, and Google's implementation in their (similarly named) QUIC library.

This version of BBR implements the core BBR algorithm, the packet conservation loss handling mechanism, and the long-term sampling functionality which so far has only been implemented in the Linux kernel implementation and isn't detailed in the RFC or the QUIC implementation. I've had to make a number of adaptations to the algorithm to get it to work in quiche and there's still some features missing:

  • No ack aggregation handling.
  • Packet conservation adapted since we're not using the TCP state machine anymore, adapted from how Google handles this.
  • Under constant packet loss (1%) bitrate seems to collapse. May be a delivery rate estimation issue but needs investigating.

Also more general caveats, there's no unit testing and comments are sparse. I'm submitting this draft now to see if you'd be receptive to getting BBR merged in and what changes you'd want to see here before starting code review. As part of my research, I've done a bit of evaluation and I've seen typical BBR behaviour where the throughput is higher than what CUBIC usually gets you but it incurs higher packet loss rates.

Depends on #770
Closes #514

@normanmaurer
Copy link
Contributor

@rafibaum I am not a quiche developer and also no lawyer so it would be good to "confirm" my assumption, but if you did base some of your work on the code that you found in the linux kernel it might be problematic to pull the code in if the origin was licensed under the GPL.

@rafibaum
Copy link
Author

The BBR module is, depending on which Git repository you look at, either dual licensed under BSD (2-clause) and GPL or under Apache 2.0, either of which is compatible. The QUIC implementation is also Apache 2.0.

@normanmaurer
Copy link
Contributor

@rafibaum Perfect ... nevermind then

ghedo and others added 3 commits May 19, 2021 09:15
Currently quiche does not hold any information about the network path of
a QUIC connection (such as the address of the peer), and the application
is responsible for maintaining such information.

This means that in case of a network migration, the application is
responsible for detecting the change and switching over to new path,
however there is currently no way for an application to actually
validate a new path, as required by the QUIC spec.

Adding an ad-hoc API to only expose path validation to applications
would likely be very cumbersome, due to the synchronization needed
between an application and quiche on the state of the current path, and
any other path being probed.

Instead, this change makes quiche be aware of the network path being
used.

The application needs to communicate the destination address of a
connection upon creationg (via `accept()` or `connect()`), as well as
the source address of received packets (via `recv()` and the new
`RecvInfo` structure).

In turn quiche will provide the application with the destination address
of generated packets (via `send()` and the new `SendInfo` structure).

Currently only the destination address of a connection is tracked, which
would allow quiche to handle responding to migrations transparently from
the application (but this will be added as a separate change).

Additional fields can later be added to `RecvInfo` and `SendInfo`, such
as the address of the local endpoint in order to be able to initiate
migrations, rather than just respond to them.
Rather than allocating a new value on the stack, and then copying that
value into the output, just write the address into the output structure
directly.
@rafibaum
Copy link
Author

@normanmaurer no worries! Licenses are often disregarded so I'm glad you checked. I'll add attribution for both codebases later on.

@rafibaum rafibaum closed this May 24, 2021
@rafibaum rafibaum deleted the long-term branch May 24, 2021 16:19
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.

Support BBR congestion control
3 participants