Congestion controller fixes #975
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Matthias247
force-pushed
the
congestion
branch
from
6898182 to
e0bc877
Compare
Matthias247
force-pushed
the
congestion
branch
from
e0bc877 to
0c0980d
Compare
Ralith
reviewed
Jan 15, 2021
| if self.bytes_acked >= self.window { | ||
| self.bytes_acked -= self.window; | ||
| self.window += self.config.max_datagram_size; | ||
| } |
There was a problem hiding this comment.
I think it'd be helpful to cite the RFC we got this logic from.
quinn-proto/src/connection/mod.rs
Outdated
| @@ -422,6 +428,8 @@ where | |||
| || self.path_response.is_some() | |||
| }); | |||
|
|
|||
| let mut was_congestion_blocked = false; | |||
There was a problem hiding this comment.
Elsewhere (e.g. handle_packet) we use "was_*" to refer to the state prior to running some logic which may change that state, whereas this is referring to the state after we're done. Could we say is_ here instead? Also, maybe transport_blocked (as opposed to application) since pacing can be responsible as well as congestion?
There was a problem hiding this comment.
Fine with removing was_. But I think even with pacing congestion_blocked is fine, because pacing is a form of congestion control
| if self.window >= self.ssthresh { | ||
| // Exiting slow start | ||
| // Initialize `bytes_acked` for congestion avoidance | ||
| self.bytes_acked = self.window - self.ssthresh |
There was a problem hiding this comment.
What's this logic based on? Also, style nit:
Suggested change
| self.bytes_acked = self.window - self.ssthresh | |
| self.bytes_acked = self.window - self.ssthresh; |
This change fixes 2 issues in the congestion controller: Extra slow slow start === The congestion controller had an issue where it didn't ramp up by doubling the window in the slow start phase as expected. The reason here was the usage of the "congestion_blocked" field, which aims to remove the avoid growing the window when there is no data to send. The issue with the field was that the first incoming ACK for every batch would increase the congestion window, which would let the next call to `congestion_blocked()` return false, and thereby let the congestion controller ignore all of the following ACKs in a received batch up the point where the next `poll_transmit()` call is made and the window is filled again. Since often all ACKs for one round-trip arrive in a single batch that means only the first ACK had an effect and the others where ignored -> which lead to increasing the window by 1 MTU instead of doubling it in the slow start phase. The new approach introduces a new `app_limited` field which caches whether the last `poll_transmit` attempt couldn't produce data because there was no application data available. Numeric precision issue in congestion avoidance === The formula ``` self.window += self.config.max_datagram_size * bytes / self.window; ``` which is specified in the RFC requires floating point arithmetic. If integer arithmetic is used, the multiplication of the first 2 values yields about 1.7MB for 1300 byte packets. As soon as the window is bigger than this value, the window won't change at all due the division leading to 0. The new implementation follow this guidance from the quic recovery specification: > In congestion avoidance, implementers that use an integer > representation for congestion_window should be careful with division, > and can use the alternative approach suggested in Section 2.1 of > [RFC3465].
Matthias247
force-pushed
the
congestion
branch
from
0c0980d to
f3d025e
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This change fixes 2 issues in the congestion controller:
Extra slow slow start
The congestion controller had an issue where it didn't ramp up by
doubling the window in the slow start phase as expected. The reason
here was the usage of the "congestion_blocked" field, which aims to
remove the avoid growing the window when there is no data to send.
The issue with the field was that the first incoming ACK for every batch
would increase the congestion window, which would let the next
call to
congestion_blocked()return false, and thereby let thecongestion controller ignore all of the following ACKs in a received
batch up the point where the next
poll_transmit()call is made andthe window is filled again. Since often all ACKs for one round-trip
arrive in a single batch that means only the first ACK had an effect
and the others where ignored -> which lead to increasing the
window by 1 MTU instead of doubling it in the slow start phase.
The new approach introduces a new
app_limitedfield whichcaches whether the last
poll_transmitattempt couldn't producedata because there was no application data available.
Numeric precision issue in congestion avoidance
The formula
which is specified in the RFC requires floating point arithmetic.
If integer arithmetic is used, the multiplication of the first 2 values
yields about 1.7MB for 1300 byte packets. As soon as the window is
bigger than this value, the window won't change at all due the
division leading to 0.
The new implementation follow this guidance from the quic recovery
specification: