Add support for 0RTT-BDP extension #1209

francklinSIMO · 2021-05-06T16:32:32Z

This PR implements the 0RTT BDP draft. It extends this PR Remember RTT and CWIn from previous connections to allow server to offload previous connections data on the client as defined by the draft.

The BDP option added to picoquicdemo ("-j") and picoquic API ("bdp_option" ) allows to choose :

(0) to disable BDP extension
(1) to enable BDP extension and store connections data locally in unilateral way as proposed in Remember RTT and CWIn from previous connections
(2) to enable BDP extension and store both client and server connections data on the client using BDP extension frame

The TP enable_bdp is defined at both client and server side to negociate the extension behavior

The BDP Frame is added to carry BDP parameters

Other related mecanisms especially to hold BDP parameters and to store them, to verify that the parameters have not changed and to interact with CCs are reused from Remember RTT and CWIn from previous connections

Testing with 250 Mpbs satellite connection, the simulation shows that a 100MB upload time with bdp_option = 0 drops from just under 6 seconds to just under 4.8 seconds when remembering the CWIN and RTT (bdp_option = 1 or 2).

Since the draft is still in progress, we (@NicoKos : https://github.com/NicoKos) thought this PR can be useful.

huitema

Nice work. We are on the right track. You will find lots of detailed comments in the review, but the main one is that this should be a "bdp_frame_option", leaving the current code that remembers data at the server side unchanged.

I appreciate that you added the tests!

huitema · 2021-05-07T04:13:12Z

picoquic/config.c

@@ -92,6 +92,7 @@ static option_table_line_t option_table[] = {
    { picoquic_option_Preemptive_Repeat, 'V', "preemptive_repeat", 0, "", "enable preemptive repeat" },
    { picoquic_option_Version_Upgrade, 'U', "version_upgrade", 1, "", "Version upgrade if server agrees, e.g. -U 00000002" },
    { picoquic_option_No_GSO, '0', "no_gso", 0, "", "Do not use UDP GSO or equivalent" },
+    { picoquic_option_BDP, 'j', "bdp", 1, "number", "bdp_option: disable bdp extension(0), store bdp parameters locally(1), use bdp extension frame(2). Default=0" },
    { picoquic_option_HELP, 'h', "help", 0, "This help message" }


I am not sure that this triple choice works. Over time, the "local" option can well evolve, for example to set a max delay after the previous connection, or to specify the maximum size of the LRU lists. When we do that, we will need to set a specific API and option. I would much rather keep the local store unchanged for now, and focus the present PR on just the BDP frame.

How about renaming the option to something like "picoquic_option_BDP_frame", to be more specific?

huitema · 2021-05-07T04:13:55Z

picoquic/config.c

+            config->bdp_option = v;
+        }
+        break;
+    }
    case picoquic_option_HELP:


See above, simplify to 1 (support BDP frame) and 0 (don't).

huitema · 2021-05-07T04:14:27Z

picoquic/config.c

@@ -828,6 +840,8 @@ picoquic_quic_t* picoquic_create_and_configure(picoquic_quic_config_t* config,
            }
        }

+        picoquic_set_default_bdp_option(quic, config->bdp_option);
+


Same issue, make that specific to the bdp frame.

huitema · 2021-05-07T04:15:38Z

picoquic/frames.c

@@ -2217,7 +2217,7 @@ void picoquic_update_path_rtt(picoquic_cnx_t* cnx, picoquic_path_t* old_path, pi
        }

        /* On first update, validate seeed data */
