Reimplement recovery and congestion control #1742
Conversation
vkrasnov
force-pushed
the
vlad/gcongestion_split
branch
3 times, most recently
from
2ca0301
to
6575ed1
Compare
|
needs a rebase |
|
|
||
| trace!("{} packet newly acked {}", trace_id, unacked.pkt_num); |
quiche/src/recovery/rtt.rs
Outdated
| &mut self, latest_rtt: Duration, mut ack_delay: Duration, now: Instant, | ||
| handshake_confirmed: bool, |
There was a problem hiding this comment.
Making the ack_delay mutable and changing it based on handshake status seems to be a functional change
There was a problem hiding this comment.
It just does what the RFC says, and adjusts for max_ack_delay only after handshake is complete, and not unconditionally
There was a problem hiding this comment.
If we believe there is a functional bug in the current implementation, then tracking and fixing that independent of refactor is a lot clearer.
There was a problem hiding this comment.
Up to you if you think it is a bug or not
| pub(super) min_rtt: Minmax<Duration>, | ||
| pub(super) smoothed_rtt: Duration, | ||
| pub(super) rttvar: Duration, | ||
| pub(super) first_rtt_sample: Option<Instant>, |
There was a problem hiding this comment.
AFAICT the value of first_rtt_samle is never used anywhere, so maybe just needs to be a bool?
| } | ||
|
|
||
| #[cfg(test)] | ||
| pub(crate) mod test_sender { |
There was a problem hiding this comment.
Having this in the Reno CC module seems a bit weird when it's used by other of CC modules for tests
There was a problem hiding this comment.
I agree, but then again all other modules use Reno at this point
| @@ -1286,6 +1285,9 @@ pub struct Connection { | |||
| /// Packet number spaces. | |||
| pkt_num_spaces: [packet::PktNumSpace; packet::Epoch::count()], | |||
|
|
|||
| /// Next packet number | |||
| next_pkt_num: u64, | |||
There was a problem hiding this comment.
It looks like this merges the packet numbers from all packet number spaces into the same sequence, which is a significant functional changes, see https://datatracker.ietf.org/doc/html/rfc9000#section-12.3-8
There was a problem hiding this comment.
I wouldn't call it a functional change, the RFC from what I can tell does not forbid merging the packet numbers, as long as they are not reused, and it simplifies the CC packet number accounting, as it no longer needs to take epoch into concideration
quiche/src/recovery/mod.rs
Outdated
| } else { | ||
| Duration::from_micros(0) | ||
| }; | ||
| let ack_delay = Duration::from_micros(ack_delay); |
There was a problem hiding this comment.
This also seems like a functional change since it will affect how update_rtt works, albeit related to a79dbfa#r1555902791
Simplify the code by performing CC ops across epochs.
This creates a clearer separation between congestion and recovery.
rebased, and fixed a few issues in the process |
vkrasnov
force-pushed
the
vlad/gcongestion_split
branch
from
444da98
to
e452b00
Compare
vkrasnov
force-pushed
the
vlad/gcongestion_split
branch
from
e452b00
to
b4d82b0
Compare
…ion control implementations
vkrasnov
force-pushed
the
vlad/gcongestion_split
branch
from
b4d82b0
to
df50522
Compare
This change replaces the CC and pacing algorithms using the C++ google/quiche implementations rewritten in Rust.
In addition it improves on the general recovery as well.
This is essentially the same as #1490 only split into more incremental commits for easier review.