(D)CUBIC, NewReno and Prague refactoring #1818
+494
−491
There are no files selected for viewing
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
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -34,6 +34,7 @@ uint64_t picoquic_cc_get_sequence_number(picoquic_cnx_t* cnx, picoquic_path_t* p | |
| else { | ||
| sequence_number = cnx->pkt_ctx[picoquic_packet_context_application].send_sequence; | ||
| } | ||
|
|
||
| return sequence_number; | ||
| } | ||
|
|
||
|
|
@@ -47,6 +48,7 @@ uint64_t picoquic_cc_get_ack_number(picoquic_cnx_t* cnx, picoquic_path_t* path_x | |
| else { | ||
| highest_acknowledged = cnx->pkt_ctx[picoquic_packet_context_application].highest_acknowledged; | ||
| } | ||
|
|
||
| return highest_acknowledged; | ||
| } | ||
|
|
||
|
|
@@ -60,16 +62,16 @@ uint64_t picoquic_cc_get_ack_sent_time(picoquic_cnx_t* cnx, picoquic_path_t* pat | |
| else { | ||
| latest_time_acknowledged = cnx->pkt_ctx[picoquic_packet_context_application].latest_time_acknowledged; | ||
| } | ||
|
|
||
| return latest_time_acknowledged; | ||
| } | ||
|
|
||
|
|
||
| void picoquic_cc_filter_rtt_min_max(picoquic_min_max_rtt_t * rtt_track, uint64_t rtt) | ||
| { | ||
| int x = rtt_track->sample_current; | ||
| int x_max; | ||
|
|
||
| rtt_track->samples[x] = rtt; | ||
|
|
||
| rtt_track->sample_current = x + 1; | ||
|
|
@@ -92,7 +94,7 @@ void picoquic_filter_rtt_min_max(picoquic_min_max_rtt_t * rtt_track, uint64_t rt | |
| } | ||
| } | ||
|
|
||
| int picoquic_cc_hystart_loss_test(picoquic_min_max_rtt_t* rtt_track, picoquic_congestion_notification_t event, | ||
| uint64_t lost_packet_number, double error_rate_max) | ||
| { | ||
| int ret = 0; | ||
|
|
@@ -125,7 +127,7 @@ int picoquic_hystart_loss_test(picoquic_min_max_rtt_t* rtt_track, picoquic_conge | |
| return ret; | ||
| } | ||
|
|
||
| int picoquic_cc_hystart_loss_volume_test(picoquic_min_max_rtt_t* rtt_track, picoquic_congestion_notification_t event, uint64_t nb_bytes_newly_acked, uint64_t nb_bytes_newly_lost) | ||
| { | ||
| int ret = 0; | ||
|
|
||
|
|
@@ -154,12 +156,12 @@ int picoquic_hystart_loss_volume_test(picoquic_min_max_rtt_t* rtt_track, picoqui | |
| return ret; | ||
| } | ||
|
|
||
| int picoquic_cc_hystart_test(picoquic_min_max_rtt_t* rtt_track, uint64_t rtt_measurement, uint64_t packet_time, uint64_t current_time, int is_one_way_delay_enabled) | ||
| { | ||
| int ret = 0; | ||
|
|
||
| if(current_time > rtt_track->last_rtt_sample_time + 1000) { | ||
| picoquic_cc_filter_rtt_min_max(rtt_track, rtt_measurement); | ||
| rtt_track->last_rtt_sample_time = current_time; | ||
|
|
||
| if (rtt_track->is_init) { | ||
|
|
@@ -189,9 +191,85 @@ int picoquic_hystart_test(picoquic_min_max_rtt_t* rtt_track, uint64_t rtt_measur | |
| return ret; | ||
| } | ||
|
|
||
| uint64_t picoquic_cc_slow_start_increase(picoquic_path_t * path_x, uint64_t nb_delivered) { | ||
| /* App limited. */ | ||
| /* TODO discuss | ||
| * path_x->cwin < path_x->bytes_in_transit returns false in cc code | ||
| * path_x->cnx->cwin_blocked is set to true | ||
| * (path_x->cwin < path_x->bytes_in_transit) != path_x->cnx->cwin_blocked? | ||
| */ | ||
| if (!path_x->cnx->cwin_blocked) { | ||
| return 0; | ||
| } | ||
|
|
||
| return nb_delivered; | ||
| } | ||
|
|
||
| uint64_t picoquic_cc_slow_start_increase_ex(picoquic_path_t * path_x, uint64_t nb_delivered, int in_css) | ||
| { | ||
| if (in_css) { | ||
| /* In consecutive Slow Start. */ | ||
| return picoquic_cc_slow_start_increase(path_x, nb_delivered / PICOQUIC_HYSTART_PP_CSS_GROWTH_DIVISOR); | ||
| } | ||
|
|
||
| /* Fallback to traditional Slow Start. */ | ||
| return picoquic_cc_slow_start_increase(path_x, nb_delivered); /* nb_delivered; */ | ||
| } | ||
|
|
||
| uint64_t picoquic_cc_slow_start_increase_ex2(picoquic_path_t* path_x, uint64_t nb_delivered, int in_css, uint64_t prague_alpha) { | ||
| if (prague_alpha != 0) { /* monitoring of ECN */ | ||
| uint64_t delta = nb_delivered; | ||
|
|
||
| /* Calculate delta based on prague_ahpha. */ | ||
| if (path_x->smoothed_rtt <= PICOQUIC_TARGET_RENO_RTT) { | ||
| /* smoothed_rtt <= 100ms */ | ||
| delta *= (1024 - prague_alpha); | ||
| delta /= 1024; | ||
| } else { | ||
| delta *= path_x->smoothed_rtt; | ||
| delta *= (1024 - prague_alpha); | ||
| delta /= PICOQUIC_TARGET_RENO_RTT; | ||
| delta /= 1024; | ||
| } | ||
|
|
||
| return picoquic_cc_slow_start_increase_ex(path_x, delta, in_css); | ||
| } | ||
|
|
||
| /* Fallback to HyStart++ Consecutive Slow Start. */ | ||
| return picoquic_cc_slow_start_increase_ex(path_x, nb_delivered, in_css); | ||
| } | ||
|
|
||
| uint64_t picoquic_cc_bandwidth_estimation(picoquic_path_t* path_x) { | ||
| /* RTT measurements will happen after the bandwidth is estimated. */ | ||
| uint64_t max_win = path_x->peak_bandwidth_estimate * path_x->smoothed_rtt / 1000000; | ||
| uint64_t min_win = max_win / 2; | ||
|
|
||
| /* Return increased cwin, if larger than current cwin. */ | ||
| if (min_win > path_x->cwin) { | ||
| return min_win; | ||
| } | ||
|
|
||
| /* Otherwise, return current cwin. */ | ||
| return path_x->cwin; | ||
| } | ||
|
|
||
|
There was a problem hiding this comment. This function is not estimating bandwidth. It is estimating a value of cwin, similar to the "target cwin" used in BBR. Maybe rename to There was a problem hiding this comment. That's true. |
||
| uint64_t picoquic_cc_increase_cwin_for_long_rtt(picoquic_path_t * path_x) { | ||
| uint64_t min_cwnd; | ||
|
|
||
| if (path_x->rtt_min > PICOQUIC_TARGET_SATELLITE_RTT) { | ||
| min_cwnd = (uint64_t)((double)PICOQUIC_CWIN_INITIAL * (double)PICOQUIC_TARGET_SATELLITE_RTT / (double)PICOQUIC_TARGET_RENO_RTT); | ||
| } | ||
| else { | ||
| min_cwnd = (uint64_t)((double)PICOQUIC_CWIN_INITIAL * (double)path_x->rtt_min / (double)PICOQUIC_TARGET_RENO_RTT); | ||
| } | ||
|
|
||
| /* Return increased cwin, if larger than current cwin. */ | ||
| if (min_cwnd > path_x->cwin) { | ||
| return min_cwnd; | ||
| } | ||
|
|
||
| /* Otherwise, return current cwin. */ | ||
| return path_x->cwin; | ||
| } | ||
|
|
||
| uint64_t picoquic_cc_increased_window(picoquic_cnx_t* cnx, uint64_t previous_window) | ||
|
|
@@ -203,7 +281,7 @@ uint64_t picoquic_cc_increased_window(picoquic_cnx_t* cnx, uint64_t previous_win | |
| else { | ||
| double w = (double)previous_window; | ||
| w /= (double)PICOQUIC_TARGET_RENO_RTT; | ||
| w *= (cnx->path[0]->rtt_min > PICOQUIC_TARGET_SATELLITE_RTT) ? PICOQUIC_TARGET_SATELLITE_RTT : (double)cnx->path[0]->rtt_min; | ||
| new_window = (uint64_t)w; | ||
|
There was a problem hiding this comment. You copied the existing code, and that's fine. But it is a bit funky. It is means to open the window faster when the RTT is long, but since that we came out with "careful resume" that solves a large part of the problem when a previous bandwidth estimate is available, and then with increases based on peak bandwidth estimate, which also provides for rapid growth of cwin in these high delay links. So maybe we should add a note that this code should be revisited and perhaps removed. If we keep it, it would be nice to use integer arithmetic instead of (double). But let's keep the code as is for now. Maybe add a comment. There was a problem hiding this comment. In our careful resume (CR) implementation this part of the code is already removed. :) We submitted a paper about CR and I think it will be published in March. It focuses on CR and first measurements on satellite connections. (GEO, LEO, ...) I want it to share it, but I think the IEEE copyright forbids me to. Maybe I can share a pre-submitted version of it. I have to take a look. But yes, these part of code can be removed, but currently don't have to. |
||
| } | ||
| return new_window; | ||
|
|
||
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
Oops, something went wrong.
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this meant for hystart++ support?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it is.
I copied a part of my HyStart++ macros/constants to the code for CSS version of the increase function.
So the current code defines the recommended growth divisor (=4, as specified by the HyStart++ RFC) as a macro.
Another option is to change the
in_cssparameter to a double to specify the divisor from the outside of the function. (more flexible)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My next paper will be about HyStart++. I will share my results here. But I can't tell any date, because I'm very busy with careful resume currently.