-        if (is_first && cnx->seed_cwin != 0){
+        if (is_first && cnx->seed_cwin != 0 && cnx->quic->default_bdp_option > 0){
            if (cnx->seed_rtt_min <= path_x->smoothed_rtt &&


I would rather leave the current code unchanged.

huitema · 2021-05-07T04:16:40Z

picoquic/frames.c

+    if ((bytes = picoquic_parse_bdp_frame(cnx, bytes, bytes_max, &lifetime, &recon_bytes_in_flight, &recon_min_rtt))  != NULL) {
+        /* Store received BDP */
+        if (cnx->quic->default_bdp_option == 2) {
+            if (cnx->client_mode) {


Simplify to yes or no test.

huitema · 2021-05-07T04:43:12Z

picoquic/picoquic_internal.h

@@ -549,6 +552,7 @@ typedef uint64_t picoquic_tp_enum;
 #define picoquic_tp_enable_multipath 0xbaba 
 #define picoquic_tp_enable_simple_multipath 0xbab5 
 #define picoquic_tp_version_negotiation 0x73db
+#define picoquic_tp_enable_bdp 0xbaba4 /* per draft-kuhn-quic-0rtt-bdp-09 */



See comment on picoquic_frame_type_enum_t. But really we should not be using the data type picoquic_frame_type_enum_t any more, because there is no real guarantee in C on the size and sign of the integer to which an enum is mapped.

huitema · 2021-05-07T04:44:15Z

picoquic/quicctx.c

@@ -1110,6 +1115,7 @@ void picoquic_init_transport_parameters(picoquic_tp_t* tp, int client_mode)
    tp->enable_loss_bit = 2;
    tp->min_ack_delay = PICOQUIC_ACK_DELAY_MIN;
    tp->enable_time_stamp = 0;
+    tp->enable_bdp = 0;
 }



Again, please change to bdp_frame option.

huitema · 2021-05-07T04:49:04Z

picoquic/sender.c

+        if (cnx->quic->default_bdp_option == 2 && (cnx->local_parameters.enable_bdp == 1 || cnx->local_parameters.enable_bdp == 3)) {
+            bytes_next = picoquic_format_bdp_frame(cnx, bytes_next, bytes_max, path_x, &more_data, &is_pure_ack);
+        }
+


Please add a test that the server sent a BDP frame in the previous session. Also, do you really intent to add a BDP frame in every 0-RTT packet?

huitema · 2021-05-07T04:53:02Z

picoquic/sender.c

+                        if(!cnx->client_mode && cnx->quic->default_bdp_option == 2 && (cnx->remote_parameters.enable_bdp == 2 || cnx->remote_parameters.enable_bdp == 3)) {
+                           bytes_next = picoquic_format_bdp_frame(cnx, bytes_next, bytes_max, path_x, &more_data, &is_pure_ack);
+                        }
+


This is too soon for the server -- server cannot possibly have a good value of RTT and CWIN before reaching the "ready" state. On the other hand, it might be a good idea to do that on the client, if the client did not yet have a chance to send the frame.

huitema · 2021-05-07T05:06:01Z

picoquic/sender.c

+                        if(!cnx->client_mode && cnx->quic->default_bdp_option == 2 && (cnx->remote_parameters.enable_bdp == 2 || cnx->remote_parameters.enable_bdp == 3)) {
+                           bytes_next = picoquic_format_bdp_frame(cnx, bytes_next, bytes_max, path_x, &more_data, &is_pure_ack);
+                        }
+


There is a better way to do that. Inside picoquic_seed_ticket, when new values of the CWIN are learned, the code places them in the server ticket hash table. At the same time, it could post a miscellaneous frame to queue transmission of the BDP frame.

huitema · 2021-07-25T04:54:01Z

Now that we have added the IP parameters, there are only a few issues left:

Should tie sending the BDP frame to the evaluation of the CWIN, not send it before that.
Should align processing of incoming DP frame at server to processing of "local knowledge"
Should add a test of BDP frame (satellite is not enough, because in that test only the client sends data)
Should add test verifying that the BDP frame is ignored if the client IP changes
Should add test verifying that the BDP frame is ignored if the link RTT changed.

huitema · 2021-07-27T05:40:04Z

The logical tests are now all working, but there is was a flow control issue. The rate was limited by the value of the initial flow control parameter, which prevented taking full advantage of the increased window. I am doing a test in which this initial flow control parameter is set to a large value, but then I find another issue. The CWIN is set at a value that is too large, which causes a lot of losses and in turn slows the link:

huitema · 2021-07-27T18:07:27Z

Added a check of the CWIN value before storing it for BDP frame transmission. The CWIN value can overshoot by the end of slow start. Using that value will cause a wild overshoot during the next connection. Instead, we cap it to estimated_bandwidth*rtt_min, which produces better results. But there is still a significant overshot.

huitema · 2021-07-28T05:21:12Z

Fixed the overshot issues for RENO and BBR. There are still too many losses in the case of Cubic, but that investigation will have to happen later.

Francklin SIMO added 2 commits May 6, 2021 20:54

Define TP enable_bdp and BDP option

efb0b09

Add BDP Frame and corresponding tests

d9f8dee

francklinSIMO force-pushed the 0rtt_bdp branch from 75b564e to d9f8dee Compare May 6, 2021 21:04

huitema reviewed May 7, 2021

View reviewed changes

huitema added 4 commits July 19, 2021 13:53

Rename BDP option to BDP frame option

8b2e152

Add client address to ticket store and to BDP frame

3cc17de

Fix BDP frame formatting

3dea183

Fix test of bad bdp frames

c9ac31d

huitema added 3 commits July 26, 2021 21:45

Add BDP Basic test and fix bugs

a12eb27

remove unused variable

ace675a

Add BDP tests for IP change and long delay

af2ba6c

Sanity check cwin before storing BDP values

5516fd4

Add BBR tests of multiple CC algorithms

c639b81

Disable cubic test for Linux for nwo.

9c67465

huitema merged commit 77d8545 into private-octopus:master Jul 28, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for 0RTT-BDP extension #1209

Add support for 0RTT-BDP extension #1209

francklinSIMO commented May 6, 2021 •

edited

huitema left a comment

huitema May 7, 2021

huitema May 7, 2021

huitema May 7, 2021

huitema May 7, 2021

huitema May 7, 2021

huitema May 7, 2021

huitema May 7, 2021

huitema May 7, 2021

huitema May 7, 2021

huitema May 7, 2021

huitema commented Jul 25, 2021

huitema commented Jul 27, 2021

huitema commented Jul 27, 2021

huitema commented Jul 28, 2021

Add support for 0RTT-BDP extension #1209

Add support for 0RTT-BDP extension #1209

Conversation

francklinSIMO commented May 6, 2021 • edited

huitema left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

huitema commented Jul 25, 2021

huitema commented Jul 27, 2021

huitema commented Jul 27, 2021

huitema commented Jul 28, 2021

francklinSIMO commented May 6, 2021 •

edited