diff --git a/lib/ngtcp2_acktr.c b/lib/ngtcp2_acktr.c index 8e4e759573b46c7b12960055df0d83d55cd47df4..ca79388576c30d6ec49d8ea3ed16eeae5ed3de9b 100644 --- a/lib/ngtcp2_acktr.c +++ b/lib/ngtcp2_acktr.c @@ -48,8 +48,7 @@ void ngtcp2_acktr_entry_del(ngtcp2_acktr_entry *ent, ngtcp2_mem *mem) { static int greater(int64_t lhs, int64_t rhs) { return lhs > rhs; } -int ngtcp2_acktr_init(ngtcp2_acktr *acktr, int delayed_ack, ngtcp2_log *log, - ngtcp2_mem *mem) { +int ngtcp2_acktr_init(ngtcp2_acktr *acktr, ngtcp2_log *log, ngtcp2_mem *mem) { int rv; rv = ngtcp2_ringbuf_init(&acktr->acks, 128, sizeof(ngtcp2_acktr_ack_entry), @@ -66,9 +65,9 @@ int ngtcp2_acktr_init(ngtcp2_acktr *acktr, int delayed_ack, ngtcp2_log *log, acktr->log = log; acktr->mem = mem; - acktr->flags = - delayed_ack ? NGTCP2_ACKTR_FLAG_DELAYED_ACK : NGTCP2_ACKTR_FLAG_NONE; + acktr->flags = NGTCP2_ACKTR_FLAG_NONE; acktr->first_unacked_ts = UINT64_MAX; + acktr->rx_npkt = 0; return 0; } @@ -322,23 +321,17 @@ int ngtcp2_acktr_recv_ack(ngtcp2_acktr *acktr, const ngtcp2_ack *fr, void ngtcp2_acktr_commit_ack(ngtcp2_acktr *acktr) { acktr->flags &= (uint16_t) ~(NGTCP2_ACKTR_FLAG_ACTIVE_ACK | - NGTCP2_ACKTR_FLAG_DELAYED_ACK_EXPIRED); + NGTCP2_ACKTR_FLAG_IMMEDIATE_ACK); acktr->first_unacked_ts = UINT64_MAX; + acktr->rx_npkt = 0; } int ngtcp2_acktr_require_active_ack(ngtcp2_acktr *acktr, uint64_t max_ack_delay, ngtcp2_tstamp ts) { return (acktr->flags & NGTCP2_ACKTR_FLAG_ACTIVE_ACK) && - (!(acktr->flags & NGTCP2_ACKTR_FLAG_DELAYED_ACK) || - (acktr->flags & NGTCP2_ACKTR_FLAG_DELAYED_ACK_EXPIRED) || - acktr->first_unacked_ts <= ts - max_ack_delay); + acktr->first_unacked_ts <= ts - max_ack_delay; } -void ngtcp2_acktr_expire_delayed_ack(ngtcp2_acktr *acktr) { - acktr->flags |= NGTCP2_ACKTR_FLAG_DELAYED_ACK_EXPIRED; - acktr->first_unacked_ts = UINT64_MAX; -} - -int ngtcp2_acktr_delayed_ack(ngtcp2_acktr *acktr) { - return acktr->flags & NGTCP2_ACKTR_FLAG_DELAYED_ACK; +void ngtcp2_acktr_immediate_ack(ngtcp2_acktr *acktr) { + acktr->flags |= NGTCP2_ACKTR_FLAG_IMMEDIATE_ACK; } diff --git a/lib/ngtcp2_acktr.h b/lib/ngtcp2_acktr.h index 0dfcab22b6172ce6b2c0a75688a6f976fbb077d4..0b7f569b7191526d1182e50fee698223c51ff8cb 100644 --- a/lib/ngtcp2_acktr.h +++ b/lib/ngtcp2_acktr.h @@ -39,6 +39,10 @@ which ngtcp2_acktr stores. */ #define NGTCP2_ACKTR_MAX_ENT 1024 +/* NGTCP2_NUM_IMMEDIATE_ACK_PKT is the maximum number of received + packets which triggers the immediate ACK. */ +#define NGTCP2_NUM_IMMEDIATE_ACK_PKT 2 + struct ngtcp2_conn; typedef struct ngtcp2_conn ngtcp2_conn; @@ -78,9 +82,9 @@ typedef struct { typedef enum { NGTCP2_ACKTR_FLAG_NONE = 0x00, - /* NGTCP2_ACKTR_FLAG_DELAYED_ACK indicates that delayed ACK is - enabled. */ - NGTCP2_ACKTR_FLAG_DELAYED_ACK = 0x01, + /* NGTCP2_ACKTR_FLAG_IMMEDIATE_ACK indicates that immediate + acknowledgement is required. */ + NGTCP2_ACKTR_FLAG_IMMEDIATE_ACK = 0x01, /* NGTCP2_ACKTR_FLAG_ACTIVE_ACK indicates that there are pending protected packet to be acknowledged. */ NGTCP2_ACKTR_FLAG_ACTIVE_ACK = 0x02, @@ -92,9 +96,6 @@ typedef enum { acknowledgement for ACK which acknowledges the last handshake packet from client (which contains TLSv1.3 Finished message). */ NGTCP2_ACKTR_FLAG_ACK_FINISHED_ACK = 0x80, - /* NGTCP2_ACKTR_FLAG_DELAYED_ACK_EXPIRED is set when delayed ACK - timer is expired. */ - NGTCP2_ACKTR_FLAG_DELAYED_ACK_EXPIRED = 0x0100, } ngtcp2_acktr_flag; /* @@ -112,11 +113,12 @@ typedef struct { /* first_unacked_ts is timestamp when ngtcp2_acktr_entry is added first time after the last outgoing protected ACK frame. */ ngtcp2_tstamp first_unacked_ts; + /* rx_npkt is the number of packets received without sending ACK. */ + size_t rx_npkt; } ngtcp2_acktr; /* - * ngtcp2_acktr_init initializes |acktr|. If |delayed_ack| is - * nonzero, delayed ack is enabled. + * ngtcp2_acktr_init initializes |acktr|. * * This function returns 0 if it succeeds, or one of the following * negative error codes: @@ -124,8 +126,7 @@ typedef struct { * NGTCP2_ERR_NOMEM * Out of memory. */ -int ngtcp2_acktr_init(ngtcp2_acktr *acktr, int delayed_ack, ngtcp2_log *log, - ngtcp2_mem *mem); +int ngtcp2_acktr_init(ngtcp2_acktr *acktr, ngtcp2_log *log, ngtcp2_mem *mem); /* * ngtcp2_acktr_free frees resources allocated for |acktr|. It frees @@ -208,16 +209,9 @@ int ngtcp2_acktr_require_active_ack(ngtcp2_acktr *acktr, uint64_t max_ack_delay, ngtcp2_tstamp ts); /* - * ngtcp2_acktr_expire_delayed_ack expires delayed ACK timer. This - * function sets NGTCP2_ACKTR_FLAG_DELAYED_ACK_EXPIRED so that we know - * that the timer has expired. - */ -void ngtcp2_acktr_expire_delayed_ack(ngtcp2_acktr *acktr); - -/* - * ngtcp2_acktr_delayed_ack returns nonzero if |acktr| enables delayed - * ACK. + * ngtcp2_acktr_immediate_ack tells |acktr| that immediate + * acknowledgement is required. */ -int ngtcp2_acktr_delayed_ack(ngtcp2_acktr *acktr); +void ngtcp2_acktr_immediate_ack(ngtcp2_acktr *acktr); #endif /* NGTCP2_ACKTR_H */ diff --git a/lib/ngtcp2_conn.c b/lib/ngtcp2_conn.c index 125c3bedec5af4176a98edb49681fa0e9e161450..019899b2a96697693a48560778411c2cb89e144b 100644 --- a/lib/ngtcp2_conn.c +++ b/lib/ngtcp2_conn.c @@ -159,8 +159,8 @@ static int crypto_offset_less(const ngtcp2_pq_entry *lhs, return lfrc->fr.offset < rfrc->fr.offset; } -static int pktns_init(ngtcp2_pktns *pktns, int delayed_ack, ngtcp2_cc_stat *ccs, - ngtcp2_log *log, ngtcp2_mem *mem) { +static int pktns_init(ngtcp2_pktns *pktns, ngtcp2_cc_stat *ccs, ngtcp2_log *log, + ngtcp2_mem *mem) { int rv; rv = ngtcp2_gaptr_init(&pktns->pngap, mem); @@ -170,7 +170,7 @@ static int pktns_init(ngtcp2_pktns *pktns, int delayed_ack, ngtcp2_cc_stat *ccs, pktns->last_tx_pkt_num = (uint64_t)-1; - rv = ngtcp2_acktr_init(&pktns->acktr, delayed_ack, log, mem); + rv = ngtcp2_acktr_init(&pktns->acktr, log, mem); if (rv != 0) { ngtcp2_gaptr_free(&pktns->pngap); return rv; @@ -269,20 +269,17 @@ static int conn_new(ngtcp2_conn **pconn, const ngtcp2_cid *dcid, ngtcp2_log_init(&(*pconn)->log, &(*pconn)->scid, settings->log_printf, settings->initial_ts, user_data); - rv = pktns_init(&(*pconn)->in_pktns, 0 /* delayed_ack */, &(*pconn)->ccs, - &(*pconn)->log, mem); + rv = pktns_init(&(*pconn)->in_pktns, &(*pconn)->ccs, &(*pconn)->log, mem); if (rv != 0) { goto fail_in_pktns_init; } - rv = pktns_init(&(*pconn)->hs_pktns, 0 /* delayed_ack */, &(*pconn)->ccs, - &(*pconn)->log, mem); + rv = pktns_init(&(*pconn)->hs_pktns, &(*pconn)->ccs, &(*pconn)->log, mem); if (rv != 0) { goto fail_hs_pktns_init; } - rv = pktns_init(&(*pconn)->pktns, 1 /* delayed_ack */, &(*pconn)->ccs, - &(*pconn)->log, mem); + rv = pktns_init(&(*pconn)->pktns, &(*pconn)->ccs, &(*pconn)->log, mem); if (rv != 0) { goto fail_pktns_init; } @@ -515,7 +512,7 @@ static uint64_t conn_compute_ack_delay(ngtcp2_conn *conn) { */ static int conn_create_ack_frame(ngtcp2_conn *conn, ngtcp2_frame **pfr, ngtcp2_acktr *acktr, ngtcp2_tstamp ts, - int nodelay, uint8_t ack_delay_exponent) { + uint8_t ack_delay_exponent) { uint64_t first_pkt_num; uint64_t last_pkt_num; ngtcp2_ack_blk *blk; @@ -530,11 +527,10 @@ static int conn_create_ack_frame(ngtcp2_conn *conn, ngtcp2_frame **pfr, size_t num_blks_max = 8; size_t blk_idx; int rv; - uint64_t max_ack_delay = (nodelay || !ngtcp2_acktr_delayed_ack(acktr)) - ? 0 - : conn_compute_ack_delay(conn); - if (!ngtcp2_acktr_require_active_ack(acktr, max_ack_delay, ts)) { + if (!(acktr->flags & NGTCP2_ACKTR_FLAG_IMMEDIATE_ACK) && + !ngtcp2_acktr_require_active_ack(acktr, conn_compute_ack_delay(conn), + ts)) { return 0; } @@ -995,7 +991,7 @@ static ssize_t conn_write_handshake_pkt(ngtcp2_conn *conn, uint8_t *dest, ctx.user_data = conn; if (type != NGTCP2_PKT_0RTT_PROTECTED) { - rv = conn_create_ack_frame(conn, &ackfr, &pktns->acktr, ts, 0 /* nodelay */, + rv = conn_create_ack_frame(conn, &ackfr, &pktns->acktr, ts, NGTCP2_DEFAULT_ACK_DELAY_EXPONENT); if (rv != 0) { return rv; @@ -1216,7 +1212,6 @@ static ssize_t conn_write_handshake_ack_pkt(ngtcp2_conn *conn, uint8_t *dest, ackfr = NULL; rv = conn_create_ack_frame(conn, &ackfr, &pktns->acktr, ts, - force_initial /* nodelay */, NGTCP2_DEFAULT_ACK_DELAY_EXPONENT); if (rv != 0) { return rv; @@ -1433,8 +1428,7 @@ static ssize_t conn_write_handshake_pkts(ngtcp2_conn *conn, uint8_t *dest, } static ssize_t conn_write_protected_ack_pkt(ngtcp2_conn *conn, uint8_t *dest, - size_t destlen, int nodelay, - ngtcp2_tstamp ts); + size_t destlen, ngtcp2_tstamp ts); static ssize_t conn_write_server_handshake(ngtcp2_conn *conn, uint8_t *dest, size_t destlen, ngtcp2_tstamp ts) { @@ -1455,8 +1449,7 @@ static ssize_t conn_write_server_handshake(ngtcp2_conn *conn, uint8_t *dest, /* Acknowledge 0-RTT packet here. */ if (conn->pktns.tx_ckm) { - nwrite = conn_write_protected_ack_pkt(conn, dest, destlen, - res > 0 /* nodelay */, ts); + nwrite = conn_write_protected_ack_pkt(conn, dest, destlen, ts); if (nwrite < 0) { if (nwrite != NGTCP2_ERR_NOBUF) { return nwrite; @@ -1881,7 +1874,6 @@ tx_strmq_finish: } rv = conn_create_ack_frame(conn, &ackfr, &pktns->acktr, ts, - !pkt_empty /* nodelay */, conn->local_settings.ack_delay_exponent); if (rv != 0) { assert(ngtcp2_err_is_fatal(rv)); @@ -2070,15 +2062,14 @@ static ssize_t conn_write_single_frame_pkt(ngtcp2_conn *conn, uint8_t *dest, * Out of memory. */ static ssize_t conn_write_protected_ack_pkt(ngtcp2_conn *conn, uint8_t *dest, - size_t destlen, int nodelay, - ngtcp2_tstamp ts) { + size_t destlen, ngtcp2_tstamp ts) { int rv; ssize_t spktlen; ngtcp2_frame *ackfr; ngtcp2_acktr *acktr = &conn->pktns.acktr; ackfr = NULL; - rv = conn_create_ack_frame(conn, &ackfr, acktr, ts, nodelay, + rv = conn_create_ack_frame(conn, &ackfr, acktr, ts, conn->local_settings.ack_delay_exponent); if (rv != 0) { return rv; @@ -2177,7 +2168,7 @@ static ssize_t conn_write_probe_ping(ngtcp2_conn *conn, uint8_t *dest, ngtcp2_log_tx_fr(&conn->log, &hd, &frc->fr); - rv = conn_create_ack_frame(conn, &ackfr, &pktns->acktr, ts, 1 /* nodelay */, + rv = conn_create_ack_frame(conn, &ackfr, &pktns->acktr, ts, conn->local_settings.ack_delay_exponent); if (rv != 0) { goto fail; @@ -2303,8 +2294,7 @@ ssize_t ngtcp2_conn_write_pkt(ngtcp2_conn *conn, uint8_t *dest, size_t destlen, } if (cwnd < NGTCP2_MIN_PKTLEN) { - nwrite = conn_write_protected_ack_pkt(conn, dest, origlen, - 0 /* nodelay */, ts); + nwrite = conn_write_protected_ack_pkt(conn, dest, origlen, ts); if (nwrite) { return nwrite; } @@ -3471,6 +3461,10 @@ static ssize_t conn_recv_handshake_pkt(ngtcp2_conn *conn, const uint8_t *pkt, pktns->max_rx_pkt_num = ngtcp2_max(pktns->max_rx_pkt_num, hd.pkt_num); + if (++pktns->acktr.rx_npkt >= NGTCP2_NUM_IMMEDIATE_ACK_PKT) { + ngtcp2_acktr_immediate_ack(&pktns->acktr); + } + rv = pktns_commit_recv_pkt_num(pktns, hd.pkt_num); if (rv != 0) { return rv; @@ -4197,6 +4191,10 @@ static int conn_recv_delayed_handshake_pkt(ngtcp2_conn *conn, pktns->max_rx_pkt_num = ngtcp2_max(pktns->max_rx_pkt_num, hd->pkt_num); + if (++pktns->acktr.rx_npkt >= NGTCP2_NUM_IMMEDIATE_ACK_PKT) { + ngtcp2_acktr_immediate_ack(&pktns->acktr); + } + rv = pktns_commit_recv_pkt_num(pktns, hd->pkt_num); if (rv != 0) { return rv; @@ -4567,6 +4565,10 @@ static ssize_t conn_recv_pkt(ngtcp2_conn *conn, const uint8_t *pkt, pktns->max_rx_pkt_num = ngtcp2_max(pktns->max_rx_pkt_num, hd.pkt_num); + if (++pktns->acktr.rx_npkt >= NGTCP2_NUM_IMMEDIATE_ACK_PKT) { + ngtcp2_acktr_immediate_ack(&pktns->acktr); + } + rv = pktns_commit_recv_pkt_num(pktns, hd.pkt_num); if (rv != 0) { return rv; @@ -4743,27 +4745,6 @@ int ngtcp2_conn_read_pkt(ngtcp2_conn *conn, const uint8_t *pkt, size_t pktlen, return rv; } -/* - * conn_handle_delayed_ack_expiry handles expired delayed ACK timer - * during handshake. The delayed ACK timer is only used for ACK frame - * in protected packet, but it starts during handshake. Application - * may be awakened by this timer and attempt to send ACK, but this is - * ACK for protected packet. We have no key so we cannot send any - * protected packet. Timer keeps firing and it could become busy loop - * until handshake finishes. We can disable it during handshake, but - * 0-RTT stuff complicates this. This function checks that delayed - * ACK timer has expired, and if so disarm timer. We have internal - * flag which shows that timer has expired. - */ -static void conn_handle_delayed_ack_expiry(ngtcp2_conn *conn, - ngtcp2_tstamp ts) { - if (ngtcp2_conn_ack_delay_expiry(conn) > ts) { - return; - } - - ngtcp2_acktr_expire_delayed_ack(&conn->pktns.acktr); -} - static int conn_check_pkt_num_exhausted(ngtcp2_conn *conn) { return conn->in_pktns.last_tx_pkt_num == NGTCP2_MAX_PKT_NUM || conn->hs_pktns.last_tx_pkt_num == NGTCP2_MAX_PKT_NUM || @@ -4796,8 +4777,6 @@ int ngtcp2_conn_read_handshake(ngtcp2_conn *conn, const uint8_t *pkt, pktlen); } - conn_handle_delayed_ack_expiry(conn, ts); - switch (conn->state) { case NGTCP2_CS_CLIENT_INITIAL: /* TODO Better to log something when we ignore input */ @@ -4896,8 +4875,6 @@ static ssize_t conn_write_handshake(ngtcp2_conn *conn, uint8_t *dest, conn->log.last_ts = ts; - conn_handle_delayed_ack_expiry(conn, ts); - if (conn_check_pkt_num_exhausted(conn)) { return NGTCP2_ERR_PKT_NUM_EXHAUSTED; } @@ -5895,8 +5872,7 @@ ssize_t ngtcp2_conn_writev_stream(ngtcp2_conn *conn, uint8_t *dest, } if (cwnd < NGTCP2_MIN_PKTLEN) { - nwrite = conn_write_protected_ack_pkt(conn, dest, origlen, - 0 /* nodelay */, ts); + nwrite = conn_write_protected_ack_pkt(conn, dest, origlen, ts); if (nwrite) { return nwrite; } diff --git a/tests/ngtcp2_acktr_test.c b/tests/ngtcp2_acktr_test.c index 19a78a7f16d219d7426da14e43d8df6967631e17..f16ef16f1cddbb30025035747ffcfd52c2146d7a 100644 --- a/tests/ngtcp2_acktr_test.c +++ b/tests/ngtcp2_acktr_test.c @@ -41,7 +41,7 @@ void test_ngtcp2_acktr_add(void) { ngtcp2_log log; ngtcp2_log_init(&log, NULL, NULL, 0, NULL); - ngtcp2_acktr_init(&acktr, 0 /* delayed_ack */, &log, mem); + ngtcp2_acktr_init(&acktr, &log, mem); ngtcp2_acktr_entry_new(&ents[0], 1, 1000, mem); ngtcp2_acktr_entry_new(&ents[1], 5, 1001, mem); @@ -82,7 +82,7 @@ void test_ngtcp2_acktr_add(void) { ngtcp2_acktr_free(&acktr); /* Check duplicates */ - ngtcp2_acktr_init(&acktr, 0 /* delayed_ack */, &log, mem); + ngtcp2_acktr_init(&acktr, &log, mem); ngtcp2_acktr_entry_new(&ents[0], 1, 1000, mem); rv = ngtcp2_acktr_add(&acktr, ents[0], 1, 999); @@ -106,7 +106,7 @@ void test_ngtcp2_acktr_eviction(void) { ngtcp2_ksl_it it; ngtcp2_log_init(&log, NULL, NULL, 0, NULL); - ngtcp2_acktr_init(&acktr, 0 /* delayed_ack */, &log, mem); + ngtcp2_acktr_init(&acktr, &log, mem); for (i = 0; i < NGTCP2_ACKTR_MAX_ENT + extra; ++i) { ngtcp2_acktr_entry_new(&ent, i, 0, mem); @@ -125,7 +125,7 @@ void test_ngtcp2_acktr_eviction(void) { ngtcp2_acktr_free(&acktr); /* Invert insertion order */ - ngtcp2_acktr_init(&acktr, 0 /* delayed_ack */, &log, mem); + ngtcp2_acktr_init(&acktr, &log, mem); for (i = NGTCP2_ACKTR_MAX_ENT + extra; i > 0; --i) { ngtcp2_acktr_entry_new(&ent, i - 1, 0, mem); @@ -153,7 +153,7 @@ void test_ngtcp2_acktr_forget(void) { ngtcp2_ksl_it it; ngtcp2_log_init(&log, NULL, NULL, 0, NULL); - ngtcp2_acktr_init(&acktr, 0 /* delayed_ack */, &log, mem); + ngtcp2_acktr_init(&acktr, &log, mem); for (i = 0; i < 7; ++i) { ngtcp2_acktr_entry_new(&ent, i, 0, mem); @@ -211,7 +211,7 @@ void test_ngtcp2_acktr_recv_ack(void) { ngtcp2_ksl_it it; ngtcp2_log_init(&log, NULL, NULL, 0, NULL); - ngtcp2_acktr_init(&acktr, 0 /* delayed_ack */, &log, mem); + ngtcp2_acktr_init(&acktr, &log, mem); for (i = 0; i < arraylen(rpkt_nums); ++i) { ngtcp2_acktr_entry_new(&ent, rpkt_nums[i], 1, mem);