MEDIUM: quic: Rework of the TX packets memory handling
The TX packet refcounting had come with the multithreading support but not only.
It is very useful to ease the management of the memory allocated for TX packets
with TX frames attached to. At some locations of the code we have to move TX
frames from a packet to a new one during retranmission when the packet has been
deemed as lost or not. When deemed lost the memory allocated for the paquet must
be released contrary to when its frames are retransmitted when probing (PTO).
For now on, thanks to this patch we handle the TX packets memory this way. We
increment the packet refcount when:
- we insert it in its packet number space tree,
- we attache an ack-eliciting frame to it.
And reciprocally we decrement this refcount when:
- we remove an ack-eliciting frame from the packet,
- we delete the packet from its packet number space tree.
Note that an optimization WOULD NOT be to fully reuse (without releasing its
memorya TX packet to retransmit its contents (its ack-eliciting frames). Its
information (timestamp, in flight length) to be processed by packet loss detection
and the congestion control.
diff --git a/include/haproxy/quic_frame-t.h b/include/haproxy/quic_frame-t.h
index 76957de..2e94e05 100644
--- a/include/haproxy/quic_frame-t.h
+++ b/include/haproxy/quic_frame-t.h
@@ -231,6 +231,7 @@
struct quic_frame {
struct list list;
+ struct quic_tx_packet *pkt;
unsigned char type;
union {
struct quic_padding padding;
diff --git a/include/haproxy/xprt_quic.h b/include/haproxy/xprt_quic.h
index 9035fd9..3135ce5 100644
--- a/include/haproxy/xprt_quic.h
+++ b/include/haproxy/xprt_quic.h
@@ -1002,6 +1002,21 @@
return eb64_entry(&ar->node, struct quic_arng_node, first)->last;
}
+/* Increment the reference counter of <pkt> */
+static inline void quic_tx_packet_refinc(struct quic_tx_packet *pkt)
+{
+ HA_ATOMIC_ADD(&pkt->refcnt, 1);
+}
+
+/* Decrement the reference counter of <pkt> */
+static inline void quic_tx_packet_refdec(struct quic_tx_packet *pkt)
+{
+ if (!HA_ATOMIC_SUB_FETCH(&pkt->refcnt, 1)) {
+ BUG_ON(!LIST_ISEMPTY(&pkt->frms));
+ pool_free(pool_head_quic_tx_packet, pkt);
+ }
+}
+
/* Discard <pktns> packet number space attached to <qc> QUIC connection.
* Its loss information are reset. Deduce the outstanding bytes for this
* packet number space from the outstanding bytes for the path of this
@@ -1030,10 +1045,11 @@
node = eb64_next(node);
list_for_each_entry_safe(frm, frmbak, &pkt->frms, list) {
LIST_DELETE(&frm->list);
+ quic_tx_packet_refdec(frm->pkt);
pool_free(pool_head_quic_frame, frm);
}
eb64_delete(&pkt->pn_node);
- pool_free(pool_head_quic_tx_packet, pkt);
+ quic_tx_packet_refdec(pkt);
}
}
@@ -1171,19 +1187,6 @@
} while (refcnt && !HA_ATOMIC_CAS(&pkt->refcnt, &refcnt, refcnt - 1));
}
-/* Increment the reference counter of <pkt> */
-static inline void quic_tx_packet_refinc(struct quic_tx_packet *pkt)
-{
- HA_ATOMIC_ADD(&pkt->refcnt, 1);
-}
-
-/* Decrement the reference counter of <pkt> */
-static inline void quic_tx_packet_refdec(struct quic_tx_packet *pkt)
-{
- if (!HA_ATOMIC_SUB_FETCH(&pkt->refcnt, 1))
- pool_free(pool_head_quic_tx_packet, pkt);
-}
-
/* Delete all RX packets for <qel> QUIC encryption level */
static inline void qc_el_rx_pkts_del(struct quic_enc_level *qel)
{
diff --git a/src/xprt_quic.c b/src/xprt_quic.c
index 1b164c7..364b611 100644
--- a/src/xprt_quic.c
+++ b/src/xprt_quic.c
@@ -1396,6 +1396,7 @@
frm_node = eb64_first(&qcs->tx.acked_frms);
while (frm_node) {
struct quic_stream *strm;
+ struct quic_frame *frm;
strm = eb64_entry(&frm_node->node, struct quic_stream, offset);
if (strm->offset.key > qcs->tx.ack_offset)
@@ -1417,17 +1418,10 @@
frm_node = eb64_next(frm_node);
eb64_delete(&strm->offset);
- /* TODO
- *
- * memleak: The quic_frame container of the quic_stream should
- * be liberated here, as in qc_treat_acked_tx_frm. However this
- * code seems to cause a bug which can lead to interrupted
- * transfers.
- *
- * struct quic_frame frm = container_of(strm, struct quic_frame, stream);
- * LIST_DELETE(&frm->list);
- * pool_free(pool_head_quic_frame, frm);
- */
+ frm = container_of(strm, struct quic_frame, stream);
+ LIST_DELETE(&frm->list);
+ quic_tx_packet_refdec(frm->pkt);
+ pool_free(pool_head_quic_frame, frm);
}
return ret;
@@ -1462,6 +1456,7 @@
}
LIST_DELETE(&frm->list);
+ quic_tx_packet_refdec(frm->pkt);
pool_free(pool_head_quic_frame, frm);
}
else {
@@ -1473,6 +1468,7 @@
break;
default:
LIST_DELETE(&frm->list);
+ quic_tx_packet_refdec(frm->pkt);
pool_free(pool_head_quic_frame, frm);
}
@@ -1540,6 +1536,9 @@
list_for_each_entry_safe(frm, frmbak, pkt_frm_list, list) {
LIST_DELETE(&frm->list);
+ quic_tx_packet_refdec(frm->pkt);
+ /* This frame is not freed but removed from its packet */
+ frm->pkt = NULL;
TRACE_PROTO("to resend frame", QUIC_EV_CONN_PRSAFRM, qc, frm);
LIST_APPEND(&tmp, &frm->list);
}
@@ -1547,6 +1546,24 @@
LIST_SPLICE(pktns_frm_list, &tmp);
}
+/* Free <pkt> TX packet and its attached frames.
+ * This is the responsability of the caller to remove this packet of
+ * any data structure it was possibly attached to.
+ */
+static inline void free_quic_tx_packet(struct quic_tx_packet *pkt)
+{
+ struct quic_frame *frm, *frmbak;
+
+ if (!pkt)
+ return;
+
+ list_for_each_entry_safe(frm, frmbak, &pkt->frms, list) {
+ LIST_DELETE(&frm->list);
+ pool_free(pool_head_quic_frame, frm);
+ }
+ pool_free(pool_head_quic_tx_packet, pkt);
+}
+
/* Free the TX packets of <pkts> list */
static inline void free_quic_tx_pkts(struct list *pkts)
{
@@ -1555,7 +1572,7 @@
list_for_each_entry_safe(pkt, tmp, pkts, list) {
LIST_DELETE(&pkt->list);
eb64_delete(&pkt->pn_node);
- quic_tx_packet_refdec(pkt);
+ free_quic_tx_packet(pkt);
}
}
@@ -2256,6 +2273,7 @@
list_for_each_entry_safe(frm, frmbak, &pkt->frms, list) {
TRACE_PROTO("to resend frame", QUIC_EV_CONN_PRSAFRM, qc, frm);
LIST_DELETE(&frm->list);
+ frm->pkt = NULL;
LIST_APPEND(tmp, &frm->list);
}
@@ -2874,13 +2892,8 @@
tmpbuf.size = tmpbuf.data = dglen;
TRACE_PROTO("to send", QUIC_EV_CONN_SPPKTS, qc);
- for (pkt = first_pkt; pkt; pkt = pkt->next)
- quic_tx_packet_refinc(pkt);
- if(qc_snd_buf(qc, &tmpbuf, tmpbuf.data, 0) <= 0) {
- for (pkt = first_pkt; pkt; pkt = pkt->next)
- quic_tx_packet_refdec(pkt);
+ if(qc_snd_buf(qc, &tmpbuf, tmpbuf.data, 0) <= 0)
break;
- }
cb_del(cbuf, dglen + headlen);
qc->tx.bytes += tmpbuf.data;
@@ -2900,8 +2913,8 @@
qc_set_timer(qc);
TRACE_PROTO("sent pkt", QUIC_EV_CONN_SPPKTS, qc, pkt);
next_pkt = pkt->next;
+ quic_tx_packet_refinc(pkt);
eb64_insert(&pkt->pktns->tx.pkts, &pkt->pn_node);
- quic_tx_packet_refdec(pkt);
}
}
@@ -5049,6 +5062,7 @@
int64_t rx_largest_acked_pn;
int add_ping_frm;
struct list frm_list = LIST_HEAD_INIT(frm_list);
+ struct quic_frame *cf;
/* Length field value with CRYPTO frames if present. */
len_frms = 0;
@@ -5186,8 +5200,6 @@
/* Ack-eliciting frames */
if (!LIST_ISEMPTY(&frm_list)) {
- struct quic_frame *cf;
-
list_for_each_entry(cf, &frm_list, list) {
if (!qc_build_frm(&pos, end, cf, pkt, qc)) {
ssize_t room = end - pos;
@@ -5195,6 +5207,8 @@
qc, NULL, NULL, &room);
break;
}
+ quic_tx_packet_refinc(pkt);
+ cf->pkt = pkt;
}
}
@@ -5246,24 +5260,9 @@
LIST_INIT(&pkt->frms);
pkt->next = NULL;
pkt->largest_acked_pn = -1;
- pkt->refcnt = 1;
+ pkt->refcnt = 0;
}
-/* Free <pkt> TX packet which has not already attached to any tree. */
-static inline void free_quic_tx_packet(struct quic_tx_packet *pkt)
-{
- struct quic_frame *frm, *frmbak;
-
- if (!pkt)
- return;
-
- list_for_each_entry_safe(frm, frmbak, &pkt->frms, list) {
- LIST_DELETE(&frm->list);
- pool_free(pool_head_quic_frame, frm);
- }
- quic_tx_packet_refdec(pkt);
-}
-
/* Build a packet into <buf> packet buffer with <pkt_type> as packet
* type for <qc> QUIC connection from <qel> encryption level from <frms> list
* of prebuilt frames.
@@ -5350,6 +5349,9 @@
return pkt;
err:
+ /* TODO: what about the frames which have been built
+ * for this packet.
+ */
free_quic_tx_packet(pkt);
TRACE_DEVEL("leaving in error", QUIC_EV_CONN_HPKT, qc);
return NULL;