From 9be80151fd5275aa2b9d584c28e03e1275a3c2fb Mon Sep 17 00:00:00 2001 From: Dave Cottlehuber Date: Sat, 11 Nov 2023 23:02:15 +0000 Subject: [PATCH 1/4] add missing u_* types for FreeBSD /usr/include/netinet/ip.h:53:2: error: unknown type name 'u_char'; did you mean 'char'? --- lib/quicly.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/lib/quicly.c b/lib/quicly.c index e3cd4d77..26c2598b 100644 --- a/lib/quicly.c +++ b/lib/quicly.c @@ -21,6 +21,9 @@ */ #include #include +#if defined(__FreeBSD__) +#include +#endif #include #include #include From dc9905bf0c42bb146e625a354a9508cbab62508d Mon Sep 17 00:00:00 2001 From: Kazuho Oku Date: Mon, 13 Nov 2023 15:54:25 +0900 Subject: [PATCH 2/4] [reno][pico] do not increase CWND when the connection is in app-limited state; ATM the state being referred to is that when `quicly_send` is called the last time. For Cubic, this is a TODO. --- include/quicly.h | 4 ++++ include/quicly/cc.h | 2 +- lib/cc-cubic.c | 4 +++- lib/cc-pico.c | 5 +++-- lib/cc-reno.c | 12 ++++++++---- lib/defaults.c | 2 ++ lib/quicly.c | 24 ++++++++++++++++++------ 7 files changed, 39 insertions(+), 14 deletions(-) diff --git a/include/quicly.h b/include/quicly.h index 1870a083..f94202d8 100644 --- a/include/quicly.h +++ b/include/quicly.h @@ -338,6 +338,10 @@ struct st_quicly_context_t { * if pacing should be used */ unsigned use_pacing : 1; + /** + * if CC should take app-limited into consideration + */ + unsigned cc_recognize_app_limited : 1; /** * */ diff --git a/include/quicly/cc.h b/include/quicly/cc.h index 9da074c6..5bdb9117 100644 --- a/include/quicly/cc.h +++ b/include/quicly/cc.h @@ -160,7 +160,7 @@ struct st_quicly_cc_type_t { * Called when a packet is newly acknowledged. */ void (*cc_on_acked)(quicly_cc_t *cc, const quicly_loss_t *loss, uint32_t bytes, uint64_t largest_acked, uint32_t inflight, - uint64_t next_pn, int64_t now, uint32_t max_udp_payload_size); + int cc_limited, uint64_t next_pn, int64_t now, uint32_t max_udp_payload_size); /** * Called when a packet is detected as lost. * @param bytes bytes declared lost, or zero iff ECN_CE is observed diff --git a/lib/cc-cubic.c b/lib/cc-cubic.c index 5c5202b0..468bb71c 100644 --- a/lib/cc-cubic.c +++ b/lib/cc-cubic.c @@ -62,13 +62,15 @@ static uint32_t calc_w_est(const quicly_cc_t *cc, cubic_float_t t_sec, cubic_flo /* TODO: Avoid increase if sender was application limited. */ static void cubic_on_acked(quicly_cc_t *cc, const quicly_loss_t *loss, uint32_t bytes, uint64_t largest_acked, uint32_t inflight, - uint64_t next_pn, int64_t now, uint32_t max_udp_payload_size) + int cc_limited, uint64_t next_pn, int64_t now, uint32_t max_udp_payload_size) { assert(inflight >= bytes); /* Do not increase congestion window while in recovery. */ if (largest_acked < cc->recovery_end) return; + /* TODO: respect cc_limited */ + /* Slow start. */ if (cc->cwnd < cc->ssthresh) { cc->cwnd += bytes; diff --git a/lib/cc-pico.c b/lib/cc-pico.c index 00472af0..fb01c8f9 100644 --- a/lib/cc-pico.c +++ b/lib/cc-pico.c @@ -62,7 +62,7 @@ static uint32_t calc_bytes_per_mtu_increase(uint32_t cwnd, uint32_t rtt, uint32_ /* TODO: Avoid increase if sender was application limited. */ static void pico_on_acked(quicly_cc_t *cc, const quicly_loss_t *loss, uint32_t bytes, uint64_t largest_acked, uint32_t inflight, - uint64_t next_pn, int64_t now, uint32_t max_udp_payload_size) + int cc_limited, uint64_t next_pn, int64_t now, uint32_t max_udp_payload_size) { assert(inflight >= bytes); @@ -70,7 +70,8 @@ static void pico_on_acked(quicly_cc_t *cc, const quicly_loss_t *loss, uint32_t b if (largest_acked < cc->recovery_end) return; - cc->state.pico.stash += bytes; + if (!cc_limited) + return; /* Calculate the amount of bytes required to be acked for incrementing CWND by one MTU. */ uint32_t bytes_per_mtu_increase; diff --git a/lib/cc-reno.c b/lib/cc-reno.c index 8e31cbf3..7eaab1e2 100644 --- a/lib/cc-reno.c +++ b/lib/cc-reno.c @@ -25,7 +25,7 @@ /* TODO: Avoid increase if sender was application limited. */ static void reno_on_acked(quicly_cc_t *cc, const quicly_loss_t *loss, uint32_t bytes, uint64_t largest_acked, uint32_t inflight, - uint64_t next_pn, int64_t now, uint32_t max_udp_payload_size) + int cc_limited, uint64_t next_pn, int64_t now, uint32_t max_udp_payload_size) { assert(inflight >= bytes); /* Do not increase congestion window while in recovery. */ @@ -34,13 +34,17 @@ static void reno_on_acked(quicly_cc_t *cc, const quicly_loss_t *loss, uint32_t b /* Slow start. */ if (cc->cwnd < cc->ssthresh) { - cc->cwnd += bytes; - if (cc->cwnd_maximum < cc->cwnd) - cc->cwnd_maximum = cc->cwnd; + if (cc_limited) { + cc->cwnd += bytes; + if (cc->cwnd_maximum < cc->cwnd) + cc->cwnd_maximum = cc->cwnd; + } return; } /* Congestion avoidance. */ cc->pacer_multiplier = QUICLY_PACER_CALC_MULTIPLIER(1.2); + if (!cc_limited) + return; cc->state.reno.stash += bytes; if (cc->state.reno.stash < cc->cwnd) return; diff --git a/lib/defaults.c b/lib/defaults.c index 23568b9e..de18909d 100644 --- a/lib/defaults.c +++ b/lib/defaults.c @@ -52,6 +52,7 @@ const quicly_context_t quicly_spec_context = {NULL, 0, /* enlarge_client_hello */ 1, /* enable_ecn */ 0, /* use_pacing */ + 1, /* cc_recognize_app_limited */ NULL, NULL, /* on_stream_open */ &quicly_default_stream_scheduler, @@ -84,6 +85,7 @@ const quicly_context_t quicly_performant_context = {NULL, 0, /* enlarge_client_hello */ 1, /* enable_ecn */ 0, /* use_pacing */ + 1, /* cc_recognize_app_limited */ NULL, NULL, /* on_stream_open */ &quicly_default_stream_scheduler, diff --git a/lib/quicly.c b/lib/quicly.c index 214fbd51..75c606b8 100644 --- a/lib/quicly.c +++ b/lib/quicly.c @@ -350,6 +350,10 @@ struct st_quicly_conn_t { * have to be sent. There could be false positives; logic for sending each of these frames have the capability of detecting such * false positives. The purpose of this bit is to consolidate information as an optimization. */ #define QUICLY_PENDING_FLOW_OTHERS_BIT (1 << 6) + /** + * if the most recent call to `quicly_send` ended in CC-limited state + */ + uint8_t cc_limited : 1; /** * pending RETIRE_CONNECTION_ID frames to be sent */ @@ -1378,6 +1382,8 @@ static void update_send_alarm(quicly_conn_t *conn, int can_send_stream_data, int static void update_cc_limited(quicly_conn_t *conn, int is_cc_limited) { + conn->egress.cc_limited = is_cc_limited; + if (quicly_ratemeter_is_cc_limited(&conn->egress.ratemeter) != is_cc_limited) { if (is_cc_limited) { quicly_ratemeter_enter_cc_limited(&conn->egress.ratemeter, conn->egress.packet_number); @@ -4907,15 +4913,14 @@ static int do_send(quicly_conn_t *conn, quicly_send_context_t *s) commit_send_packet(conn, s, 0); } if (ret == 0) { - /* update timers, start / stop delivery rate estimator */ + /* update timers, cc and delivery rate estimator states */ if (conn->application == NULL || conn->application->super.unacked_count == 0) conn->egress.send_ack_at = INT64_MAX; /* we have sent ACKs for every epoch (or before address validation) */ int can_send_stream_data = scheduler_can_send(conn); update_send_alarm(conn, can_send_stream_data, 1); - int is_cc_limited = can_send_stream_data && (s->num_datagrams == s->max_datagrams || - conn->egress.loss.sentmap.bytes_in_flight >= conn->egress.cc.cwnd || - pacer_can_send_at(conn) > conn->stash.now); - update_cc_limited(conn, is_cc_limited); + update_cc_limited(conn, can_send_stream_data && (s->num_datagrams == s->max_datagrams || + conn->egress.loss.sentmap.bytes_in_flight >= conn->egress.cc.cwnd || + pacer_can_send_at(conn) > conn->stash.now)); if (s->num_datagrams != 0) update_idle_timeout(conn, 0); } @@ -5411,9 +5416,16 @@ static int handle_ack_frame(quicly_conn_t *conn, struct st_quicly_handle_payload /* OnPacketAcked and OnPacketAckedCC */ if (bytes_acked > 0) { + /* Here, we pass `conn->egress.cc_limited` - a boolean flag indicating if last the last call to `quicly_send` ended with + * data being throttled by CC or pacer - to CC to determine if CWND can be grown. + * This might not be the best way to do this, but would likely be sufficient, as the flag being passed would be true only if + * the connection was CC-limited for at least one RTT. Hopefully, itwould also be aggressive enough during the slow start + * phase. */ + int cc_limited = conn->super.ctx->cc_recognize_app_limited ? conn->egress.cc_limited : 1; conn->egress.cc.type->cc_on_acked(&conn->egress.cc, &conn->egress.loss, (uint32_t)bytes_acked, frame.largest_acknowledged, (uint32_t)(conn->egress.loss.sentmap.bytes_in_flight + bytes_acked), - conn->egress.packet_number, conn->stash.now, conn->egress.max_udp_payload_size); + cc_limited, conn->egress.packet_number, conn->stash.now, + conn->egress.max_udp_payload_size); QUICLY_PROBE(QUICTRACE_CC_ACK, conn, conn->stash.now, &conn->egress.loss.rtt, conn->egress.cc.cwnd, conn->egress.loss.sentmap.bytes_in_flight); } From fb19cecccbb403a48bddda82478ae7f1ac6ccd06 Mon Sep 17 00:00:00 2001 From: Kazuho Oku Date: Tue, 14 Nov 2023 09:23:37 +0900 Subject: [PATCH 3/4] Update lib/quicly.c --- lib/quicly.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/lib/quicly.c b/lib/quicly.c index 26c2598b..cd862f6e 100644 --- a/lib/quicly.c +++ b/lib/quicly.c @@ -21,9 +21,7 @@ */ #include #include -#if defined(__FreeBSD__) #include -#endif #include #include #include From 4b2f102c9812d6f7d716cf8fdb4c37c8520a437f Mon Sep 17 00:00:00 2001 From: Kazuho Oku Date: Thu, 26 Oct 2023 09:40:56 +0900 Subject: [PATCH 4/4] retain only the most-recently-received PATH_CHALLENGE payload; this is backport from kazuho/multipath-05 that had already adopted the approach --- lib/quicly.c | 84 +++++++++++++++++++++------------------------------- 1 file changed, 33 insertions(+), 51 deletions(-) diff --git a/lib/quicly.c b/lib/quicly.c index cd862f6e..55e67a3c 100644 --- a/lib/quicly.c +++ b/lib/quicly.c @@ -109,12 +109,6 @@ struct st_quicly_cipher_context_t { ptls_cipher_context_t *header_protection; }; -struct st_quicly_pending_path_challenge_t { - struct st_quicly_pending_path_challenge_t *next; - uint8_t is_response; - uint8_t data[QUICLY_PATH_CHALLENGE_DATA_LEN]; -}; - struct st_quicly_pn_space_t { /** * acks to be sent to remote peer @@ -279,8 +273,9 @@ struct st_quicly_conn_t { * */ struct { - struct st_quicly_pending_path_challenge_t *head, **tail_ref; - } path_challenge; + uint8_t send_; + uint8_t data[QUICLY_PATH_CHALLENGE_DATA_LEN]; + } path_response; /** * */ @@ -936,25 +931,6 @@ void quicly_stream_sync_recvbuf(quicly_stream_t *stream, size_t shift_amount) } } -static int schedule_path_challenge_frame(quicly_conn_t *conn, int is_response, const uint8_t *data) -{ - struct st_quicly_pending_path_challenge_t *pending; - - if ((pending = malloc(sizeof(struct st_quicly_pending_path_challenge_t))) == NULL) - return PTLS_ERROR_NO_MEMORY; - - pending->next = NULL; - pending->is_response = is_response; - memcpy(pending->data, data, QUICLY_PATH_CHALLENGE_DATA_LEN); - - *conn->egress.path_challenge.tail_ref = pending; - conn->egress.path_challenge.tail_ref = &pending->next; - - conn->egress.pending_flows |= QUICLY_PENDING_FLOW_OTHERS_BIT; - - return 0; -} - /** * calculate how many CIDs we provide to the remote peer */ @@ -1699,11 +1675,6 @@ void quicly_free(quicly_conn_t *conn) quicly_maxsender_dispose(&conn->ingress.max_data.sender); quicly_maxsender_dispose(&conn->ingress.max_streams.uni); quicly_maxsender_dispose(&conn->ingress.max_streams.bidi); - while (conn->egress.path_challenge.head != NULL) { - struct st_quicly_pending_path_challenge_t *pending = conn->egress.path_challenge.head; - conn->egress.path_challenge.head = pending->next; - free(pending); - } quicly_loss_dispose(&conn->egress.loss); kh_destroy(quicly_stream_t, conn->streams); @@ -2248,7 +2219,6 @@ static quicly_conn_t *create_connection(quicly_context_t *ctx, uint32_t protocol conn->egress.max_udp_payload_size = conn->super.ctx->initial_egress_max_udp_payload_size; init_max_streams(&conn->egress.max_streams.uni); init_max_streams(&conn->egress.max_streams.bidi); - conn->egress.path_challenge.tail_ref = &conn->egress.path_challenge.head; conn->egress.ack_frequency.update_at = INT64_MAX; conn->egress.send_ack_at = INT64_MAX; conn->super.ctx->init_cc->cb(conn->super.ctx->init_cc, &conn->egress.cc, initcwnd, conn->stash.now); @@ -4487,6 +4457,25 @@ static int send_retire_connection_id(quicly_conn_t *conn, quicly_send_context_t return 0; } +static int send_path_challenge(quicly_conn_t *conn, quicly_send_context_t *s, int is_response, const uint8_t *data) +{ + int ret; + + if ((ret = do_allocate_frame(conn, s, QUICLY_PATH_CHALLENGE_FRAME_CAPACITY, ALLOCATE_FRAME_TYPE_ACK_ELICITING_NO_CC)) != 0) + return ret; + + s->dst = quicly_encode_path_challenge_frame(s->dst, is_response, data); + s->target.full_size = 1; /* ensure that the path can transfer full-size packets */ + + if (!is_response) { + ++conn->super.stats.num_frames_sent.path_challenge; + } else { + ++conn->super.stats.num_frames_sent.path_response; + } + + return 0; +} + static int update_traffic_key_cb(ptls_update_traffic_key_t *self, ptls_t *tls, int is_enc, size_t epoch, const void *secret) { quicly_conn_t *conn = *ptls_get_data_ptr(tls); @@ -4592,23 +4581,10 @@ static int send_other_control_frames(quicly_conn_t *conn, quicly_send_context_t int ret; /* respond to all pending received PATH_CHALLENGE frames */ - if (conn->egress.path_challenge.head != NULL) { - do { - struct st_quicly_pending_path_challenge_t *c = conn->egress.path_challenge.head; - if ((ret = do_allocate_frame(conn, s, QUICLY_PATH_CHALLENGE_FRAME_CAPACITY, ALLOCATE_FRAME_TYPE_NON_ACK_ELICITING)) != - 0) - return ret; - s->dst = quicly_encode_path_challenge_frame(s->dst, c->is_response, c->data); - if (c->is_response) { - ++conn->super.stats.num_frames_sent.path_response; - } else { - ++conn->super.stats.num_frames_sent.path_challenge; - } - conn->egress.path_challenge.head = c->next; - free(c); - } while (conn->egress.path_challenge.head != NULL); - conn->egress.path_challenge.tail_ref = &conn->egress.path_challenge.head; - s->target.full_size = 1; /* datagrams carrying PATH_CHALLENGE / PATH_RESPONSE have to be full-sized */ + if (conn->egress.path_response.send_) { + if ((ret = send_path_challenge(conn, s, 1, conn->egress.path_response.data)) != 0) + return ret; + conn->egress.path_response.send_ = 0; } /* MAX_STREAMS */ @@ -5552,7 +5528,13 @@ static int handle_path_challenge_frame(quicly_conn_t *conn, struct st_quicly_han if ((ret = quicly_decode_path_challenge_frame(&state->src, state->end, &frame)) != 0) return ret; - return schedule_path_challenge_frame(conn, 1, frame.data); + + /* schedule the emission of PATH_RESPONSE frame */ + memcpy(conn->egress.path_response.data, frame.data, QUICLY_PATH_CHALLENGE_DATA_LEN); + conn->egress.path_response.send_ = 1; + conn->egress.pending_flows |= QUICLY_PENDING_FLOW_OTHERS_BIT; + + return 0; } static int handle_path_response_frame(quicly_conn_t *conn, struct st_quicly_handle_payload_state_t *state)