search

quinn-rs / quinn

Async-friendly QUIC implementation in Rust
Apache License 2.0
3.76k stars 380 forks source link

BBR implementation ignores persistent congestion #1225

Closed Ralith closed 2 years ago

Ralith commented 2 years ago

Similar to the issue resolved in #1224.

CC @FrankSpitulski

FrankSpitulski commented 2 years ago

@Ralith I'm not sure BBR should be using this flag. Besides going into recovery mode, it largely ignores congestion flags. It just keeps trying to fill the network with what it believes is the max throughput. Even in recovery mode it still tries to grow towards this value. It already has a mechanism for entering that recovery mode, so I'm not sure where this flag would be used.

FrankSpitulski commented 2 years ago

there's an explanation for that here. https://github.com/google/bbr/blob/v2alpha/net/ipv4/tcp_bbr2.c#L7

I've also checked the (google) quiche impl and don't see anything in there paying attention to persistent congestion.

Ralith commented 2 years ago

Hmm, that does seem to check out. I was concerned because Cubic was definitely missing it (cf. cloudflare's quiche), but perhaps that was coincidence. Word on the QUIC slack was:

If it's an existing TCP CCA (e.g. Cubic and BBR) there's a natural path to follow [on persistent congestion], which is to take the action those do on RTO

but indeed Google quiche stubs RTO out at https://quiche.googlesource.com/quiche/+/refs/heads/main/quic/core/congestion_control/bbr_sender.h#125 and I don't see anything that looks spiritually similar.