Merge pull request #1578 from private-octopus/wifi-hiccup-bbr

Restore Cwin in BBR after suspension
private-octopus · Nov 15, 2023 · b6daa94 · b6daa94
2 parents 3130311 + c2539a3
commit b6daa94
Show file tree

Hide file tree

Showing 8 changed files with 128 additions and 66 deletions.
diff --git a/UnitTest1/unittest1.cpp b/UnitTest1/unittest1.cpp
@@ -2532,13 +2532,11 @@ namespace UnitTest1
             Assert::AreEqual(ret, 0);
         }
 
-#if 0
         TEST_METHOD(h09_multi_file_preemptive) {
             int ret = h09_multi_file_preemptive_test();
 
             Assert::AreEqual(ret, 0);
         }
-#endif
 
         TEST_METHOD(h3_multi_file) {
             int ret = h3_multi_file_test();
@@ -2552,13 +2550,11 @@ namespace UnitTest1
             Assert::AreEqual(ret, 0);
         }
 
-#if 0
         TEST_METHOD(h3_multi_file_preemptive) {
             int ret = h3_multi_file_preemptive_test();
 
             Assert::AreEqual(ret, 0);
         }
-#endif
 
         TEST_METHOD(h3zero_settings) {
             int ret = h3zero_settings_test();

diff --git a/picohttp_t/picohttp_t.c b/picohttp_t/picohttp_t.c
@@ -74,12 +74,12 @@ static const picoquic_test_def_t test_table[] = {
     { "h3_long_file_name", h3_long_file_name_test },
     { "h3_multi_file", h3_multi_file_test },
     { "h3_multi_file_loss", h3_multi_file_loss_test },
-#if 0
+#if 1
     { "h3_multi_file_preemptive", h3_multi_file_preemptive_test },
 #endif
     { "h09_multi_file", h09_multi_file_test },
     { "h09_multi_file_loss", h09_multi_file_loss_test },
-#if 0
+#if 1
     { "h09_multi_file_preemptive", h09_multi_file_preemptive_test },
 #endif
     { "h3zero_settings", h3zero_settings_test },

diff --git a/picoquic/bbr.c b/picoquic/bbr.c
@@ -184,6 +184,30 @@ Hystart instead of startup if the RTT is above the Reno target of
  * timeout, the timeout will still be handled.
  */
 
+/*
+* Handling of suspension
+* 
+* After a timeout, the path is suspended, and the congestion window is
+* immediately reduced. If do not do anything in particular, the
+* suspended state will be cleared on the first next acknowledgement,
+* and the congestion window will be restored gradually.
+* 
+* This is correct in general, when the timeout is due to some series
+* of packet loss events. It is not so good in the particular case of
+* Wi-Fi suspension, when the timeout is caused by the Wi-Fi link
+* being "suspended" for the time needed to scan other channels. In that
+* case, the code will receive a "spurious time out" notification,
+* typically triggered when an ACK queued "in the network" is delivered
+* when transmission resume. Waiting for the next ACK has two
+* downsides:
+* 
+* - it comes some times later, something like 1/2 RTT to 1 full RTT.
+* - the CWIND is lower than if the suspension had not happened.
+*
+* The reasonable solution is to exit the suspended state upon
+* notification of spurious reset, and restore the prior cwin.
+*/
+
 typedef enum {
     picoquic_bbr_alg_startup = 0,
     picoquic_bbr_alg_drain,
@@ -252,9 +276,9 @@ typedef struct st_picoquic_bbr_state_t {
     uint64_t previous_sampling_lost;
     uint64_t loss_interval_start; /* Time in microsec when last loss considered */
     uint64_t congestion_sequence; /* sequence number after congestion notification */
+    uint64_t cwin_before_suspension; /* So it can be restored if suspension stops. */
 
     uint64_t wifi_shadow_rtt; /* Shadow RTT used for wifi connections. */
-
     unsigned int filled_pipe : 1;
     unsigned int round_start : 1;
     unsigned int rt_prop_expired : 1;
@@ -266,6 +290,7 @@ typedef struct st_picoquic_bbr_state_t {
     unsigned int lt_is_sampling : 1;
     unsigned int last_loss_was_timeout : 1;
     unsigned int cycle_on_loss : 1;
+    unsigned int is_suspended;
 
 } picoquic_bbr_state_t;
 
@@ -1022,6 +1047,10 @@ void picoquic_bbr_notify_congestion(
     }
     path_x->cwin = path_x->cwin / 2;
     if (is_timeout || path_x->cwin < PICOQUIC_CWIN_MINIMUM) {
+        if (!bbr_state->is_suspended) {
+            bbr_state->is_suspended = 1;
+            bbr_state->cwin_before_suspension = path_x->cwin;
+        }
         path_x->cwin = PICOQUIC_CWIN_MINIMUM;
     }
 
@@ -1042,6 +1071,25 @@ void picoquic_bbr_notify_congestion(
     }
 }
 
+/*
+* Exit from suspension, after notification of spurious repeat.
+*/
+void picoquic_bbr_exit_suspension(
+    picoquic_bbr_state_t* bbr_state,
+    picoquic_cnx_t * cnx,
+    picoquic_path_t* path_x,
+    uint64_t lost_packet_number)
+{
+    if (bbr_state->is_suspended &&
+        bbr_state->cwin_before_suspension > 0) {
+        if (bbr_state->congestion_sequence >= lost_packet_number) {
+            bbr_state->is_suspended = 0;
+            path_x->cwin = bbr_state->cwin_before_suspension;
+            /* Set the pacing rate in picoquic sender */
+            picoquic_update_pacing_rate(cnx, path_x, bbr_state->pacing_rate, bbr_state->send_quantum);
+        }
+    }
+}
 
 /*
  * In order to implement BBR, we map generic congestion notification
@@ -1067,6 +1115,9 @@ static void picoquic_bbr_notify(
         switch (notification) {
         case picoquic_congestion_notification_acknowledgement:
             /* sum the amount of data acked per packet */
+            if (bbr_state->is_suspended) {
+                bbr_state->is_suspended = 0;
+            }
             bbr_state->bytes_delivered += nb_bytes_acknowledged;
             break;
         case picoquic_congestion_notification_ecn_ec:
@@ -1085,6 +1136,9 @@ static void picoquic_bbr_notify(
             }
             break;
         case picoquic_congestion_notification_spurious_repeat:
+            if (bbr_state->is_suspended) {
+                picoquic_bbr_exit_suspension(bbr_state, cnx, path_x, lost_packet_number);
+            }
             break;
         case picoquic_congestion_notification_rtt_measurement:
             if (bbr_state->state == picoquic_bbr_alg_startup && path_x->rtt_min > BBR_HYSTART_THRESHOLD_RTT) {

diff --git a/picoquic/loss_recovery.c b/picoquic/loss_recovery.c
@@ -922,7 +922,7 @@ static void picoquic_count_and_notify_loss(
         if (cnx->congestion_alg != NULL && cnx->cnx_state >= picoquic_state_ready && old_p->send_path != NULL) {
             cnx->congestion_alg->alg_notify(cnx, old_p->send_path,
                 (timer_based_retransmit == 0) ? picoquic_congestion_notification_repeat : picoquic_congestion_notification_timeout,
-                0, 0, 0, old_p->sequence_number, current_time);
+                0, 0, 0, old_p->path_packet_number, current_time);
         }
     }
 }

diff --git a/picoquictest/app_limited.c b/picoquictest/app_limited.c
@@ -607,8 +607,8 @@ int app_limited_rpr_test()
     config.ccalgo = picoquic_cubic_algorithm;
     config.do_preemptive_repeat = 1;
     config.loss_mask = 0x1482481224818214ull;
-    config.completion_target = 39000000;
-    config.nb_losses_max = 1960;
+    config.completion_target = 44000000;
+    config.nb_losses_max = 1980;
     config.rtt_max = 275000;
 
     return app_limited_test_one(&config);

diff --git a/picoquictest/h3zerotest.c b/picoquictest/h3zerotest.c
@@ -2577,15 +2577,17 @@ int h3_multi_file_test()
     return http_multi_file_test_one(PICOHTTP_ALPN_H3_LATEST, h3zero_callback, 0, 0);
 }
 
+#define H3ZERO_MULTI_LOSS_PATTERN 0xa242EDB710000ull
+
 int h3_multi_file_loss_test()
 {
-    uint64_t loss_pattern = 0xa243FFB700000ull;
+    uint64_t loss_pattern = H3ZERO_MULTI_LOSS_PATTERN;
     return http_multi_file_test_one(PICOHTTP_ALPN_H3_LATEST, h3zero_callback, loss_pattern, 0);
 }
 
 int h3_multi_file_preemptive_test()
 {
-    uint64_t loss_pattern = 0xa243FFB700000ull;
+    uint64_t loss_pattern = H3ZERO_MULTI_LOSS_PATTERN;
     return http_multi_file_test_one(PICOHTTP_ALPN_H3_LATEST, h3zero_callback, loss_pattern, 1);
 }
 
@@ -2596,14 +2598,14 @@ int h09_multi_file_test()
 
 int h09_multi_file_loss_test()
 {
-    uint64_t loss_pattern = 0xa243FFB700000ull; 
+    uint64_t loss_pattern = H3ZERO_MULTI_LOSS_PATTERN;
     return http_multi_file_test_one(PICOHTTP_ALPN_HQ_LATEST, picoquic_h09_server_callback, 
         loss_pattern, 0);
 }
 
 int h09_multi_file_preemptive_test()
 {
-    uint64_t loss_pattern = 0xa243FFB700000ull;
+    uint64_t loss_pattern = H3ZERO_MULTI_LOSS_PATTERN;
     return http_multi_file_test_one(PICOHTTP_ALPN_HQ_LATEST, picoquic_h09_server_callback,
         loss_pattern, 1);
 }

diff --git a/picoquictest/path_quality_server_ref.txt b/picoquictest/path_quality_server_ref.txt
@@ -59,61 +59,71 @@ Time, Path-ID, Event, Pacing_rate, Receive Rate, CWIN, RTT
 158446, 1, 23, 3614021, 40507, 77262, 26723
 163050, 1, 23, 3665388, 40507, 82958, 28291
 168805, 1, 23, 2941418, 40507, 90078, 30624
-169956, 0, 23, 533321, 42902, 10666, 39455
 171109, 1, 23, 2858869, 40507, 90123, 31524
 173413, 1, 23, 2808740, 40507, 90169, 32103
 174564, 1, 23, 2753587, 40507, 90191, 32754
+174564, 0, 23, 1056260, 42902, 42130, 39886
 176866, 1, 23, 2656764, 40507, 90237, 33965
 179164, 1, 23, 2567601, 40507, 90282, 35162
 181466, 1, 23, 2485088, 40507, 90328, 36348
 184443, 1, 23, 2416002, 40507, 90373, 37406
-186745, 1, 23, 595605, 40507, 22689, 38094
-199250, 0, 23, 533321, 42902, 6977, 33994
-287383, 0, 23, 392761, 12023, 12024, 30614
-299345, 1, 23, 595605, 11296, 10918, 32831
-319353, 1, 23, 360238, 8069, 11827, 32831
-319353, 0, 23, 449047, 8481, 13017, 28988
-345568, 1, 23, 410899, 8595, 12991, 31616
-345568, 0, 23, 505148, 7927, 14228, 28166
-369590, 0, 23, 561438, 9321, 15343, 27328
-374800, 1, 23, 484828, 8595, 14205, 29299
-391794, 0, 23, 616592, 9321, 16381, 26567
-403600, 1, 23, 545438, 9432, 15455, 28335
-409378, 0, 23, 667756, 9731, 17357, 25993
-431882, 0, 23, 728704, 9445, 18504, 25393
-431882, 1, 23, 605624, 9548, 16734, 27631
-447334, 0, 23, 789339, 9445, 19267, 24409
-447334, 1, 23, 669222, 9723, 17456, 26084
-457389, 1, 23, 723233, 9723, 18036, 24938
-468865, 0, 23, 851817, 10406, 20407, 23957
-480128, 1, 23, 781688, 10781, 19142, 24488
-485406, 0, 23, 910895, 10054, 21294, 23377
-490684, 1, 23, 833902, 9815, 19776, 23715
-506343, 1, 23, 894211, 9815, 20591, 23027
-506343, 0, 23, 968222, 12401, 22425, 23161
-522446, 0, 23, 1022761, 12401, 23411, 22890
-527510, 1, 23, 958408, 10490, 21661, 22601
-548661, 0, 23, 1077080, 10490, 24775, 23002
-553725, 1, 23, 1010882, 10490, 23129, 22880
-585235, 1, 23, 1064717, 10490, 24842, 23332
-585235, 0, 23, 1127672, 10490, 26692, 23670
-616481, 1, 23, 1122839, 10490, 26444, 23551
-616481, 0, 23, 1187942, 10490, 28336, 23853
-647974, 1, 23, 1173473, 10476, 28100, 23946
-647974, 0, 23, 1242678, 10490, 29956, 24106
-679736, 1, 23, 1223901, 10490, 29802, 24350
-679736, 0, 23, 1297093, 10490, 31688, 24430
-711015, 1, 23, 1274115, 10490, 31542, 24756
-782580, 1, 23, 1327607, 5087, 33623, 25326
-782580, 0, 23, 1356542, 13157, 34836, 25680
-799845, 1, 23, 1381838, 9556, 34528, 24987
-799845, 0, 23, 1411590, 9556, 35709, 25297
-817110, 1, 23, 1433812, 9556, 35408, 24695
-914945, 0, 23, 1463688, 9556, 41055, 28049
-955230, 1, 23, 1485795, 9556, 41788, 28125
-960985, 0, 23, 1520684, 9556, 43008, 28282
-972495, 0, 23, 1466014, 9556, 43482, 29660
-995515, 0, 23, 1409047, 11149, 44416, 31522
-1007025, 1, 23, 1431288, 9556, 43942, 30701
-1270480, 1, 22,
-1270480, 0, 22,
+186745, 1, 23, 1187667, 40507, 45243, 38094
+194931, 0, 23, 1130382, 42902, 43002, 38042
+196082, 0, 23, 1191832, 42902, 43049, 36120
+198384, 0, 23, 1247614, 42902, 43145, 34582
+201842, 0, 23, 1333015, 48819, 43287, 32473
+206451, 0, 23, 1384630, 48819, 43476, 31399
+209904, 1, 23, 1136925, 46044, 46141, 40584
+211053, 0, 23, 1435371, 48819, 43664, 30420
+215649, 0, 23, 1491818, 48819, 43852, 29395
+221404, 0, 23, 1548417, 48819, 44085, 28471
+224857, 1, 23, 1187816, 46044, 46718, 39331
+225290, 1, 23, 1252162, 46044, 46762, 37345
+226441, 1, 23, 1309194, 40006, 46805, 35751
+228743, 1, 23, 1364914, 40006, 46893, 34356
+228743, 0, 23, 1599048, 48819, 44364, 27744
+240287, 1, 23, 1429244, 40006, 47328, 33114
+257552, 1, 23, 1501063, 40006, 47974, 31960
+269062, 0, 23, 1545451, 9545, 45954, 29735
+274817, 1, 23, 1573324, 11933, 48611, 30897
+286327, 0, 23, 1489139, 9545, 46619, 31306
+292082, 1, 23, 1631273, 11933, 49240, 30185
+309390, 1, 23, 1577430, 9556, 49861, 31609
+326698, 1, 23, 1520331, 9556, 50475, 33200
+338208, 1, 23, 1463288, 9533, 50880, 34771
+361314, 1, 23, 1408393, 9533, 51681, 36695
+378622, 1, 23, 699846, 9536, 26429, 37764
+401685, 0, 23, 1437873, 9533, 50836, 35355
+407234, 1, 23, 771265, 9536, 28307, 36702
+430252, 1, 23, 835477, 9585, 29164, 34907
+430252, 0, 23, 1383927, 9536, 51835, 37455
+441762, 1, 23, 887392, 9585, 29583, 33337
+458819, 1, 23, 947001, 9557, 30269, 31963
+469594, 1, 23, 1037510, 9557, 30674, 29565
+469594, 0, 23, 697099, 9606, 26869, 38544
+479953, 1, 23, 1108328, 10024, 31072, 28035
+490312, 1, 23, 1159948, 10024, 31597, 27240
+490312, 0, 23, 748203, 10156, 28211, 37705
+500947, 0, 23, 807188, 10156, 28790, 35667
+500947, 1, 23, 1210409, 10024, 32048, 26477
+511349, 0, 23, 872848, 10156, 29003, 33228
+521708, 0, 23, 950232, 10156, 29003, 30522
+527212, 1, 23, 1261732, 10464, 32048, 25400
+558135, 0, 23, 1014932, 12976, 29635, 29199
+580947, 1, 23, 1322551, 5957, 33856, 25599
+580947, 0, 23, 1082180, 12976, 30656, 28328
+598212, 1, 23, 1378127, 9626, 34755, 25219
+603967, 0, 23, 1151482, 9626, 31774, 27594
+620375, 0, 23, 1205620, 9626, 32477, 26938
+620375, 1, 23, 1428554, 9626, 35861, 25103
+630734, 0, 23, 1258417, 9626, 33041, 26256
+653797, 0, 23, 1328916, 10194, 34201, 25736
+676609, 0, 23, 1395504, 9626, 35323, 25312
+699629, 0, 23, 1460801, 9542, 36466, 24963
+751424, 1, 23, 1481518, 9556, 41884, 28271
+808974, 1, 23, 1423298, 9556, 44266, 31101
+837792, 0, 23, 1512989, 11468, 42689, 28215
+866610, 0, 23, 1450110, 9542, 43876, 30257
+895428, 0, 23, 1395920, 9533, 45031, 32259
+1064552, 1, 22,
+1064552, 0, 22,
diff --git a/picoquictest/tls_api_test.c b/picoquictest/tls_api_test.c
@@ -4744,7 +4744,7 @@ int mtu_drop_test()
         picoquic_bbr_algorithm
     };
     uint64_t algo_time[5] = {
-        11100000,
+        11600000,
         9450000,
         9200000,
         11400000,