From dc9905bf0c42bb146e625a354a9508cbab62508d Mon Sep 17 00:00:00 2001 From: Kazuho Oku Date: Mon, 13 Nov 2023 15:54:25 +0900 Subject: [PATCH] [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); }