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

Quic Session Datagram Queue is ignored when considering whether App limited #28

Open
mpelletan opened this issue Dec 1, 2022 · 1 comment

Comments

@mpelletan
Copy link

mpelletan commented Dec 1, 2022

Doing some prototyping using Quiche with WebTransport+BBR2, I found the datagram queue appeared to be ignored when assessing whether the app is limiting the bitrate:

  1. QuicSession::datagram_queue_:
    QuicDatagramQueue datagram_queue_;
  2. QuicSession::WillingAndAbleToWrite() does not consider the datagram queue, and therefore returns false when data is being paced by congestion control:
    bool QuicSession::WillingAndAbleToWrite() const {
  3. WillingAndAbleToWrite() is used by QuicConnection to determine whether congestion control is app limited:
    void QuicConnection::CheckIfApplicationLimited() {

In my test, where the pipe is filled with datagrams, BBR constantly marks datapoints as app limited and never gets out of STARTUP phase.

Simply considering a non-empty datagram queue as willingness to write allows BBR to get into PROBE_BW, although I'm sill seeing poor behavior, where the RTT increases quite dramatically from ~40ms (min RTT) to sustained ~200ms when we hit a bitrate close to congestion, but I haven't looked into what's causing it (no draining on latency increase?)

@ianswett
Copy link
Collaborator

ianswett commented Dec 1, 2022

Wow, good catch. That is indeed a bug.

516025 pushed a commit to 516025/quiche that referenced this issue Apr 18, 2023
Included changes:

  - c7b993eb750e60c307e82f75763600d9c06a6de1 Merge pull request google#28 from vasilvv/master by Victor Vasiliev <vasilvv@google.com>
  - da778f0a1cf158d3a6fbfa573d603efc4ead4b7e Add a missing deps in the BUILD file by Victor Vasiliev <vasilvv@google.com>
  - 0146ddf01a02c32761e64c2e0ef35f3e56c64d9a Merge pull request google#27 from vasilvv/master by Victor Vasiliev <vasilvv@google.com>
  - 764cca256a881c6189245d967fa81adf5a0676c6 Replace "const std::string&" with absl::string_view by Victor Vasiliev <vasilvv@google.com>

PiperOrigin-RevId: 441304196
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

No branches or pull requests

2 participants