From 54329ce287e0c7a4b7070fff8d7c4618e46cff2a Mon Sep 17 00:00:00 2001 From: Kazuho Oku Date: Sat, 11 Nov 2023 03:34:40 +0100 Subject: [PATCH 1/4] s/cwnd_limited/cc_limited/g now that we have pacer --- include/quicly/rate.h | 4 ++-- lib/quicly.c | 6 +++--- lib/rate.c | 4 ++-- t/rate.c | 10 +++++----- 4 files changed, 12 insertions(+), 12 deletions(-) diff --git a/include/quicly/rate.h b/include/quicly/rate.h index fd546077..8029ee5f 100644 --- a/include/quicly/rate.h +++ b/include/quicly/rate.h @@ -93,11 +93,11 @@ void quicly_ratemeter_init(quicly_ratemeter_t *meter); /** * Notifies the estimator that the flow is CWND-limited at the point of sending packets *starting* from packet number `pn`. */ -void quicly_ratemeter_in_cwnd_limited(quicly_ratemeter_t *meter, uint64_t pn); +void quicly_ratemeter_in_cc_limited(quicly_ratemeter_t *meter, uint64_t pn); /** * Notifies that the estimator that the flow is not CWND-limited when the packet number of the next packet will be `pn`. */ -void quicly_ratemeter_not_cwnd_limited(quicly_ratemeter_t *meter, uint64_t pn); +void quicly_ratemeter_not_cc_limited(quicly_ratemeter_t *meter, uint64_t pn); /** * Given three values, update the estimation. * @param bytes_acked total number of bytes being acked from the beginning of the connection; i.e., diff --git a/lib/quicly.c b/lib/quicly.c index 32b88fb8..8c874e41 100644 --- a/lib/quicly.c +++ b/lib/quicly.c @@ -1388,7 +1388,7 @@ static void setup_next_send(quicly_conn_t *conn) /* When the flow becomes application-limited due to receiving some information, stop collecting delivery rate samples. */ if (!can_send_stream_data) - quicly_ratemeter_not_cwnd_limited(&conn->egress.ratemeter, conn->egress.packet_number); + quicly_ratemeter_not_cc_limited(&conn->egress.ratemeter, conn->egress.packet_number); } static int create_handshake_flow(quicly_conn_t *conn, size_t epoch) @@ -4897,9 +4897,9 @@ static int do_send(quicly_conn_t *conn, quicly_send_context_t *s) if (can_send_stream_data && (s->num_datagrams == s->max_datagrams || conn->egress.loss.sentmap.bytes_in_flight >= conn->egress.cc.cwnd)) { /* as the flow is CWND-limited, start delivery rate estimator */ - quicly_ratemeter_in_cwnd_limited(&conn->egress.ratemeter, s->first_packet_number); + quicly_ratemeter_in_cc_limited(&conn->egress.ratemeter, s->first_packet_number); } else { - quicly_ratemeter_not_cwnd_limited(&conn->egress.ratemeter, conn->egress.packet_number); + quicly_ratemeter_not_cc_limited(&conn->egress.ratemeter, conn->egress.packet_number); } if (s->num_datagrams != 0) update_idle_timeout(conn, 0); diff --git a/lib/rate.c b/lib/rate.c index 6de0986e..7ffcfbe8 100644 --- a/lib/rate.c +++ b/lib/rate.c @@ -50,7 +50,7 @@ void quicly_ratemeter_init(quicly_ratemeter_t *meter) }; } -void quicly_ratemeter_in_cwnd_limited(quicly_ratemeter_t *meter, uint64_t pn) +void quicly_ratemeter_in_cc_limited(quicly_ratemeter_t *meter, uint64_t pn) { /* bail out if already in cwnd-limited phase */ if (meter->pn_cwnd_limited.start != UINT64_MAX && meter->pn_cwnd_limited.end == UINT64_MAX) @@ -64,7 +64,7 @@ void quicly_ratemeter_in_cwnd_limited(quicly_ratemeter_t *meter, uint64_t pn) meter->pn_cwnd_limited = (quicly_range_t){.start = pn, .end = UINT64_MAX}; } -void quicly_ratemeter_not_cwnd_limited(quicly_ratemeter_t *meter, uint64_t pn) +void quicly_ratemeter_not_cc_limited(quicly_ratemeter_t *meter, uint64_t pn) { if (meter->pn_cwnd_limited.start != UINT64_MAX && meter->pn_cwnd_limited.end == UINT64_MAX) meter->pn_cwnd_limited.end = pn; diff --git a/t/rate.c b/t/rate.c index dec877e3..a492e2c6 100644 --- a/t/rate.c +++ b/t/rate.c @@ -44,7 +44,7 @@ static void test_basic(void) /* send 1KB packet every 20ms, in CWND-limited state */ for (; pn < 100; ++pn) { - quicly_ratemeter_in_cwnd_limited(&meter, pn); + quicly_ratemeter_in_cc_limited(&meter, pn); bytes_acked += 1000; now += 20; quicly_ratemeter_on_ack(&meter, now, bytes_acked, pn); @@ -53,7 +53,7 @@ static void test_basic(void) /* send at a slow rate, in application-limited state */ for (; pn < 200; ++pn) { - quicly_ratemeter_not_cwnd_limited(&meter, pn); + quicly_ratemeter_not_cc_limited(&meter, pn); bytes_acked += 10; now += 20; quicly_ratemeter_on_ack(&meter, now, bytes_acked, pn); @@ -62,7 +62,7 @@ static void test_basic(void) /* send 2KB packet every 20ms, in CWND-limited state */ for (; pn < 300; ++pn) { - quicly_ratemeter_in_cwnd_limited(&meter, pn); + quicly_ratemeter_in_cc_limited(&meter, pn); bytes_acked += 2000; now += 20; quicly_ratemeter_on_ack(&meter, now, bytes_acked, pn); @@ -78,8 +78,8 @@ static void test_burst(void) CHECK_REPORT(0, 0, 0); /* send 10 packet burst (pn=1 to 10) */ - quicly_ratemeter_in_cwnd_limited(&meter, 1); - quicly_ratemeter_not_cwnd_limited(&meter, 11); + quicly_ratemeter_in_cc_limited(&meter, 1); + quicly_ratemeter_not_cc_limited(&meter, 11); /* ack every 2 packets up to pn=9, every 20ms */ uint64_t pn = 0, bytes_acked = 0; From c8ec914490161e03f0f2c499aab284d07e1a299e Mon Sep 17 00:00:00 2001 From: Kazuho Oku Date: Sat, 11 Nov 2023 03:42:49 +0100 Subject: [PATCH 2/4] calculate delivery rate when pacer-limited --- lib/quicly.c | 28 ++++++++++++++++------------ 1 file changed, 16 insertions(+), 12 deletions(-) diff --git a/lib/quicly.c b/lib/quicly.c index 8c874e41..ff614042 100644 --- a/lib/quicly.c +++ b/lib/quicly.c @@ -3112,6 +3112,15 @@ static int is_point5rtt_with_no_handshake_data_to_send(quicly_conn_t *conn) return stream->sendstate.pending.num_ranges == 0 && stream->sendstate.acked.ranges[0].end == stream->sendstate.size_inflight; } +static int64_t pacer_can_send_at(quicly_conn_t *conn) +{ + if (conn->egress.pacer == NULL) + return 0; + + uint32_t bytes_per_msec = calc_pacer_send_rate(conn); + return quicly_pacer_can_send_at(conn->egress.pacer, bytes_per_msec, conn->egress.max_udp_payload_size); +} + int64_t quicly_get_first_timeout(quicly_conn_t *conn) { if (conn->super.state >= QUICLY_STATE_CLOSING) @@ -3121,25 +3130,19 @@ int64_t quicly_get_first_timeout(quicly_conn_t *conn) return 0; uint64_t amp_window = calc_amplification_limit_allowance(conn); - int64_t at = conn->idle_timeout.at, pacer_can_send_at = 0; - - if (conn->egress.pacer != NULL) { - uint32_t bytes_per_msec = calc_pacer_send_rate(conn); - pacer_can_send_at = quicly_pacer_can_send_at(conn->egress.pacer, bytes_per_msec, conn->egress.max_udp_payload_size); - } + int64_t at = conn->idle_timeout.at, pacer_at = pacer_can_send_at(conn); /* reduce at to the moment pacer provides credit, if we are not CC-limited and there's something to be sent over CC */ - if (pacer_can_send_at < at && calc_send_window(conn, 0, amp_window, UINT64_MAX, 0) > 0) { + if (pacer_at < at && calc_send_window(conn, 0, amp_window, UINT64_MAX, 0) > 0) { if (conn->egress.pending_flows != 0) { /* crypto streams (as indicated by lower 4 bits) can be sent whenever CWND is available; other flows need application * packet number space */ if ((conn->application != NULL && conn->application->cipher.egress.key.header_protection != NULL) || (conn->egress.pending_flows & 0xf) != 0) - at = pacer_can_send_at; + at = pacer_at; } - if (pacer_can_send_at < at && - (quicly_linklist_is_linked(&conn->egress.pending_streams.control) || scheduler_can_send(conn))) - at = pacer_can_send_at; + if (pacer_at < at && (quicly_linklist_is_linked(&conn->egress.pending_streams.control) || scheduler_can_send(conn))) + at = pacer_at; } /* if something can be sent, return the earliest timeout. Otherwise return the idle timeout. */ @@ -4895,7 +4898,8 @@ static int do_send(quicly_conn_t *conn, quicly_send_context_t *s) int can_send_stream_data = scheduler_can_send(conn); update_send_alarm(conn, can_send_stream_data, 1); if (can_send_stream_data && - (s->num_datagrams == s->max_datagrams || conn->egress.loss.sentmap.bytes_in_flight >= conn->egress.cc.cwnd)) { + (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)) { /* as the flow is CWND-limited, start delivery rate estimator */ quicly_ratemeter_in_cc_limited(&conn->egress.ratemeter, s->first_packet_number); } else { From 4d486d0e59b402146509d695c1dac3946f3b7485 Mon Sep 17 00:00:00 2001 From: Kazuho Oku Date: Sat, 11 Nov 2023 03:57:37 +0100 Subject: [PATCH 3/4] refactor, adding probes to mark enter to / exit of cc-limited state --- include/quicly/rate.h | 19 +++++++++++++++---- lib/quicly.c | 29 ++++++++++++++++++++--------- lib/rate.c | 13 ++++++------- quicly-probes.d | 3 +++ 4 files changed, 44 insertions(+), 20 deletions(-) diff --git a/include/quicly/rate.h b/include/quicly/rate.h index 8029ee5f..1dcaecea 100644 --- a/include/quicly/rate.h +++ b/include/quicly/rate.h @@ -91,13 +91,17 @@ typedef struct st_quicly_rate_t { */ void quicly_ratemeter_init(quicly_ratemeter_t *meter); /** - * Notifies the estimator that the flow is CWND-limited at the point of sending packets *starting* from packet number `pn`. + * returns if currently marked as CC-limited */ -void quicly_ratemeter_in_cc_limited(quicly_ratemeter_t *meter, uint64_t pn); +static int quicly_ratemeter_is_cc_limited(quicly_ratemeter_t *meter); /** - * Notifies that the estimator that the flow is not CWND-limited when the packet number of the next packet will be `pn`. + * Notifies the estimator that the flow is becoming CC-limited at the point of sending packets *starting* from packet number `pn`. */ -void quicly_ratemeter_not_cc_limited(quicly_ratemeter_t *meter, uint64_t pn); +void quicly_ratemeter_enter_cc_limited(quicly_ratemeter_t *meter, uint64_t pn); +/** + * Notifies that the estimator that the flow is exiting from CC-limited when the packet number of the next packet will be `pn`. + */ +void quicly_ratemeter_exit_cc_limited(quicly_ratemeter_t *meter, uint64_t pn); /** * Given three values, update the estimation. * @param bytes_acked total number of bytes being acked from the beginning of the connection; i.e., @@ -109,6 +113,13 @@ void quicly_ratemeter_on_ack(quicly_ratemeter_t *meter, int64_t now, uint64_t by */ void quicly_ratemeter_report(quicly_ratemeter_t *meter, quicly_rate_t *rate); +/* inline definitions */ + +inline int quicly_ratemeter_is_cc_limited(quicly_ratemeter_t *meter) +{ + return meter->pn_cwnd_limited.start != UINT64_MAX && meter->pn_cwnd_limited.end == UINT64_MAX; +} + #ifdef __cplusplus } #endif diff --git a/lib/quicly.c b/lib/quicly.c index ff614042..3be2fcbd 100644 --- a/lib/quicly.c +++ b/lib/quicly.c @@ -1376,6 +1376,21 @@ static void update_send_alarm(quicly_conn_t *conn, int can_send_stream_data, int can_send_stream_data, handshake_is_in_progress, conn->egress.max_data.sent, is_after_send); } +static void update_cc_limited(quicly_conn_t *conn, int 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); + QUICLY_PROBE(ENTER_CC_LIMITED, conn, conn->stash.now, conn->egress.packet_number); + QUICLY_LOG_CONN(enter_cc_limited, conn, { PTLS_LOG_ELEMENT_UNSIGNED(pn, conn->egress.packet_number); }); + } else { + quicly_ratemeter_exit_cc_limited(&conn->egress.ratemeter, conn->egress.packet_number); + QUICLY_PROBE(EXIT_CC_LIMITED, conn, conn->stash.now, conn->egress.packet_number); + QUICLY_LOG_CONN(exit_cc_limited, conn, { PTLS_LOG_ELEMENT_UNSIGNED(pn, conn->egress.packet_number); }); + } + } +} + /** * Updates the send alarm and adjusts the delivery rate estimator. This function is called from the receive path. From the sendp * path, `update_send_alarm` is called directly. @@ -1388,7 +1403,7 @@ static void setup_next_send(quicly_conn_t *conn) /* When the flow becomes application-limited due to receiving some information, stop collecting delivery rate samples. */ if (!can_send_stream_data) - quicly_ratemeter_not_cc_limited(&conn->egress.ratemeter, conn->egress.packet_number); + update_cc_limited(conn, 0); } static int create_handshake_flow(quicly_conn_t *conn, size_t epoch) @@ -4897,14 +4912,10 @@ static int do_send(quicly_conn_t *conn, quicly_send_context_t *s) 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); - if (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)) { - /* as the flow is CWND-limited, start delivery rate estimator */ - quicly_ratemeter_in_cc_limited(&conn->egress.ratemeter, s->first_packet_number); - } else { - quicly_ratemeter_not_cc_limited(&conn->egress.ratemeter, conn->egress.packet_number); - } + 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); if (s->num_datagrams != 0) update_idle_timeout(conn, 0); } diff --git a/lib/rate.c b/lib/rate.c index 7ffcfbe8..6062038d 100644 --- a/lib/rate.c +++ b/lib/rate.c @@ -50,11 +50,9 @@ void quicly_ratemeter_init(quicly_ratemeter_t *meter) }; } -void quicly_ratemeter_in_cc_limited(quicly_ratemeter_t *meter, uint64_t pn) +void quicly_ratemeter_enter_cc_limited(quicly_ratemeter_t *meter, uint64_t pn) { - /* bail out if already in cwnd-limited phase */ - if (meter->pn_cwnd_limited.start != UINT64_MAX && meter->pn_cwnd_limited.end == UINT64_MAX) - return; + assert(!quicly_ratemeter_is_cc_limited(meter)); /* if the estimator was waiting for the end of the previous phase, and if a valid partial sample exists, commit it now */ if (meter->pn_cwnd_limited.end != UINT64_MAX && meter->current.sample.elapsed != 0) @@ -64,10 +62,11 @@ void quicly_ratemeter_in_cc_limited(quicly_ratemeter_t *meter, uint64_t pn) meter->pn_cwnd_limited = (quicly_range_t){.start = pn, .end = UINT64_MAX}; } -void quicly_ratemeter_not_cc_limited(quicly_ratemeter_t *meter, uint64_t pn) +void quicly_ratemeter_exit_cc_limited(quicly_ratemeter_t *meter, uint64_t pn) { - if (meter->pn_cwnd_limited.start != UINT64_MAX && meter->pn_cwnd_limited.end == UINT64_MAX) - meter->pn_cwnd_limited.end = pn; + assert(quicly_ratemeter_is_cc_limited(meter)); + + meter->pn_cwnd_limited.end = pn; } void quicly_ratemeter_on_ack(quicly_ratemeter_t *meter, int64_t now, uint64_t bytes_acked, uint64_t pn) diff --git a/quicly-probes.d b/quicly-probes.d index b9ea4576..f81eeeb6 100644 --- a/quicly-probes.d +++ b/quicly-probes.d @@ -151,6 +151,9 @@ provider quicly { const void *src, size_t src_len); probe stream_on_receive_reset(struct st_quicly_conn_t *conn, int64_t at, struct st_quicly_stream_t *stream, int err); + probe enter_cc_limited(struct st_quicly_conn_t *conn, int64_t at, uint64_t pn); + probe exit_cc_limited(struct st_quicly_conn_t *conn, int64_t at, uint64_t pn); + probe debug_message(struct st_quicly_conn_t *conn, const char *function, int line, const char *message); probe conn_stats(struct st_quicly_conn_t *conn, int64_t at, struct st_quicly_stats_t *stats, size_t size); From 136a2917551d5c8297fd82df110c59addc317ea7 Mon Sep 17 00:00:00 2001 From: Kazuho Oku Date: Sat, 11 Nov 2023 16:17:06 +0900 Subject: [PATCH 4/4] At the moment, when a dupack is received, delivery rate estimator is invoked with pn=UINT64_MAX, causing incorrect closure of the cc-limited phase. This PR address the issue. --- lib/quicly.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/lib/quicly.c b/lib/quicly.c index 085157a9..e3cd4d77 100644 --- a/lib/quicly.c +++ b/lib/quicly.c @@ -5340,8 +5340,9 @@ static int handle_ack_frame(quicly_conn_t *conn, struct st_quicly_handle_payload QUICLY_PROBE(ACK_DELAY_RECEIVED, conn, conn->stash.now, frame.ack_delay); QUICLY_LOG_CONN(ack_delay_received, conn, { PTLS_LOG_ELEMENT_UNSIGNED(ack_delay, frame.ack_delay); }); - quicly_ratemeter_on_ack(&conn->egress.ratemeter, conn->stash.now, conn->super.stats.num_bytes.ack_received, - largest_newly_acked.pn); + if (largest_newly_acked.pn != UINT64_MAX) + quicly_ratemeter_on_ack(&conn->egress.ratemeter, conn->stash.now, conn->super.stats.num_bytes.ack_received, + largest_newly_acked.pn); /* Update loss detection engine on ack. The function uses ack_delay only when the largest_newly_acked is also the largest acked * so far. So, it does not matter if the ack_delay being passed in does not apply to the largest_newly_acked. */