From de7d6bbfb6722cefe83487810648a84c8ef3d13d Mon Sep 17 00:00:00 2001 From: Kazuho Oku Date: Mon, 6 Apr 2020 06:41:52 +0900 Subject: [PATCH 1/6] MVP of ack-frequency; set packet tolerance to 1/8 of CWND --- include/quicly.h | 4 + include/quicly/cc.h | 13 ++- include/quicly/constants.h | 3 +- include/quicly/frame.h | 48 ++++++++++ lib/quicly.c | 181 ++++++++++++++++++++++++++++++++----- quicly-probes.d | 3 + 6 files changed, 224 insertions(+), 28 deletions(-) diff --git a/include/quicly.h b/include/quicly.h index dbbc9b940..a6c57ee84 100644 --- a/include/quicly.h +++ b/include/quicly.h @@ -243,6 +243,10 @@ typedef struct st_quicly_transport_parameters_t { * in milliseconds; quicly ignores the value set for quicly_context_t::transport_parameters */ uint16_t max_ack_delay; + /** + * quicly ignores the value set for quicly_context_t::transport_parameters. Set to UINT64_MAX when not specified by peer. + */ + uint64_t min_ack_delay_usec; /** * */ diff --git a/include/quicly/cc.h b/include/quicly/cc.h index bb02362a5..f0b09a298 100644 --- a/include/quicly/cc.h +++ b/include/quicly/cc.h @@ -48,17 +48,26 @@ void quicly_cc_init(quicly_cc_t *cc); * Called when a packet is newly acknowledged. */ void quicly_cc_on_acked(quicly_cc_t *cc, uint32_t bytes, uint64_t largest_acked, uint32_t inflight); - /** * Called when a packet is detected as lost. |next_pn| is the next unsent packet number, * used for setting the recovery window. */ void quicly_cc_on_lost(quicly_cc_t *cc, uint32_t bytes, uint64_t lost_pn, uint64_t next_pn); - /** * Called when persistent congestion is observed. */ void quicly_cc_on_persistent_congestion(quicly_cc_t *cc); +/** + * Returns a boolean indicating if the CC is in startup (i.e. slow-start) + */ +static int quicly_cc_is_in_startup(quicly_cc_t *cc); + +/* inline definitions */ + +inline int quicly_cc_is_in_startup(quicly_cc_t *cc) +{ + return cc->cwnd < cc->ssthresh; +} #ifdef __cplusplus } diff --git a/include/quicly/constants.h b/include/quicly/constants.h index 5ae26f82f..c1d24039b 100644 --- a/include/quicly/constants.h +++ b/include/quicly/constants.h @@ -30,7 +30,8 @@ extern "C" { #include #include "picotls.h" -#define QUICLY_NUM_PACKETS_BEFORE_ACK 2 +#define QUICLY_DEFAULT_PACKET_TOLERANCE 2 +#define QUICLY_MAX_PACKET_TOLERANCE 100 #define QUICLY_DELAYED_ACK_TIMEOUT 25 /* milliseconds */ #define QUICLY_DEFAULT_MAX_ACK_DELAY 25 /* milliseconds */ #define QUICLY_LOCAL_MAX_ACK_DELAY 25 /* milliseconds */ diff --git a/include/quicly/frame.h b/include/quicly/frame.h index e0d2863c3..2d2d67143 100644 --- a/include/quicly/frame.h +++ b/include/quicly/frame.h @@ -57,6 +57,7 @@ extern "C" { #define QUICLY_FRAME_TYPE_TRANSPORT_CLOSE 28 #define QUICLY_FRAME_TYPE_APPLICATION_CLOSE 29 #define QUICLY_FRAME_TYPE_HANDSHAKE_DONE 30 +#define QUICLY_FRAME_TYPE_ACK_FREQUENCY 0xaf #define QUICLY_FRAME_TYPE_STREAM_BITS 0x7 #define QUICLY_FRAME_TYPE_STREAM_BIT_OFF 0x4 @@ -233,6 +234,17 @@ typedef struct st_quicly_new_token_frame_t { static int quicly_decode_new_token_frame(const uint8_t **src, const uint8_t *end, quicly_new_token_frame_t *frame); +typedef struct st_quicly_ack_frequency_frame_t { + uint64_t sequence; + uint64_t packet_tolerance; + uint64_t max_ack_delay; + uint8_t ignore_order; +} quicly_ack_frequency_frame_t; + +static uint8_t *quicly_encode_ack_frequency_frame(uint8_t *dst, uint64_t sequence, uint64_t packet_tolerance, + uint64_t max_ack_delay, int ignore_order); +static int quicly_decode_ack_frequency_frame(const uint8_t **src, const uint8_t *end, quicly_ack_frequency_frame_t *frame); + /* inline definitions */ inline uint16_t quicly_decode16(const uint8_t **src) @@ -644,6 +656,42 @@ inline int quicly_decode_new_token_frame(const uint8_t **src, const uint8_t *end return QUICLY_TRANSPORT_ERROR_FRAME_ENCODING; } +inline uint8_t *quicly_encode_ack_frequency_frame(uint8_t *dst, uint64_t sequence, uint64_t packet_tolerance, + uint64_t max_ack_delay, int ignore_order) +{ + *dst++ = QUICLY_FRAME_TYPE_ACK_FREQUENCY; + dst = quicly_encodev(dst, sequence); + dst = quicly_encodev(dst, packet_tolerance); + dst = quicly_encodev(dst, max_ack_delay); + *dst++ = !!ignore_order; + return dst; +} + +inline int quicly_decode_ack_frequency_frame(const uint8_t **src, const uint8_t *end, quicly_ack_frequency_frame_t *frame) +{ + if ((frame->sequence = quicly_decodev(src, end)) == UINT64_MAX) + goto Error; + if ((frame->packet_tolerance = quicly_decodev(src, end)) == UINT64_MAX || frame->packet_tolerance == 0) + goto Error; + if ((frame->max_ack_delay = quicly_decodev(src, end)) == UINT64_MAX) + goto Error; + if (*src == end) + goto Error; + switch (*(*src)++) { + case 0: + frame->ignore_order = 0; + break; + case 1: + frame->ignore_order = 1; + break; + default: + goto Error; + } + return 0; +Error: + return QUICLY_TRANSPORT_ERROR_FRAME_ENCODING; +} + #ifdef __cplusplus } #endif diff --git a/lib/quicly.c b/lib/quicly.c index 73625c1f4..909bb238c 100644 --- a/lib/quicly.c +++ b/lib/quicly.c @@ -59,6 +59,7 @@ #define QUICLY_TRANSPORT_PARAMETER_ID_MAX_ACK_DELAY 11 #define QUICLY_TRANSPORT_PARAMETER_ID_DISABLE_ACTIVE_MIGRATION 12 #define QUICLY_TRANSPORT_PARAMETER_ID_PREFERRED_ADDRESS 13 +#define QUICLY_TRANSPORT_PARAMETER_ID_MIN_ACK_DELAY 0xde1a #define QUICLY_EPOCH_INITIAL 0 #define QUICLY_EPOCH_0RTT 1 @@ -129,9 +130,17 @@ struct st_quicly_pn_space_t { */ uint64_t next_expected_packet_number; /** - * packet count before ack is sent + * number of ACK-eliciting packets that have not been ACKed yet */ uint32_t unacked_count; + /** + * maximum number of ACK-eliciting packets to be queued before sending an ACK + */ + uint32_t packet_tolerance; + /** + * boolean indicating if reorder should NOT trigger an immediate ack + */ + uint8_t ignore_order; }; struct st_quicly_handshake_space_t { @@ -210,6 +219,12 @@ struct st_quicly_conn_t { struct { quicly_maxsender_t *uni, *bidi; } max_streams; + /** + * + */ + struct { + uint64_t next_sequence; + } ack_frequency; } ingress; /** * @@ -275,6 +290,13 @@ struct st_quicly_conn_t { uint64_t max_acked; uint32_t num_inflight; } new_token; + /** + * + */ + struct { + int64_t update_at; + uint64_t sequence; + } ack_frequency; /** * */ @@ -357,8 +379,9 @@ static int update_traffic_key_cb(ptls_update_traffic_key_t *self, ptls_t *tls, i static int initiate_close(quicly_conn_t *conn, int err, uint64_t frame_type, const char *reason_phrase); static int discard_sentmap_by_epoch(quicly_conn_t *conn, unsigned ack_epochs); -static const quicly_transport_parameters_t default_transport_params = { - {0, 0, 0}, 0, 0, 0, 0, QUICLY_DEFAULT_ACK_DELAY_EXPONENT, QUICLY_DEFAULT_MAX_ACK_DELAY}; +static const quicly_transport_parameters_t default_transport_params = {.ack_delay_exponent = QUICLY_DEFAULT_ACK_DELAY_EXPONENT, + .max_ack_delay = QUICLY_DEFAULT_MAX_ACK_DELAY, + .min_ack_delay_usec = UINT64_MAX}; static __thread int64_t now; @@ -456,6 +479,23 @@ static void dispose_cipher(struct st_quicly_cipher_context_t *ctx) ptls_cipher_free(ctx->header_protection); } +/** + * Returns the timeout for sentmap entries. This timeout is also used as the duration of CLOSING / DRAINING state, and therefore be + * longer than 3PTO. At the moment, the value is 4PTO. + */ +static int64_t get_sentmap_expiration_time(quicly_conn_t *conn) +{ + return quicly_rtt_get_pto(&conn->egress.loss.rtt, conn->super.peer.transport_params.max_ack_delay, + conn->egress.loss.conf->min_pto) * + 4; +} + +static void ack_frequency_set_next_update_at(quicly_conn_t *conn) +{ + if (conn->super.peer.transport_params.min_ack_delay_usec != UINT64_MAX) + conn->egress.ack_frequency.update_at = now + get_sentmap_expiration_time(conn); +} + size_t quicly_decode_packet(quicly_context_t *ctx, quicly_decoded_packet_t *packet, const uint8_t *src, size_t len) { const uint8_t *src_end = src + len; @@ -1048,6 +1088,8 @@ static struct st_quicly_pn_space_t *alloc_pn_space(size_t sz) space->largest_pn_received_at = INT64_MAX; space->next_expected_packet_number = 0; space->unacked_count = 0; + space->packet_tolerance = QUICLY_DEFAULT_PACKET_TOLERANCE; + space->ignore_order = 0; if (sz != sizeof(*space)) memset((uint8_t *)space + sizeof(*space), 0, sz - sizeof(*space)); @@ -1091,7 +1133,7 @@ static int record_receipt(quicly_conn_t *conn, struct st_quicly_pn_space_t *spac if ((ret = record_pn(&space->ack_queue, pn, &is_out_of_order)) != 0) goto Exit; - ack_now = is_out_of_order && !is_ack_only; + ack_now = is_out_of_order && !space->ignore_order && !is_ack_only; /* update largest_pn_received_at (TODO implement deduplication at an earlier moment?) */ if (space->ack_queue.ranges[space->ack_queue.num_ranges - 1].end == pn + 1) @@ -1101,8 +1143,7 @@ static int record_receipt(quicly_conn_t *conn, struct st_quicly_pn_space_t *spac if (!is_ack_only) { space->unacked_count++; /* Ack after QUICLY_NUM_PACKETS_BEFORE_ACK packets or after the delayed ack timeout */ - if (space->unacked_count >= QUICLY_NUM_PACKETS_BEFORE_ACK || epoch == QUICLY_EPOCH_INITIAL || - epoch == QUICLY_EPOCH_HANDSHAKE) + if (space->unacked_count >= space->packet_tolerance || epoch == QUICLY_EPOCH_INITIAL || epoch == QUICLY_EPOCH_HANDSHAKE) ack_now = 1; } @@ -1457,6 +1498,8 @@ int quicly_encode_transport_parameter_list(ptls_buffer_t *buf, int is_client, co { ptls_buffer_push_quicint(buf, QUICLY_LOCAL_ACK_DELAY_EXPONENT); }); if (QUICLY_LOCAL_MAX_ACK_DELAY != QUICLY_DEFAULT_MAX_ACK_DELAY) PUSH_TP(buf, QUICLY_TRANSPORT_PARAMETER_ID_MAX_ACK_DELAY, { ptls_buffer_push_quicint(buf, QUICLY_LOCAL_MAX_ACK_DELAY); }); + PUSH_TP(buf, QUICLY_TRANSPORT_PARAMETER_ID_MIN_ACK_DELAY, + { ptls_buffer_push_quicint(buf, QUICLY_LOCAL_MAX_ACK_DELAY * 1000 /* in microseconds */); }); if (params->disable_active_migration) PUSH_TP(buf, QUICLY_TRANSPORT_PARAMETER_ID_DISABLE_ACTIVE_MIGRATION, {}); /* if requested, add a greasing TP of 1 MTU size so that CH spans across multiple packets */ @@ -1601,6 +1644,16 @@ int quicly_decode_transport_parameter_list(quicly_transport_parameters_t *params } params->max_ack_delay = (uint16_t)v; }); + DECODE_ONE_EXTENSION(QUICLY_TRANSPORT_PARAMETER_ID_MIN_ACK_DELAY, { + if ((params->min_ack_delay_usec = ptls_decode_quicint(&src, end)) == UINT64_MAX) { + ret = QUICLY_TRANSPORT_ERROR_TRANSPORT_PARAMETER; + goto Exit; + } + if (params->min_ack_delay_usec >= 16777216) { /* "values of 2^24 or greater are invalid" */ + ret = QUICLY_TRANSPORT_ERROR_TRANSPORT_PARAMETER; + goto Exit; + } + }); DECODE_ONE_EXTENSION(QUICLY_TRANSPORT_PARAMETER_ID_DISABLE_ACTIVE_MIGRATION, { params->disable_active_migration = 1; }); /* skip unknown extension */ if (ext_index >= 0) @@ -1608,6 +1661,14 @@ int quicly_decode_transport_parameter_list(quicly_transport_parameters_t *params }); } + /* check consistency between the transpart parameters */ + if (params->min_ack_delay_usec != UINT64_MAX) { + if (params->min_ack_delay_usec > params->max_ack_delay * 1000) { + ret = QUICLY_TRANSPORT_ERROR_PROTOCOL_VIOLATION; + goto Exit; + } + } + ret = 0; Exit: if (ret == PTLS_ALERT_DECODE_ERROR) @@ -1766,6 +1827,7 @@ static int client_collected_extensions(ptls_t *tls, ptls_handshake_properties_t /* store the results */ conn->super.peer.stateless_reset.token = conn->super.peer.stateless_reset._buf; conn->super.peer.transport_params = params; + ack_frequency_set_next_update_at(conn); Exit: return ret; /* negative error codes used to transmit QUIC errors through picotls */ @@ -1867,6 +1929,8 @@ static int server_collected_extensions(ptls_t *tls, ptls_handshake_properties_t goto Exit; } + ack_frequency_set_next_update_at(conn); + /* set transport_parameters extension to be sent in EE */ assert(properties->additional_extensions == NULL); ptls_buffer_init(&conn->crypto.transport_params.buf, "", 0); @@ -2572,13 +2636,27 @@ static int _do_allocate_frame(quicly_conn_t *conn, quicly_send_context_t *s, siz s->dst_end -= s->target.cipher->aead->algo->tag_size; assert(s->dst_end - s->dst >= QUICLY_MAX_PN_SIZE - QUICLY_SEND_PN_SIZE); - /* register to sentmap */ if (conn->super.state < QUICLY_STATE_CLOSING) { + /* register to sentmap */ uint8_t ack_epoch = get_epoch(s->current.first_byte); if (ack_epoch == QUICLY_EPOCH_0RTT) ack_epoch = QUICLY_EPOCH_1RTT; if ((ret = quicly_sentmap_prepare(&conn->egress.sentmap, conn->egress.packet_number, now, ack_epoch)) != 0) return ret; + /* adjust ack-frequency */ + if (now >= conn->egress.ack_frequency.update_at) { + if (conn->initial == NULL && conn->handshake == NULL && !quicly_cc_is_in_startup(&conn->egress.cc)) { + uint32_t eighth_of_cwnd = conn->egress.cc.cwnd / 8; + if (eighth_of_cwnd > conn->super.ctx->max_packet_size * 3) { + uint32_t packet_tolerance = eighth_of_cwnd / conn->super.ctx->max_packet_size; + if (packet_tolerance > 100) + packet_tolerance = 100; + s->dst = quicly_encode_ack_frequency_frame(s->dst, conn->egress.ack_frequency.sequence++, packet_tolerance, + conn->super.peer.transport_params.max_ack_delay * 1000, 0); + } + } + ack_frequency_set_next_update_at(conn); + } } TargetReady: @@ -2864,17 +2942,6 @@ int quicly_send_stream(quicly_stream_t *stream, quicly_send_context_t *s) return 0; } -/** - * Returns the timeout for sentmap entries. This timeout is also used as the duration of CLOSING / DRAINING state, and therefore be - * longer than 3PTO. At the moment, the value is 4PTO. - */ -static int64_t get_sentmap_expiration_time(quicly_conn_t *conn) -{ - return quicly_rtt_get_pto(&conn->egress.loss.rtt, conn->super.peer.transport_params.max_ack_delay, - conn->egress.loss.conf->min_pto) * - 4; -} - static void init_acks_iter(quicly_conn_t *conn, quicly_sentmap_iter_t *iter) { /* TODO find a better threshold */ @@ -4406,13 +4473,37 @@ static int handle_handshake_done_frame(quicly_conn_t *conn, struct st_quicly_han return 0; } +static int handle_ack_frequency_frame(quicly_conn_t *conn, struct st_quicly_handle_payload_state_t *state) +{ + quicly_ack_frequency_frame_t frame; + int ret; + + if ((ret = quicly_decode_ack_frequency_frame(&state->src, state->end, &frame)) != 0) + return ret; + + QUICLY_PROBE(ACK_FREQUENCY_RECEIVE, conn, probe_now(), frame.sequence, frame.packet_tolerance, frame.max_ack_delay, + frame.ignore_order); + + if (frame.max_ack_delay != QUICLY_LOCAL_MAX_ACK_DELAY * 1000) + return QUICLY_TRANSPORT_ERROR_PROTOCOL_VIOLATION; + + if (frame.sequence >= conn->ingress.ack_frequency.next_sequence) { + conn->ingress.ack_frequency.next_sequence = frame.sequence + 1; + conn->application->super.packet_tolerance = + (uint32_t)(frame.packet_tolerance < QUICLY_MAX_PACKET_TOLERANCE ? frame.packet_tolerance : QUICLY_MAX_PACKET_TOLERANCE); + conn->application->super.ignore_order = frame.ignore_order; + } + + return 0; +} + static int handle_payload(quicly_conn_t *conn, size_t epoch, const uint8_t *_src, size_t _len, uint64_t *offending_frame_type, int *is_ack_only) { /* clang-format off */ /* `frame_handlers` is an array of frame handlers and the properties of the frames, indexed by the ID of the frame. */ - static const struct { + static const struct st_quicly_frame_handler_t { int (*cb)(quicly_conn_t *, struct st_quicly_handle_payload_state_t *); /* callback function that handles the frame */ uint8_t permitted_epochs; /* the epochs the frame can appear, calculated as bitwise-or of `1 << epoch` */ uint8_t ack_eliciting; /* boolean indicating if the frame is ack-eliciting */ @@ -4462,6 +4553,29 @@ static int handle_payload(quicly_conn_t *conn, size_t epoch, const uint8_t *_src /* +----------------------+----+----+----+----+---------------+ */ #undef FRAME }; + static const struct { + uint64_t type; + struct st_quicly_frame_handler_t _; + } ex_frame_handlers[] = { +#define FRAME(uc, lc, i, z, h, o, ae) \ + { \ + QUICLY_FRAME_TYPE_##uc, \ + { \ + handle_##lc##_frame, \ + (i << QUICLY_EPOCH_INITIAL) | (z << QUICLY_EPOCH_0RTT) | (h << QUICLY_EPOCH_HANDSHAKE) | (o << QUICLY_EPOCH_1RTT), \ + ae \ + }, \ + } + /* +-------------------------------+-------------------+---------------+ + * | frame | permitted epochs | | + * |---------------+---------------+----+----+----+----+ ack-eliciting | + * | upper-case | lower-case | IN | 0R | HS | 1R | | + * +---------------+---------------+----+----+----+----+---------------+ */ + FRAME( ACK_FREQUENCY , ack_frequency , 0 , 0 , 0 , 1 , 1 ), + /* +---------------+---------------+-------------------+---------------+ */ +#undef FRAME + {UINT64_MAX}, + }; /* clang-format on */ struct st_quicly_handle_payload_state_t state = {_src, _src + _len, epoch}; @@ -4469,18 +4583,35 @@ static int handle_payload(quicly_conn_t *conn, size_t epoch, const uint8_t *_src int ret; do { + /* determine the frame type; fast path is available for frame types below 64 */ + const struct st_quicly_frame_handler_t *frame_handler; state.frame_type = *state.src++; - if (state.frame_type >= sizeof(frame_handlers) / sizeof(frame_handlers[0])) { - ret = QUICLY_TRANSPORT_ERROR_FRAME_ENCODING; - break; + if (state.frame_type < sizeof(frame_handlers) / sizeof(frame_handlers[0])) { + frame_handler = frame_handlers + state.frame_type; + } else { + /* slow path */ + --state.src; + if ((state.frame_type = quicly_decodev(&state.src, state.end)) == UINT64_MAX) { + ret = QUICLY_TRANSPORT_ERROR_FRAME_ENCODING; + break; + } + size_t i; + for (i = 0; ex_frame_handlers[i].type < state.frame_type; ++i) + ; + if (ex_frame_handlers[i].type != state.frame_type) { + ret = QUICLY_TRANSPORT_ERROR_FRAME_ENCODING; /* not found */ + break; + } + frame_handler = &ex_frame_handlers[i]._; } - if ((frame_handlers[state.frame_type].permitted_epochs & (1 << epoch)) == 0) { + /* check if frame is allowed, then process */ + if ((frame_handler->permitted_epochs & (1 << epoch)) == 0) { ret = QUICLY_TRANSPORT_ERROR_PROTOCOL_VIOLATION; break; } num_frames += 1; - num_frames_ack_eliciting += frame_handlers[state.frame_type].ack_eliciting; - if ((ret = (*frame_handlers[state.frame_type].cb)(conn, &state)) != 0) + num_frames_ack_eliciting += frame_handler->ack_eliciting; + if ((ret = frame_handler->cb(conn, &state)) != 0) break; } while (state.src != state.end); diff --git a/quicly-probes.d b/quicly-probes.d index f829634f3..f5915350b 100644 --- a/quicly-probes.d +++ b/quicly-probes.d @@ -89,6 +89,9 @@ provider quicly { probe stream_data_blocked_receive(struct st_quicly_conn_t *conn, int64_t at, int64_t stream_id, uint64_t limit); + probe ack_frequency_receive(struct st_quicly_conn_t *conn, int64_t at, uint64_t sequence, uint64_t packet_tolerance, + uint64_t max_ack_delay, int ignore_order); + probe quictrace_sent(struct st_quicly_conn_t *conn, int64_t at, uint64_t pn, size_t len, uint8_t packet_type); probe quictrace_recv(struct st_quicly_conn_t *conn, int64_t at, uint64_t pn); probe quictrace_send_stream(struct st_quicly_conn_t *conn, int64_t at, struct st_quicly_stream_t *stream, uint64_t off, From b740b52504989100670294cb14523c438d5473b5 Mon Sep 17 00:00:00 2001 From: Kazuho Oku Date: Tue, 7 Apr 2020 17:18:07 +0900 Subject: [PATCH 2/6] fixes --- include/quicly/frame.h | 2 +- lib/quicly.c | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/include/quicly/frame.h b/include/quicly/frame.h index 2d2d67143..2a422923f 100644 --- a/include/quicly/frame.h +++ b/include/quicly/frame.h @@ -659,7 +659,7 @@ inline int quicly_decode_new_token_frame(const uint8_t **src, const uint8_t *end inline uint8_t *quicly_encode_ack_frequency_frame(uint8_t *dst, uint64_t sequence, uint64_t packet_tolerance, uint64_t max_ack_delay, int ignore_order) { - *dst++ = QUICLY_FRAME_TYPE_ACK_FREQUENCY; + dst = quicly_encodev(dst, QUICLY_FRAME_TYPE_ACK_FREQUENCY); dst = quicly_encodev(dst, sequence); dst = quicly_encodev(dst, packet_tolerance); dst = quicly_encodev(dst, max_ack_delay); diff --git a/lib/quicly.c b/lib/quicly.c index 909bb238c..f2ffe5ec2 100644 --- a/lib/quicly.c +++ b/lib/quicly.c @@ -1761,6 +1761,7 @@ static quicly_conn_t *create_connection(quicly_context_t *ctx, const char *serve 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; quicly_cc_init(&conn->_.egress.cc); quicly_linklist_init(&conn->_.egress.pending_streams.blocked.uni); @@ -2645,7 +2646,7 @@ static int _do_allocate_frame(quicly_conn_t *conn, quicly_send_context_t *s, siz return ret; /* adjust ack-frequency */ if (now >= conn->egress.ack_frequency.update_at) { - if (conn->initial == NULL && conn->handshake == NULL && !quicly_cc_is_in_startup(&conn->egress.cc)) { + if (conn->egress.packet_number >= 1000 && conn->initial == NULL && conn->handshake == NULL) { uint32_t eighth_of_cwnd = conn->egress.cc.cwnd / 8; if (eighth_of_cwnd > conn->super.ctx->max_packet_size * 3) { uint32_t packet_tolerance = eighth_of_cwnd / conn->super.ctx->max_packet_size; From ea1a31adb63edfd2c646db9ed8a7be0b55efd68a Mon Sep 17 00:00:00 2001 From: Kazuho Oku Date: Tue, 7 Apr 2020 17:23:33 +0900 Subject: [PATCH 3/6] revert changes to cc.h, turned out to be unnecessary --- include/quicly/cc.h | 13 ++----------- 1 file changed, 2 insertions(+), 11 deletions(-) diff --git a/include/quicly/cc.h b/include/quicly/cc.h index f0b09a298..bb02362a5 100644 --- a/include/quicly/cc.h +++ b/include/quicly/cc.h @@ -48,26 +48,17 @@ void quicly_cc_init(quicly_cc_t *cc); * Called when a packet is newly acknowledged. */ void quicly_cc_on_acked(quicly_cc_t *cc, uint32_t bytes, uint64_t largest_acked, uint32_t inflight); + /** * Called when a packet is detected as lost. |next_pn| is the next unsent packet number, * used for setting the recovery window. */ void quicly_cc_on_lost(quicly_cc_t *cc, uint32_t bytes, uint64_t lost_pn, uint64_t next_pn); + /** * Called when persistent congestion is observed. */ void quicly_cc_on_persistent_congestion(quicly_cc_t *cc); -/** - * Returns a boolean indicating if the CC is in startup (i.e. slow-start) - */ -static int quicly_cc_is_in_startup(quicly_cc_t *cc); - -/* inline definitions */ - -inline int quicly_cc_is_in_startup(quicly_cc_t *cc) -{ - return cc->cwnd < cc->ssthresh; -} #ifdef __cplusplus } From d427a8cb240ef7295edca0a2b31e7a7e2afbeff0 Mon Sep 17 00:00:00 2001 From: Kazuho Oku Date: Wed, 8 Apr 2020 13:24:27 +0900 Subject: [PATCH 4/6] use constants --- include/quicly/constants.h | 7 +++++-- lib/quicly.c | 13 +++++++------ 2 files changed, 12 insertions(+), 8 deletions(-) diff --git a/include/quicly/constants.h b/include/quicly/constants.h index c1d24039b..99b55f2a8 100644 --- a/include/quicly/constants.h +++ b/include/quicly/constants.h @@ -30,8 +30,6 @@ extern "C" { #include #include "picotls.h" -#define QUICLY_DEFAULT_PACKET_TOLERANCE 2 -#define QUICLY_MAX_PACKET_TOLERANCE 100 #define QUICLY_DELAYED_ACK_TIMEOUT 25 /* milliseconds */ #define QUICLY_DEFAULT_MAX_ACK_DELAY 25 /* milliseconds */ #define QUICLY_LOCAL_MAX_ACK_DELAY 25 /* milliseconds */ @@ -41,6 +39,11 @@ extern "C" { #define QUICLY_DEFAULT_INITIAL_RTT 66 /* initial retransmission timeout is *3, i.e. 200ms */ #define QUICLY_LOSS_DEFAULT_PACKET_THRESHOLD 3 +#define QUICLY_DEFAULT_PACKET_TOLERANCE 2 +#define QUICLY_MAX_PACKET_TOLERANCE 100 +#define QUICLY_FIRST_ACK_FREQUENCY_PACKET_NUMBER 1000 +#define QUICLY_ACK_FREQUENCY_CWND_FRACTION 8 + #define QUICLY_MAX_PACKET_SIZE 1280 /* must be >= 1200 bytes */ #define QUICLY_AEAD_TAG_SIZE 16 diff --git a/lib/quicly.c b/lib/quicly.c index f2ffe5ec2..76aaadd11 100644 --- a/lib/quicly.c +++ b/lib/quicly.c @@ -2646,12 +2646,13 @@ static int _do_allocate_frame(quicly_conn_t *conn, quicly_send_context_t *s, siz return ret; /* adjust ack-frequency */ if (now >= conn->egress.ack_frequency.update_at) { - if (conn->egress.packet_number >= 1000 && conn->initial == NULL && conn->handshake == NULL) { - uint32_t eighth_of_cwnd = conn->egress.cc.cwnd / 8; - if (eighth_of_cwnd > conn->super.ctx->max_packet_size * 3) { - uint32_t packet_tolerance = eighth_of_cwnd / conn->super.ctx->max_packet_size; - if (packet_tolerance > 100) - packet_tolerance = 100; + if (conn->egress.packet_number >= QUICLY_FIRST_ACK_FREQUENCY_PACKET_NUMBER && conn->initial == NULL && + conn->handshake == NULL) { + uint32_t fraction_of_cwnd = conn->egress.cc.cwnd / QUICLY_ACK_FREQUENCY_CWND_FRACTION; + if (fraction_of_cwnd >= conn->super.ctx->max_packet_size * 3) { + uint32_t packet_tolerance = fraction_of_cwnd / conn->super.ctx->max_packet_size; + if (packet_tolerance > QUICLY_MAX_PACKET_TOLERANCE) + packet_tolerance = QUICLY_MAX_PACKET_TOLERANCE; s->dst = quicly_encode_ack_frequency_frame(s->dst, conn->egress.ack_frequency.sequence++, packet_tolerance, conn->super.peer.transport_params.max_ack_delay * 1000, 0); } From ac3b36c585d98a44be55edf694374de44e6b2ea0 Mon Sep 17 00:00:00 2001 From: Kazuho Oku Date: Thu, 9 Apr 2020 21:23:17 +0900 Subject: [PATCH 5/6] add comment --- lib/quicly.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/lib/quicly.c b/lib/quicly.c index 76aaadd11..6e5fe06d4 100644 --- a/lib/quicly.c +++ b/lib/quicly.c @@ -4486,6 +4486,8 @@ static int handle_ack_frequency_frame(quicly_conn_t *conn, struct st_quicly_hand QUICLY_PROBE(ACK_FREQUENCY_RECEIVE, conn, probe_now(), frame.sequence, frame.packet_tolerance, frame.max_ack_delay, frame.ignore_order); + /* At the moment, the only value that the peer would send is this value, because our TP.min_ack_delay and max_ack_delay are + * equal. */ if (frame.max_ack_delay != QUICLY_LOCAL_MAX_ACK_DELAY * 1000) return QUICLY_TRANSPORT_ERROR_PROTOCOL_VIOLATION; From 88f94dccc16c703fbc1ae191bf53a82300d142d2 Mon Sep 17 00:00:00 2001 From: Kazuho Oku Date: Thu, 9 Apr 2020 21:45:21 +0900 Subject: [PATCH 6/6] -00 does not have `ignore_order` field --- include/quicly/frame.h | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/include/quicly/frame.h b/include/quicly/frame.h index 2a422923f..888f8c26f 100644 --- a/include/quicly/frame.h +++ b/include/quicly/frame.h @@ -663,7 +663,9 @@ inline uint8_t *quicly_encode_ack_frequency_frame(uint8_t *dst, uint64_t sequenc dst = quicly_encodev(dst, sequence); dst = quicly_encodev(dst, packet_tolerance); dst = quicly_encodev(dst, max_ack_delay); +#if 0 // not in -00 *dst++ = !!ignore_order; +#endif return dst; } @@ -677,6 +679,9 @@ inline int quicly_decode_ack_frequency_frame(const uint8_t **src, const uint8_t goto Error; if (*src == end) goto Error; +#if 1 // not in -00 + frame->ignore_order = 0; +#else switch (*(*src)++) { case 0: frame->ignore_order = 0; @@ -687,6 +692,7 @@ inline int quicly_decode_ack_frequency_frame(const uint8_t **src, const uint8_t default: goto Error; } +#endif return 0; Error: return QUICLY_TRANSPORT_ERROR_FRAME_ENCODING;