From 696213633747dc43562eea987042ffe9b298b6a9 Mon Sep 17 00:00:00 2001
From: Tatsuhiro Tsujikawa <tatsuhiro.t@gmail.com>
Date: Mon, 3 Aug 2020 22:41:06 +0900
Subject: [PATCH] Unified PTO retransmission
---
lib/ngtcp2_conn.c | 75 ++++++++-----------------------------
lib/ngtcp2_rtb.c | 80 +++-------------------------------------
lib/ngtcp2_rtb.h | 17 ---------
tests/ngtcp2_conn_test.c | 45 +++++++++++++---------
4 files changed, 48 insertions(+), 169 deletions(-)
diff --git a/lib/ngtcp2_conn.c b/lib/ngtcp2_conn.c
index 89fa2977..db9031cd 100644
--- a/lib/ngtcp2_conn.c
+++ b/lib/ngtcp2_conn.c
@@ -1829,6 +1829,7 @@ static ngtcp2_ssize conn_write_handshake_pkt(ngtcp2_conn *conn, uint8_t *dest,
int padded = 0;
int hd_logged = 0;
uint64_t crypto_offset;
+ ngtcp2_ssize num_reclaimed;
switch (type) {
case NGTCP2_PKT_INITIAL:
@@ -1898,6 +1899,7 @@ static ngtcp2_ssize conn_write_handshake_pkt(ngtcp2_conn *conn, uint8_t *dest,
}
}
+build_pkt:
for (; ngtcp2_ksl_len(&pktns->crypto.tx.frq);) {
left = ngtcp2_ppe_left(&ppe);
@@ -1932,8 +1934,20 @@ static ngtcp2_ssize conn_write_handshake_pkt(ngtcp2_conn *conn, uint8_t *dest,
pfrc = &(*pfrc)->next;
pkt_empty = 0;
- rtb_entry_flags |=
- NGTCP2_RTB_FLAG_ACK_ELICITING | NGTCP2_RTB_FLAG_CRYPTO_PKT;
+ rtb_entry_flags |= NGTCP2_RTB_FLAG_ACK_ELICITING;
+ }
+
+ if (!(rtb_entry_flags & NGTCP2_RTB_FLAG_ACK_ELICITING) &&
+ pktns->rtb.probe_pkt_left) {
+ num_reclaimed = ngtcp2_rtb_reclaim_on_pto(&pktns->rtb, conn, pktns,
+ pktns->rtb.probe_pkt_left + 1);
+ if (num_reclaimed < 0) {
+ ngtcp2_frame_chain_list_del(frq, conn->mem);
+ return rv;
+ }
+ if (num_reclaimed) {
+ goto build_pkt;
+ }
}
/* Don't send any PING frame if client Initial has not been
@@ -5086,8 +5100,6 @@ static int conn_emit_pending_stream_data(ngtcp2_conn *conn, ngtcp2_strm *strm,
}
}
-static int conn_on_crypto_timeout(ngtcp2_conn *conn, ngtcp2_pktns *pktns);
-
/*
* conn_recv_crypto is called when CRYPTO frame |fr| is received.
* |rx_offset_base| is the offset in the entire TLS handshake stream.
@@ -5140,16 +5152,7 @@ static int conn_recv_crypto(ngtcp2_conn *conn, ngtcp2_crypto_level crypto_level,
send a packet containing unacknowledged CRYPTO data earlier
than the PTO expiry, subject to address validation limits;
... */
- rv = conn_on_crypto_timeout(conn, conn->in_pktns);
- if (rv != 0) {
- return rv;
- }
conn->in_pktns->rtb.probe_pkt_left = 1;
-
- rv = conn_on_crypto_timeout(conn, conn->hs_pktns);
- if (rv != 0) {
- return rv;
- }
conn->hs_pktns->rtb.probe_pkt_left = 1;
}
return 0;
@@ -9449,36 +9452,6 @@ void ngtcp2_conn_set_loss_detection_timer(ngtcp2_conn *conn, ngtcp2_tstamp ts) {
(uint64_t)(timeout / NGTCP2_MILLISECONDS));
}
-/*
- * conn_on_crypto_timeout is called when handshake packets which
- * belong to |pktns| are lost.
- *
- * This function returns 0 if it succeeds, or one of the following
- * negative error codes:
- *
- * NGTCP2_ERR_NOMEM
- * Out of memory.
- */
-static int conn_on_crypto_timeout(ngtcp2_conn *conn, ngtcp2_pktns *pktns) {
- ngtcp2_frame_chain *frc = NULL;
- int rv;
-
- rv = ngtcp2_rtb_on_crypto_timeout(&pktns->rtb, &frc, &conn->cstat);
- if (rv != 0) {
- assert(ngtcp2_err_is_fatal(rv));
- ngtcp2_frame_chain_list_del(frc, conn->mem);
- return rv;
- }
-
- rv = ngtcp2_conn_resched_frames(conn, pktns, &frc);
- if (rv != 0) {
- ngtcp2_frame_chain_list_del(frc, conn->mem);
- return rv;
- }
-
- return 0;
-}
-
int ngtcp2_conn_on_loss_detection_timer(ngtcp2_conn *conn, ngtcp2_tstamp ts) {
ngtcp2_conn_stat *cstat = &conn->cstat;
int rv;
@@ -9520,16 +9493,8 @@ int ngtcp2_conn_on_loss_detection_timer(ngtcp2_conn *conn, ngtcp2_tstamp ts) {
if (!conn->server && !(conn->flags & NGTCP2_CONN_FLAG_HANDSHAKE_COMPLETED)) {
if (hs_pktns->crypto.tx.ckm) {
- rv = conn_on_crypto_timeout(conn, hs_pktns);
- if (rv != 0) {
- return rv;
- }
hs_pktns->rtb.probe_pkt_left = 1;
} else {
- rv = conn_on_crypto_timeout(conn, in_pktns);
- if (rv != 0) {
- return rv;
- }
in_pktns->rtb.probe_pkt_left = 1;
}
} else {
@@ -9540,10 +9505,6 @@ int ngtcp2_conn_on_loss_detection_timer(ngtcp2_conn *conn, ngtcp2_tstamp ts) {
switch (earliest_pktns->rtb.pktns_id) {
case NGTCP2_PKTNS_ID_INITIAL:
assert(in_pktns);
- rv = conn_on_crypto_timeout(conn, in_pktns);
- if (rv != 0) {
- return rv;
- }
in_pktns->rtb.probe_pkt_left = 1;
if (!conn->server) {
break;
@@ -9552,10 +9513,6 @@ int ngtcp2_conn_on_loss_detection_timer(ngtcp2_conn *conn, ngtcp2_tstamp ts) {
/* fall through */
case NGTCP2_PKTNS_ID_HANDSHAKE:
assert(hs_pktns);
- rv = conn_on_crypto_timeout(conn, hs_pktns);
- if (rv != 0) {
- return rv;
- }
hs_pktns->rtb.probe_pkt_left = 1;
break;
case NGTCP2_PKTNS_ID_APP:
diff --git a/lib/ngtcp2_rtb.c b/lib/ngtcp2_rtb.c
index 2e8932f3..d2257097 100644
--- a/lib/ngtcp2_rtb.c
+++ b/lib/ngtcp2_rtb.c
@@ -282,7 +282,8 @@ static void rtb_on_remove(ngtcp2_rtb *rtb, ngtcp2_rtb_entry *ent,
return;
}
- if (ent->flags & NGTCP2_RTB_FLAG_ACK_ELICITING) {
+ if ((ent->flags & NGTCP2_RTB_FLAG_ACK_ELICITING) &&
+ !(ent->flags & NGTCP2_RTB_FLAG_PTO_RECLAIMED)) {
assert(rtb->num_ack_eliciting);
--rtb->num_ack_eliciting;
}
@@ -1093,80 +1094,6 @@ int ngtcp2_rtb_remove_all(ngtcp2_rtb *rtb, ngtcp2_conn *conn,
return 0;
}
-int ngtcp2_rtb_on_crypto_timeout(ngtcp2_rtb *rtb, ngtcp2_frame_chain **pfrc,
- ngtcp2_conn_stat *cstat) {
- ngtcp2_rtb_entry *ent;
- ngtcp2_ksl_it it;
- ngtcp2_frame_chain *nfrc;
- ngtcp2_frame_chain *frc;
- ngtcp2_range gap, range;
- ngtcp2_crypto *fr;
- int all_acked;
- int rv;
-
- it = ngtcp2_ksl_begin(&rtb->ents);
-
- for (; !ngtcp2_ksl_it_end(&it);) {
- ent = ngtcp2_ksl_it_get(&it);
-
- if ((ent->flags & NGTCP2_RTB_FLAG_PROBE) ||
- !(ent->flags & NGTCP2_RTB_FLAG_CRYPTO_PKT)) {
- ngtcp2_ksl_it_next(&it);
- continue;
- }
-
- all_acked = 1;
-
- for (frc = ent->frc; frc; frc = frc->next) {
- assert(frc->fr.type == NGTCP2_FRAME_CRYPTO);
-
- fr = &frc->fr.crypto;
-
- /* Don't resend CRYPTO frame if the whole region it contains has
- been acknowledged */
- gap = ngtcp2_strm_get_unacked_range_after(rtb->crypto, fr->offset);
-
- range.begin = fr->offset;
- range.end = fr->offset + ngtcp2_vec_len(fr->data, fr->datacnt);
- range = ngtcp2_range_intersect(&range, &gap);
- if (ngtcp2_range_len(&range) == 0) {
- continue;
- }
-
- all_acked = 0;
-
- if (!(ent->flags & NGTCP2_RTB_FLAG_PTO_RECLAIMED)) {
- rv = ngtcp2_frame_chain_crypto_datacnt_new(
- &nfrc, frc->fr.crypto.datacnt, rtb->mem);
- if (rv != 0) {
- return rv;
- }
-
- nfrc->fr = frc->fr;
- ngtcp2_vec_copy(nfrc->fr.crypto.data, frc->fr.crypto.data,
- frc->fr.crypto.datacnt);
-
- frame_chain_insert(pfrc, nfrc);
- }
- }
-
- if (all_acked) {
- /* If the frames that ent contains have been acknowledged,
- remove it from rtb. Otherwise crypto timer keeps firing. */
- rtb_on_remove(rtb, ent, cstat);
- ngtcp2_ksl_remove(&rtb->ents, &it, &ent->hd.pkt_num);
- ngtcp2_rtb_entry_del(ent, rtb->mem);
- continue;
- }
-
- ent->flags |= NGTCP2_RTB_FLAG_PTO_RECLAIMED;
-
- ngtcp2_ksl_it_next(&it);
- }
-
- return 0;
-}
-
int ngtcp2_rtb_empty(ngtcp2_rtb *rtb) {
return ngtcp2_ksl_len(&rtb->ents) == 0;
}
@@ -1204,6 +1131,9 @@ ngtcp2_ssize ngtcp2_rtb_reclaim_on_pto(ngtcp2_rtb *rtb, ngtcp2_conn *conn,
the next run. */
ent->flags |= NGTCP2_RTB_FLAG_PTO_RECLAIMED;
+ assert(rtb->num_ack_eliciting);
+ --rtb->num_ack_eliciting;
+
if (reclaimed) {
--num_pkts;
}
diff --git a/lib/ngtcp2_rtb.h b/lib/ngtcp2_rtb.h
index f4c7ad9d..bfd45fe6 100644
--- a/lib/ngtcp2_rtb.h
+++ b/lib/ngtcp2_rtb.h
@@ -176,9 +176,6 @@ typedef enum {
/* NGTCP2_RTB_FLAG_PROBE indicates that the entry includes a probe
packet. */
NGTCP2_RTB_FLAG_PROBE = 0x01,
- /* NGTCP2_RTB_FLAG_CRYPTO_PKT indicates that the entry includes
- handshake CRYPTO frame. */
- NGTCP2_RTB_FLAG_CRYPTO_PKT = 0x02,
/* NGTCP2_RTB_FLAG_ACK_ELICITING indicates that the entry elicits
acknowledgement. */
NGTCP2_RTB_FLAG_ACK_ELICITING = 0x04,
@@ -366,20 +363,6 @@ ngtcp2_tstamp ngtcp2_rtb_lost_pkt_ts(ngtcp2_rtb *rtb);
int ngtcp2_rtb_remove_all(ngtcp2_rtb *rtb, ngtcp2_conn *conn,
ngtcp2_pktns *pktns, ngtcp2_conn_stat *cstat);
-/*
- * ngtcp2_rtb_on_crypto_timeout copies all unacknowledged CRYPTO
- * frames and links them to |*pfrc|. The affected ngtcp2_rtb_entry
- * will have NGTCP2_RTB_FLAG_CRYPTO_TIMEOUT_RETRANSMITTED set.
- *
- * This function returns 0 if it succeeds, or one of the following
- * negative error codes:
- *
- * NGTCP2_ERR_NOMEM
- * Out of memory
- */
-int ngtcp2_rtb_on_crypto_timeout(ngtcp2_rtb *rtb, ngtcp2_frame_chain **pfrc,
- ngtcp2_conn_stat *cstat);
-
/*
* ngtcp2_rtb_empty returns nonzero if |rtb| have no entry.
*/
diff --git a/tests/ngtcp2_conn_test.c b/tests/ngtcp2_conn_test.c
index 415b56f5..2d6ee372 100644
--- a/tests/ngtcp2_conn_test.c
+++ b/tests/ngtcp2_conn_test.c
@@ -4796,8 +4796,7 @@ void test_ngtcp2_conn_handshake_probe(void) {
spktlen = ngtcp2_conn_write_pkt(conn, NULL, buf, sizeof(buf), ++t);
CU_ASSERT(spktlen > 0);
- /* We don't make the first packet lost */
- CU_ASSERT(2 == conn->in_pktns->rtb.num_ack_eliciting);
+ CU_ASSERT(1 == conn->in_pktns->rtb.num_ack_eliciting);
CU_ASSERT(0 == conn->in_pktns->rtb.probe_pkt_left);
fr.type = NGTCP2_FRAME_ACK;
@@ -4817,7 +4816,7 @@ void test_ngtcp2_conn_handshake_probe(void) {
rv = ngtcp2_conn_on_loss_detection_timer(conn, ++t);
CU_ASSERT(0 == rv);
- CU_ASSERT(0 == conn->in_pktns->rtb.num_ack_eliciting);
+ CU_ASSERT(1 == conn->in_pktns->rtb.num_ack_eliciting);
CU_ASSERT(1 == conn->in_pktns->rtb.probe_pkt_left);
/* This sends anti-deadlock padded Initial packet even if we have
@@ -4891,6 +4890,7 @@ void test_ngtcp2_conn_handshake_loss(void) {
int64_t pkt_num = -1;
ngtcp2_ksl_it it;
ngtcp2_rtb_entry *ent;
+ ngtcp2_frame_chain *frc;
rcid_init(&rcid);
setup_handshake_server(&conn);
@@ -4939,7 +4939,10 @@ void test_ngtcp2_conn_handshake_loss(void) {
ngtcp2_conn_on_loss_detection_timer(conn, t);
- for (i = 0; i < 3; ++i) {
+ CU_ASSERT(1 == conn->in_pktns->rtb.probe_pkt_left);
+ CU_ASSERT(1 == conn->hs_pktns->rtb.probe_pkt_left);
+
+ for (i = 0; i < 2; ++i) {
spktlen = ngtcp2_conn_write_pkt(conn, NULL, buf, sizeof(buf), ++t);
CU_ASSERT(spktlen > 0);
@@ -4952,8 +4955,8 @@ void test_ngtcp2_conn_handshake_loss(void) {
it = ngtcp2_ksl_begin(&conn->hs_pktns->rtb.ents);
ent = ngtcp2_ksl_it_get(&it);
- CU_ASSERT(2181 == ent->frc->fr.crypto.offset);
- CU_ASSERT(5 == ent->hd.pkt_num);
+ CU_ASSERT(996 == ent->frc->fr.crypto.offset);
+ CU_ASSERT(4 == ent->hd.pkt_num);
fr.type = NGTCP2_FRAME_ACK;
fr.ack.largest_ack = 2;
@@ -4973,7 +4976,7 @@ void test_ngtcp2_conn_handshake_loss(void) {
spktlen = ngtcp2_conn_write_pkt(conn, NULL, buf, sizeof(buf), ++t);
- CU_ASSERT(spktlen == 0);
+ CU_ASSERT(0 == spktlen);
t += 1000;
@@ -5046,7 +5049,7 @@ void test_ngtcp2_conn_handshake_loss(void) {
/* On retransmission, the first HANDSHAKE CRYPTO has additional 5
bytes of data because Initial packet does not contain ACK
frame. */
- for (i = 0; i < 3; ++i) {
+ for (i = 0; i < 2; ++i) {
spktlen = ngtcp2_conn_write_pkt(conn, NULL, buf, sizeof(buf), ++t);
CU_ASSERT(spktlen > 0);
}
@@ -5059,9 +5062,9 @@ void test_ngtcp2_conn_handshake_loss(void) {
ent = ngtcp2_ksl_it_get(&it);
CU_ASSERT(NGTCP2_FRAME_CRYPTO == ent->frc->fr.type);
- CU_ASSERT(2181 == ent->frc->fr.crypto.offset);
- CU_ASSERT(819 == ngtcp2_vec_len(ent->frc->fr.crypto.data,
- ent->frc->fr.crypto.datacnt));
+ CU_ASSERT(996 == ent->frc->fr.crypto.offset);
+ CU_ASSERT(1180 == ngtcp2_vec_len(ent->frc->fr.crypto.data,
+ ent->frc->fr.crypto.datacnt));
fr.type = NGTCP2_FRAME_ACK;
fr.ack.largest_ack = 0;
@@ -5089,10 +5092,9 @@ void test_ngtcp2_conn_handshake_loss(void) {
into account this ACK. When calculating the available space for
CRYPTO data to send, we must consider the acknowledged data and
compute correct CRYPTO offset. */
- for (i = 0; i < 2; ++i) {
- spktlen = ngtcp2_conn_write_pkt(conn, NULL, buf, sizeof(buf), ++t);
- CU_ASSERT(spktlen > 0);
- }
+ spktlen = ngtcp2_conn_write_pkt(conn, NULL, buf, sizeof(buf), ++t);
+
+ CU_ASSERT(spktlen > 0);
spktlen = ngtcp2_conn_write_pkt(conn, NULL, buf, sizeof(buf), ++t);
CU_ASSERT(0 == spktlen);
@@ -5101,9 +5103,16 @@ void test_ngtcp2_conn_handshake_loss(void) {
ent = ngtcp2_ksl_it_get(&it);
CU_ASSERT(NGTCP2_FRAME_CRYPTO == ent->frc->fr.type);
- CU_ASSERT(2176 == ent->frc->fr.crypto.offset);
- CU_ASSERT(824 == ngtcp2_vec_len(ent->frc->fr.crypto.data,
- ent->frc->fr.crypto.datacnt));
+ CU_ASSERT(991 == ent->frc->fr.crypto.offset);
+ CU_ASSERT(5 == ngtcp2_vec_len(ent->frc->fr.crypto.data,
+ ent->frc->fr.crypto.datacnt));
+
+ frc = ent->frc->next;
+
+ CU_ASSERT(NGTCP2_FRAME_CRYPTO == frc->fr.type);
+ CU_ASSERT(2176 == frc->fr.crypto.offset);
+ CU_ASSERT(824 == ngtcp2_vec_len(frc->fr.crypto.data, frc->fr.crypto.datacnt));
+ CU_ASSERT(NULL == frc->next);
ngtcp2_conn_del(conn);
}
--
GitLab