Skip to content

Commit c8499e8

Browse files
omansfeldomansfeld
committed
feat(cc): Finished scoping cubic update in code comments.
1 parent 26512cb commit c8499e8

File tree

2 files changed

+105
-14
lines changed

2 files changed

+105
-14
lines changed

‎neqo-transport/src/cc/classic_cc.rs

+4
Original file line numberDiff line numberDiff line change
@@ -552,6 +552,10 @@ impl<T: WindowAdjustment> ClassicCongestionControl<T> {
552552
self.acked_bytes,
553553
self.max_datagram_size(),
554554
);
555+
// UPDATE: Add condition for `self.cc_algorithm == cubic` here to set `ssthresh` and
556+
// `congestion_window` according to RFC 9438.
557+
//
558+
// <https://datatracker.ietf.org/doc/html/rfc9438#figure-5>
555559
self.congestion_window = max(cwnd, self.cwnd_min());
556560
self.acked_bytes = acked_bytes;
557561
self.ssthresh = self.congestion_window;

‎neqo-transport/src/cc/cubic.rs

+101-14
Original file line numberDiff line numberDiff line change
@@ -75,14 +75,14 @@ pub const CUBIC_FAST_CONVERGENCE: f64 = 0.85; // (1.0 + CUBIC_BETA) / 2.0;
7575
/// this value reduces the magnitude of the resulting growth by a constant factor.
7676
/// A value of 1.0 would mean a return to the rate used in slow start.
7777
///
78-
/// UPDATE: Not found in RFC. What is the reason for this and should we reconsider?
78+
/// UPDATE: Not found in RFC. I don't exactly understand why we're doing this?
7979
const EXPONENTIAL_GROWTH_REDUCTION: f64 = 2.0;
8080

