From e281596f69ff0b28181617f426cdcc5490164575 Mon Sep 17 00:00:00 2001
From: Tatsuhiro Tsujikawa <tatsuhiro.t@gmail.com>
Date: Wed, 21 Nov 2018 23:20:54 +0900
Subject: [PATCH] Rework acknowledgements
---
lib/ngtcp2_acktr.c | 23 ++++-------
lib/ngtcp2_acktr.h | 34 +++++++---------
lib/ngtcp2_conn.c | 84 ++++++++++++++-------------------------
tests/ngtcp2_acktr_test.c | 12 +++---
4 files changed, 58 insertions(+), 95 deletions(-)
diff --git a/lib/ngtcp2_acktr.c b/lib/ngtcp2_acktr.c
index 8e4e7595..ca793885 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 0dfcab22..0b7f569b 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 125c3bed..019899b2 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 19a78a7f..f16ef16f 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);
--
GitLab