fix: Path is not app limited if more data is available to transmit #1326
Conversation
| /// Sending is app limited if the application is not fully utilizing the available | ||
| /// congestion window currently and there is no more application data remaining to send. | ||
| fn is_app_limited(&self, path: &Path<Config>, bytes_sent: usize) -> bool { | ||
| let cwnd = path.congestion_controller.congestion_window(); |
There was a problem hiding this comment.
Would it be worth moving all of this to Path in something like is_congestio_blocked?
Is this a major concern or something that is non-ideal? Dont think acquiring more CW than needed is bad but correct me if incorrect. |
The risk with acquiring more congestion window than what is actually being used is that the network hasn't demonstrated it can sustain that size congestion window. So if you then do actually end up trying to fill this overly large congestion window, you may experience a large amount of loss. The loss will result in the congestion window decreasing though, so it is self-correcting, but ideally we would avoid that loss in the first place. Overall though, we would prefer to ensure the congestion window can grow fast enough, even if there is a slightly greater chance of loss as a result. |
Description of changes:
When a path is considered "application-limited", meaning the sending rate is constrained by the rate at which the application is providing data to transmit and not constrained by the congestion controller, the CUBIC congestion controller does not grow the congestion window. In the existing implementation, the determination of "application-limited" does not consider whether the application has additional data to transmit and only stopped sending due to the pacer. This results in the possibility of not growing the congestion window when acknowledgements are received after sending is temporarily blocked by the pacer.
This change makes a determination on whether the path is application limited external to the congestion controller. If there is no additional data waiting to be sent and the congestion controller is not blocking further transmission, the path is considered application limited.
Before:
After:
Call-outs:
Early in a connection lifetime, there is a possibility of multiple packet spaces writing packets to a single datagram. This means that each packet space might independently be "application-limited", but as a whole, the path is not. To simplify this case, the
InitialandHandshakepacket spaces do not make a determination of whether they are application-limited.Ideally we would be able to determine if a path is application-limited based on the total amount of data the application has added to the send buffer, so that we don't allow the congestion window to grow if the application did not have enough data to fully utilize the congestion window, even if sending was paused momentarily due to pacing. This would require an expensive iteration over all streams in a connection, so as a compromise, we consider the path not application-limited if there is -any- data remaining to send while interrupted by pacing. The effect of this is there is some possibility for the congestion window to grow even if the application does not fully utilize the congestion window.
Testing:
Added unit tests and updated the recovery simulations
Is this a refactor change? If so, how have you proved that the intended behavior hasn't changed? -->
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.