8181
/// Convert an integer congestion window value into a floating point value.
8282
/// This has the effect of reducing larger values to `1<<53`.
8383
/// If you have a congestion window that large, something is probably wrong.
8484
///
85-
/// UPDATE: not found in RFC.
85+
/// UPDATE: not part of RFC.
8686
pub fn convert_to_f64(v: usize) -> f64 {
8787
let mut f_64 = f64::from(u32::try_from(v >> 21).unwrap_or(u32::MAX));
8888
f_64 *= 2_097_152.0; // f_64 <<= 21
@@ -302,15 +302,17 @@ impl WindowAdjustment for Cubic {
302302
// <https://datatracker.ietf.org/doc/html/rfc9438#section-4.2-11.1>
303303
//
304304
// And the `cwnd` increase of `target - cwnd / cwnd` only applies here,
305-
// not in the Reno-friendly region. So that needs to be adjusted. See wording:
305+
// not in the Reno-friendly region. So that needs to be adjusted. Right now
306+
// we are doing the `target - cwnd / cwnd` part for all regions I think.
307+
// See wording:
306308
//
307-
// > cwnd SHOULD be set to w_est
309+
// > cwnd SHOULD be set to w_est ("set to")
308310
//
309311
// <https://datatracker.ietf.org/doc/html/rfc9438#section-4.3-8>
310312
//
311313
// vs.
312314
//
313-
// > cwnd MUST be incremented by `target - cwnd / cwnd`
315+
// > cwnd MUST be incremented by `target - cwnd / cwnd` ("incremented by")
314316
//
315317
// <https://datatracker.ietf.org/doc/html/rfc9438#name-convex-region>
316318
let time_ca = self
@@ -331,9 +333,17 @@ impl WindowAdjustment for Cubic {
331333
//
332334
// <https://datatracker.ietf.org/doc/html/rfc9438#name-reno-friendly-region>
333335
//
334-
// UPDATE: This is handled differently in the new RFC, but it's also
335-
// different from the original RFC. Still need to understand what exactly
336-
// is going on here.
336+
// UPDATE/QUESTION: This is handled differently in the new RFC, but our implementation is also
337+
// different from the original RFC. Still need to understand what exactly is going on here.
338+
//
339+
// Also note:
340+
//
341+
// > Once w_est has grown to reach the cwnd at the time of most recently setting
342+
// > ssthresh -- that is, w_est >= cwnd_prior -- the sender SHOULD set CUBIC_ALPHA to
343+
// > 1 to ensure that it can achieve the same congestion window increment rate
344+
// > as Reno, which uses AIMD(1, 0.5).
345+
//
346+
// <https://datatracker.ietf.org/doc/html/rfc9438#section-4.3-11>
337347
let max_datagram_size = convert_to_f64(max_datagram_size);
338348
let tcp_cnt = self.estimated_tcp_cwnd / CUBIC_ALPHA;
339349
let incr = (self.tcp_acked_bytes / tcp_cnt).floor();
@@ -345,7 +355,7 @@ impl WindowAdjustment for Cubic {
345355
// Take the larger cwnd of Cubic concave or convex and Cubic Reno-friendly region.
346356
//
347357
// > When receiving a new ACK in congestion avoidance (where cwnd could be greater than
348-
// > or less than Wmax), CUBIC checks whether Wcubic(t) is less than w_est. If so, CUBIC
358+
// > or less than w_max), CUBIC checks whether W_cubic(t) is less than w_est. If so, CUBIC
349359
// > is in the Reno-friendly region and cwnd SHOULD be set to w_est at each reception of a new ACK.
350360
//
351361
// <https://datatracker.ietf.org/doc/html/rfc9438#section-4.3-8>
@@ -377,6 +387,41 @@ impl WindowAdjustment for Cubic {
377387
acked_to_increase as usize
378388
}
379389

390+
// UPDATE: CUBIC RFC 9438 changes the logic for multiplicative decrease and uses
391+
// `bytes_in_flight` instead of `cwnd` for it's calculation:
392+
//
393+
// > ssthresh = bytes_in_flight * CUBIC_BETA
394+
// > cwnd_prior = cwnd
395+
// > if (reduction_on_loss) {
396+
// > cwnd = max(ssthresh, 2*MSS)
397+
// > } else if (reduction_on_ece) {
398+
// > cwnd = max(ssthresh, 1*MSS)
399+
// > }
400+
// > ssthresh = max(ssthresh, 2*MSS)
401+
//
402+
// <https://datatracker.ietf.org/doc/html/rfc9438#figure-5>
403+
//
404+
// With the note:
405+
//
406+
// > A QUIC sender that uses a cwnd value to calculate new values for cwnd and ssthresh after
407+
// > detecting a congestion event is REQUIRED to apply similar mechanisms [RFC9002].
408+
//
409+
// "similar mechanisms" refering to taking "measures to prevent cwnd from growing when the
410+
// volume of bytes in flight is smaller than cwnd".
411+
//
412+
// QUESTION: Do we already do this somewhere?
413+
//
414+
// <https://datatracker.ietf.org/doc/html/rfc9438#section-4.6>
415+
//
416+
// We are setting ssthresh in `on_congestion_event()` in `classic_cc.rs` after having returned `cwnd`
417+
// from `reduce_cwnd()` below. That is code that isn't CUBIC specific though, so we can add a condition
418+
// there to check which cc algorithm we're using and do the `ssthresh|cwnd = max(...)` outlined above
419+
// for CUBIC only.
420+
//
421+
// (or maybe don't do it at all because QUIC has a minimum congestion window of 2*MSS, as mentioned
422+
// further below)
423+
// QUESTION: Which takes precedence? The minimum congestion window of 2*MSS for QUIC or CUBIC wanting to set
424+
// cwnd to 1*MSS on reduction by ECE?
380425
fn reduce_cwnd(
381426
&mut self,
382427
curr_cwnd: usize,
@@ -386,12 +431,26 @@ impl WindowAdjustment for Cubic {
386431
let curr_cwnd_f64 = convert_to_f64(curr_cwnd);
387432
// Fast Convergence
388433
//
389-
// If congestion event occurs before the maximum congestion window before the last
390-
// congestion event, we reduce the the maximum congestion window and thereby W_max.
391-
// check cwnd + MAX_DATAGRAM_SIZE instead of cwnd because with cwnd in bytes, cwnd may be
434+
// > During a congestion event, if the current cwnd is less than w_max, this indicates
435+
// > that the saturation point experienced by this flow is getting reduced because of
436+
// > a change in available bandwidth. This flow can then release more bandwidth by
437+
// > reducing w_max further. This action effectively lengthens the time for this flow
438+
// > to increase its congestion window, because the reduced w_max forces the flow to
439+
// > plateau earlier. This allows more time for the new flow to catch up to its
440+
// > congestion window size.
441+
//
442+
// Check cwnd + MAX_DATAGRAM_SIZE instead of cwnd because with cwnd in bytes, cwnd may be
392443
// slightly off.
393444
//
394-
// <https://www.rfc-editor.org/rfc/rfc8312#section-4.6>
445+
// UPDATE: New logic for fast convergence.
446+
//
447+
// if (cwnd < w_max AND fast_convergence_enabled) {
448+
// w_max = cwnd * CUBIC_FAST_CONVERGENCE;
449+
// } else {
450+
// w_max = cwnd;
451+
// }
452+
//
453+
// <https://datatracker.ietf.org/doc/html/rfc9438#name-fast-convergence>
395454
self.last_max_cwnd =
396455
if curr_cwnd_f64 + convert_to_f64(max_datagram_size) < self.last_max_cwnd {
397456
curr_cwnd_f64 * CUBIC_FAST_CONVERGENCE
@@ -424,4 +483,32 @@ impl WindowAdjustment for Cubic {
424483

425484
// UPDATE: Things to remember that didn't make it anywhere else:
426485
//
427-
// - what about `ssthresh`
486+
// 1. Minimum congestion window:
487+
//
488+
// > Note that CUBIC MUST continue to reduce cwnd in response to congestion events
489+
// > detected by ECN-Echo ACKs until it reaches a value of 1 SMSS. If congestion events
490+
// > indicated by ECN-Echo ACKs persist, a sender with a cwnd of 1 SMSS MUST reduce its
491+
// > sending rate even further. This can be achieved by using a retransmission timer
492+
// > with exponential backoff, as described in [RFC3168].
493+
//
494+
// <https://datatracker.ietf.org/doc/html/rfc9438#section-4.6-7> VERSUS
495+
//
496+
// > QUIC therefore recommends that the minimum congestion window be two packets.
497+
//
498+
// <https://datatracker.ietf.org/doc/html/rfc9002#section-4.8>
499+
//
500+
// QUESTION: So this probably does not apply to CUBIC on QUIC and we never go below 2*MSS
501+
// (as is currently implemented)?
502+
//
503+
// 2. Timeout: <https://datatracker.ietf.org/doc/html/rfc9438#section-4.8>
504+
//
505+
// > QUIC does not collapse the congestion window when the PTO expires, since a single
506+
// > packet loss at the tail does not indicate persistent congestion.
507+
//
508+
// <https://datatracker.ietf.org/doc/html/rfc9002#section-4.7>
509+
//
510+
// QUESTION: Timeout (RTO/PTO) does not apply to us then?
511+
//
512+
// 3. Spurious fast retransmits: <https://datatracker.ietf.org/doc/html/rfc9438#section-4.9.2>
513+
//
514+
// QUESTION: Do we want to implement this? If yes then we may be able to do it in a follow up PR.

0 commit comments

Comments
 (0)